apis/v1alpha2: move ServiceImport spec fields #85

MrFreezeex · 2024-12-03T13:25:43Z

This PR is meant to supersedes #52, most commits are from this linked PR done by @mikemorris (kudos to him!) and with the few suggestions left applied and making sure everything generate well

k8s-ci-robot · 2024-12-03T13:25:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrFreezeex
Once this PR has been reviewed and has the lgtm label, please assign skitt 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

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

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>

MrFreezeex · 2024-12-03T13:43:20Z

While this adds initial support for v1alpha2 in this repo the next step involves:

Updating the coredns/multicluster plugin to detect which version is installed and selectively watch one api or the other (so that new coredns version are still compatible with v1alpha1)
Update e2e tests (that depends on the coredns plugin) and storage version to v1alpha2
Possibly updating conformance tests to this versions (cc @tpantelis for opinion on this aspect?)
Figuring out the code/recommendation for implementations for the webhooks

MrFreezeex · 2024-12-03T14:01:37Z

/test pull-mcs-api-e2e

tpantelis · 2024-12-07T17:17:01Z

pkg/apis/v1alpha1/serviceimport.go

@@ -23,6 +23,8 @@ import (

 // +genclient
 // +kubebuilder:object:root=true
+// +kubebuilder:storageversion


Shouldn't the new v1alpha2 be the stored version?

The problem is that the CoreDNS plugin (which is used in the e2e tests in this repo!) or anything watching mcs api ressources would need to support v1alpha2 explicitly or that we have the webhooks already deployed/figured out. So my plan would be to do this after CoreDNS supports v1alpha2, see this #85 (comment)

tpantelis · 2024-12-07T17:34:22Z

config/crd-base/multicluster.x-k8s.io_serviceimports.yaml

What is this file for?

I am not entirely sure tbh, it was part of the crd generation before 🤷‍♂️

tpantelis · 2024-12-07T17:38:28Z

config/crd/multicluster.x-k8s.io_serviceimports.yaml

+        - type
+        type: object
+    served: true
+    storage: false


This file does not specify any webhook conversion config so who is the intended consumer?

I wanted to figure out webhooks as a later steps when we would be actually storing v1alpha2, but we also said at the last sig meeting that we could provide only the webhook code and not the deployment manifests so I suppose that implementation would have to update that aspect if they want to have a webhook. TBD when we would be actually implementing that and figured out recomendations to integrate it for implementations.

I can work on the webhook code - shouldn't be difficult.

Sure that would be great! Note that I would like to submit the CoreDNS PR to support v1alpha2 as soon as possible with the hope that it gets merged before the winter holidays by the CoreDNS folks. So I would recommend to not block the v1alpha2 version based on having the webhook or not.

tpantelis · 2024-12-07T17:58:46Z

While this adds initial support for v1alpha2 in this repo the next step involves:

Possibly updating conformance tests to this versions (cc @tpantelis for opinion on this aspect?)

My thoughts are that the consuming test code can be updated to reference the v1alpha2 Go versions. At test suite setup, we read the CRD and and check which versions are served and create dynamic ResourceClients with the selected GVRs (biasing towards v1alpha2). The functions to read/create ServiceImports/ServiceExports would use the dynamic clients instead of the generated client sets. If v1alpha2 is served in the CRD then we can directly convert the Unstructured to the v1alpha2 Go struct (and vice versa). Otherwise, we first convert the v1alpha1 Unstructured to v1alpha2 (using the same code as the webhook converter) and then convert to the v1alpha2 Go struct.

skitt · 2024-12-09T10:41:20Z

pkg/apis/v1alpha1/serviceimport.go

@@ -23,6 +23,8 @@ import (

 // +genclient
 // +kubebuilder:object:root=true
+// +kubebuilder:storageversion
+// +kubebuilder:resource:shortName=svcimport


The short name for service imports is currently svcim (see the CRDs).

I know but as the name "svcim" is not anywhere else I think it might be more a lack of customization than something intended, I added both the new name and kept the old in my latest push. Does this looks ok to you? We can separate this change in another PR (it's already in a separated commit FYI) if needed

skitt · 2024-12-09T10:41:44Z

pkg/apis/v1alpha1/serviceexport.go

@@ -22,6 +22,8 @@ import (

 // +genclient
 // +kubebuilder:object:root=true
+// +kubebuilder:storageversion
+// +kubebuilder:resource:shortName=svcexport


The short name for service exports is currently svcex.

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>

mikemorris added 4 commits December 3, 2024 13:28

apis/v1alpha2: move ServiceImport spec fields to status

b7d110b

collapse spec/status to flatten ServiceImport definition

6c5f8da

add CEL kubebuilder validation for ClusterName

cdd2707

update codegen

63386c7

k8s-ci-robot requested review from lauralorenz and skitt December 3, 2024 13:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2024

MrFreezeex added 2 commits December 3, 2024 14:37

Applying PR suggestions

df706d6

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>

Fix generation of crd with v1alpha2

63207d6

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>

MrFreezeex force-pushed the v1alpha2-update branch from 9f8a8a9 to 7538fca Compare December 3, 2024 13:39

MrFreezeex changed the title ~~apis/v1alpha2: move ServiceImport spec fields to status~~ apis/v1alpha2: move ServiceImport spec fields Dec 4, 2024

tpantelis reviewed Dec 7, 2024

View reviewed changes

skitt reviewed Dec 9, 2024

View reviewed changes

add convenient short name

0e449b4

Signed-off-by: Arthur Outhenin-Chalandre <arthur.outhenin-chalandre@ledger.fr>

MrFreezeex force-pushed the v1alpha2-update branch from 7538fca to 0e449b4 Compare December 9, 2024 11:57

This was referenced Dec 10, 2024

Update service controller to add all the IPs from derived service #80

Open

KEP 1645: add labels and annotations export kubernetes/enhancements#4922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apis/v1alpha2: move ServiceImport spec fields #85

apis/v1alpha2: move ServiceImport spec fields #85

MrFreezeex commented Dec 3, 2024

k8s-ci-robot commented Dec 3, 2024

MrFreezeex commented Dec 3, 2024 •

edited

Loading

MrFreezeex commented Dec 3, 2024

tpantelis Dec 7, 2024

MrFreezeex Dec 7, 2024 •

edited

Loading

tpantelis Dec 7, 2024

MrFreezeex Dec 7, 2024

tpantelis Dec 7, 2024

MrFreezeex Dec 7, 2024 •

edited

Loading

tpantelis Dec 7, 2024

MrFreezeex Dec 7, 2024 •

edited

Loading

tpantelis commented Dec 7, 2024

skitt Dec 9, 2024 •

edited

Loading

MrFreezeex Dec 9, 2024

skitt Dec 9, 2024

apis/v1alpha2: move ServiceImport spec fields #85

Are you sure you want to change the base?

apis/v1alpha2: move ServiceImport spec fields #85

Conversation

MrFreezeex commented Dec 3, 2024

k8s-ci-robot commented Dec 3, 2024

MrFreezeex commented Dec 3, 2024 • edited Loading

MrFreezeex commented Dec 3, 2024

tpantelis Dec 7, 2024

Choose a reason for hiding this comment

MrFreezeex Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

tpantelis Dec 7, 2024

Choose a reason for hiding this comment

MrFreezeex Dec 7, 2024

Choose a reason for hiding this comment

tpantelis Dec 7, 2024

Choose a reason for hiding this comment

MrFreezeex Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

tpantelis Dec 7, 2024

Choose a reason for hiding this comment

MrFreezeex Dec 7, 2024 • edited Loading

Choose a reason for hiding this comment

tpantelis commented Dec 7, 2024

skitt Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

MrFreezeex Dec 9, 2024

Choose a reason for hiding this comment

skitt Dec 9, 2024

Choose a reason for hiding this comment

MrFreezeex commented Dec 3, 2024 •

edited

Loading

MrFreezeex Dec 7, 2024 •

edited

Loading

MrFreezeex Dec 7, 2024 •

edited

Loading

MrFreezeex Dec 7, 2024 •

edited

Loading

skitt Dec 9, 2024 •

edited

Loading