CNTRLPLANE-2910: feat(cpo): add Azure workload identity webhook as KAS sidecar - #7867
Merged
openshift-merge-bot[bot] merged 2 commits intoMar 17, 2026
Merged
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@csrwng: This pull request references CNTRLPLANE-2910 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Azure Workload Identity Webhook support: serving certificate and kubeconfig secrets, kube-apiserver webhook container and volumes, RBAC and MutatingWebhookConfiguration reconciliation, unit and e2e tests, and test/asset fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Guest as Guest Test / PodCreator
participant KAS as kube-apiserver
participant Webhook as azure-workload-identity-webhook
participant CA as Control Plane PKI
Guest->>KAS: Create Pod annotated for Azure workload identity
KAS->>Webhook: AdmissionRequest (mutating)
Webhook->>CA: Use serving cert + kubeconfig to validate/sign or verify tokens
Webhook-->>KAS: AdmissionResponse (inject projected token volume + AZURE_FEDERATED_TOKEN_FILE env)
KAS-->>Guest: Pod created with projected token volume and env var
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng 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
added
area/control-plane-operator
Contributor
Author
|
/test verify |
|
@csrwng: This pull request references CNTRLPLANE-2910 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Member
|
/test e2e-azure-self-managed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/e2e/util/azure.go (1)
23-58: Add cleanup for test namespace resourcesThe test creates a unique namespace but never deletes it. Add
t.Cleanupto avoid leaked resources and quota pressure in long-running e2e lanes.🧹 Suggested cleanup hook
nsName := fmt.Sprintf("azure-wi-e2e-%d", time.Now().UnixNano()) testNamespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: nsName}} g.Expect(guestClient.Create(ctx, testNamespace)).To(Succeed(), "failed to create test namespace") + t.Cleanup(func() { + _ = guestClient.Delete(context.Background(), testNamespace) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/azure.go` around lines 23 - 58, The test creates a unique namespace (nsName/testNamespace) and resources with guestClient.Create but never cleans them up; add a t.Cleanup hook that deletes the created namespace to prevent leaked resources and quota pressure. After successfully creating testNamespace (and before returning), call t.Cleanup with a function that calls guestClient.Delete(ctx, testNamespace) (handle NotFound/ignore errors) so the namespace (and its contained Pod/ServiceAccount) is removed when the test finishes; reference the existing nsName/testNamespace and guestClient.Create symbols when adding the cleanup.control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go (1)
165-205: Extract shared webhook kubeconfig adaptation logicAWS and Azure webhook kubeconfig adapters now duplicate the same fetch/build flow. Consolidating into one helper will reduce maintenance drift.
♻️ Suggested refactor
func adaptAWSPodIdentityWebhookKubeconfigSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { - csrSigner := manifests.CSRSignerCASecret(cpContext.HCP.Namespace) - if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(csrSigner), csrSigner); err != nil { - return fmt.Errorf("failed to get cluster-signer-ca secret: %v", err) - } - rootCA := manifests.RootCASecret(cpContext.HCP.Namespace) - if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(rootCA), rootCA); err != nil { - return fmt.Errorf("failed to get root ca cert secret: %w", err) - } - rootCACM := &corev1.ConfigMap{ - Data: map[string]string{ - certs.CASignerCertMapKey: string(rootCA.Data[certs.CASignerCertMapKey]), - }, - } - - if !cpContext.SkipCertificateSigning { - return pki.ReconcileServiceAccountKubeconfig(secret, csrSigner, rootCACM, cpContext.HCP, "openshift-authentication", "aws-pod-identity-webhook") - } - return nil + return adaptWorkloadIdentityWebhookKubeconfigSecret(cpContext, secret, "aws-pod-identity-webhook") } func adaptAzureWorkloadIdentityWebhookKubeconfigSecret(cpContext component.WorkloadContext, secret *corev1.Secret) error { + return adaptWorkloadIdentityWebhookKubeconfigSecret(cpContext, secret, "azure-workload-identity-webhook") +} + +func adaptWorkloadIdentityWebhookKubeconfigSecret(cpContext component.WorkloadContext, secret *corev1.Secret, serviceAccountName string) error { csrSigner := manifests.CSRSignerCASecret(cpContext.HCP.Namespace) if err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(csrSigner), csrSigner); err != nil { return fmt.Errorf("failed to get cluster-signer-ca secret: %v", err) @@ if !cpContext.SkipCertificateSigning { - return pki.ReconcileServiceAccountKubeconfig(secret, csrSigner, rootCACM, cpContext.HCP, "openshift-authentication", "azure-workload-identity-webhook") + return pki.ReconcileServiceAccountKubeconfig(secret, csrSigner, rootCACM, cpContext.HCP, "openshift-authentication", serviceAccountName) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go` around lines 165 - 205, Both adaptAWSPodIdentityWebhookKubeconfigSecret and adaptAzureWorkloadIdentityWebhookKubeconfigSecret duplicate the same fetch/build/return flow; extract that shared logic into a helper (e.g., reconcileWebhookKubeconfig(secret *corev1.Secret, cpContext component.WorkloadContext, serviceAccountName string) error) that: retrieves csrSigner via manifests.CSRSignerCASecret(cpContext.HCP.Namespace), retrieves rootCA via manifests.RootCASecret(cpContext.HCP.Namespace), builds rootCACM using certs.CASignerCertMapKey from rootCA.Data, checks cpContext.SkipCertificateSigning and, if false, calls pki.ReconcileServiceAccountKubeconfig(secret, csrSigner, rootCACM, cpContext.HCP, "openshift-authentication", serviceAccountName) and returns its error; then simplify adaptAWSPodIdentityWebhookKubeconfigSecret and adaptAzureWorkloadIdentityWebhookKubeconfigSecret to call this helper with serviceAccountName "aws-pod-identity-webhook" and "azure-workload-identity-webhook" respectively, preserving existing error semantics (wrap/return errors as before).control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go (1)
39-45: Consider extracting a shared helper for container lookup in tests.The repeated
forloop to findazure-workload-identity-webhookcan be centralized to reduce duplication and test drift.Also applies to: 78-84, 113-119, 165-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go` around lines 39 - 45, Several tests repeat the same for-loop to locate a container named "azure-workload-identity-webhook" from podSpec.Containers; extract a small shared helper (e.g., findContainerByName(containers []corev1.Container, name string) *corev1.Container or findContainerInPodSpec(podSpec corev1.PodSpec, name string) *corev1.Container) and replace each loop (the instances that set webhookContainer by iterating podSpec.Containers) to call this helper; ensure the helper returns nil when not found and update assertions in the tests to use its result (currently assigned to webhookContainer) so all occurrences (the loops currently used in the test file) are centralized.control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go (1)
117-125: Strengthen idempotency assertion by checkingtls.keytoo.The current check validates only
tls.crt. Adding atls.keyequality assertion would make this idempotency test more complete.✅ Suggested test enhancement
firstCert := make([]byte, len(secret.Data[corev1.TLSCertKey])) copy(firstCert, secret.Data[corev1.TLSCertKey]) + firstKey := make([]byte, len(secret.Data[corev1.TLSPrivateKeyKey])) + copy(firstKey, secret.Data[corev1.TLSPrivateKeyKey]) @@ if string(firstCert) != string(secret.Data[corev1.TLSCertKey]) { t.Error("expected idempotent reconciliation to produce the same certificate") } + if string(firstKey) != string(secret.Data[corev1.TLSPrivateKeyKey]) { + t.Error("expected idempotent reconciliation to preserve the same private key") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go` around lines 117 - 125, The test currently captures and compares only the certificate bytes (firstCert from secret.Data[corev1.TLSCertKey]) for idempotency; extend the assertion to also capture the private key bytes (e.g., make a second slice like firstKey from secret.Data[corev1.TLSPrivateKeyKey]) before calling ReconcileAzureWorkloadIdentityWebhookServingCert and then after the second reconcile assert that both string(firstCert) == string(secret.Data[corev1.TLSCertKey]) and string(firstKey) == string(secret.Data[corev1.TLSPrivateKeyKey]) so the test verifies both tls.crt and tls.key are unchanged by an idempotent reconcile.control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go (1)
2183-2253: Consider extracting shared identity-webhook reconciliation scaffolding.This function is structurally very close to
reconcileAWSIdentityWebhook(Line 2120-2181). A small shared helper for RBAC + webhook upsert patterns would reduce drift risk between cloud providers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go` around lines 2183 - 2253, reconcileAzureIdentityWebhook duplicates the RBAC + webhook upsert logic in reconcileAWSIdentityWebhook; extract a shared helper (e.g., reconcileIdentityWebhook) that accepts the provider-specific manifest objects or providers (clusterRole, clusterRoleBinding, webhook) plus small params (serviceAccount name/namespace, webhook.Name/URL, label key/value, and r.rootCA) and performs the CreateOrUpdate steps using r.CreateOrUpdate; then replace the body of reconcileAzureIdentityWebhook and reconcileAWSIdentityWebhook to build their provider-specific manifests (manifests.AzureWorkloadIdentityWebhookClusterRole(), manifests.AzureWorkloadIdentityWebhookClusterRoleBinding(), manifests.AzureWorkloadIdentityWebhook()) and call reconcileIdentityWebhook(ctx, r, clusterRole, clusterRoleBinding, webhook, "azure-workload-identity-webhook", "openshift-authentication", "pod-identity-webhook.azure.mutate.io", "azure.workload.identity/use", "true", r.rootCA) (and analogous args for AWS). Ensure the helper returns []error and reuses the same unique symbols CreateOrUpdate, ClusterRole, ClusterRoleBinding, and Webhook mutation logic so both reconcileAzureIdentityWebhook and reconcileAWSIdentityWebhook delegate to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go`:
- Around line 342-343: The code dereferences hcp.Spec.Platform.Azure when
building environment vars (AZURE_TENANT_ID/AZURE_ENVIRONMENT) which can panic if
Platform or Azure is nil; update the code in deployment.go where the env vars
are added (the block that uses hcp.Spec.Platform.Azure.TenantID and
hcp.Spec.Platform.Azure.Cloud) to first check that hcp.Spec.Platform and
hcp.Spec.Platform.Azure are non-nil, and if they are nil return a controlled
reconciliation error (or wrap with fmt.Errorf) instead of proceeding; ensure
subsequent use of TenantID and Cloud only happens after these nil checks so no
direct dereference can panic.
In `@test/e2e/util/azure.go`:
- Around line 88-99: The current hasProjectedTokenVolume function returns true
for any projected serviceAccountToken (including the default kube-api-access),
so update hasProjectedTokenVolume to ignore the cluster-default projected volume
by skipping volumes whose Name equals "kube-api-access" (i.e., inside the loop
check volume.Name != "kube-api-access" before inspecting Projected.Sources) and
only return true for non-default projected serviceAccountToken sources; keep the
function signature and overall loop structure unchanged.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go`:
- Around line 117-125: The test currently captures and compares only the
certificate bytes (firstCert from secret.Data[corev1.TLSCertKey]) for
idempotency; extend the assertion to also capture the private key bytes (e.g.,
make a second slice like firstKey from secret.Data[corev1.TLSPrivateKeyKey])
before calling ReconcileAzureWorkloadIdentityWebhookServingCert and then after
the second reconcile assert that both string(firstCert) ==
string(secret.Data[corev1.TLSCertKey]) and string(firstKey) ==
string(secret.Data[corev1.TLSPrivateKeyKey]) so the test verifies both tls.crt
and tls.key are unchanged by an idempotent reconcile.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go`:
- Around line 39-45: Several tests repeat the same for-loop to locate a
container named "azure-workload-identity-webhook" from podSpec.Containers;
extract a small shared helper (e.g., findContainerByName(containers
[]corev1.Container, name string) *corev1.Container or
findContainerInPodSpec(podSpec corev1.PodSpec, name string) *corev1.Container)
and replace each loop (the instances that set webhookContainer by iterating
podSpec.Containers) to call this helper; ensure the helper returns nil when not
found and update assertions in the tests to use its result (currently assigned
to webhookContainer) so all occurrences (the loops currently used in the test
file) are centralized.
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go`:
- Around line 165-205: Both adaptAWSPodIdentityWebhookKubeconfigSecret and
adaptAzureWorkloadIdentityWebhookKubeconfigSecret duplicate the same
fetch/build/return flow; extract that shared logic into a helper (e.g.,
reconcileWebhookKubeconfig(secret *corev1.Secret, cpContext
component.WorkloadContext, serviceAccountName string) error) that: retrieves
csrSigner via manifests.CSRSignerCASecret(cpContext.HCP.Namespace), retrieves
rootCA via manifests.RootCASecret(cpContext.HCP.Namespace), builds rootCACM
using certs.CASignerCertMapKey from rootCA.Data, checks
cpContext.SkipCertificateSigning and, if false, calls
pki.ReconcileServiceAccountKubeconfig(secret, csrSigner, rootCACM,
cpContext.HCP, "openshift-authentication", serviceAccountName) and returns its
error; then simplify adaptAWSPodIdentityWebhookKubeconfigSecret and
adaptAzureWorkloadIdentityWebhookKubeconfigSecret to call this helper with
serviceAccountName "aws-pod-identity-webhook" and
"azure-workload-identity-webhook" respectively, preserving existing error
semantics (wrap/return errors as before).
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 2183-2253: reconcileAzureIdentityWebhook duplicates the RBAC +
webhook upsert logic in reconcileAWSIdentityWebhook; extract a shared helper
(e.g., reconcileIdentityWebhook) that accepts the provider-specific manifest
objects or providers (clusterRole, clusterRoleBinding, webhook) plus small
params (serviceAccount name/namespace, webhook.Name/URL, label key/value, and
r.rootCA) and performs the CreateOrUpdate steps using r.CreateOrUpdate; then
replace the body of reconcileAzureIdentityWebhook and
reconcileAWSIdentityWebhook to build their provider-specific manifests
(manifests.AzureWorkloadIdentityWebhookClusterRole(),
manifests.AzureWorkloadIdentityWebhookClusterRoleBinding(),
manifests.AzureWorkloadIdentityWebhook()) and call reconcileIdentityWebhook(ctx,
r, clusterRole, clusterRoleBinding, webhook, "azure-workload-identity-webhook",
"openshift-authentication", "pod-identity-webhook.azure.mutate.io",
"azure.workload.identity/use", "true", r.rootCA) (and analogous args for AWS).
Ensure the helper returns []error and reuses the same unique symbols
CreateOrUpdate, ClusterRole, ClusterRoleBinding, and Webhook mutation logic so
both reconcileAzureIdentityWebhook and reconcileAWSIdentityWebhook delegate to
it.
In `@test/e2e/util/azure.go`:
- Around line 23-58: The test creates a unique namespace (nsName/testNamespace)
and resources with guestClient.Create but never cleans them up; add a t.Cleanup
hook that deletes the created namespace to prevent leaked resources and quota
pressure. After successfully creating testNamespace (and before returning), call
t.Cleanup with a function that calls guestClient.Delete(ctx, testNamespace)
(handle NotFound/ignore errors) so the namespace (and its contained
Pod/ServiceAccount) is removed when the test finishes; reference the existing
nsName/testNamespace and guestClient.Create symbols when adding the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32264c83-0bee-4da9-a268-03c38baee639
📒 Files selected for processing (18)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.gocontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_azure_workload_identity_webhook_kubeconfig_secret.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_controlplanecomponent.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-apiserver/azure-workload-identity-webhook-kubeconfig.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/azure_workload_identity_webhook_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/azure_workload_identity_webhook.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gotest/e2e/create_cluster_test.gotest/e2e/util/azure.go
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 6
... and 1 more failed tests |
csrwng
force-pushed
the
azure-web-identity-webhook
branch
from
March 5, 2026 22:21
9f22206 to
79cd3e1
Compare
|
@csrwng: This pull request references CNTRLPLANE-2910 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go (1)
38-45: Consider extracting repeated container lookup into a helper.The same container lookup loop pattern appears in all four test validators. Extracting this to a helper function would reduce duplication and improve maintainability.
♻️ Suggested helper extraction
func findContainer(podSpec *corev1.PodSpec, name string) *corev1.Container { for i := range podSpec.Containers { if podSpec.Containers[i].Name == name { return &podSpec.Containers[i] } } return nil }Then use it in validators:
-var webhookContainer *corev1.Container -for i := range podSpec.Containers { - if podSpec.Containers[i].Name == "azure-workload-identity-webhook" { - webhookContainer = &podSpec.Containers[i] - break - } -} +webhookContainer := findContainer(podSpec, "azure-workload-identity-webhook")Also applies to: 76-83, 111-118, 163-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go` around lines 38 - 45, Extract the repeated container lookup into a small helper like findContainer(podSpec *corev1.PodSpec, name string) *corev1.Container and replace the duplicate loops inside each validatePod closure (the validators around azure-workload-identity-webhook at the occurrences shown) with calls to findContainer; ensure the helper returns nil when not found and update tests to assert non-nil before using the returned *corev1.Container to keep behavior identical.test/e2e/util/azure.go (1)
19-86: Consider adding cleanup for test resources.The test creates a namespace, ServiceAccount, and Pod but does not clean them up after the test completes. While namespaces with unique names won't collide, they will accumulate over multiple test runs.
♻️ Suggested cleanup using t.Cleanup
func EnsureAzureWorkloadIdentityWebhookMutation(t *testing.T, ctx context.Context, guestClient crclient.Client) { t.Helper() g := NewWithT(t) nsName := fmt.Sprintf("azure-wi-e2e-%d", time.Now().UnixNano()) testNamespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: nsName}} g.Expect(guestClient.Create(ctx, testNamespace)).To(Succeed(), "failed to create test namespace") + t.Cleanup(func() { + if err := guestClient.Delete(context.Background(), testNamespace); err != nil { + t.Logf("failed to cleanup test namespace %s: %v", nsName, err) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/util/azure.go` around lines 19 - 86, The test EnsureAzureWorkloadIdentityWebhookMutation creates Kubernetes resources (Namespace, ServiceAccount, Pod) but never deletes them; add cleanup by registering t.Cleanup callbacks immediately after creating each resource to delete them via guestClient.Delete (handle and ignore NotFound errors) or delete the Namespace which will cascade; reference the created objects testNamespace, serviceAccount, and pod and call guestClient.Delete(ctx, testNamespace) / guestClient.Delete(ctx, serviceAccount) / guestClient.Delete(ctx, pod) inside t.Cleanup closures so resources are removed after the test completes.control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go (1)
15-86: Consider simplifying single-case table-driven test.The table-driven test structure with only one test case adds unnecessary complexity. Consider converting to a simple test function or adding additional test cases (e.g., error scenarios, different CA configurations).
♻️ Simplified version
func TestReconcileAzureWorkloadIdentityWebhookServingCert(t *testing.T) { - testCases := []struct { - name string - }{ - { - name: "When reconciling the serving cert it should generate a valid TLS certificate for 127.0.0.1", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { + t.Run("When reconciling the serving cert it should generate a valid TLS certificate for 127.0.0.1", func(t *testing.T) { namespace := "test-namespace" // ... rest of test body unchanged ... - }) - } + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go` around lines 15 - 86, The test TestReconcileAzureWorkloadIdentityWebhookServingCert currently uses a single-entry table-driven pattern which is unnecessary; simplify by removing the testCases slice and the t.Run loop and move the test body directly into the TestReconcileAzureWorkloadIdentityWebhookServingCert function so it executes inline, keeping the existing setup and assertions that call reconcileSelfSignedCA and ReconcileAzureWorkloadIdentityWebhookServingCert and validate secret.Data and the parsed cert; alternatively if you want to keep table-driven style, add more cases (e.g., CA error, missing CA, different SANs) referencing the same functions instead of a single-case loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.go`:
- Around line 15-86: The test
TestReconcileAzureWorkloadIdentityWebhookServingCert currently uses a
single-entry table-driven pattern which is unnecessary; simplify by removing the
testCases slice and the t.Run loop and move the test body directly into the
TestReconcileAzureWorkloadIdentityWebhookServingCert function so it executes
inline, keeping the existing setup and assertions that call
reconcileSelfSignedCA and ReconcileAzureWorkloadIdentityWebhookServingCert and
validate secret.Data and the parsed cert; alternatively if you want to keep
table-driven style, add more cases (e.g., CA error, missing CA, different SANs)
referencing the same functions instead of a single-case loop.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.go`:
- Around line 38-45: Extract the repeated container lookup into a small helper
like findContainer(podSpec *corev1.PodSpec, name string) *corev1.Container and
replace the duplicate loops inside each validatePod closure (the validators
around azure-workload-identity-webhook at the occurrences shown) with calls to
findContainer; ensure the helper returns nil when not found and update tests to
assert non-nil before using the returned *corev1.Container to keep behavior
identical.
In `@test/e2e/util/azure.go`:
- Around line 19-86: The test EnsureAzureWorkloadIdentityWebhookMutation creates
Kubernetes resources (Namespace, ServiceAccount, Pod) but never deletes them;
add cleanup by registering t.Cleanup callbacks immediately after creating each
resource to delete them via guestClient.Delete (handle and ignore NotFound
errors) or delete the Namespace which will cascade; reference the created
objects testNamespace, serviceAccount, and pod and call guestClient.Delete(ctx,
testNamespace) / guestClient.Delete(ctx, serviceAccount) /
guestClient.Delete(ctx, pod) inside t.Cleanup closures so resources are removed
after the test completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b8d6d2c2-0186-4bfe-a812-572c5cedeceb
📒 Files selected for processing (18)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/azure.gocontrol-plane-operator/controllers/hostedcontrolplane/manifests/pki.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook.gocontrol-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook_test.gocontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_azure_workload_identity_webhook_kubeconfig_secret.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_controlplanecomponent.yamlcontrol-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-apiserver/azure-workload-identity-webhook-kubeconfig.yamlcontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/azure_workload_identity_webhook_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/azure_workload_identity_webhook_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/azure_workload_identity_webhook.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gotest/e2e/create_cluster_test.gotest/e2e/util/azure.go
✅ Files skipped from review due to trivial changes (1)
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_azure_workload_identity_webhook_kubeconfig_secret.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_controlplanecomponent.yaml
- test/e2e/create_cluster_test.go
- control-plane-operator/controllers/hostedcontrolplane/manifests/pki.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.go
- control-plane-operator/controllers/hostedcontrolplane/testdata/kube-apiserver/AROSwift/zz_fixture_TestControlPlaneComponents_kube_apiserver_deployment.yaml
- control-plane-operator/controllers/hostedcontrolplane/pki/azure_workload_identity_webhook.go
Contributor
Author
|
/test e2e-azure-self-managed |
enxebre
reviewed
Mar 6, 2026
enxebre
reviewed
Mar 6, 2026
enxebre
reviewed
Mar 6, 2026
Member
|
dropped some minor comments |
|
Scheduling tests matching the |
openshift-merge-robot
added
the
needs-rebase
|
/retest-required |
2 similar comments
Contributor
|
/retest-required |
Contributor
|
/retest-required |
muraee
reviewed
Mar 13, 2026
| ) | ||
|
|
||
| func EnsureAzureWorkloadIdentityWebhookMutation(t *testing.T, ctx context.Context, guestClient crclient.Client) { | ||
| t.Helper() |
Contributor
There was a problem hiding this comment.
need to start a new subtest for this with t.Run() and gate to run only for 4.22 AtLeast(4.22)
|
/hold Revision aef7f18 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/retest |
Wrap EnsureAzureWorkloadIdentityWebhookMutation in a t.Run subtest with AtLeast(Version422) so the test is skipped on older versions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
Contributor
|
/lgtm |
1 similar comment
Member
|
/lgtm |
|
Scheduling tests matching the |
Collaborator
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Member
|
/test e2e-aws-4-21 |
1 similar comment
Member
|
/test e2e-aws-4-21 |
|
/verified by @xiuwang |
openshift-ci-robot
added
the
verified
|
@xiuwang: 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. |
openshift-merge-bot
Bot
merged commit Mar 17, 2026
213554f
into
openshift:main
24 of 25 checks passed
|
@celebdor: #7867 failed to apply on top of branch "release-4.21": 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. |
4 tasks
4 tasks
celebdor
added a commit
to celebdor/hypershift
that referenced
this pull request
Jun 15, 2026
…4.20 The Azure workload identity webhook was originally implemented for 4.22 (PR openshift#7867) but was subsequently backported to 4.20 (PR openshift#7998) and 4.21 (PR openshift#7997). Update the e2e test version gate from Version422 to Version420 so the test runs against all supported versions. Refs: CNTRLPLANE-3093, CNTRLPLANE-3096 Signed-off-by: Antoni Segura Puimedon <antoni@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deploy the Azure workload identity webhook as a sidecar container in the KAS deployment for Azure platform clusters, mirroring the existing AWS pod identity webhook pattern. This enables customer workloads to authenticate to Azure services via annotated ServiceAccounts.
Changes:
Summary by CodeRabbit
New Features
Tests