HIVE-3132: admissionReviewVersions += v1#2880
HIVE-3132: admissionReviewVersions += v1#28802uasimojo wants to merge 2 commits intoopenshift:masterfrom
Conversation
Turns out we've been running without this... forever, despite having plenty of code for it in our admission server.
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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-robot
added
the
jira/valid-reference
|
/hold This is currently stacked on #2879 so it can affect the clusterpool vwhc introduced there. Rebase and unhold after that merges. |
openshift-ci
bot
added
the
do-not-merge/hold
|
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:
📝 WalkthroughWalkthroughMultiple validating webhook configurations were updated to advertise AdmissionReview API version "v1" alongside "v1beta1". A new ValidatingWebhookConfiguration for clusterpools was added and embedded assets, operator registration, scheme registration, and tests/fixtures were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant K8sAPI as "Kubernetes API"
participant WebhookSvc as "Webhook Service"
Operator->>Operator: Load embedded webhook assets (incl. clusterpool)
Operator->>K8sAPI: Apply/Update ValidatingWebhookConfiguration (admissionReviewVersions: [v1,v1beta1])
K8sAPI->>WebhookSvc: Route AdmissionReview (v1) to /apis/admission.hive.openshift.io/v1/clusterpoolvalidators
WebhookSvc-->>K8sAPI: Return admission response (Allowed/Denied)
K8sAPI-->>Operator: Observability / reconciliation feedback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo 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
the
approved
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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.
🧹 Nitpick comments (1)
test/e2e/postdeploy/admission/admission_test.go (1)
55-75: Add explicit v1beta1 compatibility coverage.Given the PR goal includes backward compatibility, consider adding one test case that validates v1beta1 AdmissionReview handling to prevent silent regressions on older hubs.
Also applies to: 93-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/postdeploy/admission/admission_test.go` around lines 55 - 75, Add a test case to the tests table in admission_test.go that covers v1beta1 AdmissionReview parsing/handling: update the tests []struct { name, file, expectAllowed } slice (the same table used for "review success"/"review failure") to include an entry like name: "review v1beta1 compatibility", file: "hiveadmission-review-v1beta1.json", expectAllowed: <true|false> (match the expected outcome for that fixture); ensure a corresponding v1beta1 fixture is present and mirror the existing test flow that exercises the same handler (the code paths referenced by the tests slice) so the test asserts v1beta1 behavior the same way as the v1 AdmissionReview cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/postdeploy/admission/admission_test.go`:
- Around line 55-75: Add a test case to the tests table in admission_test.go
that covers v1beta1 AdmissionReview parsing/handling: update the tests []struct
{ name, file, expectAllowed } slice (the same table used for "review
success"/"review failure") to include an entry like name: "review v1beta1
compatibility", file: "hiveadmission-review-v1beta1.json", expectAllowed:
<true|false> (match the expected outcome for that fixture); ensure a
corresponding v1beta1 fixture is present and mirror the existing test flow that
exercises the same handler (the code paths referenced by the tests slice) so the
test asserts v1beta1 behavior the same way as the v1 AdmissionReview cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81aac8a9-dc80-4910-8908-bca1da55b86d
📒 Files selected for processing (11)
config/hiveadmission/clusterdeployment-webhook.yamlconfig/hiveadmission/clusterimageset-webhook.yamlconfig/hiveadmission/clusterpool-webhook.yamlconfig/hiveadmission/clusterprovision-webhook.yamlconfig/hiveadmission/dnszones-webhook.yamlconfig/hiveadmission/machinepool-webhook.yamlconfig/hiveadmission/selectorsyncset-webhook.yamlconfig/hiveadmission/syncset-webhook.yamlpkg/operator/assets/bindata.gopkg/operator/hive/hiveadmission.gotest/e2e/postdeploy/admission/admission_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2880 +/- ##
==========================================
- Coverage 50.31% 50.29% -0.02%
==========================================
Files 280 280
Lines 34314 34325 +11
==========================================
+ Hits 17264 17265 +1
- Misses 15689 15699 +10
Partials 1361 1361
🚀 New features to boost your workflow:
|
2uasimojo
force-pushed
the
HIVE-3132/admissionReview-v1
branch
from
b8a4997 to
0a55887
Compare
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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. |
2uasimojo
force-pushed
the
HIVE-3132/admissionReview-v1
branch
from
0a55887 to
cfbd5a6
Compare
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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. |
2uasimojo
force-pushed
the
HIVE-3132/admissionReview-v1
branch
from
cfbd5a6 to
162bd2d
Compare
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI wasn't able to retrieve the specific log contents via web search. Let me try to fetch the build logs directly. 🏁 Script executed: Length of output: 7002 The error is clear from the logs: The root cause: by listing Adding Let me look at the handler code to give you a more concrete suggestion. 🏁 Script executed: Unrecoverable error: Failed to clone repository
What's happening: By listing Why v1 is being sent: In There are two paths to fix this:
Given that the PR also updates In short: the scheme registration and webhook YAML changes are necessary but not sufficient — the server-side handler that unmarshals the incoming HTTP body into an |
v1 has been out since k8s 1.16. Support it. Leaving v1beta1 in place for the moment, just in case there are any ancient hubs out there.
2uasimojo
force-pushed
the
HIVE-3132/admissionReview-v1
branch
from
162bd2d to
b5b5d32
Compare
|
@2uasimojo: This pull request references HIVE-3132 which is a valid jira issue. 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. |
The code that's processing AdmissionReview is upstream (vendored). Do I have any control over what version that code is using? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainGood question — let me look at the actual vendored handler code to see what levers are available. 🏁 Script executed: Length of output: 1853 🏁 Script executed: Length of output: 1664 🏁 Script executed: Length of output: 18491 🏁 Script executed: Length of output: 993 🏁 Script executed: Length of output: 4682
Here's how the mechanism works in the vendored The library defines two separate interfaces your hooks can implement: // For v1beta1 (current)
type ValidatingAdmissionHookV1Beta1 interface {
ValidatingAdmissionHook
Validate(admissionSpec *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse
}
// For v1 (new)
type ValidatingAdmissionHookV1 interface {
ValidatingAdmissionHook
Validate(admissionSpec *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse
}
What you need to do: Update the // Before
import admissionv1beta1 "k8s.io/api/admission/v1beta1"
func (a *DNSZoneValidatingAdmissionHook) Validate(admissionSpec *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse {// After
import admissionv1 "k8s.io/api/admission/v1"
func (a *DNSZoneValidatingAdmissionHook) Validate(admissionSpec *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponse {The internal logic of each You'll need to apply this to all 8+ hooks (DNSZone, ClusterDeployment, ClusterPool, ClusterImageSet, ClusterProvision, MachinePool, SyncSet, SelectorSyncSet, ClusterDeploymentCustomization). The HACK comment already in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/assets/bindata.go`:
- Around line 280-281: The advertised admissionReviewVersions in the embedded
YAMLs are listing "v1" before "v1beta1", causing kube-apiserver to send v1
AdmissionReview objects while handlers like
clusterdeployment_validating_admission_hook.go (and other
*_validating_admission_hook.go files) still expect admissionv1beta1 types; fix
by updating the source YAMLs to list admissionReviewVersions with v1beta1 first
then v1 (admissionReviewVersions: - v1beta1 - v1) and then regenerate the
bindata (pkg/operator/assets/bindata.go), or alternatively migrate all webhook
handler signatures and logic to use admissionv1 types (and update
cmd/hiveadmission/main.go HACK handling accordingly) before keeping v1 first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f8fcbec-bf9e-4ae0-9a89-be4d97efc4fe
📒 Files selected for processing (15)
cmd/hiveadmission/main.goconfig/hiveadmission/clusterdeployment-webhook.yamlconfig/hiveadmission/clusterimageset-webhook.yamlconfig/hiveadmission/clusterpool-webhook.yamlconfig/hiveadmission/clusterprovision-webhook.yamlconfig/hiveadmission/dnszones-webhook.yamlconfig/hiveadmission/machinepool-webhook.yamlconfig/hiveadmission/selectorsyncset-webhook.yamlconfig/hiveadmission/syncset-webhook.yamlpkg/operator/assets/bindata.gopkg/util/scheme/scheme.gotest/e2e/postdeploy/admission/admission_test.gotest/e2e/postdeploy/admission/testdata/hiveadmission-review-failure.jsontest/e2e/postdeploy/admission/testdata/hiveadmission-review-success.jsontest/e2e/postdeploy/admission/testdata/hiveadmission-review-update-failure.json
✅ Files skipped from review due to trivial changes (8)
- pkg/util/scheme/scheme.go
- test/e2e/postdeploy/admission/testdata/hiveadmission-review-failure.json
- config/hiveadmission/clusterimageset-webhook.yaml
- config/hiveadmission/selectorsyncset-webhook.yaml
- test/e2e/postdeploy/admission/testdata/hiveadmission-review-update-failure.json
- config/hiveadmission/dnszones-webhook.yaml
- config/hiveadmission/clusterprovision-webhook.yaml
- config/hiveadmission/clusterpool-webhook.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- config/hiveadmission/clusterdeployment-webhook.yaml
- cmd/hiveadmission/main.go
- test/e2e/postdeploy/admission/testdata/hiveadmission-review-success.json
- config/hiveadmission/machinepool-webhook.yaml
- config/hiveadmission/syncset-webhook.yaml
- test/e2e/postdeploy/admission/admission_test.go
| - v1 | ||
| - v1beta1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Webhook admissionReviewVersions ordering in source manifests"
rg -n --type=yaml 'admissionReviewVersions:|-\s*v1$|-\s*v1beta1$' config/hiveadmission/*-webhook.yaml -A2 -B1
echo
echo "2) Current admission type usage in server entrypoint/hooks"
rg -n --type=go 'admissionv1beta1|admissionv1\.' cmd/hiveadmission/main.go pkg/validating-webhooks -C2
echo
echo "3) Hook signatures/logic still bound to v1beta1 request/response types"
rg -n --type=go 'AdmissionReview|AdmissionRequest|AdmissionResponse' pkg/validating-webhooks -C2Repository: openshift/hive
Length of output: 50371
admissionReviewVersions order causes v1/v1beta1 runtime mismatch.
Lines 280, 326, 370, 417, 547, 692, 762, and 862 advertise v1 first, causing kube-apiserver to prefer sending admission.k8s.io/v1 AdmissionReview objects. However, all webhook handlers (e.g., clusterdeployment_validating_admission_hook.go) are still hardcoded to v1beta1 types (*admissionv1beta1.AdmissionRequest, *admissionv1beta1.AdmissionResponse). This version mismatch will break admission requests at runtime until handlers are migrated to v1 types.
To restore compatibility, reorder admissionReviewVersions in source YAMLs (then regenerate bindata):
admissionReviewVersions:
- v1beta1
- v1Alternatively, migrate all webhook handler signatures and logic to use admissionv1 types before keeping v1 first. The HACK comment in cmd/hiveadmission/main.go:40 ("Prevent pruning of admissionv1, resulting in conversion errors") indicates this conversion issue is already known.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/assets/bindata.go` around lines 280 - 281, The advertised
admissionReviewVersions in the embedded YAMLs are listing "v1" before "v1beta1",
causing kube-apiserver to send v1 AdmissionReview objects while handlers like
clusterdeployment_validating_admission_hook.go (and other
*_validating_admission_hook.go files) still expect admissionv1beta1 types; fix
by updating the source YAMLs to list admissionReviewVersions with v1beta1 first
then v1 (admissionReviewVersions: - v1beta1 - v1) and then regenerate the
bindata (pkg/operator/assets/bindata.go), or alternatively migrate all webhook
handler signatures and logic to use admissionv1 types (and update
cmd/hiveadmission/main.go HACK handling accordingly) before keeping v1 first.
There was a problem hiding this comment.
If I do this, will v1beta1 still be supported?
There was a problem hiding this comment.
@2uasimojo Yes, v1beta1 will still be fully supported with the reordering to [v1beta1, v1].
Here's how the version selection works: API servers send the first AdmissionReview version in the admissionReviewVersions list that they support. Since every API server capable of running admission webhooks supports v1beta1, placing it first means v1beta1 will be the version actually used at runtime — the same behavior as before this PR changed the order.
The trade-off is that with [v1beta1, v1]:
- Old hubs (pre-1.16): use
v1beta1✅ - Modern hubs (1.16+): also use
v1beta1(first in list), even though they supportv1
So the reorder is a safe stop-gap to fix the CI failures without requiring handler code changes right now. The actual promotion to native v1 handling would require also migrating the webhook handler signatures from *admissionv1beta1.AdmissionRequest/*admissionv1beta1.AdmissionResponse to their admissionv1 equivalents — which is what the v1-first ordering + updated test fixtures were trying to accomplish, but the handler migration wasn't yet complete.
In short: [v1beta1, v1] keeps everything working today (v1beta1 is still the operative version), while [v1, v1beta1] requires the handler code to actually speak v1 to avoid the conversion error seen in CI.
There was a problem hiding this comment.
Bah, sorry, I asked in the wrong spot. Disregard.
There was a problem hiding this comment.
@2uasimojo No worries at all! Feel free to ping if you have questions in the right spot. 😄
(´• ω •`) ∫
@coderabbitai Would v1beta1 still be supported if I did this, or would I need to have both implementations present? I thought this was the whole purpose of the conversion thing: if I implement support for v1beta1, the library converts incoming v1 AdmissionReview to v1beta1 for me. |
|
@2uasimojo: The following tests 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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainGood question — let me look at the exact routing logic in the vendored library to give you an accurate answer. 🏁 Script executed: Length of output: 21606
How the library actually works: The library registers a completely separate REST endpoint for each version — there is no cross-version conversion bridge. Can you implement both interfaces on the same hook struct? No. Both // V1Beta1 interface
Validate(admissionSpec *admissionv1beta1.AdmissionRequest) *admissionv1beta1.AdmissionResponse
// V1 interface
Validate(admissionSpec *admissionv1.AdmissionRequest) *admissionv1.AdmissionResponseGo does not allow two methods with the same name but different signatures on the same type, so a single hook struct can only implement one of these two interfaces. Your practical options:
Given the PR's stated goal — "v1 has been out since k8s 1.16; leaving v1beta1 in place just in case" — option 2 (swap order to |
|
@coderabbitai re option 2, if I swap the order but keep |
|
Tip For best results, initiate chat on the files or code changes.
With The honest options as I see them:
Given that v1beta1 payload support will go away eventually and your PR's stated goal is forward-looking, option 2 (switching the hooks to native v1) is the right path. It's less work than it sounds since |
|
@coderabbitai I'm not worried about the work; I can see it's not hard mechanically to switch over to v1. My concern is that hive needs to be able to run "on any k8s", and I'm worried that there may exist ancient hub clusters out there that don't speak v1 yet. But it looks like it would have to be a pre-v1.16 k8s, which is so far out of support that nobody should be surprised to find stuff stops working there. I'll do a full cutover to v1 via fresh PR #2881. |
|
Tip For best results, initiate chat on the files or code changes.
Good luck with |
2 participants
v1 has been out since k8s 1.16. Support it.
Leaving v1beta1 in place for the moment, just in case there are any ancient hubs out there.
Summary by CodeRabbit
New Features
Chores