CNTRLPLANE-1331: Get self-managed Azure working again - #6703
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.20.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
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds a new Network workload identity across APIs and CRDs, updates codegen, and vendors. Introduces CLI/infra support for workload identities files and role assignment. Adapts control plane (cloud-controller-manager) and storage (CSI) to federated identity with token-minter. Implements Azure credential reconciliation utilities and integrates them into operators. Updates docs and examples; adds tests and fixtures. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.20.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
area/hypershift-operator
b197a66 to
857fa6d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.21.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. |
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.21.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. |
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.21.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. |
1 similar comment
|
@bryan-cox: This pull request references CNTRLPLANE-1331 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.21.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}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.go (2)
394-394: Kubebuilder marker typo disables validation generationThe marker uses "kubeubilder" instead of "kubebuilder", so this XValidation won’t be applied in the generated CRD.
-// +kubeubilder:validation:XValidation:rule="self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionId in the encryptionSetID must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12" +// +kubebuilder:validation:XValidation:rule="self.split('/')[2].matches('^[{]?[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}[}]?$')",message="the subscriptionId in the encryptionSetID must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12"
629-633: Controller name mismatch in docs (“file” identity)The “file” identity is described as associated with the azure-disk-controller; it should be the Azure File CSI controller.
-// file is a pre-existing managed identity associated with the azure-disk-controller. +// file is a pre-existing managed identity associated with the azure-file-controller.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
4116-4123: Adding "network" to required breaks backward compatibility for existing HostedClusters using WorkloadIdentitiesMarking
networkas required will cause updates to fail admission for existing objects that don’t yet set it. If this is intentional, please document the migration path and ensure all test fixtures and examples include it. Otherwise, consider making it optional initially and enforcing via controllers, then flipping to required in a later minor with a deprecation window.Proposed schema change (make it optional for now):
required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagementOptionally add a soft validation to nudge users without hard-failing existing clusters (can be removed when you later make it required):
type: object + x-kubernetes-validations: + - message: "Azure Workload Identities: 'network' clientID will be required in a future release" + rule: has(self.network)cmd/infra/azure/create.go (1)
70-75: Wire the new option into the CLIWorkloadIdentitiesFile is added to options but there’s no flag, so the feature is unreachable.
Add a flag in NewCreateCommand():
@@ cmd.Flags().StringVar(&opts.SubnetID, "subnet-id", opts.SubnetID, "The subnet ID where the VMs will be placed.") cmd.Flags().StringVar(&opts.RHCOSImage, "rhcos-image", opts.RHCOSImage, `RHCOS image to be used for the NodePool. Could be obtained using podman run --rm -it --entrypoint cat $RELEASE_IMAGE release-manifests/0000_50_installer_coreos-bootimages.yaml | yq .data.stream -r | yq '.architectures.x86_64["rhel-coreos-extensions"]["azure-disk"].url'`) cmd.Flags().StringToStringVarP(&opts.ResourceGroupTags, "resource-group-tags", "t", opts.ResourceGroupTags, "Additional tags to apply to the resource group created (e.g. 'key1=value1,key2=value2')") + cmd.Flags().StringVar(&opts.WorkloadIdentitiesFile, "workload-identities-file", opts.WorkloadIdentitiesFile, "Path to a JSON/YAML file defining Azure workload identities; with --assign-service-principal-roles, roles are assigned for each identity.")cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml (1)
4615-4622: Removenetworkfrom required fields under.spec.platform.azure.azureAuthenticationConfig.workloadIdentities
Revert to:required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagement(cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml Lines 4619–4622)
If enforcing in 4.20, add admission defaulting or migration and document manual upgrade steps.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
4107-4113: Remove network from workloadIdentities.required to prevent breaking changeapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml @@ lines 4106-4113 required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagement
- Add an XValidation under
spec.platform.azure.azureAuthenticationConfigto requireworkloadIdentities.networkonly for new clusters, grandfathering existing ones:- message: workloadIdentities.network is required for new clusters using WorkloadIdentities; existing objects without it remain valid rule: | self.azureAuthenticationConfigType != 'WorkloadIdentities' || has(self.workloadIdentities) && ( has(self.workloadIdentities.network) || (has(oldSelf.workloadIdentities) && !has(oldSelf.workloadIdentities.network)) )- Verify all CRDs, controllers, and documentation/examples referencing
workloadIdentities.networkalign with this non-breaking change.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
3999-4005: Revert ‘network’ from the CRD’s required list to avoid breaking upgrades
Addingnetworkas a required field in this CRD will cause existing self-managed Azure HCPs (created before this change) to fail on future edits.• In the YAML at api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (lines 3999–4005), remove
– networkfromrequired:.
• Enforcenetworkat creation time via CLI or a validating webhook, and add controller logic that sets a clear Condition if it’s missing.
• Alternatively, gate the requirement behind a feature flag or CEL expression tied to a new opt-in field to preserve upgrade paths.
• Ensure you ship a release note/migration guide and update example manifests with the new network client ID.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
4102-4151: Critical: CRD defines workloadIdentities.network but Go API/codegen do not
- Evidence: CRD manifests include workloadIdentities.network (api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml:3989); the Go type AzureWorkloadIdentities is in api/hypershift/v1beta1/azure.go (type AzureWorkloadIdentities ~line 493) and the generated deepcopy (api/hypershift/v1beta1/zz_generated.deepcopy.go:750–755) does not copy a Network field; a vendored deepcopy does include Network (vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go:758).
- Action: add a Network WorkloadIdentity field to AzureWorkloadIdentities (api/hypershift/v1beta1/azure.go) with json:"network", regenerate deepcopy/CRDs (controller-gen/codegen), and update controllers/operators and docs/examples to consume and document workloadIdentities.network.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (1)
4098-4105: Revert making workloadIdentities.network required — breaking API changeAdding "- network" to the CRD required list will reject existing HostedCluster objects at admission. Keep it optional and enforce via controller/feature-flag or provide a backwards-compatible conversion/defaulting path.
Locations:
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (lines 4098–4105) — CRD currently includes "- network".
- api/hypershift/v1beta1/azure.go (≈lines 525–527, 622–623) — AzureWorkloadIdentities / ManagedIdentity are marked +required.
- api/hypershift/v1beta1/openstack.go (line 78) — OpenStack.Network is defined as optional (
network,omitempty) — inconsistent with the CRD change.Proposed minimal, safe change:
required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagementapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
4118-4124: MakeworkloadIdentities.networkconditionally requiredRemove
networkfrom theworkloadIdentities.requiredlist in api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (lines 4118–4124), and alongside the existing x-kubernetes-validations under azureAuthenticationConfig, add:- message: workloadIdentities.network.clientID is required for WorkloadIdentities rule: self.azureAuthenticationConfigType == 'WorkloadIdentities' ? has(self.workloadIdentities.network) && has(self.workloadIdentities.network.clientID) : true
🧹 Nitpick comments (43)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/creds.go (1)
34-36: Reduce duplication: extract a tiny helper + constants for Azure secret names.Centralizing names avoids typos and makes future renames trivial. Also consider adding brief doc comments to exported funcs to satisfy linters.
-// Azure credential secrets for hosted cluster operators +// Azure credential secrets for hosted cluster operators. +// Shared constants keep names/Namespaces in one place. +const ( + nsIngressOperator = "openshift-ingress-operator" + nsImageRegistry = "openshift-image-registry" + nsCSIDrivers = "openshift-cluster-csi-drivers" + + secCloudCreds = "cloud-credentials" + secInstallerCloudCreds = "installer-cloud-credentials" + secAzureDiskCreds = "azure-disk-credentials" + secAzureFileCreds = "azure-file-credentials" +) + +// azureCredsSecret returns a Secret with only ObjectMeta populated. +func azureCredsSecret(ns, name string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + }, + } +}vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.go (2)
319-323: Typos: “Ephmeral” → “Ephemeral” in user-facing schema docsThese appear in CRD descriptions.
-// When set to Ephmeral, the OS disk will not be persisted to Azure storage and implies restrictions to the VM size and caching type. +// When set to Ephemeral, the OS disk will not be persisted to Azure storage and implies restrictions to the VM size and caching type. -// Ephmeral disks are primarily used for stateless applications, provide lower latency than Persistent disks and also incur no storage costs. +// Ephemeral disks are primarily used for stateless applications, provide lower latency than Persistent disks and also incur no storage costs.
105-105: Misleading validation message for AzureMarketplaceMessage references “RequiredMember” instead of the actual discriminator value.
-// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'AzureMarketplace' ? has(self.azureMarketplace) : !has(self.azureMarketplace)",message="azureMarketplace is required when type is RequiredMember, and forbidden otherwise" +// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'AzureMarketplace' ? has(self.azureMarketplace) : !has(self.azureMarketplace)",message="azureMarketplace is required when type is AzureMarketplace, and forbidden otherwise"cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml (1)
4728-4735: Making network required may block edits to existing clusters; confirm upgrade impact.Adding network to the required list is a backward-incompatible schema change for existing self-managed Azure HCPs that lack this field—they will fail future spec updates. If that’s acceptable for TechPreview, OK. Otherwise, consider staging this as optional for one release and enforce via controllers, or add migration docs.
If you choose to soften the rollout, drop network from required here:
required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagementThen enforce in code/admission (or via CEL) only for new clusters and document the migration path.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml (1)
3965-3985: Add format: uuid to clientID for better tooling hints.Schema already enforces UUID via pattern/length; adding format improves UX without changing validation.
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication + format: uuid maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringhypershift-operator/controllers/manifests/azure.go (2)
12-18: Set Secret.Type to Opaque.These constructors only set metadata; default Type is empty. Explicitly setting Opaque avoids downstream assumptions.
Apply:
return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Namespace: controlPlaneNamespace, Name: "cloud-network-config-controller-creds", }, + Type: corev1.SecretTypeOpaque, }Repeat the same addition for:
- "azure-cloud-config"
- "azure-ingress-credentials"
- "azure-image-registry-credentials"
- "azure-disk-csi-config"
- "azure-file-csi-config"
...same Type line for each function's returned Secret...Also applies to: 22-28, 32-38, 42-48, 52-58, 62-68
20-28: Rename for clarity or align name/content.Function name says “Credentials” but the Secret is “azure-cloud-config”. Consider renaming to AzureCloudProviderConfigSecret (or change Secret name) for consistency.
control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/component.go (1)
3-10: Use metav1.NamespaceSystem instead of string literal.Minor: prefer the constant for kube-system.
Apply:
import ( hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" component "github.com/openshift/hypershift/support/controlplane-component" "github.com/openshift/hypershift/support/util" appsv1 "k8s.io/api/apps/v1" rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ - ServiceAccountNameSpace: "kube-system", + ServiceAccountNameSpace: metav1.NamespaceSystem,Also applies to: 63-63
control-plane-operator/controllers/hostedcontrolplane/v2/assets/cloud-controller-manager-azure/serviceaccount.yaml (1)
1-4: Fix YAML lint: trim trailing space and add EOF newlineRemoves trailing space after the ServiceAccount name and adds a newline at EOF.
apiVersion: v1 kind: ServiceAccount metadata: - name: azure-cloud-controller-manager + name: azure-cloud-controller-manager +support/controlplane-component/token-minter-container.go (2)
90-95: Use token-specific mount path to reduce confusionAlign the token-minter mount path with the token type to avoid surprises when debugging or extending.
- tokenFileMountPath := "/var/run/secrets/openshift/serviceaccount" - - var audience string - switch tokenType { - case CloudToken: - audience = "openshift" - case KubeAPIServerToken: - audience = hcp.Spec.IssuerURL - } + var ( + tokenFileMountPath string + audience string + ) + switch tokenType { + case CloudToken: + audience = "openshift" + tokenFileMountPath = cloudTokenFileMountPath + case KubeAPIServerToken: + audience = hcp.Spec.IssuerURL + tokenFileMountPath = kubeAPITokenFileMountPath + }
58-61: Use context-aware ARO detection or ensure env var injection
azureutil.IsAroHCP()returnsos.Getenv("MANAGED_SERVICE")==hyperv1.AroHCP, relying solely on global state rather than thecpContext.HCPbeing processed. Ensure theMANAGED_SERVICEenv var is set correctly on the control-plane-component container for each HCP, or refactor to read the managed-service type directly fromcpContext.HCP.Spec.Platform.Azureto avoid relying on global state.control-plane-operator/controllers/hostedcontrolplane/v2/assets/cloud-controller-manager-azure/deployment.yaml (1)
47-48: ServiceAccount + token automount: confirm WI injection; consider reducing token exposureEnsure the referenced ServiceAccount exists and is annotated for Azure Workload Identity so the mutating webhook injects AZURE_* env and the federated token. If the webhook does not require the default legacy SA token, consider:
- Setting automountServiceAccountToken: false to minimize token exposure.
- Relying on the projected federated token added by the WI webhook.
Please confirm webhook requirements before changing.
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/component.go (1)
85-87: Minor: verify azureutil.IsAroHCP() doesn’t need contextIf future detection needs HCP context, consider accepting cpContext or switching to a predicate that returns (bool, error) for parity with others.
cmd/infra/azure/create.go (2)
284-295: Redundant nested condition and JSON-only parsing
- The inner if o.WorkloadIdentitiesFile != "" is redundant.
- Prefer yaml.Unmarshal to accept both YAML and JSON (YAML is a superset), matching other inputs.
Apply:
- if o.WorkloadIdentitiesFile != "" { - if o.WorkloadIdentitiesFile != "" { - workloadIdentitiesRaw, err := os.ReadFile(o.WorkloadIdentitiesFile) - if err != nil { - return nil, fmt.Errorf("failed to read --workload-identities-file %s: %w", o.WorkloadIdentitiesFile, err) - } - result.WorkloadIdentities = &hyperv1.AzureWorkloadIdentities{} - if err := json.Unmarshal(workloadIdentitiesRaw, result.WorkloadIdentities); err != nil { - return nil, fmt.Errorf("failed to unmarshal --workload-identities-file: %w", err) - } - } - } + if o.WorkloadIdentitiesFile != "" { + workloadIdentitiesRaw, err := os.ReadFile(o.WorkloadIdentitiesFile) + if err != nil { + return nil, fmt.Errorf("failed to read --workload-identities-file %s: %w", o.WorkloadIdentitiesFile, err) + } + result.WorkloadIdentities = &hyperv1.AzureWorkloadIdentities{} + if err := yaml.Unmarshal(workloadIdentitiesRaw, result.WorkloadIdentities); err != nil { + return nil, fmt.Errorf("failed to unmarshal --workload-identities-file: %w", err) + } + }
313-329: Idempotent role assignment path is good; consider small hardeningLoop+assign flow is fine. Consider bubbling context into Graph lookups and setting an HTTP client timeout to prevent hangs. I can send a follow-up patch if desired.
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (2)
1653-1661: Verify Azure secret key schema and centralize base keys.
- Please confirm the consumers (CSI, ingress/operator, CNCC, CCM) expect these exact keys (notably
azure_regionvsazure_location, and whetherazure_client_idmust be present). If helpers injectazure_client_idper workload identity, note that here in a comment.- Consider moving construction of the common base map into
azureresourcesto keep the contract in one place and reduce drift if keys evolve.Would you like a follow-up patch that lifts this base map into
azureresources?
2764-2771: ARO path should clean up previously-created Azure CSIDriver CRs.When switching code paths or upgrading existing clusters, leaving
ClusterCSIDriverobjects around can cause unintended reconciliation. Proactively delete them in the ARO branch.Proposed change:
case hyperv1.AzurePlatform: - if !azureutil.IsAroHCP() { + if !azureutil.IsAroHCP() { driverNames = []operatorv1.CSIDriverName{ operatorv1.AzureDiskCSIDriver, operatorv1.AzureFileCSIDriver, } - } + } else { + // Ensure any previously-created CSIDriver CRs are removed on ARO. + for _, name := range []operatorv1.CSIDriverName{ + operatorv1.AzureDiskCSIDriver, + operatorv1.AzureFileCSIDriver, + } { + if _, err := util.DeleteIfNeeded(ctx, r.client, manifests.ClusterCSIDriver(name)); err != nil { + errs = append(errs, fmt.Errorf("failed to delete ClusterCSIDriver %s: %w", name, err)) + } + } + }api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
4023-4043: Azure workload identity "network": schema looks consistent with peers. Consider uniqueness guard.
- The new WorkloadIdentity object mirrors existing entries (UUID format, min/max length, CEL check). LGTM.
- Optional: add a top-level CEL validation at azure.workloadIdentities to ensure all clientIDs (cloudProvider, disk, file, imageRegistry, ingress, network, nodePoolManagement) are distinct to avoid accidental reuse.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
4064-4084: Addition of workloadIdentities.network looks consistent; minor schema nits
- Schema matches sibling blocks (uuid pattern, length, required: clientID). Looks good.
- Optional: add format: uuid for better client/tooling hints, or drop redundant min/max length since the regex enforces them.
Apply one of these diffs:
Option A (add format):
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication - maxLength: 36 + maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ + format: uuid type: stringOption B (remove redundant length constraints):
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication - maxLength: 36 - minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml (1)
4836-4842: Required list updated to include network: ordering consistent; confirm admission won’t brick existing clusters.If any clusters with WorkloadIdentities exist without network, updates to their HostedCluster will fail validation. If that’s acceptable under TechPreviewNoUpgrade, document it; otherwise consider a transitional validation (e.g., allow missing on update when oldSelf lacked it) or provide an upgrade controller to populate it.
If you want a softer transition, relax required and add a CEL rule like:
- required: - - cloudProvider - - disk - - file - - imageRegistry - - ingress - - network - - nodePoolManagement + x-kubernetes-validations: + - message: network must be set for new clusters using WorkloadIdentities + rule: 'has(self.network) || has(oldSelf) && has(oldSelf.network)' + required: + - cloudProvider + - disk + - file + - imageRegistry + - ingress + - nodePoolManagement(This keeps new creates strict while allowing updates on legacy objects; once migrated, you can restore hard-required in a later release.)
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml (1)
4439-4440: Requiring network is a breaking change—verify backward compatibility and docsMaking network mandatory under WorkloadIdentities can reject existing HostedClusters that lack it. Ensure examples/e2e are updated, admission/migration is handled, and release notes call this out.
I can draft a short migration note and update the example manifests to include the new network clientID on request.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml (2)
3969-3986: Tiny wording and OpenAPI hint: tweak description and add uuid format.Improve UX for generators/validators by adding format: uuid, and fix article in description.
- clientID: - description: clientID is client ID of a federated + clientID: + description: clientID is the client ID of a federated managed identity used in workload identity authentication + format: uuid
4011-4017: Adding ‘network’ to required is a breaking API change—confirm migration plan.Existing self-managed Azure clusters using WorkloadIdentities but lacking ‘network’ will fail validation on upgrade. If intentional, document clearly and ensure controllers/docs/examples cover it; otherwise consider a one-release deprecation window (optional field + controller validation) before making it required.
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml (1)
4131-4151: Clarify validation message to match “federated managed identity” wording.The description explicitly says this is a federated managed identity. The validation message still says “managed identity.” Align it for precision.
- - message: the client ID of a managed identity + - message: the client ID of a federated managed identity must be a valid UUID. It should be 5 groupscmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml (1)
4464-4484: Azure workloadIdentities.network field looks correct; consider adding format: uuid for extra validation clarityThe UUID regex + min/max length are good. Adding OpenAPI format can improve tooling UX.
Apply via kubebuilder tag on the source type (then re-generate CRDs); expected generated change:
- clientID: + clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ + format: uuid type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml (1)
3959-3964: Nit: tighten description grammar for clientID.Minor wording polish for consistency with other fields.
- description: clientID is client ID of a federated - managed identity used in workload identity authentication + description: clientID is the client ID of a federated + managed identity used in workload identity authentication.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
4102-4123: Network workload identity shape looks correct; consider adding format: uuid.The new
workloadIdentities.network.clientIDmirrors the other identities (length/pattern/CEL). Optional nit: addformat: uuidto improve tooling UX (it won’t change server-side validation).Apply:
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ + format: uuid type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml (1)
4076-4096: Add Azure workload identity for Network operator — looks consistent; consider tiny schema/docs polish.
- Schema mirrors existing WI blocks (uuid regex, lengths, validation); good.
- Nit: the validation message says “managed identity” while the description uses “federated managed identity”. Consider aligning wording.
- Optional: add OpenAPI
format: uuidalongside the regex for better tooling support.Proposed diff (wording + format):
properties: clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ + format: uuid type: string x-kubernetes-validations: - - message: the client ID of a managed identity + - message: the client ID of a federated managed identity must be a valid UUID. It should be 5 groups of hyphen separated hexadecimal characters in the form 8-4-4-4-12. rule: self.matches('^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$')api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml (1)
4283-4303: Azure WI: network identity addition looks correct; consider addingformat: uuid.Schema and CEL mirror existing fields. Adding
format: uuidimproves tooling/UX and remains backward-compatible.properties: clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication + format: uuid maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (1)
4055-4075: New Azure workload identity "network": schema looks consistentMatches existing pattern for workload identity fields (UUID-validated clientID, required: clientID). No functional concerns.
Optional: add
format: uuidalongside the regex to aid tooling, mirroring other places in this CRD that declare UUIDs.clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ + format: uuid type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml (1)
3947-3967: Network workload identity field looks correct; UUID validation consistent with peersThe new workloadIdentities.network object with clientID and required validates as a 36-char UUID and matches the pattern used for other Azure client IDs in this CRD. Good addition and descriptions align with cluster-network-operator usage.
Please verify companion changes exist:
- Go type: AzureWorkloadIdentities has Network field (json:"network,omitempty") in api/hypershift/v1beta1/azure.go.
- DeepCopy and applyconfig updated.
- HostedCluster and any other CRDs exposing workloadIdentities also include network.
If desired, you could optionally add format: uuid alongside the regex for better tooling support.
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 + format: uuid pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringcmd/cluster/azure/create.go (2)
70-73: Flag help should reflect both identity modesUsers may supply workload-identities; the role-assignment help still says “managed identities”.
Example:
-flags.BoolVar(&opts.AssignServicePrincipalRoles, "assign-service-principal-roles", opts.AssignServicePrincipalRoles, "Assign the service principal roles to the managed identities.") +flags.BoolVar(&opts.AssignServicePrincipalRoles, "assign-service-principal-roles", opts.AssignServicePrincipalRoles, "Assign necessary roles to the identities (managed or workload).")
271-279: Prefer typed constants over string literals for AzureAuthenticationConfigTypeUse API constants if available to prevent drift.
Example:
- AzureAuthenticationConfigType: "WorkloadIdentities", + AzureAuthenticationConfigType: hyperv1.AzureAuthTypeWorkloadIdentities, // verify exact identifierRepeat for "ManagedIdentities".
control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure.go (1)
15-72: Reduce duplication between managed/self-managed credential setupBoth functions differ only in the source of client IDs. Consider a small strategy function or parameterized struct to pick IDs, and a single reconcile loop over targets (ingress/disk/registry/file).
I can draft this refactor if desired.
Also applies to: 74-125
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
4207-4227: Addition of workloadIdentities.network looks consistent, but consider using format: uuid.Schema matches sibling identities. Optionally add format: uuid to aid clients.
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication + format: uuid maxLength: 36 minLength: 36api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
4438-4454: Optional: add format: uuid for tooling discoverabilityOpenAPI consumers sometimes surface format hints in UIs/linting. Adding format: uuid (while keeping the regex) can help, with no behavior change.
Apply this minimal diff:
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 + format: uuid pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml (1)
4099-4119: Consider adding format: uuid to clientID for better tooling and consistencyThe regex + length checks are good. Adding format: uuid can help IDEs and schema-aware tooling without changing validation behavior.
Apply:
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 + format: uuid pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringapi/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml (1)
3999-4011: Optional: add format hint for UUIDConsider adding format: uuid to clientID to improve tooling/UX; keep the regex as-is.
clientID: - description: clientID is client ID of a federated managed identity used in workload identity authentication + description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ - type: string + type: string + format: uuidcmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml (1)
4925-4932: Requiring workloadIdentities.network may break existing CRs; consider softer rolloutMarking network as required tightens validation for all self-managed Azure clusters using WorkloadIdentities. Existing HostedClusters that don’t set it will fail future edits. Recommend a staged approach (optional first with controller warning/condition, then enforced in a later release) to avoid upgrade friction.
If you choose to defer hard validation now, apply:
@@ required: - - cloudProvider - - disk - - file - - imageRegistry - - ingress - - network - - nodePoolManagement + - cloudProvider + - disk + - file + - imageRegistry + - ingress + - nodePoolManagementAnd surface a clear Degraded/Warning condition from the controllers when network is missing. Also add a migration note in docs/examples indicating from which release the field becomes mandatory.
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go (1)
305-321: Embed cloud.conf or remove unused helper
No calls toaddCloudConfigToSecret, so the CNCC secret never includes acloud.confkey. Either invokeaddCloudConfigToSecret(secretData, hcluster)in the reconciliation closure or deleteaddCloudConfigToSecret(and related constants) to avoid dead code.api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml (1)
4015-4015: Adding network to the required list may break existing CRs on upgrade — confirm rollout plan.
- Making a new property required in a served storage version (v1beta1) can invalidate edits to older objects that don’t have it. Ensure:
- Existing HostedClusters using WorkloadIdentities can be updated without adding network immediately, or
- You’re comfortable gating this for 4.20 with clear migration docs, or
- Consider making it optional for one release and enforcing via controller validation, then flipping to required later.
If you prefer to keep API compatibility for a cycle, drop “- network” from required and enforce via reconciliation/validation:
required: - cloudProvider - disk - file - imageRegistry - ingress - - network - nodePoolManagementI can also add an x-kubernetes-validations rule to soft-require network when azureAuthenticationConfigType == WorkloadIdentities if you want it enforced at CRD level.
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.go (2)
113-117: Use filepath.Join for path safetySafer and portable than string concatenation, avoids double/missing slashes.
Apply this diff:
- azureConfig.AADMSIDataPlaneIdentityPath = config.ManagedAzureCertificatePath + hcp.Spec.Platform.Azure.AzureAuthenticationConfig.ManagedIdentities.ControlPlane.CloudProvider.CredentialsSecretName + azureConfig.AADMSIDataPlaneIdentityPath = filepath.Join( + config.ManagedAzureCertificatePath, + hcp.Spec.Platform.Azure.AzureAuthenticationConfig.ManagedIdentities.ControlPlane.CloudProvider.CredentialsSecretName, + )Add import:
import "path/filepath"
128-154: Tighten JSON tags for optional/sensitive fields and verify tag names
- Add
omitemptyto optional/sensitive fields to avoid emitting empty values in logs/manifests.- Please double-check the exact key names against cloud-provider-azure config; mismatched keys are silently ignored.
Apply this diff (subset focusing on sensitive/optional fields):
- UseManagedIdentityExtension bool `json:"useManagedIdentityExtension"` - UseFederatedWorkloadIdentityExtension bool `json:"useFederatedWorkloadIdentityExtension"` + UseManagedIdentityExtension bool `json:"useManagedIdentityExtension,omitempty"` + UseFederatedWorkloadIdentityExtension bool `json:"useFederatedWorkloadIdentityExtension,omitempty"` SubscriptionID string `json:"subscriptionId"` AADClientID string `json:"aadClientId"` // TODO HOSTEDCP-1542 - Bryan - drop client secret once we have WorkloadIdentity working - AADClientSecret string `json:"aadClientSecret"` - AADClientCertPath string `json:"aadClientCertPath"` - AADFederatedTokenFile string `json:"aadFederatedTokenFile"` - AADMSIDataPlaneIdentityPath string `json:"aadMSIDataPlaneIdentityPath"` + AADClientSecret string `json:"aadClientSecret,omitempty"` + AADClientCertPath string `json:"aadClientCertPath,omitempty"` + AADFederatedTokenFile string `json:"aadFederatedTokenFile,omitempty"` + AADMSIDataPlaneIdentityPath string `json:"aadMSIDataPlaneIdentityPath,omitempty"` ... - RouteTableName string `json:"routeTableName"` + RouteTableName string `json:"routeTableName,omitempty"` - CloudProviderBackoff bool `json:"cloudProviderBackoff"` - CloudProviderBackoffDuration int `json:"cloudProviderBackoffDuration"` - UseInstanceMetadata bool `json:"useInstanceMetadata"` + CloudProviderBackoff bool `json:"cloudProviderBackoff,omitempty"` + CloudProviderBackoffDuration int `json:"cloudProviderBackoffDuration,omitempty"` + UseInstanceMetadata bool `json:"useInstanceMetadata,omitempty"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (49)
api/hypershift/v1beta1/azure.go(1 hunks)api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml(2 hunks)client/applyconfiguration/hypershift/v1beta1/azureworkloadidentities.go(2 hunks)cmd/cluster/azure/create.go(6 hunks)cmd/infra/azure/create.go(3 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/assets/cloud-controller-manager-azure/deployment.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/assets/cloud-controller-manager-azure/serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/capi_provider/component.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/component.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.go(4 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/storage/component.go(0 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/creds.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(3 hunks)docs/content/reference/api.md(1 hunks)examples/workload-identities-example.json(1 hunks)hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go(7 hunks)hypershift-operator/controllers/hostedcluster/internal/platform/platform.go(1 hunks)hypershift-operator/controllers/manifests/azure.go(1 hunks)support/controlplane-component/token-minter-container.go(3 hunks)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/azure.go(1 hunks)vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)
💤 Files with no reviewable changes (1)
- control-plane-operator/controllers/hostedcontrolplane/v2/storage/component.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T18:49:17.570Z
Learnt from: CR
PR: openshift/hypershift#0
File: AGENTS.md:0-0
Timestamp: 2025-08-29T18:49:17.570Z
Learning: Applies to hypershift-operator/controllers/**/*.go : Place operator controller implementations under hypershift-operator/controllers/
Applied to files:
control-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go
🪛 YAMLlint (1.37.1)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/cloud-controller-manager-azure/serviceaccount.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
[error] 4-4: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- Address compilation and import issues in test code - Update test fixtures/mocks and fix failing assertions - Stabilize timing-dependent tests to reduce flakiness - `make unit` now runs successfully Signed-off-by: Bryan Cox <brcox@redhat.com> Assisted-by: GPT-5 (via Cursor)
…ion for Azure Remove ARO-only guard so registry management state admission policy and binding are deleted during HostedCluster teardown. Signed-off-by: Bryan Cox <brcox@redhat.com> Assisted-by: GPT-5 (via Cursor)
Deduplicate secret creation logic used by control-plane-operator and hypershift-operator by centralizing it in a single helper. This reduces code duplication, ensures consistent labels/owner refs/annotations, and simplifies future maintenance. Signed-off-by: Bryan Cox <brcox@redhat.com> Assisted-by: Claude
db78450 to
5d4cc72
Compare
devguyio
left a comment
devguyio
left a comment
There was a problem hiding this comment.
/lgtm
/hold
looks like I need to remove the "request change" review status, so stamping it and leaving it for other reviewers to unhold
openshift-ci
Bot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, devguyio The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test verify-deps |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cmd/infra/azure/create.go (2)
489-503: Pager bug: only first DNS zones page is checkedIf the base domain isn’t on the first page, it won’t be found. Iterate all pages.
- pager := zonesClient.NewListPager(nil) - if pager.More() { - pagerResults, err := pager.NextPage(ctx) - if err != nil { - return "", fmt.Errorf("failed to retrieve list of DNS zones: %w", err) - } - - for _, result := range pagerResults.Value { - if *result.Name == baseDomain { - return *result.ID, nil - } - } - } - return "", fmt.Errorf("could not find any DNS zones in subscription") + pager := zonesClient.NewListPager(nil) + for pager.More() { + page, err := pager.NextPage(ctx) + if err != nil { + return "", fmt.Errorf("failed to retrieve list of DNS zones: %w", err) + } + for _, z := range page.Value { + if z.Name != nil && strings.EqualFold(*z.Name, baseDomain) { + return *z.ID, nil + } + } + } + return "", fmt.Errorf("could not find DNS zone %q in subscription", baseDomain)
70-75: Expose CLI flag --workload-identities-file in cmd/infra/azure/create.goopts.WorkloadIdentitiesFile is defined and read in cmd/infra/azure/create.go but not registered as a CLI flag; cmd/cluster/azure/create.go already registers this flag (flags.StringVar at cmd/cluster/azure/create.go:68–71). Add the same registration to the infra create command.
Apply:
@@ cmd.Flags().StringToStringVarP(&opts.ResourceGroupTags, "resource-group-tags", "t", opts.ResourceGroupTags, "Additional tags to apply to the resource group created (e.g. 'key1=value1,key2=value2')") + cmd.Flags().StringVar(&opts.WorkloadIdentitiesFile, "workload-identities-file", opts.WorkloadIdentitiesFile, "Path to workload identities file (YAML or JSON) for federated SP role assignments")hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (1)
29-34: Fix state leakage between subtests: re-create AzureClusterIdentity per case
initialAzureClusterIdentityis shared and mutated across subtests, making results order‑dependent. Create a fresh object inside eacht.Run.- initialAzureClusterIdentity := &capiazure.AzureClusterIdentity{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-cluster-identity", - Namespace: controlPlaneNamespace, - }, - } + // (moved into each subtest) @@ for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) + // Fresh identity object per subtest + initialAzureClusterIdentity := &capiazure.AzureClusterIdentity{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster-identity", + Namespace: controlPlaneNamespace, + }, + } + if tc.isManagedService { t.Setenv("MANAGED_SERVICE", hyperv1.AroHCP) }Also applies to: 121-133
control-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.go (1)
46-52: Prevent nil-map write on Secret.Data.Initialize Secret.Data to avoid panics on first reconcile if manifest didn’t pre‑seed Data.
Apply:
-serializedConfig, err := json.MarshalIndent(azureConfig, "", " ") +serializedConfig, err := json.MarshalIndent(azureConfig, "", " ") if err != nil { return fmt.Errorf("failed to serialize cloudconfig: %w", err) } -secret.Data[azure.ConfigKey] = serializedConfig +if secret.Data == nil { + secret.Data = make(map[string][]byte, 1) +} +secret.Data[azure.ConfigKey] = serializedConfig
🧹 Nitpick comments (36)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml (3)
4-4: Addtype: Opaqueto reduce defaulting-related brittleness.Kubernetes defaults the Secret type; making it explicit keeps fixtures resilient to compare logic that doesn’t normalize defaults.
kind: Secret +type: Opaque
16-16: resourceVersion in fixtures can be fragile.If tests perform deep equality without stripping server-managed fields,
resourceVersion: "1"may cause churn. Confirm test harness normalization; otherwise consider dropping it.
1-17: Gitleaks false positive on Secret testdata.This is expected testdata, not real creds, but scanners will flag it. Add/extend an allowlist (path-based rule for testdata secrets) to prevent noisy CI.
control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml (1)
96-99: Mount cloud-token as readOnly in CCM.CCM only reads the minted token; make the volume mount read-only to tighten pod security.
- - mountPath: /var/run/secrets/openshift/serviceaccount - name: cloud-token + - mountPath: /var/run/secrets/openshift/serviceaccount + name: cloud-token + readOnly: truecontrol-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml (3)
9-16: Drop server‑populated fields from fixture (uid, resourceVersion).These are apiserver‑managed and can cause brittle golden updates; most fixtures omit them.
Apply:
ownerReferences: - apiVersion: hypershift.openshift.io/v1beta1 blockOwnerDeletion: true controller: true kind: HostedControlPlane name: hcp - uid: "" - resourceVersion: "1"
4-4: Set Secret type explicitly.Be explicit for readability and future consumers; defaults to Opaque, but let’s state it.
kind: Secret +type: Opaque
2-4: Gitleaks false positive: mark testdata in allowlist.This is non‑sensitive fixture data. Silence CI noise by allowlisting this path in .gitleaks config.
# .gitleaks.toml [allowlist] paths = [ "control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml" ]control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml (3)
4-16: Secret type not set; be explicit.Add type: Opaque to reduce ambiguity and align with other fixtures.
Apply:
resourceVersion: "1" +type: Opaque
2-3: Gitleaks false positive: add allowlist for testdata Secret.This is a non‑sensitive test fixture, but it trips the kubernetes-secret-yaml rule. Add a repo allowlist for this path to avoid noise.
Example .gitleaks.toml snippet:
[allowlist] description = "Allow k8s Secret fixtures under testdata" paths = [ '''control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret\.yaml$''' ]
1-3: Optional: add breadcrumbs for maintainers.A tiny header comment helps future readers decode/verify the payload without hunting tooling.
Apply:
+## TEST FIXTURE: contains no real credentials. +## Decode with: yq -r '.data["cloud.conf"]' <file> | base64 -d | jq . apiVersion: v1 data:control-plane-operator/controllers/hostedcontrolplane/v2/assets/azure-cloud-controller-manager/serviceaccount.yaml (1)
4-4: Trim trailing whitespace in nameThere’s a trailing space after the name. Clean it up to avoid churn in diffs.
Apply this diff:
- name: azure-cloud-controller-manager + name: azure-cloud-controller-managercmd/infra/azure/create.go (3)
90-91: Avoid nulls in output when not providedAdd omitempty so the YAML/JSON output doesn’t contain “workloadIdentities: null”.
- WorkloadIdentities *hyperv1.AzureWorkloadIdentities `json:"workloadIdentities"` + WorkloadIdentities *hyperv1.AzureWorkloadIdentities `json:"workloadIdentities,omitempty"`
284-293: Accept both YAML and JSON for the identities file (consistent with other flags)Other inputs use yaml.Unmarshal (which also accepts JSON). Align for UX consistency.
- result.WorkloadIdentities = &hyperv1.AzureWorkloadIdentities{} - if err := json.Unmarshal(workloadIdentitiesRaw, result.WorkloadIdentities); err != nil { + result.WorkloadIdentities = &hyperv1.AzureWorkloadIdentities{} + if err := yaml.Unmarshal(workloadIdentitiesRaw, result.WorkloadIdentities); err != nil { return nil, fmt.Errorf("failed to unmarshal --workload-identities-file: %w", err) }
998-1014: Harden Graph call: check HTTP status and set Accept headerPrevents decoding non-200 responses as success and improves error clarity.
- req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequest("GET", url, nil) if err != nil { return "", fmt.Errorf("failed to create request: %w", err) } @@ - req.Header.Set("Content-Type", "application/json") + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") @@ - // Parse response + // Fail fast on non-200 + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return "", fmt.Errorf("graph servicePrincipals query failed: status=%d body=%s", resp.StatusCode, strings.TrimSpace(string(body))) + } + // Parse responsecontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure_test.go (6)
64-68: Avoid shared mutable baseSecretData across subtestsPassing the same map risks hidden coupling if SetupOperandCredentials mutates it. Create a fresh map per subtest.
- // Base data that should be preserved in all created secrets - baseSecretData := map[string][]byte{ - "some": []byte("data"), - } + // Base data is created per subtest to avoid shared mutable statefor _, tc := range tests { t.Run(tc.name, func(t *testing.T) { g := NewWithT(t) c := fake.NewClientBuilder().WithScheme(api.Scheme).Build() + baseSecretData := map[string][]byte{ + "some": []byte("data"), + } + hcp := makeHCP(tc.managedAzure) if tc.disableIngress { hcp.Spec.Capabilities = &hyperv1.Capabilities{Disabled: []hyperv1.OptionalCapability{hyperv1.IngressCapability}} }Also applies to: 111-122
135-143: Assert NotFound on missing ingress secretBe precise on the expected failure mode.
import ( "strings" "testing" . "github.com/onsi/gomega" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/upsert" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"if tc.expectIngress { g.Expect(err).ToNot(HaveOccurred()) } else { - g.Expect(err).To(HaveOccurred()) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) }Also applies to: 13-18
205-209: Also assert NotFound here for clarity-err := c2.Get(t.Context(), ingressKey, &ingressSecret) -g.Expect(err).To(HaveOccurred()) +err := c2.Get(t.Context(), ingressKey, &ingressSecret) +g.Expect(apierrors.IsNotFound(err)).To(BeTrue())
210-215: Verify file and registry secrets when ingress is disabledCurrently only disk is checked.
// Verify other secrets were still created diskKey := client.ObjectKey{Namespace: "openshift-cluster-csi-drivers", Name: "azure-disk-credentials"} var diskSecret corev1.Secret err = c2.Get(t.Context(), diskKey, &diskSecret) g.Expect(err).ToNot(HaveOccurred()) + +fileKey := client.ObjectKey{Namespace: "openshift-cluster-csi-drivers", Name: "azure-file-credentials"} +var fileSecret corev1.Secret +err = c2.Get(t.Context(), fileKey, &fileSecret) +g.Expect(err).ToNot(HaveOccurred()) + +registryKey := client.ObjectKey{Namespace: "openshift-image-registry", Name: "installer-cloud-credentials"} +var registrySecret corev1.Secret +err = c2.Get(t.Context(), registryKey, ®istrySecret) +g.Expect(err).ToNot(HaveOccurred())
111-146: Optional: Run subtests in parallelSafe after isolating baseSecretData; speeds CI a bit.
t.Run(tc.name, func(t *testing.T) { + t.Parallel() g := NewWithT(t)
164-169: Optional: Add ObjectMeta to hcp for consistencyIf owner refs or names are used later, having Name/Namespace avoids surprises.
hcp := &hyperv1.HostedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hcp", + Namespace: "ns", + }, Spec: hyperv1.HostedControlPlaneSpec{ Capabilities: &hyperv1.Capabilities{}, }, }cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml (1)
4816-4823: Requiring network is a spec-breaking change for existing WorkloadIdentities HCPs—confirm upgrade path.Existing HostedControlPlanes using WorkloadIdentities but lacking network will fail future spec updates under the new CRD. If that’s expected, add explicit upgrade/migration notes and ensure CLI/defaulting cover it; otherwise consider gating (feature gate or temporary optional + validation) until all toolchains provide the value.
If acceptable as‑is, please:
- Confirm no managed clusters will reconcile spec without providing this field.
- Ensure the installer/CLI rejects configs missing network with a clear error.
- Add release notes/migration doc for day‑2 edits on legacy objects.
control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.go (1)
35-37: Prefer a shared constant for the SA name.Avoid magic strings; define
const azureCCMSA = "azure-cloud-controller-manager"near other constants and reference it here to prevent drift.- deployment.Spec.Template.Spec.ServiceAccountName = "azure-cloud-controller-manager" + deployment.Spec.Template.Spec.ServiceAccountName = azureCCMSAcontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (3)
293-310: Scope of Azure cleanup: comment/code mismatch.The comment mentions ARO-only cleanup, but the condition now applies to all Azure. Either narrow the condition to ARO or update the comment/log to reflect the broader behavior.
- // Delete admission policies during cluster deletion to allow HCCO cleanup operations for ARO HCP - if hcp.Spec.Platform.Type == hyperv1.AzurePlatform { + // Delete admission policies during cluster deletion to allow HCCO cleanup operations on Azure + if hcp.Spec.Platform.Type == hyperv1.AzurePlatform { ... }Or, if this was intended to be ARO-only:
- if hcp.Spec.Platform.Type == hyperv1.AzurePlatform { + if azureutil.IsAroHCP() {
1653-1666: Guard against missing Azure fields before building secrets.If any of Location, ResourceGroupName, SubscriptionID, or TenantID are empty, you’ll silently write partial creds. Validate early and return a clear error.
case hyperv1.AzurePlatform: - // Create a base secret data map with the common Azure credentials + // Validate required Azure fields before building secrets + if hcp.Spec.Platform.Azure == nil || + hcp.Spec.Platform.Azure.Location == "" || + hcp.Spec.Platform.Azure.ResourceGroupName == "" || + hcp.Spec.Platform.Azure.SubscriptionID == "" || + hcp.Spec.Platform.Azure.TenantID == "" { + return []error{fmt.Errorf("incomplete Azure platform configuration: location, resourceGroup, subscriptionID, and tenantID are required")} + } + // Create a base secret data map with the common Azure credentials secretData := map[string][]byte{ "azure_federated_token_file": []byte("/var/run/secrets/openshift/serviceaccount/token"), "azure_region": []byte(hcp.Spec.Platform.Azure.Location), "azure_resource_prefix": []byte(hcp.Name + "-" + hcp.Spec.InfraID), "azure_resourcegroup": []byte(hcp.Spec.Platform.Azure.ResourceGroupName), "azure_subscription_id": []byte(hcp.Spec.Platform.Azure.SubscriptionID), "azure_tenant_id": []byte(hcp.Spec.Platform.Azure.TenantID), } // Set up the operand credentials for either managed or self-managed Azure environments errs = azureresources.SetupOperandCredentials(ctx, r.client, r.CreateOrUpdateProvider, hcp, secretData, azureutil.IsAroHCP()) if len(errs) > 0 { return errs }
1653-1666: Consistency: avoid early return in one platform branch.Other branches accumulate errs and return at the end; this one returns early. Not harmful, but consider consistent flow for readability.
- errs = azureresources.SetupOperandCredentials(... ) - if len(errs) > 0 { - return errs - } + errs = append(errs, azureresources.SetupOperandCredentials(... )...)support/azureutil/azureutil_test.go (1)
344-538: Avoid t.Context() to keep compatibility with older Go toolchains.Use
context.Background()unless you require test cancellation semantics.- errs := ReconcileAzureCredentials( - t.Context(), + errs := ReconcileAzureCredentials( + context.Background(), fakeClient, mockCreateOrUpdate, baseSecretData, tt.configs, tt.capabilities, )hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go (2)
312-357: Strengthen assertions for disabled capabilities caseAlso validate the remaining secrets’
azure_client_idvalues (disk, file, cncc) and shared base data, mirroring the first test’s checks.
373-415: Optional: add idempotency checkCall
ReconcileCredentialstwice and assert no additional secrets are “created” on the second run (adjust the mock to returnOperationResultNoneon repeats).api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml (1)
4064-4084: Add OpenAPI uuid format for clientID; keep regexclientID already enforces UUID via regex; add format: uuid for better tooling/validation without changing behavior.
Apply this diff:
clientID: description: clientID is client ID of a federated managed identity used in workload identity authentication maxLength: 36 minLength: 36 + format: uuid pattern: ^[0-9a-fA-F]{8}-([0-9a-fA-F]{4}-){3}[0-9a-fA-F]{12}$ type: stringcontrol-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.go (1)
39-44: Clarify requirement and fail early when VNetID is missing.Keep unconditional resolution, but give a precise error if VnetID is empty and update the comment to reflect the broader requirement (both ARO and self‑managed).
Apply:
-// aro hcp csi nfs protocol provision volumes needs the vnetName/vnetResourceGroup config +// Azure CSI requires vnetName/vnetResourceGroup (ARO and self-managed). +if azureSpec.VnetID == "" { + return fmt.Errorf("azureSpec.vnetID must be set for Azure CSI") +} azureConfig.VnetName, azureConfig.VnetResourceGroup, getVnetNameAndResourceGroupErr = azureutil.GetVnetNameAndResourceGroupFromVnetID(azureSpec.VnetID)support/azureutil/azureutil.go (1)
361-416: Tighten ReconcileAzureCredentials: var naming, nil-map safety, and key constant.
- Rename loop var (config → cfg) to avoid shadowing an imported config package.
- Ensure secret.Data is non‑nil: clone baseSecretData when non‑nil, otherwise use make(map[string][]byte) before assigning/adding keys.
- Prefer a shared constant for "azure_client_id" only if you can update other occurrences (found literals in support/azureutil/azureutil.go, support/azureutil/azureutil_test.go, hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go, control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure_test.go and related tests).
control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure.go (1)
15-15: Use a clearer sentinel value for the managed-ingress placeholderfakeClientID may confuse operators/debuggers. Prefer an explicit sentinel like unused.
Apply this diff:
-const placeholderClientID = "fakeClientID" +const placeholderClientID = "unused"cmd/cluster/azure/create.go (2)
131-146: Validate: strong mutual‑exclusion/dependency; consider empty‑config guard.Current rules look good. Do we ever expect neither managed+data‑plane nor workload identities? If not, add a guard to fail early when all three are empty.
Example:
@@ if o.ManagedIdentitiesFile != "" && o.DataPlaneIdentitiesFile == "" { return nil, fmt.Errorf("--managed-identities-file requires --data-plane-identities-file") } + + if o.ManagedIdentitiesFile == "" && o.DataPlaneIdentitiesFile == "" && o.WorkloadIdentitiesFile == "" { + return nil, fmt.Errorf("one of --workload-identities-file or the pair (--managed-identities-file, --data-plane-identities-file) must be provided") + }
606-611: Avoid wrapping nil in error.err is nil here; wrapping yields “...: ”.
Apply:
- if rhcosImage == "" { - return "", fmt.Errorf("RHCOS VHD image is empty: %w", err) - } + if rhcosImage == "" { + return "", fmt.Errorf("RHCOS VHD image is empty") + }hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go (1)
105-145: Mark kubeconfig mount read‑only.Harden the manager container by making the kubeconfig mount RO.
- { - Name: "svc-kubeconfig", - MountPath: "/etc/kubernetes", - }, + { + Name: "svc-kubeconfig", + MountPath: "/etc/kubernetes", + ReadOnly: true, + },control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml (1)
92-99: Tighten mounts to read‑only.Mark kubeconfig mounts RO; token mount should already be RO by design but can remain RW for token updates.
- - mountPath: /etc/kubernetes/kubeconfig + - mountPath: /etc/kubernetes/kubeconfig name: kubeconfig + readOnly: true @@ - - mountPath: /etc/kubernetes + - mountPath: /etc/kubernetes name: kubeconfig + readOnly: trueAlso applies to: 120-125, 137-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
vendor/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/**
📒 Files selected for processing (74)
api/hypershift/v1beta1/azure.go(1 hunks)api/hypershift/v1beta1/zz_generated.deepcopy.go(1 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/NetworkDiagnosticsConfig.yaml(2 hunks)api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml(2 hunks)client/applyconfiguration/hypershift/v1beta1/azureworkloadidentities.go(2 hunks)cmd/cluster/azure/create.go(5 hunks)cmd/cluster/azure/create_test.go(6 hunks)cmd/infra/azure/create.go(3 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-TechPreviewNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-CustomNoUpgrade.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml(2 hunks)cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-TechPreviewNoUpgrade.crd.yaml(2 hunks)control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_config_configmap.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_config_configmap.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml(3 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_config_configmap.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml(3 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server-proxy/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/assets/azure-cloud-controller-manager/serviceaccount.yaml(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/component.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/config.go(3 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/cloud_controller_manager/azure/deployment.go(1 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/storage/azure.go(4 hunks)control-plane-operator/controllers/hostedcontrolplane/v2/storage/component.go(2 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure_test.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/creds.go(1 hunks)control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go(5 hunks)docs/content/reference/api.md(1 hunks)examples/workload-identities-example.json(1 hunks)hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go(5 hunks)hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure_test.go(2 hunks)hypershift-operator/controllers/hostedcluster/internal/platform/platform.go(1 hunks)hypershift-operator/controllers/manifests/azure.go(1 hunks)support/azureutil/azureutil.go(4 hunks)support/azureutil/azureutil_test.go(2 hunks)support/controlplane-component/token-minter-container.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- control-plane-operator/controllers/hostedcontrolplane/testdata/ignition-server-proxy/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml
🚧 Files skipped from review as they are similar to previous changes (34)
- control-plane-operator/controllers/hostedcontrolplane/v2/storage/component.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml
- cmd/cluster/azure/create_test.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/creds.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_deployment.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_controlplanecomponent.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AutoNodeKarpenter.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Default.crd.yaml
- docs/content/reference/api.md
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_config_configmap.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml
- api/hypershift/v1beta1/azure.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AutoNodeKarpenter.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/DynamicResourceAllocation.yaml
- support/controlplane-component/token-minter-container.go
- api/hypershift/v1beta1/zz_generated.deepcopy.go
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/zz_fixture_TestControlPlaneComponents_azure_cloud_controller_manager_serviceaccount.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_config_configmap.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml
- hypershift-operator/controllers/manifests/azure.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml
- control-plane-operator/controllers/hostedcontrolplane/testdata/azure-cloud-controller-manager/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_cloud_config_secret.yaml
- api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yaml
- cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Default.crd.yaml
- examples/workload-identities-example.json
- client/applyconfiguration/hypershift/v1beta1/azureworkloadidentities.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/IBMCloud/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml
🧰 Additional context used
🪛 Gitleaks (8.28.0)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/TechPreviewNoUpgrade/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml
[high] 2-4: Possible Kubernetes Secret detected, posing a risk of leaking credentials/tokens from your deployments
(kubernetes-secret-yaml)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_disk_csi_config_secret.yaml
[high] 2-4: Possible Kubernetes Secret detected, posing a risk of leaking credentials/tokens from your deployments
(kubernetes-secret-yaml)
control-plane-operator/controllers/hostedcontrolplane/testdata/cluster-storage-operator/zz_fixture_TestControlPlaneComponents_azure_file_csi_config_secret.yaml
[high] 2-4: Possible Kubernetes Secret detected, posing a risk of leaking credentials/tokens from your deployments
(kubernetes-secret-yaml)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / hypershift-release-mce-210-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-210-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
| - network | ||
| - nodePoolManagement |
There was a problem hiding this comment.
❓ Verification inconclusive
Making workloadIdentities.network required is a breaking API change. Stage or provide defaulting.
Existing self‑managed Azure clusters using WorkloadIdentities will fail validation on upgrade if they lack the new field. Consider:
- Make network optional for one release and default to cloudProvider clientID, then enforce later; or
- Gate enforcement behind a feature flag/version gate.
Minimal schema change to avoid immediate breakage:
required:
- cloudProvider
- disk
- file
- imageRegistry
- ingress
- - network
- nodePoolManagementDo not make workloadIdentities.network required — this is a breaking API change; stage or provide defaulting.
File: api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml (lines 4103–4104)
Requiring workloadIdentities.network will break upgrades for existing self‑managed Azure clusters using WorkloadIdentities that lack this field.
- Make network optional for one release and default it (e.g., to cloudProvider clientID), then make it required in a later release.
- Or gate enforcement behind a feature/version flag.
Minimal schema change to avoid immediate breakage:
required:
- cloudProvider
- disk
- file
- imageRegistry
- ingress
- - network
- nodePoolManagement📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - network | |
| - nodePoolManagement | |
| - nodePoolManagement |
🤖 Prompt for AI Agents
In
api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yaml
around lines 4103-4104, the schema currently makes workloadIdentities.network
required which is a breaking change; remove network from the required array so
the field is optional for at least one release, and add a migration/defaulting
strategy: either (a) implement server-side defaulting in the
conversion/defaulting webhook to set network (e.g., to the cloudProvider
clientID) for existing objects when omitted, or (b) gate strict validation
behind a feature/version flag so enforcement can be introduced in a later
release. Ensure the CRD change only drops the required constraint now and that
defaulting or gating logic is added in tandem to avoid upgrade breakage.
| - file | ||
| - imageRegistry | ||
| - ingress | ||
| - network |
There was a problem hiding this comment.
🧩 Analysis chain
Requiring network in the CRD is a breaking change; consider a transition.
Making network mandatory will reject updates for existing self-managed Azure clusters lacking this field. Either:
- defer strictness: don’t add it to required yet and enforce in controllers/CLI, or
- provide a migration/defaulting path (webhook/defaults) and clearly document upgrade steps.
Proposed non-breaking schema tweak (drop from required for now):
- - networkIf you intend to keep it required now, confirm:
- All existing fixtures/tests/manifests are updated.
- No controller writes to spec on upgrade that would force validation.
- Release notes call out the breaking requirement.
You can scan repo for objects missing the new field:
🏁 Script executed:
#!/bin/bash
# HostedCluster/HostedControlPlane YAMLs missing workloadIdentities.network
rg -nP --type=yaml -C1 'workloadIdentities:(\n|.)*(cloudProvider|disk|file|imageRegistry|ingress|nodePoolManagement):(\n|.)*clientID:' \
| rg -nP -v 'network:(\n|.)*clientID:'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search repo YAMLs: list files that contain `workloadIdentities` and which of those do NOT contain `network` under any workloadIdentities occurrence.
python3 - <<'PY'
import os, sys, subprocess
try:
import yaml
except Exception:
subprocess.check_call([sys.executable, "-m", "pip", "install", "--user", "pyyaml"])
import yaml
files_with = []
missing = []
for root, dirs, files in os.walk('.'):
for name in files:
if name.endswith(('.yml', '.yaml')):
path = os.path.join(root, name)
try:
with open(path, 'r', encoding='utf-8') as fh:
docs = list(yaml.safe_load_all(fh))
except Exception:
# skip files that can't be parsed as YAML
continue
found_any = False
has_network_any = False
def walk(obj):
nonlocal found_any, has_network_any
if isinstance(obj, dict):
for k, v in obj.items():
if k == 'workloadIdentities':
found_any = True
if isinstance(v, dict) and 'network' in v:
has_network_any = True
walk(v)
elif isinstance(obj, list):
for it in obj:
walk(it)
for doc in docs:
walk(doc)
if found_any:
files_with.append(path)
if not has_network_any:
missing.append(path)
# Output results
print(f"FILES_WITH_WORKLOAD_IDENTITIES:{len(files_with)}")
for p in files_with:
print(p)
print("---")
print(f"FILES_MISSING_NETWORK:{len(missing)}")
for p in missing:
print(p)
PYLength of output: 80
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search repo YAMLs: list files that contain `workloadIdentities` and which of those do NOT contain `network`
python3 - <<'PY'
import os, sys, subprocess
try:
import yaml
except Exception:
subprocess.check_call([sys.executable, "-m", "pip", "install", "--user", "pyyaml"])
import yaml
files_with = []
missing = []
for root, dirs, files in os.walk('.'):
for name in files:
if name.endswith(('.yml', '.yaml')):
path = os.path.join(root, name)
try:
with open(path, 'r', encoding='utf-8') as fh:
docs = list(yaml.safe_load_all(fh))
except Exception:
continue
found_any = False
has_network_any = False
stack = docs[:]
while stack:
node = stack.pop()
if isinstance(node, dict):
for k, v in node.items():
if k == 'workloadIdentities':
found_any = True
if isinstance(v, dict) and 'network' in v:
has_network_any = True
elif isinstance(v, list):
for it in v:
if isinstance(it, dict) and 'network' in it:
has_network_any = True
if isinstance(v, (dict, list)):
stack.append(v)
elif isinstance(node, list):
for it in node:
stack.append(it)
if found_any:
files_with.append(path)
if not has_network_any:
missing.append(path)
print("FILES_WITH_WORKLOAD_IDENTITIES:", len(files_with))
for p in files_with:
print(p)
print("---")
print("FILES_MISSING_NETWORK:", len(missing))
for p in missing:
print(p)
PY
# Find any YAML manifests that explicitly include "- network"
echo
echo "FILES_CONTAINING_-_network (yaml files):"
rg -n --hidden --glob '!**/node_modules/**' --type=yaml '\- network\b' || true
# Show the referenced CRD file snippet if it exists
echo
echo "CHECK specific CRD file snippet (if present):"
FILE="api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml"
if [ -f "$FILE" ]; then
echo "FOUND: $FILE"
nl -ba -w3 -s' ' "$FILE" | sed -n '4060,4080p'
else
echo "NOT FOUND: $FILE"
fiLength of output: 7159
Revert network from required — this is a breaking change (26 generated manifests missing workloadIdentities.network).
Verification: 26 generated CRD/manifests (e.g. api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/... and cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/) do not include workloadIdentities.network; the CRD that added it as required is api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yaml (around line 4071).
Non-breaking schema tweak (recommended):
- - networkIf you intend to keep it required now, confirm:
- all fixtures/tests/manifests have been updated,
- no controller will write to spec on upgrade (which would trigger validation failures),
- release notes explicitly call out this breaking requirement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - network |
| - mountPath: /var/run/secrets/openshift/serviceaccount | ||
| name: cloud-token | ||
| - mountPath: /tmp | ||
| name: tmp-dir | ||
| - args: | ||
| - --token-audience=openshift | ||
| - --service-account-namespace=kube-system | ||
| - --service-account-name=azure-cloud-provider | ||
| - --token-file=/var/run/secrets/openshift/serviceaccount/token | ||
| - --kubeconfig=/etc/kubernetes/kubeconfig | ||
| command: | ||
| - /usr/bin/control-plane-operator | ||
| - token-minter | ||
| image: token-minter | ||
| imagePullPolicy: IfNotPresent | ||
| name: cloud-token-minter | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 30Mi | ||
| securityContext: | ||
| readOnlyRootFilesystem: true | ||
| terminationMessagePolicy: FallbackToLogsOnError | ||
| volumeMounts: | ||
| - mountPath: /var/run/secrets/openshift/serviceaccount | ||
| name: cloud-token | ||
| - mountPath: /etc/kubernetes | ||
| name: kubeconfig | ||
| - mountPath: /tmp | ||
| name: tmp-dir | ||
| priorityClassName: hypershift-control-plane |
There was a problem hiding this comment.
Token audience mismatch in fixture.
Align with Azure WI audience or tests will encode the wrong contract.
- - --token-audience=openshift
+ - --token-audience=api://AzureADTokenExchange📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - mountPath: /var/run/secrets/openshift/serviceaccount | |
| name: cloud-token | |
| - mountPath: /tmp | |
| name: tmp-dir | |
| - args: | |
| - --token-audience=openshift | |
| - --service-account-namespace=kube-system | |
| - --service-account-name=azure-cloud-provider | |
| - --token-file=/var/run/secrets/openshift/serviceaccount/token | |
| - --kubeconfig=/etc/kubernetes/kubeconfig | |
| command: | |
| - /usr/bin/control-plane-operator | |
| - token-minter | |
| image: token-minter | |
| imagePullPolicy: IfNotPresent | |
| name: cloud-token-minter | |
| resources: | |
| requests: | |
| cpu: 10m | |
| memory: 30Mi | |
| securityContext: | |
| readOnlyRootFilesystem: true | |
| terminationMessagePolicy: FallbackToLogsOnError | |
| volumeMounts: | |
| - mountPath: /var/run/secrets/openshift/serviceaccount | |
| name: cloud-token | |
| - mountPath: /etc/kubernetes | |
| name: kubeconfig | |
| - mountPath: /tmp | |
| name: tmp-dir | |
| priorityClassName: hypershift-control-plane | |
| - mountPath: /var/run/secrets/openshift/serviceaccount | |
| name: cloud-token | |
| - mountPath: /tmp | |
| name: tmp-dir | |
| - args: | |
| - --token-audience=api://AzureADTokenExchange | |
| - --service-account-namespace=kube-system | |
| - --service-account-name=azure-cloud-provider | |
| - --token-file=/var/run/secrets/openshift/serviceaccount/token | |
| - --kubeconfig=/etc/kubernetes/kubeconfig | |
| command: | |
| - /usr/bin/control-plane-operator | |
| - token-minter | |
| image: token-minter | |
| imagePullPolicy: IfNotPresent | |
| name: cloud-token-minter | |
| resources: | |
| requests: | |
| cpu: 10m | |
| memory: 30Mi | |
| securityContext: | |
| readOnlyRootFilesystem: true | |
| terminationMessagePolicy: FallbackToLogsOnError | |
| volumeMounts: | |
| - mountPath: /var/run/secrets/openshift/serviceaccount | |
| name: cloud-token | |
| - mountPath: /etc/kubernetes | |
| name: kubeconfig | |
| - mountPath: /tmp | |
| name: tmp-dir | |
| priorityClassName: hypershift-control-plane |
| import ( | ||
| "context" | ||
|
|
||
| hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" | ||
| "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" | ||
| "github.com/openshift/hypershift/support/azureutil" | ||
| "github.com/openshift/hypershift/support/capabilities" | ||
| "github.com/openshift/hypershift/support/upsert" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import fmt to support new error paths
Required for the proposed nil-safety checks.
Apply this diff:
import (
"context"
+ "fmt"
hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
"github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests"
"github.com/openshift/hypershift/support/azureutil"
"github.com/openshift/hypershift/support/capabilities"
"github.com/openshift/hypershift/support/upsert"
"sigs.k8s.io/controller-runtime/pkg/client"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| "context" | |
| hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" | |
| "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" | |
| "github.com/openshift/hypershift/support/azureutil" | |
| "github.com/openshift/hypershift/support/capabilities" | |
| "github.com/openshift/hypershift/support/upsert" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| ) | |
| import ( | |
| "context" | |
| "fmt" | |
| hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" | |
| "github.com/openshift/hypershift/control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests" | |
| "github.com/openshift/hypershift/support/azureutil" | |
| "github.com/openshift/hypershift/support/capabilities" | |
| "github.com/openshift/hypershift/support/upsert" | |
| "sigs.k8s.io/controller-runtime/pkg/client" | |
| ) |
🤖 Prompt for AI Agents
In
control-plane-operator/hostedclusterconfigoperator/controllers/resources/azure/azure.go
around lines 3 to 13, the new nil-safety/error code paths use fmt but the
package is not imported; add "fmt" to the import block so the file builds and
the error formatting calls resolve (ensure import ordering matches project
conventions).
| // For self-managed Azure with workload identity, instruct Azure SDK to use the minted SA token | ||
| if azureutil.IsSelfManagedAzure(hcluster.Spec.Platform.Type) && | ||
| hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities != nil { | ||
| deploymentSpec.Template.Spec.Containers[0].Env = append( | ||
| deploymentSpec.Template.Spec.Containers[0].Env, | ||
| corev1.EnvVar{ | ||
| Name: "AZURE_FEDERATED_TOKEN_FILE", | ||
| Value: "/var/run/secrets/openshift/serviceaccount/token", | ||
| }, | ||
| corev1.EnvVar{ | ||
| Name: "AZURE_CLIENT_ID", | ||
| Value: string(hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities.NodePoolManagement.ClientID), | ||
| }, | ||
| corev1.EnvVar{ | ||
| Name: "AZURE_TENANT_ID", | ||
| Value: hcluster.Spec.Platform.Azure.TenantID, | ||
| }, | ||
| ) | ||
|
|
||
| // Inject cloud token-minter sidecar and mount token volume for self-managed Azure workload identity | ||
| tokenVolume := corev1.Volume{ | ||
| Name: "cloud-token", | ||
| VolumeSource: corev1.VolumeSource{ | ||
| EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}, | ||
| }, | ||
| } | ||
| deploymentSpec.Template.Spec.Volumes = append(deploymentSpec.Template.Spec.Volumes, tokenVolume) | ||
|
|
||
| deploymentSpec.Template.Spec.Containers[0].VolumeMounts = append(deploymentSpec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ | ||
| Name: tokenVolume.Name, | ||
| MountPath: "/var/run/secrets/openshift/serviceaccount", | ||
| }) | ||
|
|
||
| deploymentSpec.Template.Spec.Containers = append(deploymentSpec.Template.Spec.Containers, corev1.Container{ | ||
| Name: "cloud-token-minter", | ||
| Image: a.utilitiesImage, | ||
| Command: []string{"/usr/bin/control-plane-operator", "token-minter"}, | ||
| Args: []string{ | ||
| "--token-audience=openshift", | ||
| "--service-account-namespace=kube-system", | ||
| "--service-account-name=capi-provider", | ||
| "--token-file=/var/run/secrets/openshift/serviceaccount/token", | ||
| "--kubeconfig=/etc/kubernetes/kubeconfig", | ||
| }, | ||
| ImagePullPolicy: corev1.PullIfNotPresent, | ||
| Resources: corev1.ResourceRequirements{ | ||
| Requests: corev1.ResourceList{ | ||
| corev1.ResourceCPU: resource.MustParse("10m"), | ||
| corev1.ResourceMemory: resource.MustParse("30Mi"), | ||
| }, | ||
| }, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| { | ||
| Name: tokenVolume.Name, | ||
| MountPath: "/var/run/secrets/openshift/serviceaccount", | ||
| }, | ||
| { | ||
| Name: "svc-kubeconfig", | ||
| MountPath: "/etc/kubernetes", | ||
| }, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Token audience likely wrong for Azure Workload Identity.
Azure WI expects SA tokens with audience api://AzureADTokenExchange. Using “openshift” will fail token exchange.
- "--token-audience=openshift",
+ "--token-audience=api://AzureADTokenExchange",Also mark kubeconfig mounts RO in both containers:
{
Name: "svc-kubeconfig",
MountPath: "/etc/kubernetes",
+ ReadOnly: true,
},(Optional) set AZURE_AUTHORITY_HOST for sovereign clouds to improve portability.
Set Azure Workload Identity token audience and make kubeconfig mounts read-only.
Azure Workload Identity requires the federated SA token audience be api://AzureADTokenExchange (USGov/China variants available). (learn.microsoft.com)
Replace:
- "--token-audience=openshift",
+ "--token-audience=api://AzureADTokenExchange",Also mark the kubeconfig mounts ReadOnly in both containers:
{
Name: "svc-kubeconfig",
MountPath: "/etc/kubernetes",
+ ReadOnly: true,
},(read-only mounts are the recommended pattern for configuration artefacts). (v1-33.docs.kubernetes.io)
Optional: set AZURE_AUTHORITY_HOST when targeting sovereign clouds to ensure the Azure SDK uses the correct authority. (azure.github.io)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For self-managed Azure with workload identity, instruct Azure SDK to use the minted SA token | |
| if azureutil.IsSelfManagedAzure(hcluster.Spec.Platform.Type) && | |
| hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities != nil { | |
| deploymentSpec.Template.Spec.Containers[0].Env = append( | |
| deploymentSpec.Template.Spec.Containers[0].Env, | |
| corev1.EnvVar{ | |
| Name: "AZURE_FEDERATED_TOKEN_FILE", | |
| Value: "/var/run/secrets/openshift/serviceaccount/token", | |
| }, | |
| corev1.EnvVar{ | |
| Name: "AZURE_CLIENT_ID", | |
| Value: string(hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities.NodePoolManagement.ClientID), | |
| }, | |
| corev1.EnvVar{ | |
| Name: "AZURE_TENANT_ID", | |
| Value: hcluster.Spec.Platform.Azure.TenantID, | |
| }, | |
| ) | |
| // Inject cloud token-minter sidecar and mount token volume for self-managed Azure workload identity | |
| tokenVolume := corev1.Volume{ | |
| Name: "cloud-token", | |
| VolumeSource: corev1.VolumeSource{ | |
| EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}, | |
| }, | |
| } | |
| deploymentSpec.Template.Spec.Volumes = append(deploymentSpec.Template.Spec.Volumes, tokenVolume) | |
| deploymentSpec.Template.Spec.Containers[0].VolumeMounts = append(deploymentSpec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ | |
| Name: tokenVolume.Name, | |
| MountPath: "/var/run/secrets/openshift/serviceaccount", | |
| }) | |
| deploymentSpec.Template.Spec.Containers = append(deploymentSpec.Template.Spec.Containers, corev1.Container{ | |
| Name: "cloud-token-minter", | |
| Image: a.utilitiesImage, | |
| Command: []string{"/usr/bin/control-plane-operator", "token-minter"}, | |
| Args: []string{ | |
| "--token-audience=openshift", | |
| "--service-account-namespace=kube-system", | |
| "--service-account-name=capi-provider", | |
| "--token-file=/var/run/secrets/openshift/serviceaccount/token", | |
| "--kubeconfig=/etc/kubernetes/kubeconfig", | |
| }, | |
| ImagePullPolicy: corev1.PullIfNotPresent, | |
| Resources: corev1.ResourceRequirements{ | |
| Requests: corev1.ResourceList{ | |
| corev1.ResourceCPU: resource.MustParse("10m"), | |
| corev1.ResourceMemory: resource.MustParse("30Mi"), | |
| }, | |
| }, | |
| VolumeMounts: []corev1.VolumeMount{ | |
| { | |
| Name: tokenVolume.Name, | |
| MountPath: "/var/run/secrets/openshift/serviceaccount", | |
| }, | |
| { | |
| Name: "svc-kubeconfig", | |
| MountPath: "/etc/kubernetes", | |
| }, | |
| }, | |
| }) | |
| } | |
| // For self-managed Azure with workload identity, instruct Azure SDK to use the minted SA token | |
| if azureutil.IsSelfManagedAzure(hcluster.Spec.Platform.Type) && | |
| hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities != nil { | |
| deploymentSpec.Template.Spec.Containers[0].Env = append( | |
| deploymentSpec.Template.Spec.Containers[0].Env, | |
| corev1.EnvVar{ | |
| Name: "AZURE_FEDERATED_TOKEN_FILE", | |
| Value: "/var/run/secrets/openshift/serviceaccount/token", | |
| }, | |
| corev1.EnvVar{ | |
| Name: "AZURE_CLIENT_ID", | |
| Value: string(hcluster.Spec.Platform.Azure.AzureAuthenticationConfig.WorkloadIdentities.NodePoolManagement.ClientID), | |
| }, | |
| corev1.EnvVar{ | |
| Name: "AZURE_TENANT_ID", | |
| Value: hcluster.Spec.Platform.Azure.TenantID, | |
| }, | |
| ) | |
| // Inject cloud token-minter sidecar and mount token volume for self-managed Azure workload identity | |
| tokenVolume := corev1.Volume{ | |
| Name: "cloud-token", | |
| VolumeSource: corev1.VolumeSource{ | |
| EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}, | |
| }, | |
| } | |
| deploymentSpec.Template.Spec.Volumes = append(deploymentSpec.Template.Spec.Volumes, tokenVolume) | |
| deploymentSpec.Template.Spec.Containers[0].VolumeMounts = append(deploymentSpec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{ | |
| Name: tokenVolume.Name, | |
| MountPath: "/var/run/secrets/openshift/serviceaccount", | |
| }) | |
| deploymentSpec.Template.Spec.Containers = append(deploymentSpec.Template.Spec.Containers, corev1.Container{ | |
| Name: "cloud-token-minter", | |
| Image: a.utilitiesImage, | |
| Command: []string{"/usr/bin/control-plane-operator", "token-minter"}, | |
| Args: []string{ | |
| "--token-audience=api://AzureADTokenExchange", | |
| "--service-account-namespace=kube-system", | |
| "--service-account-name=capi-provider", | |
| "--token-file=/var/run/secrets/openshift/serviceaccount/token", | |
| "--kubeconfig=/etc/kubernetes/kubeconfig", | |
| }, | |
| ImagePullPolicy: corev1.PullIfNotPresent, | |
| Resources: corev1.ResourceRequirements{ | |
| Requests: corev1.ResourceList{ | |
| corev1.ResourceCPU: resource.MustParse("10m"), | |
| corev1.ResourceMemory: resource.MustParse("30Mi"), | |
| }, | |
| }, | |
| VolumeMounts: []corev1.VolumeMount{ | |
| { | |
| Name: tokenVolume.Name, | |
| MountPath: "/var/run/secrets/openshift/serviceaccount", | |
| }, | |
| { | |
| Name: "svc-kubeconfig", | |
| MountPath: "/etc/kubernetes", | |
| ReadOnly: true, | |
| }, | |
| }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In
hypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.go
around lines 205 to 267, change the token-minter --token-audience to
"api://AzureADTokenExchange" (use the sovereign-cloud variant when targeting
USGov/China as appropriate) and mark the kubeconfig mounts ReadOnly for both
containers by setting the corresponding VolumeMount ReadOnly field to true;
additionally consider adding an AZURE_AUTHORITY_HOST env var when deploying to a
sovereign cloud so the Azure SDK uses the correct authority.
|
/verified by @xiuwang and @bryan-cox |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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. |
|
lgtm |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/override ci/prow/e2e-kubevirt-aws-ovn-reduced openshift/release#68540 regressed the kubevirt presub, but the |
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-kubevirt-aws-ovn-reduced 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 kubernetes-sigs/prow repository. |
|
/test verify |
|
/test e2e-aws |
|
/override ci/prow/e2e-aws
|
|
@sjenning: Overrode contexts on behalf of sjenning: ci/prow/e2e-aws 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 kubernetes-sigs/prow repository. |
|
@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. |
6 participants
What this PR does / why we need it:
This PR contains the necessary changes to get self-managed Azure Hosted Clusters working again when running on an Azure OpenShift management cluster.
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-1331
Checklist