feat(azure): add spec-level validation for workload identity config#8657
feat(azure): add spec-level validation for workload identity config#8657chdeshpa-hue wants to merge 1 commit into
Conversation
Azure HCP clusters using WorkloadIdentities authentication can enter an opaque degraded state when `hypershift create iam azure` is skipped or fails silently. The operator reconciles credentials with empty client IDs, producing confusing errors. This adds validateWorkloadIdentityConfig to ReconcileCredentials that checks all 7 required clientID fields are present before proceeding. Surfaces a descriptive error via PlatformCredentialsFound condition listing exactly which fields are missing. Phase 2 will wire ValidAzureWorkloadIdentity as a first-class condition on HostedCluster status. Ref: RFE-9351 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThis pull request introduces validation for Azure Workload Identity configuration in self-managed Azure HostedClusters. It defines a new 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @chdeshpa-hue. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (2)
553-553: ⚡ Quick winUse the "When ... it should ..." naming format for the new test cases.
The new subtest names use plain descriptions (e.g.,
non-Azure platform returns nil,WorkloadIdentities with missing clientIDs returns incomplete) rather than the required Gherkin style.♻️ Suggested renames
- name: "non-Azure platform returns nil", + name: "When platform is not Azure it should return nil",- name: "Azure with ManagedIdentities auth returns nil", + name: "When Azure auth is ManagedIdentities it should return nil",- name: "WorkloadIdentities auth with nil WorkloadIdentities returns not configured", + name: "When WorkloadIdentities auth has a nil WorkloadIdentities block it should return not-configured",- name: "WorkloadIdentities with missing clientIDs returns incomplete", + name: "When WorkloadIdentities is missing required clientIDs it should return incomplete",- name: "WorkloadIdentities with all clientIDs returns valid", + name: "When WorkloadIdentities has all clientIDs it should return valid",As per coding guidelines: "Always use 'When ... it should ...' format for describing test cases when creating unit tests".
Also applies to: 564-564, 580-580, 600-600, 629-629
🤖 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 `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` at line 553, Rename the failing subtest titles to follow the Gherkin-style "When ... it should ..." pattern; specifically update the t.Run strings that currently read like "non-Azure platform returns nil", "WorkloadIdentities with missing clientIDs returns incomplete" and the other plain descriptions (also at the other occurrences flagged around lines 564, 580, 600, 629) to descriptive phrases such as "When platform is non-Azure it should return nil" and "When WorkloadIdentities have missing clientIDs it should be incomplete" so the tests in azure_test.go use the required "When ... it should ..." format; keep the existing test bodies and only change the subtest name strings used in t.Run calls (e.g., the subtests where the helper functions under test are invoked).
626-626: 💤 Low valueBrittle assertion on missing-field ordering.
expectContains: "ingress, file"couples the test to the exact iteration order of the missing-field message. If the helper ever reorders its checks (e.g., alphabetizes or changes struct traversal), this passes/fails for unrelated reasons. Consider asserting each missing field independently (e.g.,ContainSubstring("ingress")andContainSubstring("file")) so ordering changes don't break the test.🤖 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 `@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go` at line 626, The test is brittle because it asserts the exact ordering via expectContains: "ingress, file"; update the assertion to check each missing field independently instead of an ordered string. Locate the table case using expectContains in azure_test.go and replace the single ordered expectation with two independent substring checks (e.g., assert the error/message contains "ingress" and contains "file") or change expectContains to be a slice of expected substrings and loop to assert each with ContainSubstring; keep the rest of the test and helper behavior unchanged.
🤖 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
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go`:
- Around line 529-549: The validation currently checks equality to empty string
which permits whitespace-only client IDs; update each check (e.g.,
wi.CloudProvider.ClientID, wi.NodePoolManagement.ClientID, wi.Ingress.ClientID,
wi.ImageRegistry.ClientID, wi.Disk.ClientID, wi.File.ClientID,
wi.Network.ClientID) to reject values that are empty after trimming whitespace
(use strings.TrimSpace or equivalent) so that purely whitespace client IDs are
treated as missing and appended to the missing slice.
---
Nitpick comments:
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Line 553: Rename the failing subtest titles to follow the Gherkin-style "When
... it should ..." pattern; specifically update the t.Run strings that currently
read like "non-Azure platform returns nil", "WorkloadIdentities with missing
clientIDs returns incomplete" and the other plain descriptions (also at the
other occurrences flagged around lines 564, 580, 600, 629) to descriptive
phrases such as "When platform is non-Azure it should return nil" and "When
WorkloadIdentities have missing clientIDs it should be incomplete" so the tests
in azure_test.go use the required "When ... it should ..." format; keep the
existing test bodies and only change the subtest name strings used in t.Run
calls (e.g., the subtests where the helper functions under test are invoked).
- Line 626: The test is brittle because it asserts the exact ordering via
expectContains: "ingress, file"; update the assertion to check each missing
field independently instead of an ordered string. Locate the table case using
expectContains in azure_test.go and replace the single ordered expectation with
two independent substring checks (e.g., assert the error/message contains
"ingress" and contains "file") or change expectContains to be a slice of
expected substrings and loop to assert each with ContainSubstring; keep the rest
of the test and helper behavior unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1874c375-7ab8-41a8-8bd6-c6ac03fff72e
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
api/hypershift/v1beta1/hostedcluster_conditions.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go
| if wi.CloudProvider.ClientID == "" { | ||
| missing = append(missing, "cloudProvider") | ||
| } | ||
| if wi.NodePoolManagement.ClientID == "" { | ||
| missing = append(missing, "nodePoolManagement") | ||
| } | ||
| if wi.Ingress.ClientID == "" { | ||
| missing = append(missing, "ingress") | ||
| } | ||
| if wi.ImageRegistry.ClientID == "" { | ||
| missing = append(missing, "imageRegistry") | ||
| } | ||
| if wi.Disk.ClientID == "" { | ||
| missing = append(missing, "disk") | ||
| } | ||
| if wi.File.ClientID == "" { | ||
| missing = append(missing, "file") | ||
| } | ||
| if wi.Network.ClientID == "" { | ||
| missing = append(missing, "network") | ||
| } |
There was a problem hiding this comment.
Reject whitespace-only client IDs during WI validation.
clientID == "" allows values like " ", so invalid config can pass this gate and fail later with less actionable errors.
Suggested fix
- if wi.CloudProvider.ClientID == "" {
+ if strings.TrimSpace(string(wi.CloudProvider.ClientID)) == "" {
missing = append(missing, "cloudProvider")
}
- if wi.NodePoolManagement.ClientID == "" {
+ if strings.TrimSpace(string(wi.NodePoolManagement.ClientID)) == "" {
missing = append(missing, "nodePoolManagement")
}
- if wi.Ingress.ClientID == "" {
+ if strings.TrimSpace(string(wi.Ingress.ClientID)) == "" {
missing = append(missing, "ingress")
}
- if wi.ImageRegistry.ClientID == "" {
+ if strings.TrimSpace(string(wi.ImageRegistry.ClientID)) == "" {
missing = append(missing, "imageRegistry")
}
- if wi.Disk.ClientID == "" {
+ if strings.TrimSpace(string(wi.Disk.ClientID)) == "" {
missing = append(missing, "disk")
}
- if wi.File.ClientID == "" {
+ if strings.TrimSpace(string(wi.File.ClientID)) == "" {
missing = append(missing, "file")
}
- if wi.Network.ClientID == "" {
+ if strings.TrimSpace(string(wi.Network.ClientID)) == "" {
missing = append(missing, "network")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if wi.CloudProvider.ClientID == "" { | |
| missing = append(missing, "cloudProvider") | |
| } | |
| if wi.NodePoolManagement.ClientID == "" { | |
| missing = append(missing, "nodePoolManagement") | |
| } | |
| if wi.Ingress.ClientID == "" { | |
| missing = append(missing, "ingress") | |
| } | |
| if wi.ImageRegistry.ClientID == "" { | |
| missing = append(missing, "imageRegistry") | |
| } | |
| if wi.Disk.ClientID == "" { | |
| missing = append(missing, "disk") | |
| } | |
| if wi.File.ClientID == "" { | |
| missing = append(missing, "file") | |
| } | |
| if wi.Network.ClientID == "" { | |
| missing = append(missing, "network") | |
| } | |
| if strings.TrimSpace(string(wi.CloudProvider.ClientID)) == "" { | |
| missing = append(missing, "cloudProvider") | |
| } | |
| if strings.TrimSpace(string(wi.NodePoolManagement.ClientID)) == "" { | |
| missing = append(missing, "nodePoolManagement") | |
| } | |
| if strings.TrimSpace(string(wi.Ingress.ClientID)) == "" { | |
| missing = append(missing, "ingress") | |
| } | |
| if strings.TrimSpace(string(wi.ImageRegistry.ClientID)) == "" { | |
| missing = append(missing, "imageRegistry") | |
| } | |
| if strings.TrimSpace(string(wi.Disk.ClientID)) == "" { | |
| missing = append(missing, "disk") | |
| } | |
| if strings.TrimSpace(string(wi.File.ClientID)) == "" { | |
| missing = append(missing, "file") | |
| } | |
| if strings.TrimSpace(string(wi.Network.ClientID)) == "" { | |
| missing = append(missing, "network") | |
| } |
🤖 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
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go`
around lines 529 - 549, The validation currently checks equality to empty string
which permits whitespace-only client IDs; update each check (e.g.,
wi.CloudProvider.ClientID, wi.NodePoolManagement.ClientID, wi.Ingress.ClientID,
wi.ImageRegistry.ClientID, wi.Disk.ClientID, wi.File.ClientID,
wi.Network.ClientID) to reject values that are empty after trimming whitespace
(use strings.TrimSpace or equivalent) so that purely whitespace client IDs are
treated as missing and appended to the missing slice.
1 participant
Summary
hypershift create iam azureis skipped or fails silently — the operator reconciles with empty client IDs producing confusing errorsvalidateWorkloadIdentityConfigtoReconcileCredentialsthat checks all 7 required clientID fields before proceeding, surfacing a descriptive error viaPlatformCredentialsFoundconditionValidAzureWorkloadIdentityto a first-class condition on HostedCluster statusTest plan
TestReconcileCredentialssuite passes (fixtures updated to include all required fields)go vet/gofmtcleanRef: RFE-9351
Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Tests