Skip to content

[E2E] add MSI-based ARM Machine registration E2E - #291

Open
Wenxuan W (wenxuan0923) wants to merge 5 commits into
mainfrom
wenx/arm-machine-registration-e2e
Open

[E2E] add MSI-based ARM Machine registration E2E#291
Wenxuan W (wenxuan0923) wants to merge 5 commits into
mainfrom
wenx/arm-machine-registration-e2e

Conversation

@wenxuan0923

@wenxuan0923 Wenxuan W (wenxuan0923) commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Why

The existing E2E suite exercises controller-backed Machine fixtures, but it does not prove that a real node configured with agent.machineClient.mode: arm can register a missing AKS Machine through ARM.

This PR adds that live coverage for the registration workflow discussed in #285. It is intentionally separate from the ARM client retry change in #290.

What this tests

The new arm-registration scenario runs before the existing controller-backed lifecycle suite and reuses the managed-identity VM:

  1. Clean up state left by an earlier or interrupted attempt, then prove the target Machine is absent.
  2. Wait until the VM's existing cluster-scoped AKS Contributor assignment authorizes ARM Machine requests.
  3. Bootstrap the node with machineClient.mode: arm and requireMachineRegistration: true, without pre-creating a controller Machine fixture.
  4. Verify EnsureMachine takes the create path and ARM returns a Machine with the expected name, Kubernetes version, non-empty ETag, and Succeeded provisioning state.
  5. Verify the corresponding Kubernetes Node becomes Ready.
  6. Reset the host, remove the Node, delete the ARM Machine through the agent-pool deleteMachines action, and prove the resource is absent.
  7. Reuse the clean MSI host for the existing controller-backed MSI lifecycle tests.

The same cleanup path runs before a retry and after a failed attempt, so arm-registration is safe to rerun against the same E2E infrastructure. ARM responses and bootstrap logs are retained as workflow artifacts.

Existing Arc E2E hardening

Full-suite validation exposed a pre-existing 403 in the later Arc listBootstrapData step; the same failure reproduced on main and was not caused by the new Machine registration scenario.

This PR keeps the successful full-suite fix that:

  • verifies the Arc principal's AKS Contributor assignment at the exact cluster scope instead of trusting persisted arc_role_assigned=true state;
  • creates and polls the assignment when it is absent;
  • includes bounded, control-character-safe, token-redacted ARM error code/message and request IDs in bootstrap-data failures.

Validation

  • go test ./...
  • go test ./pkg/bootstrapdata
  • shell syntax checks and ShellCheck for the changed E2E scripts
  • mocked ARM Machine absence/registration response harness
  • git diff --check
  • live E2E run 33013937970: ARM registration and the complete E2E test step passed
  • GitHub Build, Test, Lint, Code Quality, Security Scan, Dependency Review, and CodeQL checks passed

make check reached lint locally, but the installed golangci-lint was built with Go 1.25.4 and cannot lint the repository's Go 1.26.5 target. The canonical GitHub Lint check passed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the AKS Flex Node E2E suite with a dedicated MSI-based scenario that validates real ARM Machine registration (machineClient.mode: arm) before running the existing controller-backed lifecycle coverage, and preserves ARM responses + bootstrap logs as artifacts for debugging.

Changes:

  • Add a new arm-registration E2E command and run it as part of the default all flow.
  • Introduce an ARM Machine registration scenario that verifies Machine absence, performs bootstrap-driven registration, validates the ARM resource + create-path log, then resets/reuses the MSI host.
  • Add configuration/documentation plumbing for E2E_ARM_MACHINE_API_VERSION and update workflow/docs to reflect the new coverage.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
hack/e2e/run.sh Adds arm-registration command and runs ARM registration as part of all.
hack/e2e/README.md Documents the new ARM registration phase and environment variable.
hack/e2e/lib/node-join-msi.sh Refactors MSI join to support both in-cluster and ARM Machine backends.
hack/e2e/lib/common.sh Adds default/config logging for E2E_ARM_MACHINE_API_VERSION.
hack/e2e/lib/arm-machine-registration.sh New ARM registration E2E scenario implementation (MSI-host reuse + validation).
.github/workflows/e2e-tests.yml Updates workflow description to include ARM Machine registration coverage.
.env.example Adds example E2E_ARM_MACHINE_API_VERSION setting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread hack/e2e/run.sh Outdated
Comment thread hack/e2e/lib/arm-machine-registration.sh Outdated
@wenxuan0923 Wenxuan W (wenxuan0923) changed the title test: add MSI-based ARM Machine registration E2E [Test] add MSI-based ARM Machine registration E2E Aug 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

hack/e2e/run.sh:10

  • The usage text for the all command no longer matches what cmd_all actually runs (it omits validate-absent, lifecycle, agent-upgrade, upgrade-drift, and log collection). This can mislead anyone using --help.
#   all           Run the full E2E flow (default): build, infra, CNI, controller,
#                 ARM registration, join, validate, unjoin, rejoin, cleanup

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

hack/e2e/lib/arm-machine-registration.sh:229

  • The ARM authorization probe passes the Authorization header via curl -H ..., which places the access token in the process arguments on the VM while the request is running. This can leak credentials via ps/process inspection and is inconsistent with the safer curl --config - pattern used elsewhere in this script.
  status="$(curl -sS -o "${response_file}" -w '%{http_code}' \
    -H "Authorization: Bearer ${token}" \
    "${ARM_MACHINE_URL}" || true)"

@karenychen

Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. The MSI authorization probe expands the ARM bearer token into the curl -H argument, making the credential visible in the VM process command line while the request is in flight. This defeats the file’s own safer curl --config - pattern used for deletion and can expose the cluster-scoped AKS Contributor token to another local process. Feed this header through stdin as well.

token="$(curl -fsS \
-H Metadata:true \
'http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https%3A%2F%2Fmanagement.azure.com%2F' |
python3 -c 'import json, sys; print(json.load(sys.stdin)["access_token"])' 2>/dev/null || true)"
if [[ -z "${token}" ]]; then
echo "Managed identity token is not available yet; retrying..."
sleep 10
continue
fi
status="$(curl -sS -o "${response_file}" -w '%{http_code}' \
-H "Authorization: Bearer ${token}" \
"${ARM_MACHINE_URL}" || true)"
unset token

Feed the ARM authorization header to curl through stdin so local process inspection cannot expose the bearer token.
@wenxuan0923

Copy link
Copy Markdown
Collaborator Author

Code review

Found 1 issue:

  1. The MSI authorization probe expands the ARM bearer token into the curl -H argument, making the credential visible in the VM process command line while the request is in flight. This defeats the file’s own safer curl --config - pattern used for deletion and can expose the cluster-scoped AKS Contributor token to another local process. Feed this header through stdin as well.

token="$(curl -fsS \
-H Metadata:true \
'http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https%3A%2F%2Fmanagement.azure.com%2F' |
python3 -c 'import json, sys; print(json.load(sys.stdin)["access_token"])' 2>/dev/null || true)"
if [[ -z "${token}" ]]; then
echo "Managed identity token is not available yet; retrying..."
sleep 10
continue
fi
status="$(curl -sS -o "${response_file}" -w '%{http_code}' \
-H "Authorization: Bearer ${token}" \
"${ARM_MACHINE_URL}" || true)"
unset token

Fixed. The auth header is now passed to curl through stdin with --config -, so the MSI token no longer shows up in the process args. Thanks!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

hack/e2e/run.sh:14

  • The arm-registration command help text doesn’t mention that it requires infra to have been run already (it relies on the provisioned cluster + MSI VM state). This makes ./hack/e2e/run.sh --help inconsistent with other commands like join that explicitly call out the prerequisite.
#   arm-registration Validate MSI-based registration through the ARM Machine API

@karenychen

Copy link
Copy Markdown
Collaborator

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@wenxuan0923 Wenxuan W (wenxuan0923) changed the title [Test] add MSI-based ARM Machine registration E2E [E2E] add MSI-based ARM Machine registration E2E Aug 26, 2026
@bcho
hbc (bcho) requested a balanced review from Copilot August 28, 2026 00:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants