chore: update controller runtime#5930
Conversation
|
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:
WalkthroughValidators were converted from untyped runtime.Object handlers to typed generic interfaces (admission.Validator[T]) for ExternalSecret, SecretStore, and ClusterSecretStore; webhook builder calls were adjusted to use ctrl.NewWebhookManagedBy(mgr, obj); widespread go.mod dependency upgrades across Kubernetes, OpenAPI, protobuf, and golang.org/x modules. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apis/externalsecrets/v1/externalsecret_validator.go`:
- Around line 27-28: Update the stale comment above the type assertion to
reference the current generic interface: replace mention of
admission.CustomValidator with admission.Validator[*ExternalSecret], so the
comment accurately describes the assertion var _
admission.Validator[*ExternalSecret] = &ExternalSecretValidator{} and refers to
ExternalSecretValidator and the generic admission.Validator[*ExternalSecret]
symbol.
In `@apis/externalsecrets/v1/secretstore_validator.go`:
- Around line 29-30: The comment above the interface assertion is stale: it
references ExternalSecretValidator but the assertion verifies
GenericStoreValidator; update the comment to accurately describe the assertion
(e.g., "Ensures GenericStoreValidator implements the admission.Validator
interface") or change the asserted type to match the intended validator (swap
GenericStoreValidator to ExternalSecretValidator) so the comment and the
assertion are consistent; locate the assertion variable _
admission.Validator[runtime.Object] = &GenericStoreValidator{} and update either
the comment text or the concrete type accordingly.
In `@e2e/go.mod`:
- Around line 70-76: The go.mod requires k8s.io/client-go at v1.5.2 which is
incompatible with the other Kubernetes staging modules at v0.35.0; update the
k8s.io/client-go requirement to the matching v0.35.0 (or the same v0.x.y used by
k8s.io/api, k8s.io/apimachinery, and k8s.io/apiextensions-apiserver) so all
k8s.io/* modules use the same minor version and avoid version skew—edit the
require block replacing k8s.io/client-go v1.5.2 with k8s.io/client-go v0.35.0
(or the exact v0.35.0 tag used elsewhere) and run go mod tidy to verify
dependency resolution.
In `@go.mod`:
- Around line 421-423: The go.mod contains a malformed combined dependency
entry: "github.com/go-openapi/jsonreference v0.21.4 // indirect;
indirectgithub.com/go-openapi/strfmt v0.21.7 // indirect"; fix it by splitting
into two separate module lines so each dependency is its own entry (one for
github.com/go-openapi/jsonreference v0.21.4 // indirect and one for
github.com/go-openapi/strfmt v0.21.7 // indirect), ensuring the "// indirect"
comment is correctly placed for each module and no extra characters (like the
stray "indirect" prefix) remain.
🧹 Nitpick comments (1)
apis/externalsecrets/v1beta1/externalsecret_validator.go (1)
48-51: Consider clarifying the nil-check error message.With the strongly-typed interface,
nilis a valid*ExternalSecretpointer (just nil), not an "unexpected type". The error message could be more accurate.✏️ Suggested improvement
func validateExternalSecret(es *ExternalSecret) (admission.Warnings, error) { if es == nil { - return nil, errors.New("unexpected type") + return nil, errors.New("ExternalSecret cannot be nil") }
| // Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly. | ||
| var _ admission.CustomValidator = &GenericStoreValidator{} | ||
| var _ admission.Validator[runtime.Object] = &GenericStoreValidator{} |
There was a problem hiding this comment.
Stale comment refers to wrong validator type.
The comment mentions ExternalSecretValidator but the assertion is for GenericStoreValidator.
📝 Proposed fix
-// Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly.
+// Ensures GenericStoreValidator implements the admission.Validator interface correctly.
var _ admission.Validator[runtime.Object] = &GenericStoreValidator{}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly. | |
| var _ admission.CustomValidator = &GenericStoreValidator{} | |
| var _ admission.Validator[runtime.Object] = &GenericStoreValidator{} | |
| // Ensures GenericStoreValidator implements the admission.Validator interface correctly. | |
| var _ admission.Validator[runtime.Object] = &GenericStoreValidator{} |
🤖 Prompt for AI Agents
In `@apis/externalsecrets/v1/secretstore_validator.go` around lines 29 - 30, The
comment above the interface assertion is stale: it references
ExternalSecretValidator but the assertion verifies GenericStoreValidator; update
the comment to accurately describe the assertion (e.g., "Ensures
GenericStoreValidator implements the admission.Validator interface") or change
the asserted type to match the intended validator (swap GenericStoreValidator to
ExternalSecretValidator) so the comment and the assertion are consistent; locate
the assertion variable _ admission.Validator[runtime.Object] =
&GenericStoreValidator{} and update either the comment text or the concrete type
accordingly.
9044d98 to
0d36811
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/go.mod (1)
7-36:⚠️ Potential issue | 🟠 MajorVersion skew between require and replace directives: k8s.io packages should be aligned to v0.35.0.
The require block specifies k8s.io/api, k8s.io/apiextensions-apiserver, and k8s.io/apimachinery at v0.35.0, but the replace directives (lines 7-36) override most k8s.io/* packages to v0.34.0. Since replace directives take precedence, the effective versions will be v0.34.0, misaligning with controller-runtime v0.23.1, which expects k8s.io packages at v0.35.x. Update the replace block to use v0.35.0 for consistency.
🤖 Fix all issues with AI agents
In `@apis/externalsecrets/v1beta1/externalsecret_validator.go`:
- Around line 27-29: The comment above the interface assertion is stale: update
or replace the reference to admission.CustomValidator so it matches the actual
assertion admission.Validator[*ExternalSecret]; specifically edit the comment
that references admission.CustomValidator to refer to
admission.Validator[*ExternalSecret] (or remove the redundant comment) next to
the var _ admission.Validator[*ExternalSecret] = &ExternalSecretValidator{}
declaration to ensure the comment accurately describes ExternalSecretValidator's
implemented interface.
| // Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly. | ||
| var _ admission.Validator[*ExternalSecret] = &ExternalSecretValidator{} | ||
|
|
There was a problem hiding this comment.
Update the stale interface comment.
Line 27 still mentions admission.CustomValidator, but the assertion is admission.Validator[*ExternalSecret].
🛠️ Proposed fix
-// Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly.
+// Ensures ExternalSecretValidator implements the admission.Validator interface correctly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensures ExternalSecretValidator implements the admission.CustomValidator interface correctly. | |
| var _ admission.Validator[*ExternalSecret] = &ExternalSecretValidator{} | |
| // Ensures ExternalSecretValidator implements the admission.Validator interface correctly. | |
| var _ admission.Validator[*ExternalSecret] = &ExternalSecretValidator{} | |
🤖 Prompt for AI Agents
In `@apis/externalsecrets/v1beta1/externalsecret_validator.go` around lines 27 -
29, The comment above the interface assertion is stale: update or replace the
reference to admission.CustomValidator so it matches the actual assertion
admission.Validator[*ExternalSecret]; specifically edit the comment that
references admission.CustomValidator to refer to
admission.Validator[*ExternalSecret] (or remove the redundant comment) next to
the var _ admission.Validator[*ExternalSecret] = &ExternalSecretValidator{}
declaration to ensure the comment accurately describes ExternalSecretValidator's
implemented interface.
569c19b to
760770e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/go.mod (1)
10-35:⚠️ Potential issue | 🔴 CriticalCritical: Replace directives pin k8s.io modules to v0.34.0 while controller-runtime v0.23.1 requires v0.35.0.
The replace block (lines 10-35) forces all k8s.io modules to v0.34.0, but sigs.k8s.io/controller-runtime v0.23.1 (line 77) explicitly requires k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/apiserver, k8s.io/client-go, and k8s.io/component-base at v0.35.0. Since Go module replace directives override require statements, the build will use v0.34.0 at runtime, creating a version mismatch that will cause failures.
Update all k8s.io module versions in the replace block to v0.35.0 to align with controller-runtime v0.23.1 requirements.
🔧 Proposed fix
replace ( github.com/external-secrets/external-secrets v0.0.0 => ../ github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 - k8s.io/api => k8s.io/api v0.34.0 - k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.34.0 - k8s.io/apimachinery => k8s.io/apimachinery v0.34.0 - k8s.io/apiserver => k8s.io/apiserver v0.34.0 - k8s.io/cli-runtime => k8s.io/cli-runtime v0.34.0 - k8s.io/client-go => k8s.io/client-go v0.34.0 - k8s.io/cloud-provider => k8s.io/cloud-provider v0.34.0 - k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.34.0 - k8s.io/code-generator => k8s.io/code-generator v0.34.0 - k8s.io/component-base => k8s.io/component-base v0.34.0 - k8s.io/component-helpers => k8s.io/component-helpers v0.34.0 - k8s.io/controller-manager => k8s.io/controller-manager v0.34.0 - k8s.io/cri-api => k8s.io/cri-api v0.34.0 - k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.34.0 - k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.34.0 - k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.34.0 - k8s.io/kube-proxy => k8s.io/kube-proxy v0.34.0 - k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.34.0 - k8s.io/kubectl => k8s.io/kubectl v0.34.0 - k8s.io/kubelet => k8s.io/kubelet v0.34.0 - k8s.io/kubernetes => k8s.io/kubernetes v1.34.0 - k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.34.0 - k8s.io/metrics => k8s.io/metrics v0.34.0 - k8s.io/mount-utils => k8s.io/mount-utils v0.34.0 - k8s.io/pod-security-admission => k8s.io/pod-security-admission v0.34.0 - k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.34.0 + k8s.io/api => k8s.io/api v0.35.0 + k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.35.0 + k8s.io/apimachinery => k8s.io/apimachinery v0.35.0 + k8s.io/apiserver => k8s.io/apiserver v0.35.0 + k8s.io/cli-runtime => k8s.io/cli-runtime v0.35.0 + k8s.io/client-go => k8s.io/client-go v0.35.0 + k8s.io/cloud-provider => k8s.io/cloud-provider v0.35.0 + k8s.io/cluster-bootstrap => k8s.io/cluster-bootstrap v0.35.0 + k8s.io/code-generator => k8s.io/code-generator v0.35.0 + k8s.io/component-base => k8s.io/component-base v0.35.0 + k8s.io/component-helpers => k8s.io/component-helpers v0.35.0 + k8s.io/controller-manager => k8s.io/controller-manager v0.35.0 + k8s.io/cri-api => k8s.io/cri-api v0.35.0 + k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.35.0 + k8s.io/kube-aggregator => k8s.io/kube-aggregator v0.35.0 + k8s.io/kube-controller-manager => k8s.io/kube-controller-manager v0.35.0 + k8s.io/kube-proxy => k8s.io/kube-proxy v0.35.0 + k8s.io/kube-scheduler => k8s.io/kube-scheduler v0.35.0 + k8s.io/kubectl => k8s.io/kubectl v0.35.0 + k8s.io/kubelet => k8s.io/kubelet v0.35.0 + k8s.io/kubernetes => k8s.io/kubernetes v1.35.0 + k8s.io/legacy-cloud-providers => k8s.io/legacy-cloud-providers v0.35.0 + k8s.io/metrics => k8s.io/metrics v0.35.0 + k8s.io/mount-utils => k8s.io/mount-utils v0.35.0 + k8s.io/pod-security-admission => k8s.io/pod-security-admission v0.35.0 + k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.35.0 )
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/go.mod (1)
10-15:⚠️ Potential issue | 🟠 MajorVersion mismatch: replace directives use v0.34.0 but require block uses v0.35.0.
The replace block pins k8s.io/* packages to v0.34.0, but the require block (lines 72-74, 76-77) specifies v0.35.0 for k8s.io/api, k8s.io/apiextensions-apiserver, k8s.io/apimachinery, k8s.io/utils, and controller-runtime v0.23.1. For consistency and to avoid unexpected version skew, update the replace directives to v0.35.0 to match the rest of the PR.
🔧 Proposed fix
k8s.io/api => k8s.io/api v0.34.0 - k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.34.0 + k8s.io/apiextensions-apiserver => k8s.io/apiextensions-apiserver v0.35.0 - k8s.io/apimachinery => k8s.io/apimachinery v0.34.0 + k8s.io/apimachinery => k8s.io/apimachinery v0.35.0 - k8s.io/apiserver => k8s.io/apiserver v0.34.0 + k8s.io/apiserver => k8s.io/apiserver v0.35.0 - k8s.io/cli-runtime => k8s.io/cli-runtime v0.34.0 + k8s.io/cli-runtime => k8s.io/cli-runtime v0.35.0 - k8s.io/client-go => k8s.io/client-go v0.34.0 + k8s.io/client-go => k8s.io/client-go v0.35.0Apply similar updates to all other k8s.io/* replace directives (lines 16-35).
🧹 Nitpick comments (1)
apis/externalsecrets/v1beta1/externalsecret_validator.go (1)
48-131: Consider extracting shared validation logic between v1 and v1beta1.The
validateExternalSecret,validateSourceRef,validateFindExtractSourceRef,validateExtractFindGenerator,validatePolicies, andvalidateDuplicateKeysfunctions are nearly identical between thev1andv1beta1packages. If the types are compatible (or can be made so via an interface), extracting these into a shared internal package would reduce the maintenance burden of keeping both in sync.This is pre-existing duplication, not introduced by this PR, so deferring is fine.
adamkasztenny
left a comment
There was a problem hiding this comment.
I'm not a maintainer, but LGTM, thanks for fixing this!
The breaking changes are causing issues for projects that import the latest controller-runtime and ESO.
|
I'm wondering, is the case for importing ESO that you'd like to create its API objects? |
760770e to
a8e4b10
Compare
That's our use case, yes. |
4b35510 to
1fb009d
Compare
|
/ok-to-test sha=1fb009d2146de597c317a4d2e69e686fb128f271 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@e2e/go.mod`:
- Line 30: The replace directive pins k8s.io/kubernetes to v1.34.0, causing a
version skew with other k8s.io/* staging modules at v0.35.0; update the replace
directive for k8s.io/kubernetes from v1.34.0 to v1.35.0 (referencing the symbol
k8s.io/kubernetes) and then run go mod tidy / go mod vendor as needed to resolve
deps and verify the build and tests compile against the new v1.35.0 version.
🧹 Nitpick comments (1)
providers/v1/bitwarden/go.mod (1)
92-92: Pseudo-version forstructured-merge-diff.This is a pre-release pseudo-version (
v6.3.2-0.20260122202528-d9cc6641c482), which is common in the Kubernetes ecosystem when aligning with unreleased upstream changes. Just ensure this gets updated to a tagged release once one is available.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apis/externalsecrets/v1beta1/secretstore_webhook.go`:
- Around line 26-28: Remove the unnecessary runtime.Object cast when registering
the webhook: call ctrl.NewWebhookManagedBy(mgr, c) (pass the receiver concrete
object directly) instead of ctrl.NewWebhookManagedBy(mgr, runtime.Object(c)) so
the generic builder can infer the correct GVK; update the other similar
occurrences (the other call sites noted around lines 32-35) to match; then
remove the now-unused runtime import. Ensure the
WithValidator(&GenericStoreValidator{}) and Complete() chain remain unchanged.
In `@e2e/go.mod`:
- Line 30: The go.mod replace for k8s.io/kubernetes is pinned to v1.34.0 while
staging modules (e.g., k8s.io/api, k8s.io/client-go, k8s.io/apimachinery) are at
v0.35.0; update the k8s.io/kubernetes replace directive to v1.35.0 so
k8s.io/kubernetes and the staging modules are aligned (or alternatively
downgrade the staging module replaces to v0.34.0) to eliminate the Kubernetes
1.34 vs 1.35 version skew.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apis/externalsecrets/v1/secretstore_validator.go`:
- Around line 40-41: The doc comment above GenericClusterStoreValidator is
misleading—update it to state this validator implements webhook validation for
ClusterSecretStore resources only (and note that GenericStoreValidator handles
SecretStore resources) so readers know which struct covers which resource;
modify the comment text associated with the GenericClusterStoreValidator type
accordingly.
| // GenericClusterStoreValidator implements webhook validation for SecretStore and ClusterSecretStore resources. | ||
| type GenericClusterStoreValidator struct{} |
There was a problem hiding this comment.
Doc comment on GenericClusterStoreValidator is inaccurate.
It says "for SecretStore and ClusterSecretStore" but this struct only handles ClusterSecretStore. GenericStoreValidator handles SecretStore.
📝 Suggested fix
-// GenericClusterStoreValidator implements webhook validation for SecretStore and ClusterSecretStore resources.
+// GenericClusterStoreValidator implements webhook validation for ClusterSecretStore resources.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GenericClusterStoreValidator implements webhook validation for SecretStore and ClusterSecretStore resources. | |
| type GenericClusterStoreValidator struct{} | |
| // GenericClusterStoreValidator implements webhook validation for ClusterSecretStore resources. | |
| type GenericClusterStoreValidator struct{} |
🤖 Prompt for AI Agents
In `@apis/externalsecrets/v1/secretstore_validator.go` around lines 40 - 41, The
doc comment above GenericClusterStoreValidator is misleading—update it to state
this validator implements webhook validation for ClusterSecretStore resources
only (and note that GenericStoreValidator handles SecretStore resources) so
readers know which struct covers which resource; modify the comment text
associated with the GenericClusterStoreValidator type accordingly.
|
/ok-to-test sha=bdd6704a2a719926e7c4a456b946130e95e4185e |
|
/ok-to-test sha=cde2f70a176a68a04ab953b8164a02706d7ff408 |
evrardj-roche
left a comment
There was a problem hiding this comment.
Not tested, but it looks okay with my eyes. I did not review the go.mod/go.sum changes, only the api/ and docs/ .
I did not run this, as CI will most likely run this.
I don't see a problem with this, it's good it's done.
I would have prefered more documentation in the git log about how you did this, and the problems you got. This way the pain is recorded and useful for the next ones that come in the future.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
We can't use a generic conversion because the GVK information is lost and the validation will fail. Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
cde2f70 to
adbe808
Compare
|
/ok-to-test-managed sha=adbe8089a834b7bb838213b1fbe7cf21b7bb603c provider=aws |
|
Yay, it passed. :) |
|
Signed-off-by: Nattapong Ekudomsuk <nuttapong_mos@hotmail.com>
Problem Statement
Jesus this was painful.
Related Issue
Fixes #5926
Proposed Changes
How do you like to solve the issue and why?
Format
Please ensure that your PR follows the following format for the title:
Where
scopeis optionally one of:Checklist
git commit --signoffmake testmake reviewableSummary
Upgrades controller-runtime from v0.22.3 → v0.23.1 and aligns many Kubernetes and ecosystem dependencies to the corresponding newer versions (k8s.io/* → v0.35.0, multiple go-openapi, protobuf, prometheus, golang.org/x/*, etc.).
Key Changes
Webhook registration
Validators / Admission API
Dependency updates
Notable files touched
Purpose
Make code compatible with controller-runtime v0.23.x API changes (webhook builder and typed admission.Validator) so the project builds with the updated dependency set.