Extend apis.kcp.io/v1alpha2 with APIBinding

[xmudrii]

Summary

This PR extends the apis.kcp.io/v1alpha2 with APIBindings and related types. This new version is a 1:1 copy of the current v1alpha1 API at the moment. The new v1alpha2 version will be changed after this PR is merged (to make this PR easier to review) as part of implementing #3255

All the code was updated to refer to v1alpha2.APIBindings.

#3318 has been used as the inspiration for this PR.

What Type of PR Is This?

/kind feature

Related Issue(s)

None

Release Notes

Extended `apis.kcp.io/v1alpha2` with `APIBinding`

[kcp-ci-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS
• config/crds/OWNERS
• pkg/admission/OWNERS
• sdk/apis/OWNERS

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

[sttts]

Sgtm.

[sttts]

we are breaking rollback with this

[xmudrii]

@sttts I believe this is coming from:

kcp/sdk/apis/apis/v1alpha2/types_apibinding.go

Line 34 in b564d60

// +kubebuilder:storageversion

I took that from APIExports v1alpha2:

kcp/sdk/apis/apis/v1alpha2/types_apiexport.go

Line 48 in e2ba960

// +kubebuilder:storageversion

I don't know if this is needed for APIExports and not needed for APIBindings, but my thought was to stay consistent. Given that, this would break rollback for APIExports too, right?

Do you have any alternative idea what to do about this?

[xmudrii]

@sttts I discussed this with @xrstf (since he did the APIExports/v1alpha2 implementation) and came to the following realisation. I think the idea here is that we might change this API in the backwards incompatible way especially in the development phase (i.e. until the v0.27.0 release). Making such changes while the version is the storage version means it could break reading from the etcd, as the API server could run into fields and types that it can't recognize. And implementing a migration mechanism for that case specifically sounds like lots of work, especially that we already have conversion from v1alpha1 to v1alpha2.

That said, I think the idea is that the previous version should be the storage version at least until just before the release or until we're confident into the new API. In this case, both APIBindings and APIExports should use v1alpha1 as the storage version.

Did I get this correctly?

[sttts]

While in general this looks good, the timing is questionable. Why don't we first get the API in place properly with the fields we want, and AFTER that do the "now everything will use it" PR ? Seems to be much more natural rather than having a limbo phase where the version is not ready, but already used.

[mjudeikis]

While in general this looks good, the timing is questionable. Why don't we first get the API in place properly with the fields we want, and AFTER that do the "now everything will use it" PR ? Seems to be much more natural rather than having a limbo phase where the version is not ready, but already used.

I think the bot works IF we can get the next PR "chained to this PR " and merge them together. Would want to see "implementation" first before start merging.

[xmudrii]

While in general this looks good, the timing is questionable. Why don't we first get the API in place properly with the fields we want, and AFTER that do the "now everything will use it" PR ? Seems to be much more natural rather than having a limbo phase where the version is not ready, but already used.

I'm fine with that, I'm already working on applying the implementation to v1alpha2 and I'll do as @mjudeikis proposed.

[xmudrii]

@sttts @mjudeikis I created #3402 based on this PR with implementation for #3255. The PR is in the WIP state because I'm yet to add some more tests, but other than that, the implementation is complete and ready for review.

[xmudrii]

/retest

[kcp-ci-bot]

@xmudrii: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kcp-test-e2e-multiple-runs	fe73019	link	true	/test pull-kcp-test-e2e-multiple-runs

Full PR test history

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 kubernetes/test-infra repository. I understand the commands that are listed here.