Skip to content

fix: disable VAP/VAPB check in image registry policy test case for MSFT environments#5385

Merged
openshift-merge-bot[bot] merged 2 commits into
Azure:mainfrom
mvacula02:mvacula/aro-27298
May 26, 2026
Merged

fix: disable VAP/VAPB check in image registry policy test case for MSFT environments#5385
openshift-merge-bot[bot] merged 2 commits into
Azure:mainfrom
mvacula02:mvacula/aro-27298

Conversation

@mvacula02
Copy link
Copy Markdown
Collaborator

@mvacula02 mvacula02 commented May 26, 2026

ARO-27298

What

Remove object existence check for ValidationAdmissionPolicy and ValidatingAdmissionPolicyBinding

Why

  • The e2e service account in STG/PROD lacks RBAC to get cluster-scoped VAP and VAPB, causing the test to fail consistently
  • Since the policy is audit only in those environments there is no denial behavior to assert, only that the
    deployment happened. The namespaced ConfigMap check covers that already

Testing

This is a test bug fix.

PR Checklist

  • PR is scoped to a single task (no mixed concerns)
  • Title follows Conventional Commits format
  • Summary explains the "Why" behind the change
  • Linked to relevant ticket/issue
  • Screenshots included (if graph/UI/metrics changes)
  • Self-reviewed the diff
  • CI/CD checks are passing (ignore Tide)
  • Draft PR used for WIP (if applicable)
  • Commit history is clean (rebased/squashed)
  • Tricky code blocks are commented
  • Specific reviewers tagged
  • All comment threads resolved before merge

remove failing and redundant existence assertion
https://redhat.atlassian.net/browse/ARO-27298
Copilot AI review requested due to automatic review settings May 26, 2026 08:24
@openshift-ci openshift-ci Bot requested review from mgahagan73 and patriksuba May 26, 2026 08:24
@mvacula02
Copy link
Copy Markdown
Collaborator Author

/test stage-e2e-parallel

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the Image Registry Policy E2E test to stop asserting the presence of ValidatingAdmissionPolicy/Binding resources and only validate the allowlist ConfigMap content.

Changes:

  • Removed checks that image-registry-allowlist-policy (ValidatingAdmissionPolicy) exists
  • Removed checks that image-registry-allowlist-policy-binding (ValidatingAdmissionPolicyBinding) exists
  • Left ConfigMap allowlist validation as the primary assertion

Comment thread test/e2e/image_registry_policy.go
@mvacula02
Copy link
Copy Markdown
Collaborator Author

potential follow-up: rework the deny test to run everywhere by removing the binding read and inferring the mode from behavior

@mbukatov mbukatov requested a review from raelga May 26, 2026 08:54
@mbukatov
Copy link
Copy Markdown
Collaborator

mbukatov commented May 26, 2026

@raelga @avollmer-redhat please review and provide comments or lgtm

@mbukatov mbukatov requested a review from avollmer-redhat May 26, 2026 09:05
Copy link
Copy Markdown
Collaborator

@mbukatov mbukatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asking for a change.

Comment thread test/e2e/image_registry_policy.go
@mvacula02 mvacula02 changed the title fix: remove VAP/VAPB check in image registry policy test case fix: disable VAP/VAPB check in image registry policy test case for MSFT environments May 26, 2026
@raelga
Copy link
Copy Markdown
Collaborator

raelga commented May 26, 2026

+1 on this PR. I traced the identity/kubeconfig path end-to-end and the gate is exactly the right scope. Sharing the trace for ARO-22152 context.

Why this can't be an RBAC chase:

The Prow runner (aro-hcp-test-persistent-commands.sh) authenticates with the customer-tenant SP from /var/run/aro-hcp-${VAULT_SECRET_PROFILE}/{client-id,client-secret,tenant} — for integration-e2e-parallel, that's the ARO HCP E2E subscription in the Test Tenant (93b21e64-…). It never calls az aks get-credentials, so there is no KUBECONFIG in the test pod in MSFT envs.

The image-registry-allowlist-policy VAP is deployed by KubeApplier.management.deploy-uksouth-1 onto int-uksouth-mgmt-1 — different Entra tenant (MS 72f988bf-…), different subscription, AKS local accounts disabled. No path for the customer-tenant SP to reach that cluster's API regardless of k8s RBAC.

This isn't an RBAC oversight to fix. It's the same constraint that makes exporter_metrics.go (uses the same clientcmd.NewDefaultClientConfigLoadingRules() pattern, targets aro-hcp-exporter ns on mgmt) carry labels.DevelopmentOnly. Once this PR adds the same label to the second It, the file matches the established convention.

Confirmed on int-uksouth-mgmt-1:

enableAzureRbac:        true
disableLocalAccounts:   true
aadAdminGroupObjectIDs: null
Azure RBAC at cluster scope:
  31dfce36-…  ServicePrincipal  AKS RBAC Cluster Admin
  433b36b3-…  ServicePrincipal  AKS RBAC Cluster Admin
(no E2E identity, no other bindings)

Aside: the upstream view/admin/edit aggregated ClusterRoles do not include admissionregistration.k8s.io/validatingadmissionpolicies (known upstream gap). A new ClusterRole with rbac.authorization.k8s.io/aggregate-to-view: "true" would close that, but it's moot here — nothing is bound to view in the first place.

For ARO-22152: continuous policy-effect verification in INT/STG/PROD would need either (a) a separate MSFT-side mgmt-cluster verifier identity (new Prow secret, new SP in the MS tenant, separate code path), or (b) propagating VAP enforcement to the customer worker clusters via HyperShift rendering so it's testable through the customer-tenant kubeconfig. Both are real follow-up conversations and out of scope here.

No change requested — happy to ship this as-is.

/lgtm
/approve

Copy link
Copy Markdown
Collaborator

@mbukatov mbukatov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 26, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbukatov, mvacula02, raelga

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mvacula02
Copy link
Copy Markdown
Collaborator Author

/test stage-e2e-parallel

@mvacula02
Copy link
Copy Markdown
Collaborator Author

/hold until we see the whole test passing in Stage

@mvacula02
Copy link
Copy Markdown
Collaborator Author

/test prod-e2e-parallel

@mvacula02
Copy link
Copy Markdown
Collaborator Author

/unhold to not delay batch

follow-up: address other potential issues with the test, consider #5391

@raelga
Copy link
Copy Markdown
Collaborator

raelga commented May 26, 2026

/unhold

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 26, 2026

@mvacula02: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/stage-e2e-parallel ef0d392 link false /test stage-e2e-parallel
ci/prow/prod-e2e-parallel ef0d392 link false /test prod-e2e-parallel

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot Bot merged commit bb7c0d4 into Azure:main May 26, 2026
15 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants