CNTRLPLANE-3070: Support KMS on self-managed Azure without affecting ARO HCP#8088
CNTRLPLANE-3070: Support KMS on self-managed Azure without affecting ARO HCP#8088bryan-cox wants to merge 7 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
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
|
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. |
|
[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
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 40.00% 40.24% +0.24%
==========================================
Files 751 751
Lines 92838 93012 +174
==========================================
+ Hits 37137 37431 +294
+ Misses 53014 52885 -129
- Partials 2687 2696 +9
... and 1 file with indirect coverage changes
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
openshift-ci
Bot
removed
the
needs-rebase
![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
🤖 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
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
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
3 times, most recently
from
1facc17 to
781113e
Compare
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
from
781113e to
58054b7
Compare
|
I now have all the evidence needed. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll six Root CauseThe PR introduces a new envtest test suite file The envtest framework (
This adds 3 additional install/uninstall cycles to the existing ~30 cycles from the 11 pre-existing test suites. The HostedCluster CRD is one of the largest and most complex CRDs in the project (526KB with extensive CEL validation), making each install/uninstall cycle expensive. The combined effect pushes total test execution from ~403s on The CRD schema changes themselves (new Recommendations
Evidence
|
Add SelfManagedAzureKMS struct and refactor AzureKMSSpec to support both ARO HCP (managed) and self-managed KMS authentication paths: - SelfManagedAzureKMS with ClientID and TenantID for workload identity - CEL validation ensuring managed KMS uses KeyVaultResourceGroup while self-managed KMS uses ClientID + TenantID - Both paths share KeyVaultName, KeyName, and KeyVersion fields 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 --enable-kms flag to create/destroy cluster and create iam commands - Create kms managed identity with federated credentials when KMS enabled - Populate AzureKMSSpec.SelfManaged with ClientID and TenantID - Add KMS Key Vault and key configuration flags with validation - Wire KMS identity through infra and IAM output JSON files Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…ed clusters - Propagate SelfManaged KMS config from HostedCluster to HostedControlPlane - Add KMS service account to token-minter with azure-kms audience - Add AzureKMSClientID constant and helper for KMS audience generation - Add IsAzureSelfManagedKMS utility for detecting self-managed KMS config Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
…naged clusters - Add self-managed Azure KMS provider using workload identity federation with projected service account tokens instead of MSI - Mount KMS token via token-minter sidecar with azure-kms audience - SecretProviderClass uses workload identity env vars (CLIENT_ID, TENANT_ID, TOKEN_FILE) instead of MSI for Key Vault access - Refactor KMS volume/mount helpers to support both managed and self-managed authentication paths - Update ARO HCP fixture for renamed KMS SecretProviderClass field Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add e2e test verifying KMS encryption on self-managed Azure clusters: creates cluster with --enable-kms, validates KAS has KMS encryption provider configured, and verifies etcd data is encrypted - Add envtest CEL validation test suite for AzureKMSSpec ensuring mutual exclusivity between managed and self-managed KMS fields Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
- Add KMS section to self-managed Azure cluster creation guide with Key Vault setup, key creation, and access policy configuration - Document --enable-kms flag and KMS-related CLI parameters - Add KMS identity setup to workload identity documentation - Regenerate API reference and aggregated docs Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
/test e2e-aks |
|
/test e2e-azure-self-managed |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
bryan-cox
force-pushed
the
CNTRLPLANE-3070
branch
from
c74d21b to
802baac
Compare
|
I verified this works and the report verification report is located here - https://redhat.atlassian.net/browse/CNTRLPLANE-3070?focusedCommentId=16980328 |
|
@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. |
2 participants
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: