refactor(configure): replace az/gcloud CLI shell-outs with native SDK calls#1279
Open
cristim wants to merge 5 commits into
Open
refactor(configure): replace az/gcloud CLI shell-outs with native SDK calls#1279cristim wants to merge 5 commits into
cristim wants to merge 5 commits into
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR replaces Azure and GCP setup paths with SDK-based flows, updates Azure sanity checks to use Azure SDK clients, and adds Azure service principal provisioning with rollback and retry handling. It also updates dependencies and expands tests around Azure JSON encoding and provisioning behavior. ChangesSDK Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci_cd_sanity_tests/pkg/sanity/azure/azure.go`:
- Line 263: The Subscription.State field is optional and is being dereferenced
unsafely in the azureSubscriptionInfo construction, which can panic when the SDK
omits it. Update the logic around the code that builds azureSubscriptionInfo in
the Azure sanity tests to guard sub.State before dereferencing it, following the
same nil-check pattern already used for other optional fields in this flow. If
the state is absent, leave the value empty or handle it consistently with the
existing optional-field handling.
In `@cmd/configure_azure.go`:
- Around line 70-72: The Azure Service Principal role name is inconsistent
between the help text and the wizard flow; update the documented role string in
configure_azure.go so it matches the role requested by the interactive command
in the Azure setup path (the configureAzure wizard and its related help/output),
and keep the same wording everywhere this role is referenced.
- Around line 261-266: The subscription picker is currently using
DefaultAzureCredential, which can resolve to a different principal than the
active az login session. Update listAzureSubscriptions to use Azure CLI auth
directly via azidentity.NewAzureCLICredential(nil), or adjust the credential
chain so AzureCLICredential is prioritized before other sources, keeping the
subscription list aligned with the rest of the wizard.
In `@cmd/configure_gcp.go`:
- Around line 533-536: The prompt handlers around read-and-confirm flows are
ignoring ReadString errors and treating empty input as approval, which can
trigger cloud mutations on EOF or read failure. Update the confirmation logic in
the createGCPServiceAccount path and the related role/key prompt handlers to
check the error returned by reader.ReadString before switching on the trimmed
choice, and abort with the corresponding error message instead of defaulting to
the run path when input cannot be read.
- Around line 288-410: The GCP SDK calls in listGCPProjects,
createGCPServiceAccount, grantGCPIAMRole, and createGCPServiceAccountKey still
use the inherited background context, so they can hang and block fallback
behavior. Update each helper to derive a short-lived timeout context before
calling newGCPAPIOption, cloudresourcemanager.NewService, iamv1.NewService, and
the subsequent Do()/Pages() calls. Keep the timeout scoped inside each helper
and ensure the derived context is used consistently for the client creation and
API request methods.
- Around line 357-386: The IAM policy update flow in the project policy helper
needs to preserve conditional bindings by using policy version 3. Update the
GetIamPolicy call in the policy modification logic to request version 3, then
ensure the returned policy has Version set to 3 before modifying bindings and
passing it to SetIamPolicy. Keep the fix localized to the
GetIamPolicy/SetIamPolicy sequence and the binding append logic in the project
IAM helper.
- Around line 607-628: The key-file flow in createGCPServiceAccountKey currently
returns keyFile even when no file was created or the fallback prompt was
skipped, which leaves getGCPCredentialsFilePath with a false path. Update the
switch handling for the run/skip/unknown cases so createGCPServiceAccountKey
only returns the actual keyFile path after a successful write, and returns an
empty string when the user skips, enters an unknown option, or declines/skips
promptAndRunGCPCommand. Make sure getGCPCredentialsFilePath can then detect the
empty result and continue prompting instead of assuming a file exists.
- Around line 407-421: The createGCPServiceAccountKey flow creates a remote IAM
key before the local file write succeeds, so a decode or os.WriteFile failure
can leave an orphaned active key. Update the logic around
createGCPServiceAccountKey to reserve the destination file first using exclusive
create semantics, and if any step after svc.Projects.ServiceAccounts.Keys.Create
fails, explicitly delete the newly created key before returning. Keep the
cleanup tied to the key creation result so fallback logic cannot mint duplicate
live keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01d4076f-babd-499c-bcd3-2f549ec9ebb4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
ci_cd_sanity_tests/pkg/sanity/azure/azure.goci_cd_sanity_tests/pkg/sanity/azure/azure_test.gocmd/configure_azure.gocmd/configure_gcp.gogo.mod
cristim
added a commit
that referenced
this pull request
Jun 24, 2026
Replace the remaining az/gcloud CLI fallbacks with native SDK calls that fail loud, per owner decision on PR #1279. Azure service principal creation (the create-for-rbac equivalent): - New cmd/configure_azure_sp.go creates the AAD application registration (name "CUDly"), adds a password credential, creates the service principal, resolves the "Reservations Administrator" role definition by display name at subscription scope, and assigns it -- all via the Microsoft Graph SDK (applications + service principals + addPassword) and armauthorization/v2 (RoleDefinitionsClient.NewListPager with a roleName filter, RoleAssignmentsClient.Create). DefaultAzureCredential reuses the az login session via its AzureCLICredential leg. - The resulting appId (client ID), generated client secret, and tenant ID are printed in the same shape az ad sp create-for-rbac prints, so the operator can feed them into the credential collection step. The secret comes from the addPassword response (PasswordCredential.GetSecretText) and is only available at creation time. - Tenant ID is resolved from the subscription via armsubscriptions. - A narrow azureSPProvisioner interface wraps the four cloud operations so the orchestration is unit-testable; cmd/configure_azure_sp_test.go mocks it and asserts the app name = "CUDly", role = "Reservations Administrator", and scope = /subscriptions/<id>, plus per-step error propagation (no role resolve/assign after an earlier failure). Remove silent CLI fallbacks -> fail loud: - configure_azure.go subscription list: on SDK error, return an error telling the operator to run "az login" first (was: silently run az account list). - configure_gcp.go project list, SA create, role grant, key create: on SDK error, return the error (was: print "falling back to gcloud" and run the CLI). ADC failures hint to run "gcloud auth application-default login". Remaining cloud-CLI exec.Command calls (the only three left, each //nolint:gosec with a precise reason -- interactive auth bootstrap or local state, no SDK equivalent): - az login (interactive browser OAuth) - gcloud auth login (interactive browser OAuth) - gcloud config set project (writes local ~/.config/gcloud state) Removing the az ad sp create-for-rbac subprocess eliminates its gosec G204 finding; the three retained calls carry justified nolints. go.mod: add github.com/microsoftgraph/msgraph-sdk-go v1.99.0 and github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/ armauthorization/v2 v2.2.0 as direct deps (root module). go mod tidy pulled the kiota transitive deps and bumped azcore/azidentity/otel to satisfy msgraph requirements.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/configure_gcp.go (1)
488-510: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the selected project against the SDK results before writing local config.
listGCPProjects(ctx)only gates listing success, whilevalidateGCPProjectIDonly checks format. A valid-looking typo can still be written bygcloud config set projecton Line 510, leaving local gcloud state pointed at an inaccessible/nonexistent project before later SDK calls fail. HavelistGCPProjectsreturn the discovered project IDs and reject entries not in that set before Line 510.Suggested shape
- if err := listGCPProjects(ctx); err != nil { + projectIDs, err := listGCPProjects(ctx) + if err != nil { return "", fmt.Errorf("failed to list GCP projects via SDK: %w\n"+ "Ensure Application Default Credentials are set: run 'gcloud auth application-default login' first", err) } @@ if err := validateGCPProjectID(projectID); err != nil { return "", err } + if _, ok := projectIDs[projectID]; !ok { + return "", fmt.Errorf("project ID %q was not found in the SDK project list", projectID) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_gcp.go` around lines 488 - 510, The current flow in configureGcpProject only checks that listGCPProjects(ctx) succeeds and that validateGCPProjectID passes format checks, but it still lets an unknown project ID be written by exec.Command("gcloud", "config", "set", "project", projectID). Update listGCPProjects to return the discovered project IDs, then in configureGcpProject verify the user-selected projectID exists in that returned set before setting local gcloud config. Keep the existing validateGCPProjectID check, but add a membership check against the SDK results and reject invalid selections before the gcloud config write.
♻️ Duplicate comments (1)
cmd/configure_azure.go (1)
343-359: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse Azure CLI auth consistently for the interactive Azure wizard.
Step 1 explicitly establishes an
az loginsession, but Step 2/3 switch toDefaultAzureCredential, which can resolve a different principal first. That can list subscriptions, resolve the tenant, or provision the service principal against the wrong account/subscription. Useazidentity.NewAzureCLICredential(nil)here (or a chain that puts Azure CLI first) and reuse that same credential for the rest of this wizard flow. This duplicates the earlier auth-chain finding and it still appears unresolved.Also applies to: 376-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_azure.go` around lines 343 - 359, The Azure wizard is still using DefaultAzureCredential in the subscription/tenant/service-principal steps, which can pick a different signed-in identity than the one established by Step 1. Update azureStepListSubscriptions and the related Step 3 flow to use azidentity.NewAzureCLICredential(nil) or a shared credential chain that prioritizes Azure CLI, and pass that same credential through the rest of the interactive wizard so all operations target the same account/subscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/configure_azure_sp.go`:
- Around line 68-99: The createAzureServicePrincipal flow leaves partially
created Azure resources behind when later steps fail after CreateApplication
succeeds. Update createAzureServicePrincipal to add compensating cleanup or
recovery using the existing azureSPProvisioner methods and identifiers
(CreateApplication, AddPassword, CreateServicePrincipal,
ResolveRoleDefinitionID, AssignRole) so failures in password creation,
service-principal creation, or role assignment do not orphan the app/secret/SP.
Ensure the error paths either delete any already-created resources or detect and
reuse the existing application/service principal on retry rather than creating
duplicates.
---
Outside diff comments:
In `@cmd/configure_gcp.go`:
- Around line 488-510: The current flow in configureGcpProject only checks that
listGCPProjects(ctx) succeeds and that validateGCPProjectID passes format
checks, but it still lets an unknown project ID be written by
exec.Command("gcloud", "config", "set", "project", projectID). Update
listGCPProjects to return the discovered project IDs, then in
configureGcpProject verify the user-selected projectID exists in that returned
set before setting local gcloud config. Keep the existing validateGCPProjectID
check, but add a membership check against the SDK results and reject invalid
selections before the gcloud config write.
---
Duplicate comments:
In `@cmd/configure_azure.go`:
- Around line 343-359: The Azure wizard is still using DefaultAzureCredential in
the subscription/tenant/service-principal steps, which can pick a different
signed-in identity than the one established by Step 1. Update
azureStepListSubscriptions and the related Step 3 flow to use
azidentity.NewAzureCLICredential(nil) or a shared credential chain that
prioritizes Azure CLI, and pass that same credential through the rest of the
interactive wizard so all operations target the same account/subscription.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be576c36-7e09-4fee-a364-bd5fb887ce7c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/configure_azure.gocmd/configure_azure_sp.gocmd/configure_azure_sp_test.gocmd/configure_gcp.gogo.mod
cristim
added a commit
that referenced
this pull request
Jun 24, 2026
Resolve the 7 Major CodeRabbit findings on PR #1279 at root cause. 1. azure sanity: guard sub.State before dereferencing it. State is an optional pointer; build azureSubscriptionInfo with the same nil-check pattern used for the other optional fields, so an omitted state cannot panic. 2. azure wizard: bind to the Azure CLI session explicitly. The wizard now builds credentials via a shared newAzureWizardCredential() backed by azidentity.NewAzureCLICredential, instead of DefaultAzureCredential whose chain prioritizes environment / workload / managed identity and could pick a different principal than the operator's "az login". The CI sanity test keeps DefaultAzureCredential (it must resolve the CI service-principal env vars). listAzureSubscriptions, resolveAzureTenantID and the SP provisioner all use the CLI credential. 3. gcp: bound every SDK helper with a 60s timeout. listGCPProjects, createGCPServiceAccount, grantGCPIAMRole and createGCPServiceAccountKey inherited context.Background() and could hang; each now derives a context.WithTimeout(ctx, gcpSDKCallTimeout). 4. gcp: preserve conditional IAM bindings. GetIamPolicy now requests RequestedPolicyVersion 3 and the policy is written back at Version 3, so a read-modify-write no longer silently drops condition bindings. Binding mutation is extracted into addMemberToPolicyBinding to keep complexity in check. 5. gcp: prevent orphaned service-account keys. createGCPServiceAccountKey reserves the destination file with O_EXCL before minting the remote key, and deletes the newly created remote key if decode/write fails, so a local failure cannot leave an active unused credential behind. 6. gcp: return an empty key path when no key was written. gcpStepCreateKey returns the key file path only after a successful write; on skip or an unknown choice it returns "" so getGCPCredentialsFilePath prompts for an existing credentials file instead of loading a missing one. 7. azure SP: roll back partial creation. createAzureServicePrincipal now deletes the just-created application (cascading to its password credential and derived service principal) if any later step -- add-password, SP create, role resolve, or role assign -- fails. A new DeleteApplication provisioner method backs this; if the compensating delete also fails the error names the orphaned app so the operator can remove it by hand. Unit tests assert rollback fires for each post-create failure, not on success, and that a failed rollback is surfaced.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jun 26, 2026
… calls Replace exec.Command shell-outs to cloud CLIs with native SDK calls where the replacement is sound and does not require new heavyweight dependencies: Azure (ci_cd_sanity_tests/): - az account set/show -> armsubscriptions.Client.Get (programmatic CI path) - az group list -> armresources.ResourceGroupsClient.NewListPager - az vm list -> armcompute.VirtualMachinesClient.NewListAllPager Azure (cmd/configure_azure.go): - az account list -> armsubscriptions.Client.NewListPager (with CLI fallback) GCP (cmd/configure_gcp.go): - gcloud projects list -> cloudresourcemanager/v1 Projects.List - gcloud iam service-accounts create -> iam/v1 Projects.ServiceAccounts.Create - gcloud projects add-iam-policy-binding -> cloudresourcemanager/v1 SetIamPolicy - gcloud iam service-accounts keys create -> iam/v1 SA Keys.Create (writes key data to file; key is base64-decoded from PrivateKeyData response field) Calls left as CLI (documented in each function): - az login / gcloud auth login: interactive browser OAuth; no SDK equivalent for an operator establishing an initial credential. - gcloud config set project: writes to local gcloud config file; no API equivalent. - az ad sp create-for-rbac: would require msgraph-sdk-go + armauthorization/v2, neither of which is in the module graph; the interactive wizard path does not justify adding two new heavy SDKs. Auth model: DefaultAzureCredential (azure.go / configure_azure.go) picks up the az login session via its AzureCLICredential leg. google.DefaultTokenSource (configure_gcp.go) picks up gcloud application-default credentials; the wizard now notes that operators must also run 'gcloud auth application-default login' in addition to 'gcloud auth login' for the SDK steps to work. go.mod: armcompute/v5 and armsubscriptions promoted from indirect to direct; armresources v1.2.0 promoted from transitive to direct. No new SDK versions or packages outside the existing module graph were added (google.golang.org/api iam/v1 and cloudresourcemanager/v1 are sub-packages of the already-required google.golang.org/api v0.274.0). Removes the gosec G204 subprocess findings from ci_cd_sanity_tests/azure.go (the only nolint-free exec.Command site in the CI path). The remaining exec.Command calls in configure_azure.go and configure_gcp.go are in interactive wizard paths where the CLI is intentionally required; those are addressed separately.
Replace the remaining az/gcloud CLI fallbacks with native SDK calls that fail loud, per owner decision on PR #1279. Azure service principal creation (the create-for-rbac equivalent): - New cmd/configure_azure_sp.go creates the AAD application registration (name "CUDly"), adds a password credential, creates the service principal, resolves the "Reservations Administrator" role definition by display name at subscription scope, and assigns it -- all via the Microsoft Graph SDK (applications + service principals + addPassword) and armauthorization/v2 (RoleDefinitionsClient.NewListPager with a roleName filter, RoleAssignmentsClient.Create). DefaultAzureCredential reuses the az login session via its AzureCLICredential leg. - The resulting appId (client ID), generated client secret, and tenant ID are printed in the same shape az ad sp create-for-rbac prints, so the operator can feed them into the credential collection step. The secret comes from the addPassword response (PasswordCredential.GetSecretText) and is only available at creation time. - Tenant ID is resolved from the subscription via armsubscriptions. - A narrow azureSPProvisioner interface wraps the four cloud operations so the orchestration is unit-testable; cmd/configure_azure_sp_test.go mocks it and asserts the app name = "CUDly", role = "Reservations Administrator", and scope = /subscriptions/<id>, plus per-step error propagation (no role resolve/assign after an earlier failure). Remove silent CLI fallbacks -> fail loud: - configure_azure.go subscription list: on SDK error, return an error telling the operator to run "az login" first (was: silently run az account list). - configure_gcp.go project list, SA create, role grant, key create: on SDK error, return the error (was: print "falling back to gcloud" and run the CLI). ADC failures hint to run "gcloud auth application-default login". Remaining cloud-CLI exec.Command calls (the only three left, each //nolint:gosec with a precise reason -- interactive auth bootstrap or local state, no SDK equivalent): - az login (interactive browser OAuth) - gcloud auth login (interactive browser OAuth) - gcloud config set project (writes local ~/.config/gcloud state) Removing the az ad sp create-for-rbac subprocess eliminates its gosec G204 finding; the three retained calls carry justified nolints. go.mod: add github.com/microsoftgraph/msgraph-sdk-go v1.99.0 and github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/ armauthorization/v2 v2.2.0 as direct deps (root module). go mod tidy pulled the kiota transitive deps and bumped azcore/azidentity/otel to satisfy msgraph requirements.
Resolve the 7 Major CodeRabbit findings on PR #1279 at root cause. 1. azure sanity: guard sub.State before dereferencing it. State is an optional pointer; build azureSubscriptionInfo with the same nil-check pattern used for the other optional fields, so an omitted state cannot panic. 2. azure wizard: bind to the Azure CLI session explicitly. The wizard now builds credentials via a shared newAzureWizardCredential() backed by azidentity.NewAzureCLICredential, instead of DefaultAzureCredential whose chain prioritizes environment / workload / managed identity and could pick a different principal than the operator's "az login". The CI sanity test keeps DefaultAzureCredential (it must resolve the CI service-principal env vars). listAzureSubscriptions, resolveAzureTenantID and the SP provisioner all use the CLI credential. 3. gcp: bound every SDK helper with a 60s timeout. listGCPProjects, createGCPServiceAccount, grantGCPIAMRole and createGCPServiceAccountKey inherited context.Background() and could hang; each now derives a context.WithTimeout(ctx, gcpSDKCallTimeout). 4. gcp: preserve conditional IAM bindings. GetIamPolicy now requests RequestedPolicyVersion 3 and the policy is written back at Version 3, so a read-modify-write no longer silently drops condition bindings. Binding mutation is extracted into addMemberToPolicyBinding to keep complexity in check. 5. gcp: prevent orphaned service-account keys. createGCPServiceAccountKey reserves the destination file with O_EXCL before minting the remote key, and deletes the newly created remote key if decode/write fails, so a local failure cannot leave an active unused credential behind. 6. gcp: return an empty key path when no key was written. gcpStepCreateKey returns the key file path only after a successful write; on skip or an unknown choice it returns "" so getGCPCredentialsFilePath prompts for an existing credentials file instead of loading a missing one. 7. azure SP: roll back partial creation. createAzureServicePrincipal now deletes the just-created application (cascading to its password credential and derived service principal) if any later step -- add-password, SP create, role resolve, or role assign -- fails. A new DeleteApplication provisioner method backs this; if the compensating delete also fails the error names the orphaned app so the operator can remove it by hand. Unit tests assert rollback fires for each post-create failure, not on success, and that a failed rollback is surfaced.
The PR replaced CLI shell-outs with SDK calls and introduced new
interactive steps in the Azure SP and GCP SA wizards. Each step
discarded the error from reader.ReadString('\n') with the legacy
`choice, _ :=` shape inherited from the surrounding code, which
golangci-lint's errcheck (check-blank: true) flags as a new violation
and which conflicts with the project's "no silent fallbacks" rule
for fail-loud error handling.
Surface the read error from the four PR-introduced prompts so an
unexpected EOF / closed-stdin propagates an explicit error instead of
silently treating it as the default "run" choice:
* cmd/configure_azure.go: azureStepCreateServicePrincipal
* cmd/configure_gcp.go: gcpStepCreateServiceAccount,
gcpStepGrantRole, gcpStepCreateKey
Also fix the new json.Marshal in
ci_cd_sanity_tests/pkg/sanity/azure/azure.go:encodeAccountJSON to check
the (impossible-for-this-struct) error and return nil so the caller
skips the expected-checks step rather than feeding the validator a
partially-encoded payload.
Pre-existing reader.ReadString errcheck patterns on the same files
(promptAndRun*Command, executeExplicit/GCPCommand, getGCPCredentialsFilePath,
gcpStepSelectProject, azureStepListSubscriptions) are present on main
and outside this PR's scope; tracked separately.
All cmd/ and ci_cd_sanity_tests/ tests pass (767 tests across 13
packages).
cristim
force-pushed
the
refactor/az-cli-to-sdk
branch
from
a526f1a to
42bc7ad
Compare
Member
Author
|
Rebased onto main (42bc7ad). Summary of conflict resolution below. Conflict origin: main's PR #1343 (two commits: Conflicts resolved across 4 commits: Commit 1 (
Commit 2 (
Commit 4 (
Additional fix (amend on commit 4): During conflict resolution the Standards applied to surviving shell-out code (
Gates:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/configure_azure_sp.go`:
- Around line 195-246: The RoleAssignment creation path in
graphSPProvisioner.AssignRole should tolerate ARM propagation delay after
CreateServicePrincipal by retrying on PrincipalNotFound instead of failing
immediately. Add a bounded backoff/retry around g.roleAsgn.Create, detect the
PrincipalNotFound case from the returned error, and only return a fatal error
after the retry limit is exceeded; keep the existing CreateServicePrincipal,
ResolveRoleDefinitionID, and AssignRole flow unchanged otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e441242b-21c7-4255-984b-a26ade2ec38e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
ci_cd_sanity_tests/pkg/sanity/azure/azure.goci_cd_sanity_tests/pkg/sanity/azure/azure_test.gocmd/configure_azure.gocmd/configure_azure_sp.gocmd/configure_azure_sp_test.gocmd/configure_gcp.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go
- ci_cd_sanity_tests/pkg/sanity/azure/azure.go
- cmd/configure_azure.go
- go.mod
- cmd/configure_gcp.go
…rincipalNotFound
Azure AD replication is eventually consistent: a service principal created
moments ago may not yet be visible to the ARM role-assignment API in a different
region, returning PrincipalNotFound or ServicePrincipalNotFound. Without retry
the wizard fails immediately in this race window.
- Extract roleAssigner interface so the retry loop is unit-testable without
hitting Azure.
- Add isPrincipalNotFoundErr helper covering the canonical error codes
(PrincipalNotFound, ServicePrincipalNotFound) and the older message-body form
("does not exist in the directory").
- Retry g.roleAsgn.Create with bounded exponential back-off (5s initial,
doubles to 30s cap, 3-minute total budget) on PrincipalNotFound; fail loud
with a diagnostic message and the MS troubleshooting URL if the budget
expires.
- Use ctx-aware select in the sleep to treat context cancellation as terminal
(not accumulated as lastErr).
- Add graphSPProvisioner.retryInitial / retryBudget fields (set to constants in
production, overridable to millisecond values in tests) so tests complete in
<100ms.
- Add TestGraphSPProvisioner_AssignRole_* and TestIsPrincipalNotFoundErr
covering: retry-then-succeed, budget-exhausted, non-retryable error,
context-cancellation, and error-code detection.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
exec.Commandshell-outs to cloud CLIs with native SDK calls. After the owner-requested follow-up, all reducible cloud operations now go through the SDK and fail loud on error -- no silent CLI fallbacks remain. Only three genuinely irreducible CLI calls are kept (interactive auth bootstrap + local gcloud state).Replaced calls
Azure - CI sanity test (programmatic/CI path)
ci_cd_sanity_tests/pkg/sanity/azure/azure.go:az account set->armsubscriptions.Client.Get(verify subscription reachable)az account show -o json->armsubscriptions.Client.Get(subscription/tenant identity, re-encoded as the same JSON shape sovalidateAccountExpectationsis unchanged)az group list->armresources.ResourceGroupsClient.NewListPageraz vm list->armcompute.VirtualMachinesClient.NewListAllPagerAzure - configure wizard
cmd/configure_azure.go:az account list --output table->armsubscriptions.Client.NewListPager(fails loud on auth error)az ad sp create-for-rbac-> Microsoft Graph SDK + armauthorization/v2 (see below)Azure service principal creation (
cmd/configure_azure_sp.go)The
create-for-rbacequivalent, fully via SDK:CUDly) -> Microsoft GraphApplications().PostApplications().ByApplicationId(...).AddPassword().Post(returns the secret text -- only available at creation, exactly like create-for-rbac)ServicePrincipals().PostRoleDefinitionsClient.NewListPagerwith aroleName eq '...'filter/subscriptions/<id>-> armauthorizationRoleAssignmentsClient.CreateThe resulting appId (client ID), generated client secret, and tenant ID are printed in the same shape
az ad sp create-for-rbacprints, so the operator feeds them into the credential collection step. Tenant ID is resolved from the subscription viaarmsubscriptions.A narrow
azureSPProvisionerinterface wraps the four cloud operations so the orchestration is unit-tested with a mock (the Graph / armauthorization concrete clients are not interfaces). Tests assert app name =CUDly, role =Reservations Administrator, scope =/subscriptions/<id>, that the secret is surfaced, and per-step error propagation (no resolve/assign after an earlier failure).GCP - configure wizard
cmd/configure_gcp.go(all fail loud on SDK error):gcloud projects list->cloudresourcemanager/v1 Projects.Listgcloud iam service-accounts create->iam/v1 Projects.ServiceAccounts.Creategcloud projects add-iam-policy-binding --role roles/compute.admin->cloudresourcemanager/v1 GetIamPolicy+SetIamPolicygcloud iam service-accounts keys create->iam/v1 Projects.ServiceAccounts.Keys.Create(key written from base64-decodedPrivateKeyData)Fail-loud, no silent fallback
All previous "falls back to the CLI silently" paths were removed (project rule: no silent fallbacks). On an SDK/ADC auth failure each step now returns a clear error instructing the operator to run
az login/gcloud auth login/gcloud auth application-default loginfirst.Calls left as CLI (the only three remaining)
Each is an interactive auth bootstrap or a local-state write with no SDK equivalent, and carries a justified
//nolint:gosec // G204: ...:az logingcloud auth logingcloud config set project~/.config/gcloudstate; no cloud-API equivalentRemoving the
az ad sp create-for-rbacsubprocess eliminates its gosec G204 finding; the three retained calls carry precise nolints. (PR #1265 nolints G204 broadly; reconciliation happens at merge time -- #1265's branch is untouched.)Auth model confirmation
DefaultAzureCredentialthroughout. Its chain includesAzureCLICredential, so theaz loginsession is reused. In CI it resolves viaAZURE_CLIENT_ID/AZURE_TENANT_ID/AZURE_CLIENT_SECRET. Graph calls use thehttps://graph.microsoft.com/.defaultscope.google.DefaultTokenSource(ADC).gcloud auth loginupdates the user session but does NOT populate the ADC cache; the wizard documents that operators must also rungcloud auth application-default login.New SDK deps
github.com/microsoftgraph/msgraph-sdk-go v1.99.0(direct, root module) -- application + service principal + addPasswordgithub.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0(direct, root module) -- role definition resolution + role assignmentgo mod tidypulled the kiota transitive deps and bumpedazcore/azidentity/otelto satisfy msgraph requirements.armcompute/v5,armsubscriptionspromoted to direct;armresources v1.2.0promoted to direct;google.golang.org/api/iam/v1andcloudresourcemanager/v1are sub-packages of the already-requiredgoogle.golang.org/api.Tests / verification
go build ./...andgo test ./...pass (5,486 tests across 38 packages).golangci-lint runon touched files: no new findings; the only gosec nolints are on the three irreducible CLI calls (G204 fully suppressed there). Remaining gosec G117/G115 findings are pre-existing on unchanged lines.gocyclo -over 10: no touched function exceeds 10.Summary by CodeRabbit