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

Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster #327

Merged
merged 7 commits into from
Apr 3, 2024

Conversation

crobby
Copy link
Contributor

@crobby crobby commented Jan 26, 2024

Issue: rancher/rancher#22417

Problem

ClusterProxyConfigs are limited to one per downstream cluster.

Solution

This validation webhook check makes certain that you can never create more than one clusterproxyconfig per downstream cluster.

CheckList

  • Test
  • Docs

@crobby
Copy link
Contributor Author

crobby commented Jan 26, 2024

This can not be merged until rancher/rancher#43147 is merged because ClusterProxyConfigs do not exist yet. Once that is done, I'll fix-up the dependencies.

@crobby crobby force-pushed the clusterproxy branch 3 times, most recently from db4ea76 to 7dcae15 Compare January 29, 2024 20:00
Copy link
Contributor

@pmatseykanets pmatseykanets left a comment

Choose a reason for hiding this comment

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

go.mod Outdated
@@ -6,6 +6,7 @@ go 1.21
replace github.com/rancher/wrangler v1.1.1 => github.com/rancher/wrangler v1.1.1-0.20230831050635-df1bd5aae9df

replace (
github.com/rancher/rancher/pkg/apis => github.com/pmatseykanets/rancher/pkg/apis v0.0.0-20240123143352-5df802d576a6
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this temporary or just something that never got removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's temporary #327 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, temporary

go.mod Outdated
@@ -20,6 +21,7 @@ replace (
k8s.io/controller-manager => k8s.io/controller-manager v0.27.4
k8s.io/cri-api => k8s.io/cri-api v0.27.4
k8s.io/csi-translation-lib => k8s.io/csi-translation-lib v0.27.4
k8s.io/dynamic-resource-allocation => k8s.io/dynamic-resource-allocation v0.28.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: is this to help with Goland's failures to run go list? I am experiencing a similar issue, and adding this line fixes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is exactly why it's in here. I figured I'd clean it up with the rest of the deps once the rancher side merges. Not sure if it's the "right" fix or not long-term.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the issue with Goland and the need for this line?

@crobby crobby force-pushed the clusterproxy branch 5 times, most recently from f6c8e39 to bf1a429 Compare January 30, 2024 15:37
@crobby
Copy link
Contributor Author

crobby commented Jan 30, 2024

It would be great to also add an integration test https://github.com/rancher/webhook/wiki/Adding-a-New-Resource#integration-tests

Integration test added

@crobby
Copy link
Contributor Author

crobby commented Jan 30, 2024

It would be great to also add an integration test https://github.com/rancher/webhook/wiki/Adding-a-New-Resource#integration-tests

Integration test added

I think the integration test failure is expected until rancher/rancher#43147 is merged

@crobby crobby changed the title Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster [WIP] Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster Feb 2, 2024
@crobby crobby changed the title [WIP] Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster Feb 15, 2024
@crobby
Copy link
Contributor Author

crobby commented Mar 27, 2024

Note, some deps changed due to rancher/rancher being on go 1.22 now.

@crobby
Copy link
Contributor Author

crobby commented Mar 27, 2024

Waiting for #341 to merge so we can pick up the required rancher/rancher changes

@crobby
Copy link
Contributor Author

crobby commented Mar 28, 2024

Ok, the rancher/rancher PR merged, so I've updated the deps here. Should be good to go.

@crobby crobby requested a review from maxsokolovsky March 28, 2024 20:30
pmatseykanets
pmatseykanets previously approved these changes Apr 1, 2024
Copy link

@enrichman enrichman left a comment

Choose a reason for hiding this comment

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

A small comment, otherwise LGTM.

@crobby crobby merged commit 8e3d969 into rancher:release/v0.5 Apr 3, 2024
1 check passed
crobby added a commit to crobby/webhook that referenced this pull request Apr 9, 2024
…reated for a given cluster (rancher#327)

* Add validator for clusterproxyconfigs to make sure only one is ever created for a given cluster
* Add integration test for clusterProxyConfigs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants