Repository navigation
refactor(configure): replace az/gcloud CLI shell-outs with native SDK calls - #1279
Merged
Merged
Conversation
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 for encoding, IAM updates, key handling, and provisioning behavior. ChangesSDK Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)Azure service principal provisioningsequenceDiagram
participant Operator
participant AzureSetup
participant MicrosoftGraph
participant ARM
Operator->>AzureSetup: select subscription
AzureSetup->>MicrosoftGraph: create application and service principal
AzureSetup->>ARM: resolve and assign subscription role
ARM-->>AzureSetup: role assignment result
AzureSetup-->>Operator: service principal credentials
GCP setupsequenceDiagram
participant Operator
participant GCPSetup
participant CloudResourceManager
participant IAM
participant KeyFile
Operator->>GCPSetup: run setup steps
GCPSetup->>CloudResourceManager: list projects
GCPSetup->>IAM: create service account and grant role
IAM-->>GCPSetup: service-account key material
GCPSetup->>KeyFile: decode and write credentials
KeyFile-->>Operator: credentials file
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
cristim
added a commit
that referenced
this pull request
Jul 3, 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.
cristim
added a commit
that referenced
this pull request
Jul 3, 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.
cristim
force-pushed
the
refactor/az-cli-to-sdk
branch
from
July 3, 2026 22:55
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
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 10, 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.
cristim
added a commit
that referenced
this pull request
Jul 10, 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.
cristim
force-pushed
the
refactor/az-cli-to-sdk
branch
from
July 10, 2026 14:25
e78087d to
8b421a0
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 16, 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.
cristim
added a commit
that referenced
this pull request
Jul 16, 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.
cristim
force-pushed
the
refactor/az-cli-to-sdk
branch
from
July 16, 2026 22:49
cb44c08 to
8384cf9
Compare
Member
Author
|
Fixed the Lint Code job's golangci-bundled gosec findings (also re-rebased onto current origin/main 597faea). Root cause of the earlier false pass: the Lint Code job runs golangci-lint v2.10.1 (pinned in
Fix: added
Verified with the pinned CI tools (true exit codes):
Pushed as 8384cf9. |
Member
Author
|
@coderabbitai review |
… 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).
…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.
Resolve the six golangci findings introduced on this branch's own lines (no base debt touched): - misspell: serialises/behaviour/cancelled to US spelling - gocritic unnamedResult: name runAccountShowCheck results - govet fieldalignment: reorder two test structs to pack pointers Whole-repo Lint Code / Security Scanning remain red on pre-existing base debt, tracked separately and not owned by this PR.
The SDK-refactor wizards kept a few exec.Command auth shell-outs and file reads that gosec flags. The suppressions used //nolint:gosec, which the standalone gosec run (Security Scanning job) ignores, so both the Security Scanning and Lint Code jobs stayed red. Convert them to real #nosec directives with justifications that name the actual guard: - G204 (az login / gcloud auth login): program and args are hardcoded literals from the caller (runAzureSetupCommands / runGCPSetupCommands), no shell, not attacker-controlled. - G204/G702 (gcloud config set project): fixed argv with projectID pre-validated by validateGCPProjectID (strict regex) and passed as a discrete argv element, so it cannot inject. golangci's newer gosec reports this line as G702 (taint) while standalone reports G204, so the directive lists both. - G304 (read operator creds file): filepath.Clean the operator-supplied --credentials-file / prompt path; it is trusted local operator input to an interactive configure command, not attacker-controlled. - G304 (reserve key file): keyFile is the sole caller's fixed filepath.Join(os.UserHomeDir(), "cudly-gcp-key.json"), program- controlled, not attacker input. Verified none of the credential structs (AzureCredentials, GCPCredentials, azureSPResult) are written to logs; the only secret surfaced is the newly minted SP password shown once on stdout for the operator to copy. Verified with the CI tools: standalone gosec v2.26.1 `gosec ./...` and its SARIF invocation both report 0 issues; golangci-lint run ./... reports 0; go build, go vet, gocyclo -over 10 on the touched files all clean.
The Lint Code job runs golangci-lint v2.10.1, whose bundled gosec is a newer ruleset than the standalone gosec v2.26.1 used by Security Scanning. It emits five findings the standalone run does not: - G117 on the four credential-input struct fields (AzureCredentials and AzureConfigOptions ClientSecret, azureSPResult ClientSecret, GCPCredentials PrivateKey): the field name / json tag matches a secret pattern. - G703 (path traversal via taint) on the operator creds-file read, which standalone gosec reports as G304. Add #nosec directives whose justification names the real guard: - The four fields are operator-supplied credential INPUT (their own Azure SP secret / GCP private key), read from config or an interactive prompt, never a hardcoded secret. Verified none are written to logs: the only secret ever emitted is the SDK-generated SP password shown once on stdout for the operator to copy (mirrors az ad sp create-for-rbac). - The creds path is trusted operator input to a local configure command, filepath.Clean applied, not attacker-controlled; the directive lists both G304 (standalone) and G703 (golangci) since the two gosec versions label the same line differently. #nosec is honored by BOTH golangci-gosec and standalone gosec, unlike //nolint which the standalone run ignores. Verified with the pinned CI tools: golangci-lint v2.10.1 run ./... = 0 issues; standalone gosec v2.26.1 gosec ./... = 0 issues; go vet = 0; gocyclo -over 10 . = clean; go build = 0.
… wizards Address the independent-review BLOCK on the CLI->SDK refactor. Green CI missed these because the GCP wizard half had no unit tests. D1 (GCP wizard broken on first run): Step 1 ran only "gcloud auth login", which does NOT populate the ADC cache that Steps 2/4/5/6 authenticate through, so a fresh operator hard-aborted at the project listing and had to re-run. Add a prompted Step 1b that runs "gcloud auth application-default login" (same interactive-auth-with-no-SDK-equivalent category as the retained login calls; hardcoded literal argv, no shell), so setup completes in one pass. Update the wizard/newGCPAPIOption docs. D2 (removed skip option compounded D1): the SDK subscription/project listings were made unconditional and fail-hard, so an operator who already knows their ID could not get past a listing failure. Restore the [R]un/[S]kip prompt (new promptRunOrSkipListing helper) around both listings: skip goes straight to the ID prompt; the listing still fails loud WHEN RUN (skip is a deliberate choice, not a silent fallback). D3 (Azure SP retry budget was dead + destructive): roleAssignRetryBudget is 3 min but the SP step ran under a 2 min context that also covered tenant resolution and the Graph calls, so the PrincipalNotFound retry loop was cut short by ctx cancellation and the rollback then deleted the just-created app, resetting the AAD replication clock every re-run. Widen the step budget to 6 min (> retryBudget + pre-assignment overhead) so the retry gets its full budget, and wrap the terminal ctx.Err() return with the "AAD role propagation can take up to ~10 min; re-run" guidance (errors.Is still matches the cause). Tests: add cmd/configure_gcp_test.go (mocks, no creds) mirroring configure_azure_sp_test.go. Refactor createGCPServiceAccountKey to extract a testable writeServiceAccountKey behind a gcpKeyProvisioner interface. Cover addMemberToPolicyBinding (append / already-bound no-op / create missing / preserve conditional bindings on the version-3 round-trip) and the key-creation rollback path (success, decode-failure rollback, rollback-failure surfaced, create-failure no-orphan, reserve-failure no-mint). Nit: help text --role "Reservation Administrator" -> "Reservations Administrator" to match azureSPRoleName.
cristim
force-pushed
the
refactor/az-cli-to-sdk
branch
from
July 17, 2026 06:00
8384cf9 to
0fe3107
Compare
Member
Author
|
Addressed the independent-review BLOCK on the CLI->SDK refactor (behavioral fixes + the missing GCP tests), rebased onto current origin/main (473f69b). D1 (GCP wizard broken on first run): Step 1 ran only D2 (removed skip option compounded D1): the SDK subscription/project listings were unconditional + fail-hard, so an operator who already knows their ID couldn't get past a listing failure. Restored the [R]un/[S]kip prompt (new D3 (Azure SP retry budget dead + destructive): Tests (the untested GCP half): added
Nit: help text Gate exit codes (pinned CI tools, true exit codes):
Pushed as 0fe3107. Behavioral change -- please re-review. |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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.go`:
- Line 276: Update the password-reading call in the configuration flow to pass
int(os.Stdin.Fd()) instead of syscall.Stdin, preserving Windows compatibility.
Remove the syscall import if it is unused elsewhere in the file.
🪄 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: 738d904c-99fd-46b3-9b75-1e96b19acf6e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
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.gocmd/configure_gcp_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/configure_azure_sp.go
- ci_cd_sanity_tests/pkg/sanity/azure/azure.go
- go.mod
- cmd/configure_gcp_test.go
- cmd/configure_azure_sp_test.go
- ci_cd_sanity_tests/pkg/sanity/azure/azure_test.go
- cmd/configure_gcp.go
| // int cast: syscall.Stdin is already int on Unix but syscall.Handle on | ||
| // Windows; term.ReadPassword takes int, so the cast keeps Windows builds working. | ||
| secret, err := term.ReadPassword(int(syscall.Stdin)) //nolint:unconvert // no-op on Unix (int), required on Windows (syscall.Handle) | ||
| secret, err := term.ReadPassword(syscall.Stdin) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether Windows is an advertised or release target.
fd -H -t f -0 'README.*|.*\.ya?ml|\.goreleaser.*|go\.mod' . \
| xargs -0 rg -n -i -C2 'windows|GOOS|release.*target'
# Check other terminal-password calls for the established portable pattern.
rg -n -C2 --type go '\bterm\.ReadPassword\s*\(' .Repository: LeanerCloud/CUDly
Length of output: 496
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files mentioning windows / GOOS / build targets =="
rg -n -i -C2 '\bwindows\b|\bGOOS\b|GOARCH|cross-compile|release target|build matrix' . \
--glob '!**/vendor/**' --glob '!**/node_modules/**' || true
echo
echo "== go files importing syscall in cmd/configure_azure.go context =="
sed -n '1,40p' cmd/configure_azure.go
echo
echo "== occurrences of term.ReadPassword in repo =="
rg -n -C2 --type go '\bterm\.ReadPassword\s*\(' .Repository: LeanerCloud/CUDly
Length of output: 24759
Preserve Windows builds here. term.ReadPassword takes an int, but syscall.Stdin is a Handle on Windows, so this line breaks GOOS=windows. Use int(os.Stdin.Fd()) instead, and drop the syscall import if nothing else needs it.
🤖 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` at line 276, Update the password-reading call in the
configuration flow to pass int(os.Stdin.Fd()) instead of syscall.Stdin,
preserving Windows compatibility. Remove the syscall import if it is unused
elsewhere in the file.
Member
Author
|
Merged to main after two adversarial review rounds (final SHIP). Replaces az/gcloud CLI shell-out with SDK calls in the configure wizard; round-2 fixed the GCP golden-path break (added prompted application-default login so ADC-dependent steps complete in one pass), restored the [R]un/[S]kip listing prompt, widened the Azure SP context to 6min so the 3min role-assignment retry budget is honored before rollback, and added configure_gcp_test.go covering the previously-untested GCP half. Only interactive-login CLI calls remain (no SDK equivalent). golangci v2.10.1 + gosec v2.26.1 clean. |
cristim
added a commit
that referenced
this pull request
Jul 20, 2026
…ow-up to #1279) (#1459) * fix(cmd): use portable os.Stdin.Fd for password read on Windows (follow-up to #1279) term.ReadPassword takes an int, but syscall.Stdin is an int only on Unix; on Windows it is a syscall.Handle (uintptr), so cmd/configure_azure.go failed to compile under GOOS=windows. Use int(os.Stdin.Fd()), which is portable across platforms, and drop the now-unused syscall import. Addresses the unresolved CodeRabbit review thread on #1279. Verified that the prior code fails `GOOS=windows go build ./cmd/...` with a type error and the fix builds cleanly for both host and windows/amd64. * fix(cmd): justify terminal descriptor conversion
cristim
added a commit
that referenced
this pull request
Sep 27, 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.
cristim
added a commit
that referenced
this pull request
Sep 27, 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.
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
refactor(configure): replace az/gcloud CLI shell-outs with native SDK calls
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
…ow-up to #1279) (#1459) * fix(cmd): use portable os.Stdin.Fd for password read on Windows (follow-up to #1279) term.ReadPassword takes an int, but syscall.Stdin is an int only on Unix; on Windows it is a syscall.Handle (uintptr), so cmd/configure_azure.go failed to compile under GOOS=windows. Use int(os.Stdin.Fd()), which is portable across platforms, and drop the now-unused syscall import. Addresses the unresolved CodeRabbit review thread on #1279. Verified that the prior code fails `GOOS=windows go build ./cmd/...` with a type error and the fix builds cleanly for both host and windows/amd64. * fix(cmd): justify terminal descriptor conversion
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