OCPBUGS-100316: Change failurePolicy to Ignore on MAPI-guarding VAPs … - #640
OCPBUGS-100316: Change failurePolicy to Ignore on MAPI-guarding VAPs …#640miyadav wants to merge 1 commit into
Conversation
…with CAPI paramKind
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@miyadav: This pull request references Jira Issue OCPBUGS-100316, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughFive validating admission policies change ChangesAdmission policy failure handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml`:
- Line 28: Make all five migration VAPs fail closed by replacing their
failurePolicy: Ignore settings with Fail, and ensure their bindings do not allow
missing parameters via parameterNotFoundAction. Apply the corresponding binding
changes in capi-operator-manifests/default/manifests.yaml at lines 25, 152, 528,
579, and 904; update the five named policy files at their specified sites. Add
coverage for CEL/evaluation errors and monitoring for policy failures, or
explicitly gate activation during bootstrap and restore fail-closed enforcement
once CAPI readiness is established.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bf91382a-e254-4439-8479-6eb583719543
📒 Files selected for processing (6)
admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yamladmission-policies/default/machine-api-machine-set-vap.yamladmission-policies/default/machine-api-machine-vap.yamladmission-policies/default/only-create-mapi-machine-if-authoritative-api-capi.yamladmission-policies/default/prevent-authoritative-mapi-machineset-create-when-capi-exists.yamlcapi-operator-manifests/default/manifests.yaml
| name: openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting | ||
| spec: | ||
| failurePolicy: Fail | ||
| failurePolicy: Ignore |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
source_files=(
admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml
admission-policies/default/machine-api-machine-set-vap.yaml
admission-policies/default/machine-api-machine-vap.yaml
admission-policies/default/only-create-mapi-machine-if-authoritative-api-capi.yaml
admission-policies/default/prevent-authoritative-mapi-machineset-create-when-capi-exists.yaml
)
for file in "${source_files[@]}"; do
grep -q '^ failurePolicy: Ignore$' "$file"
done
test "$(grep -c '^ failurePolicy: Ignore$' capi-operator-manifests/default/manifests.yaml)" -eq 5
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' \
'ValidatingAdmissionPolicy|failurePolicy|parameterNotFoundAction' . || trueRepository: openshift/cluster-capi-operator
Length of output: 169
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for file in \
admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml \
admission-policies/default/machine-api-machine-set-vap.yaml \
admission-policies/default/machine-api-machine-vap.yaml \
admission-policies/default/only-create-mapi-machine-if-authoritative-api-capi.yaml \
admission-policies/default/prevent-authoritative-mapi-machineset-create-when-capi-exists.yaml \
capi-operator-manifests/default/manifests.yaml
do
if [ -f "$file" ]; then
echo "===== $file ====="
grep -n -E '^(apiVersion|kind|metadata:| name:| failurePolicy:| validationActions:| parameterNotFoundAction:| matchConstraints:| validations:| - expression:)' "$file" || true
else
echo "MISSING $file"
fi
done
echo "===== matching policy fields ====="
rg -n --glob '*.yaml' --glob '*.yml' \
'failurePolicy:|validationActions:|parameterNotFoundAction:|kind: ValidatingAdmissionPolicy|kind: ValidatingAdmissionPolicyBinding' . || true
echo "===== repository status ====="
git status --short
git diff --statRepository: openshift/cluster-capi-operator
Length of output: 20276
🌐 Web query:
Kubernetes ValidatingAdmissionPolicy failurePolicy Ignore evaluation error parameterNotFoundAction Allow official documentation
💡 Result:
In Kubernetes ValidatingAdmissionPolicy, the behavior when parameters are missing is governed by the required parameterNotFoundAction field within the ValidatingAdmissionPolicyBinding's spec.paramRef [1]. This field determines how the system reacts when the referenced parameter resource cannot be found [1]. The parameterNotFoundAction field supports the following values [1]: - Allow: If the referenced parameters are not found, the binding treats the absence as a successful validation, effectively bypassing the policy's evaluation for that request [1]. - Deny: If the referenced parameters are not found, the binding treats the absence as a failure [1]. This failure is then subject to the ValidatingAdmissionPolicy's failurePolicy [1]. It is important to distinguish this from the failurePolicy field, which is defined at the policy level and controls how the system handles errors during the evaluation process (such as CEL expression runtime errors, type check errors, or misconfigurations) [2][1][3]. - If failurePolicy is set to Fail (the default), an error during policy evaluation causes the admission to fail [1][3]. - If failurePolicy is set to Ignore, an error during policy evaluation is ignored, and the API request is allowed to continue [1][3]. When using Deny for parameterNotFoundAction, if the parameter is missing, the resulting failure is enforced based on the configured failurePolicy [1]. If failurePolicy is set to Fail, the request is rejected; if it is set to Ignore, the error is ignored, and the request proceeds [1][3].
Citations:
- 1: https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/
- 2: https://kubernetes.io/docs/reference/kubernetes-api/admissionregistration/validating-admission-policy-v1/
- 3: https://docs.redhat.com/en/documentation/openshift_container_platform/4.21/html/extension_apis/validatingadmissionpolicy-admissionregistration-k8s-io-v1
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "===== candidate files ====="
git ls-files | grep -E '(^|/)(authoritative-api-transition-requires-capi-infrastructure-ready|machine-api-machine-set-vap|machine-api-machine-vap|only-create-mapi-machine-if-authoritative-api-capi|prevent-authoritative-mapi-machineset-create-when-capi-exists|manifests)\.yaml$' || true
echo "===== policy definitions and bindings ====="
rg -n -C 4 --glob '*.yaml' --glob '*.yml' \
'ValidatingAdmissionPolicy|ValidatingAdmissionPolicyBinding|failurePolicy|validationActions|parameterNotFoundAction' . || trueRepository: openshift/cluster-capi-operator
Length of output: 50387
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
source_files = [
Path("admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml"),
Path("admission-policies/default/machine-api-machine-set-vap.yaml"),
Path("admission-policies/default/machine-api-machine-vap.yaml"),
Path("admission-policies/default/only-create-mapi-machine-if-authoritative-api-capi.yaml"),
Path("admission-policies/default/prevent-authoritative-mapi-machineset-create-when-capi-exists.yaml"),
]
def docs(text):
return [x for x in re.split(r"(?m)^---\s*$", text) if x.strip()]
for path in source_files:
print(f"===== {path} =====")
parts = docs(path.read_text())
policy = next(x for x in parts if re.search(r"(?m)^kind: ValidatingAdmissionPolicy\s*$", x))
binding = next(x for x in parts if re.search(r"(?m)^kind: ValidatingAdmissionPolicyBinding\s*$", x))
for label, text in (("policy", policy), ("binding", binding)):
print(f"-- {label} --")
for line in text.splitlines():
if re.match(r"^\s*(name:|failurePolicy:|paramKind:|apiVersion:|kind:|parameterNotFoundAction:|validationActions:|matchConditions:|matchConstraints:|validations:| - expression:| - expression:| - expression:| message| message)", line):
print(line)
print()
generated = Path("capi-operator-manifests/default/manifests.yaml").read_text()
print("===== generated targeted policies =====")
for name in [
"machine-api-machine-vap",
"machine-api-machine-set-vap",
"openshift-only-create-mapi-machine-if-authoritative-api-capi",
"openshift-prevent-authoritative-mapi-machineset-create-when-capi-exists",
"openshift-mapi-authoritative-api-transition-requires-capi-infrastructure-ready-and-not-deleting",
]:
policy = next(
x for x in docs(generated)
if re.search(rf"(?m)^ name: {re.escape(name)}\s*$", x)
and re.search(r"(?m)^kind: ValidatingAdmissionPolicy\s*$", x)
)
print(name, re.search(r"(?m)^\s+failurePolicy:\s*(\S+)", policy).group(1))
PY
echo "===== relevant tests and documentation ====="
rg -n --glob '*.go' --glob '*_test.go' --glob '*.yaml' --glob '*.yml' --glob '*.md' \
'authoritative-api-transition-requires-capi-infrastructure-ready|machine-api-machine-set-vap|machine-api-machine-vap|only-create-mapi-machine-if-authoritative-api-capi|prevent-authoritative-mapi-machineset-create-when-capi-exists|failurePolicy: Ignore|parameterNotFoundAction' \
. | head -250 || trueRepository: openshift/cluster-capi-operator
Length of output: 21358
🌐 Web query:
site:kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy failurePolicy evaluation errors CEL runtime errors
💡 Result:
In Kubernetes, ValidatingAdmissionPolicy handles Common Expression Language (CEL) runtime errors and evaluation failures based on the configuration of the failurePolicy field [1]. The failurePolicy defines the response when an admission policy is misconfigured or when a CEL expression encounters an evaluation error [1]. The two possible values for failurePolicy are: 1. Fail: This is the default setting [1]. If a CEL expression results in an evaluation error or if the policy itself is misconfigured, the admission request is rejected, effectively failing closed [1]. 2. Ignore: If a CEL expression encounters a runtime error or the policy is misconfigured, the error is ignored, and the API request is allowed to continue, effectively failing open [1]. It is important to distinguish these behaviors from the result of a successful CEL evaluation that returns false. If a CEL expression evaluates to false, it is treated as a validation failure rather than a runtime error [1]. In such cases, the validation is always enforced regardless of the failurePolicy setting [1]. Additionally, specific rules apply to match conditions. If a match condition encounters a runtime error: - If failurePolicy is set to Fail, the API server rejects the request [1]. - If failurePolicy is set to Ignore, the API server proceeds with the request but skips the policy evaluation [1]. If any match condition evaluates to false, the policy is skipped entirely, regardless of other errors or the failurePolicy [1].
Citations:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for file_and_range in \
"pkg/controllers/machinesetsync/machineset_vap_test.go:230,390" \
"pkg/controllers/machinesetsync/machineset_vap_test.go:650,710" \
"pkg/controllers/machinesync/machine_sync_controller_test.go:1835,1885" \
"pkg/controllers/machinesync/machine_sync_controller_test.go:2180,2265" \
"e2e/machine_migration_vap_test.go:1,150"
do
file="${file_and_range%%:*}"
range="${file_and_range#*:}"
echo "===== $file:$range ====="
sed -n "${range}p" "$file"
done
echo "===== failure/error observability references ====="
rg -n --glob '*.go' --glob '*.yaml' --glob '*.yml' --glob '*.md' \
'ValidatingAdmissionPolicy|admission policy|evaluation error|CEL|failurePolicy|audit|metric|alert' \
pkg controllers e2e admission-policies capi-operator-manifests 2>/dev/null | head -250 || trueRepository: openshift/cluster-capi-operator
Length of output: 50387
Keep these migration guards fail-closed, or cover the fail-open trade-off before merge.
All five VAPs use failurePolicy: Ignore, and their bindings use parameterNotFoundAction: Allow. A false CEL result is denied, but a parameter or CEL evaluation error allows the request. Missing CAPI v1beta2 parameters also bypass the guard. This can admit conflicting MAPI creates or updates.
Existing tests cover normal denials and missing parameters. Add tests for evaluation errors and monitoring for policy failures. If fail-open behavior is required only during bootstrap, gate policy activation or restore failurePolicy: Fail after CAPI readiness.
📍 Affects 6 files
admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml#L28-L28(this comment)admission-policies/default/machine-api-machine-set-vap.yaml#L28-L28admission-policies/default/machine-api-machine-vap.yaml#L28-L28admission-policies/default/only-create-mapi-machine-if-authoritative-api-capi.yaml#L6-L6admission-policies/default/prevent-authoritative-mapi-machineset-create-when-capi-exists.yaml#L6-L6capi-operator-manifests/default/manifests.yaml#L25-L25capi-operator-manifests/default/manifests.yaml#L152-L152capi-operator-manifests/default/manifests.yaml#L528-L528capi-operator-manifests/default/manifests.yaml#L579-L579capi-operator-manifests/default/manifests.yaml#L904-L904
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@admission-policies/default/authoritative-api-transition-requires-capi-infrastructure-ready.yaml`
at line 28, Make all five migration VAPs fail closed by replacing their
failurePolicy: Ignore settings with Fail, and ensure their bindings do not allow
missing parameters via parameterNotFoundAction. Apply the corresponding binding
changes in capi-operator-manifests/default/manifests.yaml at lines 25, 152, 528,
579, and 904; update the five named policy files at their specified sites. Add
coverage for CEL/evaluation errors and monitoring for policy failures, or
explicitly gate activation during bootstrap and restore fail-closed enforcement
once CAPI readiness is established.
Source: MCP tools
|
@miyadav: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
…with CAPI paramKind.
looks good to me as immediate fix for unblocking tp installs on vsphere
When the CAPI CRDs (cluster.x-k8s.io/v1beta2) are not yet established during install, the apiserver cannot resolve the paramKind and the VAP is "misconfigured". With
failurePolicy: Fail, this denies every MAPI machine update — including status patches from machine-api-controllers, which are normally exempted via matchConditions but never reach thatevaluation. Workers clone successfully in vCenter but the machinecontroller can never persist the provider task ID, leaving all workers stuck in Provisioning and failing the install.
Affected files:
generated by claude
I understand there might be a reason we set them as
Failearlier , so we may have to dig more for a way to establish that CRDs are established and then we change it to fail ( that might need more in depth analysis )Summary by CodeRabbit