🐛 Add cross-version compatibility with client-go 1.27 by howardjohn · Pull Request #2223 · kubernetes-sigs/controller-runtime

howardjohn · 2023-03-03T18:04:24Z

I have manually verified this compiles with client-go 1.26 and 1.27, but I am not sure how to get test coverage of this; likely we will not be able to.

k8s-ci-robot · 2023-03-03T18:04:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

lavalamp · 2023-03-03T18:22:29Z

+			}
+		}
+	}
+	return true


seems like this will give a problematic answer with the old client?

I would expect an old client can never call this since they would see a type ResourceEventHandlerRegistration interface{} as the return type. Of course they could attempt to type-cast that and would always get true, but that seems pretty obscure?

I see, gotcha

lavalamp · 2023-03-03T18:29:55Z

 			return nil, err
 		}
-		handles[ns] = registration
+		handles.handles[ns] = registration


won't the multiple informers collide and overwrite the prior one's handle?

wait, there's a different informer per namespace? ok nvm, sorry.

lavalamp · 2023-03-03T18:37:07Z

+}
+
+func (e eventHandlerWrapper) OnAdd(obj interface{}) {
+	if m, ok := e.handler.(modernResourceEventHandler); ok {


clever, lol

This remind me of the http handlerfunc tech in the std library.

lavalamp · 2023-03-03T18:38:54Z

LGTM but I'm not a reviewer here :)

howardjohn · 2023-03-09T17:38:59Z

/assign @alvaroaleman

alvaroaleman · 2023-03-11T19:32:17Z

+// eventHandlerWrapper wraps a ResourceEventHandler in a manner that is compatible with client-go 1.27+ and older.
+// The interface was changed in these versions.
+type eventHandlerWrapper struct {
+	handler any


Can we use generics to type this to one of the two interfaces?

I don't think so, or at least do not know how to. See golang/go#49054

alvaroaleman · 2023-03-11T19:44:00Z

+
+// HasSynced asserts that the handler has been called for the full initial state of the informer.
+// This uses syncer to be compatible between client-go 1.27+ and older versions when the interface changed.
+func (h handlerRegistration) HasSynced() bool {


I think I am blind, where do we call this?

We don't call this, if allows the interface to be implemented on 0.27

Since they added this function

k8s-ci-robot · 2023-03-15T23:42:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, howardjohn

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:

~~OWNERS~~ [alvaroaleman]

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

Ref fix: kubernetes-sigs/controller-runtime#2223 vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:308:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:321:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:326:17: impossible type assertion: h.(map[string]toolscache.ResourceEventHandlerRegistration) map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) make: *** [vendor/github.com/openshift/build-machinery-go/make/targets/golang/build.mk:16: build] Error 1 Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

go get sigs.k8s.io/controller-runtime@v0.15.0-alpha.1 go mod tidy go mod vendor Reference fix: kubernetes-sigs/controller-runtime#2223 to address the following build issue: vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:308:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:321:9: cannot use handles (variable of type map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration) as "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration value in return statement: map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) vendor/sigs.k8s.io/controller-runtime/pkg/cache/multi_namespace_cache.go:326:17: impossible type assertion: h.(map[string]toolscache.ResourceEventHandlerRegistration) map[string]"k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration does not implement "k8s.io/client-go/tools/cache".ResourceEventHandlerRegistration (missing method HasSynced) make: *** [vendor/github.com/openshift/build-machinery-go/make/targets/golang/build.mk:16: build] Error 1 Signed-off-by: Flavio Fernandes <flaviof@redhat.com>

Updating k8s libs to v1.27.1 requires updating sigs.k8s.io/controller-runtime to an unreleased version with kubernetes-sigs/controller-runtime#2223 in place. This in turn requires some additional adjustments to changed APIs in the controller-runtime, mainly in operator/pkg/gateway-api. The service.kubernetes.io/topology-aware-hints annotation got deprecated with k8s 1.27 in favor of service.kubernetes.io/topology-mode (see kubernetes/kubernetes#116522). This change keeps support for the deprecated annotation. Support for the new annotation will be added in a successive commit. Signed-off-by: Nate Sweet <nathanjsweet@pm.me>

Updating k8s libs to v1.27.2 requires updating sigs.k8s.io/controller-runtime to the 0.15 version with kubernetes-sigs/controller-runtime#2223 in place. This in turn requires some additional adjustments to changed APIs in the controller-runtime, mainly in operator/pkg/gateway-api. Specifically, around request methods and testing. The service.kubernetes.io/topology-aware-hints annotation got deprecated with k8s 1.27 in favor of service.kubernetes.io/topology-mode (see kubernetes/kubernetes#116522). This change keeps support for the deprecated annotation. Support for the new annotation will be added in a successive commit. Signed-off-by: Tobias Klauser <tobias@cilium.io> Signed-off-by: Nate Sweet <nathanjsweet@pm.me>

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 3, 2023

k8s-ci-robot requested review from varshaprasad96 and vincepri March 3, 2023 18:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 3, 2023

lavalamp reviewed Mar 3, 2023

View reviewed changes

Comment thread pkg/internal/source/event_handler.go

Add cross-version compatibility with client-go 1.27

13bba74

howardjohn marked this pull request as ready for review March 3, 2023 18:53

howardjohn force-pushed the client/1-27 branch from ccc77d3 to 13bba74 Compare March 3, 2023 18:53

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2023

k8s-ci-robot requested a review from inteon March 3, 2023 18:53

howardjohn mentioned this pull request Mar 7, 2023

✨ upgrade to k8s 0.27.1 #2189

Merged

k8s-ci-robot assigned alvaroaleman Mar 9, 2023

twz123 mentioned this pull request Mar 10, 2023

Bump Kubernetes to v1.27 k0sproject/k0s#2879

Closed

11 tasks

alvaroaleman reviewed Mar 11, 2023

View reviewed changes

howardjohn requested review from alvaroaleman and removed request for inteon, varshaprasad96 and vincepri March 15, 2023 15:33

alvaroaleman approved these changes Mar 15, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2023

k8s-ci-robot merged commit a26de2d into kubernetes-sigs:main Mar 15, 2023

sbueringer mentioned this pull request May 17, 2023

Consider stabilizing the API and moving to v1 and beyond #2327

Closed

gcs278 mentioned this pull request May 17, 2023

OCPBUGS-13965: Bump vendors k8s libraries to 0.27.2 openshift/cluster-dns-operator#368

Merged

flavio-fernandes mentioned this pull request May 17, 2023

[WIP] SDN-3901: CNO 4.14 kubernetes 1.27 openshift/cluster-network-operator#1806

Closed

nathanjsweet mentioned this pull request May 22, 2023

Update k8s tests and libraries to v1.27.2 cilium/cilium#25602

Merged

Huang-Wei mentioned this pull request Jul 26, 2023

controller-runtime version 0.14.X incompatible with client-go 0.27 kubernetes-sigs/scheduler-plugins#606

Closed

ckadner mentioned this pull request Jan 24, 2024

chore: Update indirect dependency to knative.dev/serving v0.37.5 kserve/modelmesh-serving#468

Merged

ysdongAmazon mentioned this pull request Mar 4, 2024

Bump helm.sh/helm/v3 from 3.11.3 to 3.14.2 aws/aws-app-mesh-controller-for-k8s#759

Closed

aaroniscode mentioned this pull request May 9, 2024

Upgrade Kubernetes libs to v0.29.x. kubernetes-sigs/aws-load-balancer-controller#3675

Closed

4 tasks

sbueringer mentioned this pull request Mar 16, 2025

✨ Leverage isInInitialList of ResourceEventHandler.OnAdd, Add IsInInitialList to TypedCreateEvent #3162

Merged

Conversation

howardjohn commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lavalamp commented Mar 3, 2023

Uh oh!

howardjohn commented Mar 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

howardjohn commented Mar 3, 2023 •

edited

Loading