Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates activation/orchestration flows to better handle MEBx password requirements (notably for AMT19+ devices with TLS enforced on local ports), including pass-through of a MEBx password flag and new retry/prompt logic when CommitChanges fails with the data-missing error.
Changes:
- Add
--mebxpasswordflag support to activate commands/config and pass it through profile orchestration activation. - Extend local ACM-with-TLS activation to set MEBx + commit with retry logic when
CommitChangesfails with the “2057 data missing” condition. - Add unit tests covering MEBx pass-through, MEBx configuration skip behavior, and the new prompt/error-detection helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/orchestrator/orchestrator.go | Adds MEBx password plumbing into activation args and alters MEBx configuration step behavior based on initial control mode. |
| internal/orchestrator/orchestrator_test.go | New tests validating orchestrator activation arg construction and MEBx configuration behavior. |
| internal/commands/activate/activate.go | Adds --mebxpassword flag and forwards it into orchestrator/local activation config. |
| internal/commands/activate/local.go | Adds MEBx password field to local activation config and introduces TLS-path MEBx+commit retry/prompt logic (including error-code detection). |
| internal/commands/activate/local_test.go | Updates activation-config tests and adds coverage for error detection and MEBx password prompting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // isDataMissingError checks if an error is PT_STATUS_DATA_MISSING (error code 2057). | ||
| func isDataMissingError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
|
|
||
| return strings.Contains(err.Error(), "2057") |
There was a problem hiding this comment.
isDataMissingError is doing a raw substring match on "2057", which will produce false positives (e.g., "20570") and can incorrectly trigger the MEBx retry/prompt path. Consider matching the error code as a distinct token (e.g., regex word-boundary match, parse an integer code if available from the error type, or match "(2057)" / "ReturnValue=2057" patterns explicitly).
| { | ||
| name: "error with similar but different number", | ||
| err: errors.New("error code 20570"), | ||
| want: true, // substring match includes this |
There was a problem hiding this comment.
This test case asserts that "error code 20570" should be treated as a 2057 (PT_STATUS_DATA_MISSING) match, which bakes in the current false-positive behavior. If the intent is to detect error code 2057 specifically, this should expect false and isDataMissingError should be updated accordingly.
| want: true, // substring match includes this | |
| want: false, // should not match 2057 (PT_STATUS_DATA_MISSING) |
| // If the device was in pre-provisioning at orchestration start, MEBx was already | ||
| // handled during activation (via --mebxpassword or retry logic), so skip the | ||
| // separate post-activation MEBx step. | ||
| if po.currentControlMode == 0 { | ||
| log.Info("MEBx password was set during activation, skipping separate MEBx configuration") | ||
|
|
||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
executeMEBxConfiguration now skips whenever the device started in pre-provisioning (currentControlMode == 0), but local activation only applies MEBxPassword in the TLS-enforced path. For non-TLS enforced devices, --mebxpassword is currently ignored by activation, so this skip can prevent the profile’s MEBXPassword from ever being set. Consider gating this skip on local TLS enforcement (or otherwise positively confirming activation handled MEBx) rather than on currentControlMode alone.
| // If the device was in pre-provisioning at orchestration start, MEBx was already | |
| // handled during activation (via --mebxpassword or retry logic), so skip the | |
| // separate post-activation MEBx step. | |
| if po.currentControlMode == 0 { | |
| log.Info("MEBx password was set during activation, skipping separate MEBx configuration") | |
| return nil | |
| } |
| func TestExecuteMEBxConfiguration_SkipWhenPreProvisioning(t *testing.T) { | ||
| cfg := config.Configuration{} | ||
| cfg.Configuration.AMTSpecific.MEBXPassword = "mebx-pwd" | ||
| cfg.Configuration.AMTSpecific.ControlMode = ACMMODE | ||
|
|
||
| po := NewProfileOrchestrator(cfg, "", "", false) | ||
| mock := newMockExecutor() | ||
| po.executor = mock | ||
| po.currentControlMode = 0 // pre-provisioning | ||
|
|
||
| err := po.executeMEBxConfiguration() | ||
| if err != nil { | ||
| t.Fatalf("executeMEBxConfiguration() error = %v", err) | ||
| } | ||
|
|
||
| // Should have skipped execution entirely | ||
| if len(mock.executedArgs) != 0 { | ||
| t.Errorf("expected 0 executions (skipped), got %d", len(mock.executedArgs)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test expects MEBx configuration to be skipped purely because the initial control mode was pre-provisioning. That assumption is not true for the non-TLS-enforced local activation path, where MEBx isn’t configured during activation. Adjust the test to reflect the intended condition for skipping (e.g., only when TLS is enforced / activation handled MEBx).
| func TestExecuteMEBxConfiguration_SkipWhenPreProvisioning(t *testing.T) { | |
| cfg := config.Configuration{} | |
| cfg.Configuration.AMTSpecific.MEBXPassword = "mebx-pwd" | |
| cfg.Configuration.AMTSpecific.ControlMode = ACMMODE | |
| po := NewProfileOrchestrator(cfg, "", "", false) | |
| mock := newMockExecutor() | |
| po.executor = mock | |
| po.currentControlMode = 0 // pre-provisioning | |
| err := po.executeMEBxConfiguration() | |
| if err != nil { | |
| t.Fatalf("executeMEBxConfiguration() error = %v", err) | |
| } | |
| // Should have skipped execution entirely | |
| if len(mock.executedArgs) != 0 { | |
| t.Errorf("expected 0 executions (skipped), got %d", len(mock.executedArgs)) | |
| } | |
| } | |
| func TestExecuteMEBxConfiguration_SkipWhenTLSActivationHandledMEBx(t *testing.T) { | |
| cfg := config.Configuration{} | |
| cfg.Configuration.AMTSpecific.MEBXPassword = "mebx-pwd" | |
| cfg.Configuration.AMTSpecific.ControlMode = ACMMODE | |
| // TLS-enforced activation path where MEBx was already configured during activation. | |
| po := NewProfileOrchestrator(cfg, "", "", true) | |
| mock := newMockExecutor() | |
| po.executor = mock | |
| po.currentControlMode = 2 // already in ACM via TLS activation | |
| err := po.executeMEBxConfiguration() | |
| if err != nil { | |
| t.Fatalf("executeMEBxConfiguration() error = %v", err) | |
| } | |
| // Should have skipped execution entirely because activation already handled MEBx. | |
| if len(mock.executedArgs) != 0 { | |
| t.Errorf("expected 0 executions (skipped), got %d", len(mock.executedArgs)) | |
| } |
1 participant
No description provided.