Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ ClusterClass patches: Allow matching all MachineDeploymentClasses #6930

Merged

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Jul 15, 2022

What this PR does / why we need it:

This PR adds a new feature to allow matching all "*" MachineDeploymentClasses.

  1. Match all if MachineDeploymentClass.Names is a "*".
matchResources:
  machineDeploymentClass:
    names : [*]
  1. Match some if explicitly mentioned.
matchResources:
  machineDeploymentClass:
    names: [something, something, something]
  1. Match some if glob matched.
matchResources:
  machineDeploymentClass:
    names: [something-*]
  1. Match none if no machineDeploymentClass is set (should produce a webhook failure)
matchResources:
  machineDeploymentClass: {}

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 #5648

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 15, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @valaparthvi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 15, 2022
@valaparthvi valaparthvi changed the title ✨ ClusterClass patches: Allow matching all MachineDeploymentClasses WIP: ✨ ClusterClass patches: Allow matching all MachineDeploymentClasses Jul 15, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2022
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this might be useful, but is there a cleaner way to do it in the API? Just not a fan of using regex-lookalike syntax if we're not supporting any real pattern matching. Would it be reasonable to have the empty selector act as a match all? i.e. https://github.com/kubernetes/apimachinery/blob/afc5e00a762a432aa7d988d711dab6dcf3427585/pkg/apis/meta/v1/helpers.go#L38

  • No selector matches nothing
  • Empty selector matches everything

@sbueringer
Copy link
Member

sbueringer commented Jul 15, 2022

I think using * which is already established in Kubernetes for ClusterRoles/Roles is better than using something like empty or no selector. It's way more explicit that way.

How would an empty selector look like for the names slice that we have today?

        matchResources:
          machineDeploymentClass:
            names:
            - default-worker

@killianmuldoon
Copy link
Contributor

How would an empty selector look like for the names slice that we have today?

        matchResources:
          machineDeploymentClass: {}

That's my understanding of how it normally works in Kubernetes with the label selector. It would require enabling it (and maybe even setting it) in the webhook.

I think it's more consistent to stick to how selectors work in Kubernetes rather than RBAC, despite it being implicit. Definitely not a blocker for implementing it as is with the '*' notation, but it does depart from convention IMO.

@killianmuldoon
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 15, 2022
@valaparthvi
Copy link
Contributor Author

@killianmuldoon @sbueringer So, what have you decided? Are we going with K8s way or RBAC way?
The K8s way does not seem very intuitive to me, or if that is the way it happens generally, it might be intuitive for others. RBAC on the other hand is quite simple.

@sbueringer
Copy link
Member

sbueringer commented Jul 18, 2022

Thought about it a bit more. I think Killian is right. It makes sense to stay consistent with Kubernetes selectors.

This would give us the following semantics:

Match specific MD classes

        matchResources:
          controlPlane: true
          machineDeploymentClass:
            names:
            - default-worker

Match all MD classes

        matchResources:
          controlPlane: true
          machineDeploymentClass: {}

Match none MD classes

        matchResources:
          controlPlane: true

We should not set the selector in the defaulting webhook. The idea in the ClusterClass API is that folks should explicitly decide to select InfraCluster, CP, MD classes, ... . If we would default to {} the selector would match all MD classes per default and folks have to set some not-matching MD class names to still be able to not match MD classes.

