CNTRLPLANE-3070: Support KMS on self-managed Azure without affecting ARO HCP#8088
Draft
bryan-cox wants to merge 7 commits intoopenshift:mainfrom
Draft
CNTRLPLANE-3070: Support KMS on self-managed Azure without affecting ARO HCP#8088bryan-cox wants to merge 7 commits intoopenshift:mainfrom
bryan-cox wants to merge 7 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
Skipping CI for Draft Pull Request. |
|
@bryan-cox: This pull request references CNTRLPLANE-3070 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
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:
📝 WalkthroughWalkthroughThis pull request adds support for self-managed Azure KMS encryption at rest and wiring for KMS workload identities. Changes include making AzureKMSSpec.KMS optional and adding a mutually-exclusive SelfManagedKMS; adding --enable-kms to IAM creation and plumbing kmsClientID through IAM/infra outputs; conditionally including a kms workload identity; control-plane/operator changes to detect self-managed Azure and render token-minter/cloud-token or managed secret-store CSI volumes accordingly; centralizing the cloud token mount path with CloudTokenMountPath; and extensive tests and docs for both managed and self-managed Azure KMS flows. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI/IAM Cmd
participant WI as Workload Identity Mgr
participant Infra as Infra Manager
participant Secret as K8s Secrets
User->>CLI: hypershift create iam azure --enable-kms
CLI->>WI: CreateWorkloadIdentitiesFromIAMOptions(enableKMS=true)
WI->>WI: GetWorkloadIdentityDefinitions(opts={IncludeKMS:true})
WI->>Infra: Create Azure identities (includes kms)
Infra->>Infra: Assign roles (if applicable)
Infra-->>WI: Return identities + kmsClientID
WI-->>CLI: Return IAM output with kmsClientID
CLI->>Secret: Write workload-identities.json (includes kmsClientID)
sequenceDiagram
participant User
participant Cluster as Cluster Create
participant Infra as Infra Extract
participant CPO as Control Plane Operator
participant KMS as KMS Provider
User->>Infra: Load workload-identities.json
Infra->>Infra: Extract kmsClientID from JSON
Infra-->>Cluster: Pass infra output (kmsClientID)
User->>Cluster: hypershift create cluster azure --encryption-key-id=<key>
Cluster->>Cluster: Build AzureKMSSpec (SelfManagedKMS if kmsClientID present else KMS)
Cluster-->>CPO: Apply SecretEncryption with AzureKMSSpec
CPO->>CPO: Detect self-managed vs managed
alt Self-Managed
CPO->>KMS: Add token-minter container + cloud-token volume
CPO->>KMS: Set AZURE_CLIENT_ID/AZURE_TENANT_ID and federated token file
KMS->>KMS: Mint federated token at runtime and authenticate to Key Vault
else Managed
CPO->>KMS: Mount credentials secret via Secret Store CSI
KMS->>KMS: Use credentials secret to authenticate to Key Vault
end
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@bryan-cox: This pull request references CNTRLPLANE-3070 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bryan-cox 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
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
2 times, most recently
from
1890fdc to
a3d1f65
Compare
openshift-ci
Bot
added
needs-rebase
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
2 times, most recently
from
21007e3 to
2d16bd3
Compare
openshift-ci
Bot
removed
the
needs-rebase
4 tasks
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
from
2d16bd3 to
82e995f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8088 +/- ##
==========================================
+ Coverage 37.44% 37.68% +0.24%
==========================================
Files 751 751
Lines 91969 92120 +151
==========================================
+ Hits 34435 34719 +284
+ Misses 54894 54757 -137
- Partials 2640 2644 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
openshift-ci
Bot
removed
the
needs-rebase
|
@bryan-cox: This pull request references CNTRLPLANE-3070 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/create_cluster_test.go (1)
326-326:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
t.Fatalfinstead oft.Fatalfor formatted output.
t.Fataldoes not support format verbs like%w. The error will be printed literally as%winstead of being formatted.🐛 Proposed fix
- t.Fatal("failed to get azure encryption key info: %w", err) + t.Fatalf("failed to get azure encryption key info: %v", err)🤖 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 `@test/e2e/create_cluster_test.go` at line 326, Replace the t.Fatal call that uses a format verb with t.Fatalf and use a proper verb for printing the error (e.g., change t.Fatal("failed to get azure encryption key info: %w", err) to t.Fatalf("failed to get azure encryption key info: %v", err)); locate the t.Fatal in the test where the azure encryption key info is fetched and update the call to t.Fatalf with %v (or %s) to ensure the error is formatted correctly.
🧹 Nitpick comments (2)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go (1)
167-191: ⚡ Quick winAdd a v1 existing-config test case for Azure parity.
At Lines 167-191, Azure coverage only asserts “no existing config” and “existing v2”. Adding an “existing v1 config preserves v1” case (as done in the AWS test) would close a regression gap in version-selection behavior.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go` around lines 167 - 191, Add a new testCase to the testCases slice that mirrors the AWS "preserve v1" scenario: create an existing v1.EncryptionConfiguration (TypeMeta APIVersion "apiserver.config.k8s.io/v1") whose Resources use config.KMSEncryptedObjects() and a Provider with KMS APIVersion "v1", and set expectedConfig to generateExpectedAzureEncryptionConfig(kmsAPIVersionV1) so the test asserts the controller preserves v1 when an existing v1 KMS config is present (follow the pattern used by the existing v2 case and the AWS test).control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go (1)
194-194: 💤 Low value
t.Fatalcaptures outer test'st, not the subtest's.The check function closure captures
tfromTestGenerateKMSPodConfig_SelfManaged, but when executed insidet.Run, the subtest shadowst. Using the outertmeans failures may not be correctly associated with the subtest in test output.Consider using Gomega's failure mechanism instead:
♻️ Suggested fix
- t.Fatal("active KMS container not found") + g.Fail("active KMS container not found")Apply the same fix to line 213 and line 294.
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go` at line 194, In TestGenerateKMSPodConfig_SelfManaged the closure uses t.Fatal which captures the outer test's t and misattributes failures; replace these t.Fatal calls (e.g., the "active KMS container not found" check inside the check closure and the similar checks later) with Gomega assertions: create a Gomega instance with g := NewWithT(t) in the subtest (or inside the closure) and use g.Expect(found).To(BeTrue(), "active KMS container not found") (and similar messages for the other two occurrences). Apply the same replacement for the other two t.Fatal usages referenced in the comment so failures are reported on the subtest.
🤖 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 `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Line 530: The CEL XValidation currently references a non-existent field
workloadIdentities.kms; update the rule to check the correct KMS path for
WorkloadIdentities by replacing
has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms) with
has(self.secretEncryption.kms.azure.selfManagedKMS). Keep the rest of the rule
logic the same so the full expression uses
self.secretEncryption.kms.azure.selfManagedKMS and the relevant symbols
AzureWorkloadIdentities, secretEncryption.kms.azure.selfManagedKMS, and
platform.azure.azureAuthenticationConfig.workloadIdentities to validate that
WorkloadIdentities auth requires the self-managed Azure KMS config.
In `@cmd/cluster/azure/create.go`:
- Around line 394-405: The current branch uses only o.infra.KMSClientID to
decide between SelfManagedKMS and ManagedIdentity, which can silently
mis-classify workload-identity clusters; change the logic to explicitly detect a
workload-identity Azure cluster (check the infra workload-identity flag on the
Azure platform spec, e.g. a field like o.infra.Platform.Azure.WorkloadIdentity
or o.infra.WorkloadIdentity) and if the cluster is workload-identity but
o.infra.KMSClientID is empty return an error rather than creating a
ManagedIdentity; otherwise keep the existing behavior (set
azureKMSSpec.SelfManagedKMS when KMSClientID is present, set azureKMSSpec.KMS
with o.KMSUserAssignedCredsSecretName and ObjectEncoding when not
workload-identity).
In `@cmd/infra/azure/identities.go`:
- Around line 371-373: GetWorkloadIdentityDefinitions is being called with a
WorkloadIdentityOptions that omits Topology, so controlPlaneOperator identities
are always included; update the call to pass the requested topology from the
incoming opts into WorkloadIdentityOptions.Topology (e.g., set Topology:
opts.Topology) alongside IncludeKMS so GetWorkloadIdentityDefinitions receives
the real cluster topology and selects identities correctly.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go`:
- Around line 250-255: The helper generateExpectedAzureEncryptionConfig
currently discards the error from util.HashStruct; change its signature to
accept testing.TB (e.g., func generateExpectedAzureEncryptionConfig(t
testing.TB, apiVersion string) *v1.EncryptionConfiguration), call t.Helper(),
call util.HashStruct(...) and check the returned error, and call t.Fatalf(...)
(or t.Fatalf with the error) if hashing fails so tests fail fast; update every
call site in this test to pass the test variable (t) and add testing to imports
as needed.
In `@docs/content/how-to/azure/azure-workload-identity-setup.md`:
- Around line 67-76: The KMS example command for "hypershift create iam azure"
is missing the --location flag; update the snippet that calls hypershift create
iam azure (the KMS example that outputs workload-identities.json and uses
--enable-kms) to include --location $LOCATION (or the appropriate location
variable) so it matches the non-KMS example and avoids relying on defaults.
In `@docs/content/how-to/azure/create-self-managed-azure-cluster.md`:
- Around line 281-290: The Azure IAM creation example is missing the --location
flag; update the hypershift command invocation (the multi-line shell command
starting with "hypershift create iam azure") to include the --location
"$LOCATION" argument so it matches other examples and ensures location is
explicitly specified when creating the IAM resources.
---
Outside diff comments:
In `@test/e2e/create_cluster_test.go`:
- Line 326: Replace the t.Fatal call that uses a format verb with t.Fatalf and
use a proper verb for printing the error (e.g., change t.Fatal("failed to get
azure encryption key info: %w", err) to t.Fatalf("failed to get azure encryption
key info: %v", err)); locate the t.Fatal in the test where the azure encryption
key info is fetched and update the call to t.Fatalf with %v (or %s) to ensure
the error is formatted correctly.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go`:
- Line 194: In TestGenerateKMSPodConfig_SelfManaged the closure uses t.Fatal
which captures the outer test's t and misattributes failures; replace these
t.Fatal calls (e.g., the "active KMS container not found" check inside the check
closure and the similar checks later) with Gomega assertions: create a Gomega
instance with g := NewWithT(t) in the subtest (or inside the closure) and use
g.Expect(found).To(BeTrue(), "active KMS container not found") (and similar
messages for the other two occurrences). Apply the same replacement for the
other two t.Fatal usages referenced in the comment so failures are reported on
the subtest.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go`:
- Around line 167-191: Add a new testCase to the testCases slice that mirrors
the AWS "preserve v1" scenario: create an existing v1.EncryptionConfiguration
(TypeMeta APIVersion "apiserver.config.k8s.io/v1") whose Resources use
config.KMSEncryptedObjects() and a Provider with KMS APIVersion "v1", and set
expectedConfig to generateExpectedAzureEncryptionConfig(kmsAPIVersionV1) so the
test asserts the controller preserves v1 when an existing v1 KMS config is
present (follow the pattern used by the existing v2 case and the AWS test).
🪄 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: 9a1a6096-d47a-46e4-91ce-fc49a46dbb2a
⛔ Files ignored due to path filters (43)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azurekmsspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/selfmanagedazurekms.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/featuregated.hostedclusters.kms.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_managed_azure_kms_secretproviderclass.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (28)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/hostedcluster_types.gocmd/cluster/azure/create.gocmd/infra/azure/create.gocmd/infra/azure/create_iam.gocmd/infra/azure/identities.gocmd/infra/azure/identities_test.gocmd/infra/azure/types.gocmd/util/azure_flag_descriptions.gocontrol-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/kas.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.godocs/content/how-to/azure/azure-workload-identity-setup.mddocs/content/how-to/azure/create-self-managed-azure-cluster.mdhypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.gosupport/azureutil/azureutil.gosupport/config/constants.gosupport/controlplane-component/token-minter-container.gosupport/controlplane-component/token-minter-container_test.gotest/e2e/create_cluster_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go (1)
128-128: ⚡ Quick winUse the subtest handle inside the table callbacks.
These
checkclosures capture the parentt, sot.Fatal(...)here reports against the outer test instead of the currentt.Run(...)case and gets fragile if these subtests are ever parallelized. Pass*testing.Tintocheck, or stick to Gomega expectations inside the callback.Also applies to: 202-203, 221-222, 255-255, 302-302
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go` at line 128, The table-driven tests define a closure type check func(g Gomega, podConfig *KMSPodConfig) that captures the outer testing.T, so calls like t.Fatal() inside those closures point at the parent test rather than the subtest; change the signature to accept the subtest handle (e.g., check func(t *testing.T, g Gomega, podConfig *KMSPodConfig)) and update each table callback and its invocations to pass the local *testing.T from t.Run(...); alternatively, remove direct t.Fatal usage from the closures and use only Gomega expectations (g.Expect(...).To(...)) so failures are reported against the current subtest—apply this change consistently to the check declarations and all places that define or call check (the check closure and its table entries).
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go`:
- Around line 291-298: The test only asserts AZURE_CLIENT_ID is absent in the
managed KMS container; update the check inside the check func that iterates
KMSPodConfig.Containers (finding container Name "azure-kms-provider-active") to
assert that AZURE_CLIENT_ID, AZURE_TENANT_ID, and AZURE_FEDERATED_TOKEN_FILE are
all not present (use Gomega.Expect on e.Name for each of these three names) so
managed mode cannot accidentally leave any workload-identity env var.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go`:
- Line 128: The table-driven tests define a closure type check func(g Gomega,
podConfig *KMSPodConfig) that captures the outer testing.T, so calls like
t.Fatal() inside those closures point at the parent test rather than the
subtest; change the signature to accept the subtest handle (e.g., check func(t
*testing.T, g Gomega, podConfig *KMSPodConfig)) and update each table callback
and its invocations to pass the local *testing.T from t.Run(...); alternatively,
remove direct t.Fatal usage from the closures and use only Gomega expectations
(g.Expect(...).To(...)) so failures are reported against the current
subtest—apply this change consistently to the check declarations and all places
that define or call check (the check closure and its table entries).
🪄 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: a63c2d2c-0fb9-4dad-b3af-c06cdb5363b4
📒 Files selected for processing (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/hypershift/v1beta1/azure.go (1)
849-857: 💤 Low valueInconsistent JSON tag for required field.
ClientIDis marked+requiredbut usesjson:"clientID,omitempty", which allows the field to be omitted during JSON marshaling when empty. For required fields, the JSON tag should typically not includeomitemptyto ensure the field is always present in serialized output.📝 Proposed fix
// SelfManagedAzureKMS contains the identity used to authenticate with Azure KMS on self-managed clusters. type SelfManagedAzureKMS struct { // clientID is the client ID of a federated managed identity used for Azure Key Vault KMS encryption // in self-managed Azure clusters. This identity must have "Key Vault Crypto User" role on the // Key Vault containing the encryption key. // // +required - ClientID AzureClientID `json:"clientID,omitempty"` + ClientID AzureClientID `json:"clientID"` }🤖 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 `@api/hypershift/v1beta1/azure.go` around lines 849 - 857, The ClientID field on SelfManagedAzureKMS is declared required but its struct tag uses json:"clientID,omitempty" which allows omission; update the json tag for ClientID (type AzureClientID) to remove "omitempty" so it is serialized as json:"clientID" to reflect the +required annotation and ensure the field is always present in JSON output.hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (1)
487-495: 💤 Low valueTest case may represent invalid API state.
This test case uses
baseHC()which has neitherkmsnorselfManagedKMSset inSecretEncryption.KMS.Azure. However, the API's XValidation rule at line 814 inazure.gorequires "one of kms or selfManagedKMS must be set".This test may be validating defensive behavior in
reconcileKMSConfigSecret, but it's worth confirming this scenario is intentional and documenting it in the test name/comment (e.g., "When self-managed Azure without KMS identity configured (edge case)").🤖 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` around lines 487 - 495, The test uses baseHC() which violates the XValidation in azure.go (one of kms or selfManagedKMS must be set); either make the test input valid by updating the HostCluster produced by baseHC() to set SecretEncryption.KMS.Azure.selfManagedKMS or kms appropriately (so reconcileKMSConfigSecret sees a valid KMS config), or explicitly mark the test as an intentional invalid/edge-case by renaming the test ("...edge case" / add comment) and keeping the existing input — reference baseHC(), SecretEncryption.KMS.Azure, selfManagedKMS/kms, and reconcileKMSConfigSecret when making the change.
🤖 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.
Nitpick comments:
In `@api/hypershift/v1beta1/azure.go`:
- Around line 849-857: The ClientID field on SelfManagedAzureKMS is declared
required but its struct tag uses json:"clientID,omitempty" which allows
omission; update the json tag for ClientID (type AzureClientID) to remove
"omitempty" so it is serialized as json:"clientID" to reflect the +required
annotation and ensure the field is always present in JSON output.
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Around line 487-495: The test uses baseHC() which violates the XValidation in
azure.go (one of kms or selfManagedKMS must be set); either make the test input
valid by updating the HostCluster produced by baseHC() to set
SecretEncryption.KMS.Azure.selfManagedKMS or kms appropriately (so
reconcileKMSConfigSecret sees a valid KMS config), or explicitly mark the test
as an intentional invalid/edge-case by renaming the test ("...edge case" / add
comment) and keeping the existing input — reference baseHC(),
SecretEncryption.KMS.Azure, selfManagedKMS/kms, and reconcileKMSConfigSecret
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 79106841-200b-43fc-99c6-3ebe3ad8d7d6
⛔ Files ignored due to path filters (43)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azurekmsspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/selfmanagedazurekms.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/featuregated.hostedclusters.kms.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_managed_azure_kms_secretproviderclass.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (28)
api/hypershift/v1beta1/azure.goapi/hypershift/v1beta1/hostedcluster_types.gocmd/cluster/azure/create.gocmd/infra/azure/create.gocmd/infra/azure/create_iam.gocmd/infra/azure/identities.gocmd/infra/azure/identities_test.gocmd/infra/azure/types.gocmd/util/azure_flag_descriptions.gocontrol-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/kas.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.godocs/content/how-to/azure/azure-workload-identity-setup.mddocs/content/how-to/azure/create-self-managed-azure-cluster.mdhypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.gosupport/azureutil/azureutil.gosupport/config/constants.gosupport/controlplane-component/token-minter-container.gosupport/controlplane-component/token-minter-container_test.gotest/e2e/create_cluster_test.go
✅ Files skipped from review due to trivial changes (10)
- cmd/util/azure_flag_descriptions.go
- control-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
- support/controlplane-component/token-minter-container.go
- hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms.go
- cmd/infra/azure/create_iam.go
- cmd/cluster/azure/create.go
- cmd/infra/azure/identities.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cmd/infra/azure/create.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws.go
- support/config/constants.go
- support/azureutil/azureutil.go
- cmd/infra/azure/types.go
- api/hypershift/v1beta1/hostedcluster_types.go
- control-plane-operator/controllers/hostedcontrolplane/manifests/kas.go
- test/e2e/create_cluster_test.go
- cmd/infra/azure/identities_test.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
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
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go`:
- Around line 326-351: The test "When self-managed backup key is specified..."
is missing an assertion that the self-managed backup container includes the
workload identity env var; update the test that creates provider via
NewAzureKMSProvider(..., AzureKMSProviderOptions{IsSelfManaged: true, ...}) and
calls provider.GenerateKMSPodConfig() to also assert that the container named
"azure-kms-provider-backup" has an env var "AZURE_FEDERATED_TOKEN_FILE" (e.g.,
add g.Expect(envMap).To(HaveKey("AZURE_FEDERATED_TOKEN_FILE")) or equivalent)
alongside the existing AZURE_CLIENT_ID and AZURE_TENANT_ID checks.
🪄 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: 6f0668f7-58fc-46fd-b431-e87da9dc6e2a
⛔ Files ignored due to path filters (18)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.kms.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.md
📒 Files selected for processing (6)
api/hypershift/v1beta1/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.godocs/content/how-to/azure/azure-workload-identity-setup.mddocs/content/how-to/azure/create-self-managed-azure-cluster.mdtest/e2e/create_cluster_test.go
✅ Files skipped from review due to trivial changes (2)
- test/e2e/create_cluster_test.go
- api/hypershift/v1beta1/azure.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure.go`:
- Around line 78-83: The constructor NewAzureKMSProvider currently validates
kmsSpec and client/tenant IDs for opts.IsSelfManaged but does not check
opts.TokenMinterImage; add a validation that when opts.IsSelfManaged is true
then opts.TokenMinterImage must be non-empty and return a clear error (e.g.,
"tokenMinterImage is required for self-managed Azure KMS") so the provider fails
fast instead of rendering a pod with an invalid sidecar image; update the
validation block that checks opts.IsSelfManaged (referencing
NewAzureKMSProvider, opts.IsSelfManaged, and opts.TokenMinterImage) to perform
this check and return an error when missing.
In `@docs/content/how-to/azure/create-self-managed-azure-cluster.md`:
- Around line 279-290: The snippet uses --infra-id "$INFRA_ID" but INFRA_ID is
never defined; add a preceding step that defines/exports INFRA_ID (the cluster
infra identifier used by Hypershift) before the hypershift create iam azure
command so copy-paste works—e.g., insert a short instruction to set INFRA_ID to
your cluster's infra ID and reference that variable before running hypershift
create iam azure (the command shown) so the generated ./workload-identities.json
and subsequent KMS identity lookup are accurate.
In
`@hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go`:
- Around line 488-494: Remove the table-driven test case whose name is "When
self-managed Azure without workload identities it should not set identity
fields" (the entry that sets hc: baseHC() and a validate func) because it
constructs an AzureKMSSpec with neither kms nor selfManagedKMS which is now
rejected by the API; delete that case from the test table and replace it with a
negative test that builds the same AzureKMSSpec and asserts validation/creation
fails (i.e., expect an error) or simply remove the entry entirely so tests only
exercise valid API shapes; reference AzureKMSSpec, baseHC(), and the validate
closure to locate the code to change.
🪄 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: a3872fcd-2af6-4cdb-8fb6-e22dbe00b3a1
⛔ Files ignored due to path filters (42)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/azurekmsspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/selfmanagedazurekms.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.kms.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_managed_azure_kms_secretproviderclass.yamlis excluded by!**/testdata/**docs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (27)
api/hypershift/v1beta1/azure.gocmd/cluster/azure/create.gocmd/infra/azure/create.gocmd/infra/azure/create_iam.gocmd/infra/azure/identities.gocmd/infra/azure/identities_test.gocmd/infra/azure/types.gocmd/util/azure_flag_descriptions.gocontrol-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/kas.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.godocs/content/how-to/azure/azure-workload-identity-setup.mddocs/content/how-to/azure/create-self-managed-azure-cluster.mdhypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.gosupport/azureutil/azureutil.gosupport/config/constants.gosupport/controlplane-component/token-minter-container.gosupport/controlplane-component/token-minter-container_test.gotest/e2e/create_cluster_test.go
✅ Files skipped from review due to trivial changes (12)
- control-plane-operator/controllers/hostedcontrolplane/cloud/azure/providerconfig.go
- support/config/constants.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/aws.go
- cmd/cluster/azure/create.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- support/controlplane-component/token-minter-container.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms.go
- support/controlplane-component/token-minter-container_test.go
- cmd/infra/azure/create_iam.go
- hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cmd/infra/azure/types.go
- support/azureutil/azureutil.go
- cmd/infra/azure/identities_test.go
- cmd/infra/azure/identities.go
- test/e2e/create_cluster_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/secretencryption_test.go
- cmd/infra/azure/create.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms/azure_test.go
- Add SelfManagedAzureKMS type with ClientID for workload identity auth - Add SelfManagedKMS field to AzureKMSSpec for self-managed clusters - Add CEL validation: kms and selfManagedKMS are mutually exclusive - Add CEL validation: kms is immutable (cannot switch between modes) - Add UseWorkloadIdentityExtension field to AzureConfig for CPO Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add --kms-key-id flag to parse Azure Key Vault URI into vault/key/version - Add --kms-client-id flag for workload identity authentication - Wire SelfManagedKMS into HostedCluster spec during cluster creation - Add KMS credential helpers to create_iam infrastructure setup - Add envtest cases for CEL validation of kms/selfManagedKMS mutual exclusivity and immutability Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…ed clusters - Add KMS config secret reconciliation for self-managed Azure with federated workload identity fields - Add validation that one of kms or selfManagedKMS credentials must be configured, returning error when neither is set - Add IsSelfManagedAzureWithWorkloadIdentity helper in azureutil - Add token-minter sidecar container builder for Azure KMS workload identity token projection - Add constants for Azure KMS service account and token paths - Add tests for credential reconciliation, KMS config, and self-managed Azure workload identity detection Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…naged clusters - Add self-managed Azure KMS provider with token-minter sidecar for workload identity token projection - Wire KMSClientID and TenantID into Azure KMS provider options when self-managed KMS is configured - Add UseWorkloadIdentityExtension field to Azure cloud config for federated credential authentication - Add KMS service account manifest for token projection - Update KAS deployment to mount token-minter sidecar volumes and configure Azure KMS socket paths - Add comprehensive tests for Azure KMS provider, encryption config reconciliation, and self-managed credential flow Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add e2e test for Azure KMS encryption on self-managed clusters with workload identity authentication - Fix t.Fatal format verb to use t.Fatalf with %v Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add KMS encryption setup section to self-managed Azure cluster creation guide with key vault and key creation steps - Add KMS workload identity setup instructions for federated credential configuration - Update API reference with new SelfManagedAzureKMS type and UseWorkloadIdentityExtension field documentation Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
I now have the complete root cause analysis. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll envtest jobs across both workflows ( Root CauseThe PR adds three new
The third rule is the most expensive: transition rules with Evidence of the hang pattern (identical across all 11 matrix jobs):
The Makefile's test timeout ( These same envtest workflows pass consistently on main (5 recent runs all Recommendations
Evidence
|
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
2 participants
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.
What this PR does / why we need it:
Enables Azure Key Vault KMS encryption (etcd encryption at rest) for self-managed Azure HyperShift clusters using workload identity federation, without breaking the existing ARO HCP (managed Azure) KMS path that uses managed identities with CSI secret store volumes.
Key Changes
API: Added
SelfManagedKMSfield (typeSelfManagedAzureKMS) toAzureKMSSpecwith aClientIDfor the workload identity that hasKey Vault Crypto Userrole on the Key Vault. CEL validation rules enforce mutual exclusivity betweenkms(managed) andselfManagedKMS(self-managed), and immutability once set.Control Plane Operator:
UseWorkloadIdentityExtension) for self-managedHyperShift Operator:
CLI:
create cluster azure: Only setsManagedIdentityKMS creds for managed Azure; self-managed usesAzureKMSSpec.SelfManagedKMScreate iam azure: Creates KMS workload identity with federated credential forkms-providerservice accountE2E: Updated
TestCreateClusterCustomConfigto handle self-managed Azure KMS assertions. Added envtest coverage for CEL validation rules (mutual exclusivity, immutability).Documentation: Added KMS encryption section to self-managed Azure cluster guide with Key Vault setup and workload identity federation instructions.
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3070
Special notes for your reviewer:
The self-managed Azure KMS authentication pattern follows the same approach used by Cloud Controller Manager (CCM) and Azure CSI storage drivers, which already support self-managed Azure with workload identity federation.
The token-minter sidecar mints OIDC tokens for the
kms-providerservice account inkube-systemnamespace, matching the pattern used by AWS KMS.kmsandselfManagedKMSare mutually exclusive and immutable once set — switching between managed and self-managed KMS auth after cluster creation is not supported.Checklist: