OCPBUGS-61056: Add ValidatingAdmissionPolicy and check for omissions next time. #546
Conversation
WalkthroughReplaced a static OpenShift admission-plugin slice with an init-time function that constructs downstreamPlugins, imports fmt/slices/options, fetches upstream RecommendedPluginOrder, validates every upstream plugin is present (panic on mismatch), and returns the downstream list. Also changed AdmissionOptions.ApplyTo to pass feature.DefaultFeatureGate. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Process as Process Start
participant Reg as register.go (IIFE)
participant K8s as options.NewAdmissionOptions()
Process->>Reg: Initialize OpenShiftAdmissionPlugins
activate Reg
Reg->>Reg: Construct downstreamPlugins (K8s defaults + OpenShift additions)
Reg->>Reg: Define omittedPlugins (empty)
Reg->>K8s: Call NewAdmissionOptions().RecommendedPluginOrder
K8s-->>Reg: Return upstreamPlugins
loop For each upstream plugin
Reg->>Reg: Check membership in downstreamPlugins ∪ omittedPlugins
alt Found
Reg->>Reg: continue
else Not found
Reg-->>Process: panic (missing upstream plugin)
end
end
Reg-->>Process: Return downstreamPlugins -> assign to OpenShiftAdmissionPlugins
deactivate Reg
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (3)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go (3)
4-6: Confirm toolchain supportsslices(Go 1.21+) or avoid it.The stdlib
slicespackage requires Go ≥1.21. If the repo or downstream consumers still build with older Go, replaceslices.Containswith a set/map lookup to avoid toolchain bumps.Apply if you want to drop
slices:- "slices" + // "slices" // avoid: prefer O(1) set membershipAnd see the set-based lookup suggestion in my later comment for lines 79–91.
Also applies to: 19-19
74-77: Track omissions with explicit reasons.You ask for “included in this list with an explanation,” but the structure can’t enforce it. Consider mapping plugin → reason and validating non-empty reasons at init.
Apply if desired:
- omittedPlugins := []string{} + // map of plugin name → explanation link/reason + omittedPlugins := map[string]string{ + // "SomePlugin": "Reason and link to upstream KEP/issue", + }Adjust the enumeration below accordingly (see next comment).
51-94: Prefer a unit test to pin upstream defaults in addition to the init-time guard.The panic-at-init will catch drift during CI, but it also risks bricking a binary at runtime if a mismatch slips in. Add a small test that computes the upstream default-enabled set and asserts coverage by downstream+omitted. This provides earlier, clearer failure.
Happy to draft
pkg/cmd/openshift-apiserver/openshiftadmission/register_test.gothat asserts:
- upstreamEnabled := opts.RecommendedPluginOrder − opts.DefaultOffPlugins
- require upstreamEnabled ⊆ (downstream ∪ omitted)
📜 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 (1)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go(3 hunks)
🔇 Additional comments (1)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go (1)
51-72: Gate MutatingAdmissionPolicy behind feature flag
- ValidatingAdmissionPolicy is correctly placed before ValidatingAdmissionWebhook and has been GA and default-enabled since Kubernetes v1.30 (no feature gate) (github.com).
- MutatingAdmissionPolicy is an alpha plugin in Kubernetes v1.32 behind the AdmissionPolicy feature gate (disabled by default upstream) (kubernetes.io); remove it or guard it with the corresponding feature gate to avoid unintentional enabling downstream.
benluddy
changed the title
openshift-ci-robot
added
jira/severity-important
|
@benluddy: This pull request references Jira Issue OCPBUGS-61056, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
The default admission plugin list requires manual synchronization every time upstream introduces a new default plugin. To make future dependency bumps less error-prone, it should now fail if upstream's default list names a plugin that is not either included in the downstream list or in a list of intentionally-omitted plugins.
benluddy
force-pushed
the
vap-enablement
branch
from
fe7b5f6 to
8677d25
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@benluddy: This pull request references Jira Issue OCPBUGS-61056, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In 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. |
|
/cc @sanchezl |
|
@benluddy: This pull request references Jira Issue OCPBUGS-61056, which is valid. 3 validation(s) were run on this bug
In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go (1)
79-91: Filter out upstream default-off plugins to avoid false-positive panics; switch to O(1) membership.As written, the check scans all RecommendedPluginOrder entries, including plugins that upstream marks DefaultOff, which will spuriously panic on a k8s bump even when upstream keeps a plugin disabled-by-default. Also, the O(n²) membership scan is avoidable.
Apply this diff:
- upstreamPlugins := options.NewAdmissionOptions().RecommendedPluginOrder - enumeratedPlugins := append(downstreamPlugins, omittedPlugins...) - for _, upstreamPluginName := range upstreamPlugins { - if !slices.Contains(enumeratedPlugins, upstreamPluginName) { + opts := options.NewAdmissionOptions() + upstreamPlugins := opts.RecommendedPluginOrder + defaultOff := opts.DefaultOffPlugins + enumerated := make(map[string]struct{}, len(downstreamPlugins)+len(omittedPlugins)) + for _, p := range downstreamPlugins { + enumerated[p] = struct{}{} + } + for _, p := range omittedPlugins { + enumerated[p] = struct{}{} + } + for _, upstreamPluginName := range upstreamPlugins { + // Enforce coverage only for upstream default-enabled plugins. + if defaultOff.Has(upstreamPluginName) { + continue + } + if _, ok := enumerated[upstreamPluginName]; !ok { // If you are reading this because you are changing the version of // the k8s.io/apiserver dependency, upstream may have introduced a // new default-enabled admission plugin. If there is a good reason // against enabling it in openshift-apiserver, its name must be // included in omittedPlugins, otherwise, it should in the // appropriate position in downstreamPlugins. panic(fmt.Sprintf("k8s.io/apiserver default admission plugins includes %q which is in neither downstreamPlugins nor omittedPlugins: %v", upstreamPluginName, upstreamPlugins)) } }And drop the now-unused
slicesimport:- "slices"Also applies to: 4-6
🧹 Nitpick comments (1)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go (1)
51-94: Avoid init-time panics in production binaries; consider shifting this drift check into a unit test.The init-time panic will hard-crash the apiserver at startup if upstream adds a default-enabled plugin and OpenShift hasn’t enumerated it yet. That’s great for catching drift early during bumps, but it also poses an availability risk if it slips into a release. Consider replicating this check in a unit test (e.g., TestUpstreamDefaultEnabledPluginsCovered) so CI fails builds while the runtime binary remains resilient.
📜 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 (2)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go(3 hunks)pkg/cmd/openshift-apiserver/openshiftapiserver/config.go(1 hunks)
🔇 Additional comments (2)
pkg/cmd/openshift-apiserver/openshiftadmission/register.go (1)
66-72: Order sanity check request.Validate that placing MutatingAdmissionPolicy/ValidatingAdmissionPolicy before the respective webhooks matches upstream’s recommended relative order for your vendored k8s version.
Would you like me to generate a small script to print the upstream RecommendedPluginOrder and DefaultOffPlugins from the vendored apiserver to confirm ordering?
pkg/cmd/openshift-apiserver/openshiftapiserver/config.go (1)
217-217: Passing DefaultFeatureGate to AdmissionOptions.ApplyTo looks correct.This aligns admission initialization with feature gates and avoids surprises when policy plugins are gated. LGTM.
|
/retest-required |
1 similar comment
|
/retest-required |
|
@benluddy: all tests passed! Full PR test history. Your PR dashboard. 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. |
Yes, if it's removed. I don't think this will catch an upstream change that removes a plugin name from the default-enabled/recommended list. |
The default admission plugin list requires manual synchronization every time upstream introduces a new default plugin. To make future dependency bumps less error-prone, it should now fail if upstream's default list names a plugin that is not either included in the downstream list or in a list of intentionally-omitted plugins.
Summary by CodeRabbit