(I think it comes down to in Kubernetes it's default: "match all", and in our cases the default is "match none")

One nice thing about {} is that it works on the machineDeploymentClass level. While * would be part of names. It seems better to have that match all only once on the machineDeploymentClass vs needing it in names and every other filter critera we might add in the future.

Independent of that, I absolutely don't like what they did there it is way to implicit for my taste.

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jul 27, 2022

@sbueringer I can't seem to run unit tests for util/patch/patch_test.go locally, neither from my branch, and nor from main branch. I see they're failing on CI as well. Do I need to run some setup? I have a kind cluster running.

Here is the output from when I run the tests.
/usr/local/go/bin/go tool test2json -t /tmp/GoLand/___TestPatchHelper_in_sigs_k8s_io_cluster_api_util_patch.test -test.v -test.paniconexit0 -test.run ^\QTestPatchHelper\E$
E0727 18:57:10.811564  125075 logr.go:265] controller-runtime/test-env "msg"="unable to start the controlplane" "error"="fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory"  "tries"=0
E0727 18:57:10.812226  125075 logr.go:265] controller-runtime/test-env "msg"="unable to start the controlplane" "error"="fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory"  "tries"=1
E0727 18:57:10.812763  125075 logr.go:265] controller-runtime/test-env "msg"="unable to start the controlplane" "error"="fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory"  "tries"=2
E0727 18:57:10.813390  125075 logr.go:265] controller-runtime/test-env "msg"="unable to start the controlplane" "error"="fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory"  "tries"=3
E0727 18:57:10.813975  125075 logr.go:265] controller-runtime/test-env "msg"="unable to start the controlplane" "error"="fork/exec /usr/local/kubebuilder/bin/etcd: no such file or directory"  "tries"=4
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x14f55af]

goroutine 1 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane.(*APIServer).Stop(0x14)
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/apiserver.go:424 +0x8f
sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane.(*ControlPlane).Stop(0xc0005af180)
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane/plane.go:97 +0x3d
sigs.k8s.io/controller-runtime/pkg/envtest.(*Environment).Stop(0xc0005af180)
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/envtest/server.go:194 +0x114
sigs.k8s.io/cluster-api/internal/test/envtest.newEnvironment({0x0, 0x0, 0xc000598600})
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/internal/test/envtest/environment.go:209 +0x22e5
sigs.k8s.io/cluster-api/internal/test/envtest.Run({0x1de86b0, 0xc0004ccf40}, {0xc000184000, {0x0, 0x0, 0x0}, 0x0, 0x0, 0x1c2d2a8, {0x0, ...}})
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/internal/test/envtest/environment.go:112 +0x66
sigs.k8s.io/cluster-api/util/patch.TestMain(0xc0000001a0)
	/home/pvala/go/src/github.com/kubernetes-sigs/cluster-api/util/patch/suite_test.go:39 +0x66
main.main()
	_testmain.go:51 +0x165

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Jul 27, 2022

The issue seems to be your env test setup - do all the tests run when you run make test ? You may need to set the KUBEBUILDER_ASSETS env variable

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jul 27, 2022

The issue seems to be your env test setup - do all the tests run when you run make test ?
You may need to set the KUBEBUILDER_ASSETS env variable

I was actually running the tests directly from my GoLand editor, and I didn't realize I had to set the envvar, but it seems like make test passed on both the branches.

I also noticed pull-cluster-api-verify-main is failing because I failed to update config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml. Is there a Make target that I should run to update this yaml or will this be a manual change?

pull-cluster-api-verify-main failure API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,GeneratePatchesResponseItem,Patch API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Items API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequest,Variables API rule violation: list_type_missing,sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1,ValidateTopologyRequestItem,Variables make[1]: Leaving directory '/home/prow/go/src/sigs.k8s.io/cluster-api' diff --git a/api/v1beta1/zz_generated.openapi.go b/api/v1beta1/zz_generated.openapi.go index 01c418b08..9868b1071 100644 --- a/api/v1beta1/zz_generated.openapi.go +++ b/api/v1beta1/zz_generated.openapi.go @@ -2715,7 +2715,7 @@ func schema_sigsk8sio_cluster_api_api_v1beta1_PatchSelectorMatch(ref common.Refe }, "machineDeploymentClass": { SchemaProps: spec.SchemaProps{ - Description: "MachineDeploymentClass selects templates referenced in specific MachineDeploymentClasses in .spec.workers.machineDeployments.", + Description: "MachineDeploymentClass selects templates referenced in specific MachineDeploymentClasses in .spec.workers.machineDeployments. Note: If set to an empty object it will match all MachineDeploymentClasses.", Ref: ref("sigs.k8s.io/cluster-api/api/v1beta1.PatchSelectorMatchMachineDeploymentClass"), }, }, diff --git a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml index d9f504c8a..b96e494dc 100644 --- a/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml +++ b/config/crd/bases/cluster.x-k8s.io_clusterclasses.yaml @@ -770,9 +770,10 @@ spec: referenced in .spec.infrastructure. + set to an empty object it will match all MachineDeploymentClasses.' properties: names: description: Names selects templates by class generated files are out of date, run make generate make: *** [Makefile:482: verify-gen] Error 1 + EXIT_VALUE=2

