Embed only a subset of metav1.ObjectMeta fields

[vincepri]

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR removes all references to metav1.ObjectMeta to embedded resources. This patch came from a discussion in the upstream kubebuilder slack. The metav1.ObjectMeta type, when embedded, contains a lot of fields that cannot be set from a client. This PR bring a copy of ObjectMeta into Cluster API including only fields that a user can set.

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 #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 [vincepri]

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

[vincepri]

/assign @ncdc

[davidewatson]

/lgtm
/hold

This looks okay to me though it is a significant breaking change so we should probably wait until a weekly meeting to ensure consensus.

[davidewatson]

Can we leave more context? For example, why is this necessary for Machine and not other CRDs? And if it's only because Machine is embedded in other CRDs, then is there an upstream ticket to track this issue?

[vincepri]

Added more context

[davidewatson]

This is a fair explanation. I was mostly thinking about the problem this attempts to solve, and how it is likely to come up again. For example, suppose someone embeds MachineDeployment in another CRD. Won't they run into these same issues? And if so, why not remove ObjectMeta from MachineDeployment too?

I guess I am wondering what the long term guidance is from api machinery...

[ncdc]

There's a couple of things going on here:

1. All objects have root-level ObjectMeta
2. Some objects, such as Machine and MachineSet, include non-root ObjectMeta somewhere in their nested structures

If anyone includes any Kubernetes object (built-in or CRD) in their own object, you'll end up with a similar situation: root-level ObjectMetadata along with non-root ObjectMeta. In the past, we've tried to design the API types such that the portions that someone might want to include are reusable (e.g. including MachineSpec in MachineSet (instead of a full Machine)). This could work, in theory, except some of these types have an ObjectMeta in them, even though they're not root-level types. It would be nice if these types used some other structure instead of ObjectMeta.

That's not necessarily a problem - we've been doing it for a while in CAPI, and core Kubernetes does it too. The problem @vincepri is running into is that the newer version of kubebuilder/controller-runtime/controller-tools generates openapi schema validation definitions that make it so you can't successfully create a Machine because of this nested ObjectMeta and a null creationTimestamp field.

[ncdc]

Also, when including a non-root ObjectMeta field, a lot of the time it's just to use it for the annotations and/or labels, but not the entire set of fields. It may be better to scope the non-root field to just the fields you need, instead of a full ObjectMeta struct.

[neolit123]

+1 on inspecting long term effects.

i think the PR should include more detail on the actual problem it's solving.
e.g. comments about:

validation failure list:
            spec.metadata.creationTimestamp in body must be of type string: "null"

something puzzling in the slack thread was that this started happening suddenly, did you figure out what caused that? was it kubernetes/kubernetes#77653 ?

[davidewatson]

/hold cancel

Merging this now unblocks v1alpha2 work. We should discuss further on Wednesday.

[ncdc]

I like it! Merge, unblock, correct later if needed. 🎉

[vincepri]

@ncdc Adding more details to the commit, one second

[vincepri]

@ncdc @neolit123 @davidewatson Added more details to the comment above ObjectMeta.

@neolit123 The issue popped up for multiple reasons, I'm unaware of the implications for the PR you mentioned. The change that broke this behavior for us is in controller-tools@v2, which now doesn't add an exception for ObjectMeta and generates validation properties for all the sub-fields.

[ncdc]

/lgtm