🌱 Stop relying on GVK being set on regular typed objects #9956

sbueringer · 2024-01-04T14:53:34Z

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:
Today controller-runtime doesn't set the GVKs of objects in all cases.

The current state is:

APIReader:
- sets GVK on Unstructured objects
- does not set GVK on regular typed objects
Cached reader:
- sets GVK on Unstructured objects
- sets GVK on regular typed objects (if disableDeepCopy = false, which is default)
fake client
- sets GVK on Unstructured objects
- sets GVK on regular typed objects
- Note: This will probably be changed with CR v0.17 (⚠️ Fakeclient: Only set TypeMeta for unstructured controller-runtime#2633)

Because of these inconsistencies (that cannot be easily changed upstream) and because it's hard to tell across entire call stacks where objects come from I would suggest to only rely on GVK being set on Unstructured objects. For Unstructured objects GVK has to be always set to be able to identify the object, for regular typed objects this is not the case.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sbueringer · 2024-01-04T15:10:29Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -416,7 +416,7 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex
 	if scope.Config.Spec.InitConfiguration == nil {
 		scope.Config.Spec.InitConfiguration = &bootstrapv1.InitConfiguration{
 			TypeMeta: metav1.TypeMeta{
-				APIVersion: "kubeadm.k8s.io/v1beta1",
+				APIVersion: "kubeadm.k8s.io/v1beta3",


@fabriziopandini I guess this wasn't a problem because this code was never actually used?

I need to dig into this, I will follow up ASAP (eventually we can drop this from this PR if you want to move forward)

I think the PR doesn't change it for the worse

sbueringer · 2024-01-04T15:13:24Z

internal/controllers/topology/cluster/patches/engine.go

@@ -193,7 +193,7 @@ func addVariablesForPatch(blueprint *scope.ClusterBlueprint, desired *scope.Clus
 		if item.HolderReference.Kind == "MachineDeployment" {
 			md, ok := mdStateIndex[item.HolderReference.Name]
 			if !ok {
-				return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KObj(md.Object))
+				return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KRef(item.HolderReference.Namespace, item.HolderReference.Name))


This panic'ed previously if we actually hit this line

Nice catch!

sbueringer · 2024-01-04T15:17:39Z

/assign @vincepri @fabriziopandini @killianmuldoon @chrischdi

sbueringer · 2024-01-04T15:17:53Z

/test pull-cluster-api-e2e-full-main

chrischdi · 2024-01-08T14:22:56Z

/lgtm

k8s-ci-robot · 2024-01-08T14:23:03Z

LGTM label has been added.

Git tree hash: 2f2609fd5a6c83cc5228ddac2174f21b59ba7782

Signed-off-by: Stefan Büringer buringerst@vmware.com

chrischdi

/lgtm

k8s-ci-robot · 2024-01-11T07:33:50Z

LGTM label has been added.

Git tree hash: 17bbc8d6dff53dad0bde996c8f0ee540cd0d9bdc

fabriziopandini

Thanks for taking care of this change, only few nits from my side

internal/controllers/topology/cluster/desired_state.go

fabriziopandini · 2024-01-11T17:02:36Z

internal/controllers/topology/cluster/patches/engine.go

@@ -193,7 +193,7 @@ func addVariablesForPatch(blueprint *scope.ClusterBlueprint, desired *scope.Clus
 		if item.HolderReference.Kind == "MachineDeployment" {
 			md, ok := mdStateIndex[item.HolderReference.Name]
 			if !ok {
-				return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KObj(md.Object))
+				return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KRef(item.HolderReference.Namespace, item.HolderReference.Name))


Nice catch!

internal/controllers/topology/cluster/patches/template.go

fabriziopandini · 2024-01-11T19:39:07Z

/lgtm
/approve

k8s-ci-robot · 2024-01-11T19:40:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

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

…7468) * fix(deps): update module sigs.k8s.io/controller-runtime to v0.17.0 * make generate * Stop relying on GVK being set on SCP object TestReconcileStackConfigPolicy_Reconcile fails because with kubernetes-sigs/controller-runtime#2633 the fakeClient does not set the TypeMeta of the StackConfigPolicy object, so its typeMeta.Kind becomes empty but CanBeOwnedBy depends on it. The behavior change to not populate TypeMeta for regular object is to be aligned with the behavior of the real client, see kubernetes-sigs/cluster-api#9956: - APIReader does not set GVK on regular typed objects - Cached reader does not set GVK on regular typed objects (if disableDeepCopy = false, which is NOT default). We could just set the Kind of the StackConfigPolicy object in the tests but it's better to stop relying on GVK being set on the StackConfigPolicy object. --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Thibault Richard <thb.krkr@gmail.com>

k8s-ci-robot requested review from JoelSpeed and stmcginnis January 4, 2024 14:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2024

sbueringer force-pushed the pr-gvk branch from e7e6e0f to 00646f6 Compare January 4, 2024 15:09

sbueringer commented Jan 4, 2024

View reviewed changes

sbueringer mentioned this pull request Jan 4, 2024

🌱 Stop relying on GVK being set on regular typed objects kubernetes-sigs/cluster-api-provider-vsphere#2591

Merged

sbueringer added the area/api Issues or PRs related to the APIs label Jan 4, 2024

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Jan 4, 2024

k8s-ci-robot assigned chrischdi Jan 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2024

Don't rely on GVK being set on regular objects

06ebe94

Signed-off-by: Stefan Büringer buringerst@vmware.com

sbueringer force-pushed the pr-gvk branch from 00646f6 to 06ebe94 Compare January 10, 2024 16:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2024

k8s-ci-robot requested a review from chrischdi January 10, 2024 16:58

chrischdi reviewed Jan 11, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2024

fabriziopandini reviewed Jan 11, 2024

View reviewed changes

k8s-ci-robot assigned fabriziopandini Jan 11, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2024

k8s-ci-robot merged commit ec2abf8 into kubernetes-sigs:main Jan 11, 2024
20 checks passed

k8s-ci-robot added this to the v1.7 milestone Jan 11, 2024

sbueringer deleted the pr-gvk branch January 12, 2024 12:38

thbkrkr mentioned this pull request Jan 30, 2024

fix(deps): update module sigs.k8s.io/controller-runtime to v0.17.0 elastic/cloud-on-k8s#7468

Merged

1 task

mboersma mentioned this pull request Mar 27, 2024

Unit test failure in CAPZ w/ CAPI v1.7.0-beta.1 #10331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Stop relying on GVK being set on regular typed objects #9956

🌱 Stop relying on GVK being set on regular typed objects #9956

sbueringer commented Jan 4, 2024

sbueringer Jan 4, 2024

fabriziopandini Jan 11, 2024

sbueringer Jan 11, 2024

sbueringer Jan 4, 2024 •

edited

fabriziopandini Jan 11, 2024

sbueringer commented Jan 4, 2024

sbueringer commented Jan 4, 2024

chrischdi commented Jan 8, 2024

k8s-ci-robot commented Jan 8, 2024

chrischdi left a comment

k8s-ci-robot commented Jan 11, 2024

fabriziopandini left a comment

fabriziopandini Jan 11, 2024

fabriziopandini commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

🌱 Stop relying on GVK being set on regular typed objects #9956

🌱 Stop relying on GVK being set on regular typed objects #9956

Conversation

sbueringer commented Jan 4, 2024

sbueringer Jan 4, 2024

Choose a reason for hiding this comment

fabriziopandini Jan 11, 2024

Choose a reason for hiding this comment

sbueringer Jan 11, 2024

Choose a reason for hiding this comment

sbueringer Jan 4, 2024 • edited

Choose a reason for hiding this comment

fabriziopandini Jan 11, 2024

Choose a reason for hiding this comment

sbueringer commented Jan 4, 2024

sbueringer commented Jan 4, 2024

chrischdi commented Jan 8, 2024

k8s-ci-robot commented Jan 8, 2024

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 11, 2024

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jan 11, 2024

Choose a reason for hiding this comment

fabriziopandini commented Jan 11, 2024

k8s-ci-robot commented Jan 11, 2024

sbueringer Jan 4, 2024 •

edited