@killianmuldoon
Copy link
Contributor

The CR definitions are created using make generate and its sub-targets. make generate takes a long time to run, so you might want to just run make generate-manifests and maybe make generate-openapi

Note - it's not a good idea to run make verify-gen locally as it runs the full make generate if you're in doubt, you can just run the full job (takes about 20 minutes for me locally, but heavily depends on setup and OS.

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending squash.

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 29, 2022
@valaparthvi
Copy link
Contributor Author

The CR definitions are created using make generate and its sub-targets. make generate takes a long time to run, so you might want to just run make generate-manifests and maybe make generate-openapi

Note - it's not a good idea to run make verify-gen locally as it runs the full make generate if you're in doubt, you can just run the full job (takes about 20 minutes for me locally, but heavily depends on setup and OS.

Thank you for this! And I think it's make generate-go-openapi, and not make generate-openapi, I couldn't find that target.

@valaparthvi valaparthvi changed the title WIP: ✨ ClusterClass patches: Allow matching all MachineDeploymentClasses ✨ ClusterClass patches: Allow matching all MachineDeploymentClasses Jul 29, 2022
@k8s-ci-robot 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 Jul 29, 2022
@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jul 29, 2022

I am not sure how to fix pull-cluster-api-verify-main job. I've run a different make targets locally, but I seem to have reached nowhere. I've run make generate, which modifies a few files, none of which contains the file in question(api/v1beta1/zz_generated.openapi.go). I've also run make generate-go-openapi which seems to be doing nothing.

O/P from `make generate`
git status
On branch 5648-allow-matching-all-mdc
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	renamed:    api/v1alpha3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/api/v1alpha3/zz_generated.conversion.go
	renamed:    api/v1alpha4/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/api/v1alpha4/zz_generated.conversion.go
	renamed:    bootstrap/kubeadm/api/v1alpha3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/bootstrap/kubeadm/api/v1alpha3/zz_generated.conversion.go
	renamed:    bootstrap/kubeadm/api/v1alpha4/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/bootstrap/kubeadm/api/v1alpha4/zz_generated.conversion.go
	renamed:    bootstrap/kubeadm/types/upstreamv1beta1/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/bootstrap/kubeadm/types/upstreamv1beta1/zz_generated.conversion.go
	renamed:    bootstrap/kubeadm/types/upstreamv1beta2/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/bootstrap/kubeadm/types/upstreamv1beta2/zz_generated.conversion.go
	renamed:    bootstrap/kubeadm/types/upstreamv1beta3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/bootstrap/kubeadm/types/upstreamv1beta3/zz_generated.conversion.go
	renamed:    controlplane/kubeadm/api/v1alpha3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/controlplane/kubeadm/api/v1alpha3/zz_generated.conversion.go
	renamed:    controlplane/kubeadm/api/v1alpha4/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/controlplane/kubeadm/api/v1alpha4/zz_generated.conversion.go
	renamed:    exp/addons/api/v1alpha3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/exp/addons/api/v1alpha3/zz_generated.conversion.go
	renamed:    exp/addons/api/v1alpha4/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/exp/addons/api/v1alpha4/zz_generated.conversion.go
	renamed:    exp/api/v1alpha3/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/exp/api/v1alpha3/zz_generated.conversion.go
	renamed:    exp/api/v1alpha4/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/exp/api/v1alpha4/zz_generated.conversion.go
	renamed:    internal/runtime/test/v1alpha1/zz_generated.conversion.go -> github.com/kubernetes-sigs/cluster-api/internal/runtime/test/v1alpha1/zz_generated.conversion.go
	new file:   test/infrastructure/docker/github.com/kubernetes-sigs/cluster-api/test/infrastructure/docker/api/v1alpha3/zz_generated.conversion.go
	new file:   test/infrastructure/docker/github.com/kubernetes-sigs/cluster-api/test/infrastructure/docker/api/v1alpha4/zz_generated.conversion.go
	new file:   test/infrastructure/docker/github.com/kubernetes-sigs/cluster-api/test/infrastructure/docker/exp/api/v1alpha3/zz_generated.conversion.go
	new file:   test/infrastructure/docker/github.com/kubernetes-sigs/cluster-api/test/infrastructure/docker/exp/api/v1alpha4/zz_generated.conversion.go

Can someone help me fix this? @killianmuldoon

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share the output of make generate-go-openapi?

I was able to run make generate-go-openapi on this PR and it generated the required change to fix the issue in pull-cluster-api-verify-main job. So, I don't the issue is with any of the changes that are part of this PR.

It is weird that when you run make generate the generated files are getting renamed 🤔 . Is your project clones outside the GOPATH (although that should not cause any problems as there is logic in the make file to take care of it: https://github.com/valaparthvi/cluster-api/blob/e0bdc2455bef5e537888f10c45149f0df6cede72/Makefile#L65-L71)?

@killianmuldoon
Copy link
Contributor

@valaparthvi When I run make generate-go-openapi locally I get changes in api/v1beta1/zz_generated.openapi.go. Could you share some information about the environment in which you're running the command? e.g. cluster api most recent commit (before your changes), os type and kernel version, and the directory in which you're running the make command.

i.e. the output of:

  1. git log --oneline | head -n 5
  2. go env | grep 'GOOS\|GOARCH\|GOVERSION\|GOPATH'
  3. The output of pwd in the directory you're running the make commands.

Note the above will likely only work for Mac / Linux. If you're in a Windows environment you may need to find a different path to providing that information.

@fabriziopandini
Copy link
Member

it seems there is a test failure to be fixed as weel 😅

@sbueringer
Copy link
Member

Alright, will take a look ASAP

internal/webhooks/patch_validation.go Outdated Show resolved Hide resolved
internal/webhooks/patch_validation.go Outdated Show resolved Hide resolved
internal/webhooks/patch_validation.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2023
@sbueringer
Copy link
Member

/retest

@sbueringer
Copy link
Member

Thank you very much!!

/lgtm

Can you please squash the commits?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 524d48e521fe764834345c1fcba637182a4ff70b

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Use glob match

Update OpenAPI docs

Update internal/controllers/topology/cluster/patches/inline/json_patch_generator.go

Co-authored-by: Stefan Büringer <4662360+sbueringer@users.noreply.github.com>

Add validation for selectors

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Using prefix and suffix match instead of regex

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Fix test failures

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Add pattern matching to validateSelectors

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Update clusterclass-quick-start.yaml to use pattern match example

Signed-off-by: Parthvi Vala <pvala@redhat.com>

Attempt at fixing pull-cluster-api-e2e-informing-main

Signed-off-by: Parthvi Vala <pvala@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023
@sbueringer
Copy link
Member

Thx!

/lgtm

/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b7af886b47804ecdf380e959a72dc707f236f5cb

@fabriziopandini
Copy link
Member

kudos to @valaparthvi for bearing with us for so long! (and sorry for the limited bandwidth in reviewing this work)
that's a nice addition!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2023
@sbueringer
Copy link
Member

/hold cancel

I absolutely agree. Thank you very much @valaparthvi for the patience and tenacity!! 🎉

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2023
@k8s-ci-robot k8s-ci-robot merged commit 0119de7 into kubernetes-sigs:main Jan 16, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Jan 16, 2023
@valaparthvi
Copy link
Contributor Author

Thank you to you both as well @sbueringer @fabriziopandini and to all those who reviewed this PR. I know I was very slow at times but I appreciate everyone's patience with me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterClass patches: Allow matching all MachineDeploymentClasses
8 participants