Skip to content

Commit 98156ae

Browse files
bryan-coxbryan-cox
committed
fix: address CodeRabbit review feedback on Azure KMS PR
- Remove overly restrictive top-level CEL rule that required selfManagedKMS for all WorkloadIdentities + Azure KMS combos. The AzureKMSSpec-level rules already enforce mutual exclusivity and "at least one of kms or selfManagedKMS must be set". - Fix +required with omitempty contradiction on SelfManagedAzureKMS.ClientID - Fix t.Fatal -> t.Fatalf with format verb in e2e test - Fix t.Fatal in closure -> gomega assertion in azure_test.go - Add HashStruct error handling in secretencryption_test.go - Add v1 Azure encryption config test case for parity with AWS - Add missing --location flag in docs examples - Rename envtest suite from featuregated to stable (KMS is not gated) - Add all required AzureWorkloadIdentities fields in envtest YAML Signed-off-by: Bryan Cox <brcox@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
1 parent 559fe15 commit 98156ae

26 files changed

Lines changed: 86 additions & 113 deletions

‎api/hypershift/v1beta1/azure.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -853,7 +853,7 @@ type SelfManagedAzureKMS struct {
853853
// Key Vault containing the encryption key.
854854
//
855855
// +required
856-
ClientID AzureClientID `json:"clientID,omitempty"`
856+
ClientID AzureClientID `json:"clientID"`
857857
}
858858

859859
type AzureKMSKey struct {

‎api/hypershift/v1beta1/hostedcluster_types.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -527,7 +527,6 @@ type Capabilities struct {
527527
// +kubebuilder:validation:XValidation:rule=`!self.services.exists(s, s.service == 'APIServer' && has(s.servicePublishingStrategy.loadBalancer) && s.servicePublishingStrategy.loadBalancer.hostname != "" && has(self.configuration) && has(self.configuration.apiServer) && self.configuration.apiServer.servingCerts.namedCertificates.exists(cert, cert.names.exists(n, n == s.servicePublishingStrategy.loadBalancer.hostname)))`, message="APIServer loadBalancer hostname cannot be in ClusterConfiguration.apiserver.servingCerts.namedCertificates[]"
528528
// +kubebuilder:validation:XValidation:rule="!has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork) || !self.operatorConfiguration.clusterNetworkOperator.disableMultiNetwork || self.networking.networkType == 'Other'",message="disableMultiNetwork can only be set to true when networkType is 'Other'"
529529
// +kubebuilder:validation:XValidation:rule="self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration) || !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)", message="ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes"
530-
// +kubebuilder:validation:XValidation:rule="!has(self.secretEncryption) || !has(self.secretEncryption.kms) || self.secretEncryption.kms.provider != 'Azure' || self.platform.type != 'Azure' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType != 'WorkloadIdentities' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)",message="workloadIdentities.kms is required when Azure KMS encryption is configured with WorkloadIdentities authentication"
531530
type HostedClusterSpec struct {
532531
// release specifies the desired OCP release payload for all the hosted cluster components.
533532
// This includes those components running management side like the Kube API Server and the CVO but also the operands which land in the hosted cluster data plane like the ingress controller, ovn agents, etc.

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6530,12 +6530,6 @@ spec:
65306530
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
65316531
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
65326532
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6533-
- message: workloadIdentities.kms is required when Azure KMS encryption
6534-
is configured with WorkloadIdentities authentication
6535-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6536-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6537-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6538-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
65396533
status:
65406534
description: status is the latest observed status of the HostedCluster.
65416535
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6513,12 +6513,6 @@ spec:
65136513
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
65146514
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
65156515
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6516-
- message: workloadIdentities.kms is required when Azure KMS encryption
6517-
is configured with WorkloadIdentities authentication
6518-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6519-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6520-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6521-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
65226516
status:
65236517
description: status is the latest observed status of the HostedCluster.
65246518
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6533,12 +6533,6 @@ spec:
65336533
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
65346534
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
65356535
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6536-
- message: workloadIdentities.kms is required when Azure KMS encryption
6537-
is configured with WorkloadIdentities authentication
6538-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6539-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6540-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6541-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
65426536
status:
65436537
description: status is the latest observed status of the HostedCluster.
65446538
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6845,12 +6845,6 @@ spec:
68456845
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
68466846
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
68476847
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6848-
- message: workloadIdentities.kms is required when Azure KMS encryption
6849-
is configured with WorkloadIdentities authentication
6850-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6851-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6852-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6853-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
68546848
status:
68556849
description: status is the latest observed status of the HostedCluster.
68566850
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6985,12 +6985,6 @@ spec:
69856985
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
69866986
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
69876987
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6988-
- message: workloadIdentities.kms is required when Azure KMS encryption
6989-
is configured with WorkloadIdentities authentication
6990-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6991-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6992-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6993-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
69946988
status:
69956989
description: status is the latest observed status of the HostedCluster.
69966990
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6976,12 +6976,6 @@ spec:
69766976
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
69776977
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
69786978
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6979-
- message: workloadIdentities.kms is required when Azure KMS encryption
6980-
is configured with WorkloadIdentities authentication
6981-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6982-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6983-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6984-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
69856979
status:
69866980
description: status is the latest observed status of the HostedCluster.
69876981
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6959,12 +6959,6 @@ spec:
69596959
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
69606960
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
69616961
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6962-
- message: workloadIdentities.kms is required when Azure KMS encryption
6963-
is configured with WorkloadIdentities authentication
6964-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6965-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6966-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6967-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
69686962
status:
69696963
description: status is the latest observed status of the HostedCluster.
69706964
properties:

‎api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yaml‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6578,12 +6578,6 @@ spec:
65786578
- message: ovnKubernetesConfig is forbidden when networkType is not OVNKubernetes
65796579
rule: self.networking.networkType == 'OVNKubernetes' || !has(self.operatorConfiguration)
65806580
|| !has(self.operatorConfiguration.clusterNetworkOperator) || !has(self.operatorConfiguration.clusterNetworkOperator.ovnKubernetesConfig)
6581-
- message: workloadIdentities.kms is required when Azure KMS encryption
6582-
is configured with WorkloadIdentities authentication
6583-
rule: '!has(self.secretEncryption) || !has(self.secretEncryption.kms)
6584-
|| self.secretEncryption.kms.provider != ''Azure'' || self.platform.type
6585-
!= ''Azure'' || !has(self.platform.azure) || self.platform.azure.azureAuthenticationConfig.azureAuthenticationConfigType
6586-
!= ''WorkloadIdentities'' || has(self.platform.azure.azureAuthenticationConfig.workloadIdentities.kms)'
65876581
status:
65886582
description: status is the latest observed status of the HostedCluster.
65896583
properties:

0 commit comments

Comments
 (0)