feat: Add webhook for ClusterResourcePlacement #249
Conversation
Fei-Guo
changed the title
ryanzhang-oss
force-pushed
the
fguo-dev1
branch
from
2fdc781
to
5cf6968
Compare
| @@ -3,7 +3,7 @@ Copyright (c) Microsoft Corporation. | |||
| Licensed under the MIT license. | |||
| */ | |||
|
|
|||
| package utils | |||
| package informermanager | |||
There was a problem hiding this comment.
this package name is the same as the main structure. We normally want to avoid the informermanager. informermanager
| ) | ||
|
|
||
| // Reconciler reconciles a MemberCluster object | ||
| type Reconciler struct { | ||
| // the informer contains the cache for all the resources we need | ||
| InformerManager utils.InformerManager | ||
| InformerManager informermanager.InformerManager |
There was a problem hiding this comment.
this does not read right. Maybe we can leave this refactor to another PR?
There was a problem hiding this comment.
or change this to informers.Manager
| allErr := make([]error, 0) | ||
|
|
||
| for _, selector := range clusterResourcePlacement.Spec.ResourceSelectors { | ||
| //TODO: make sure the selector's gvk is valid |
There was a problem hiding this comment.
looks now like you can do this because there is restMapper
| continue | ||
| } | ||
|
|
||
| if !ResourceInformer.IsClusterScopedResources(restMapping.Resource) { |
There was a problem hiding this comment.
The map inside the ResourceInformer is only initialized after the resource detector is initialized. It also has a delay when a new CRD is created so this might fail in between. I am not sure if there is a way to make sure the map is stable before this is called
There was a problem hiding this comment.
The nil check can avoid the initialization problem by suggesting retry. The delay of picking up crd can be mitigated by adding watcher to crds later. I can suggest retrying if user is sure that the gvk is a cluster scope resource in the error msg.
|
/ok-to-test |
Description of your changes
This change adds basic webhook functionality to validate the ClusterResourcePlacement object.
Besides the general selector validity check, this change check if the resource listed in the selector is a cluster scoped resource.
The implementation relies on the dynamic informer manager in the change detector controller.
I also refactor the utils package to avoid circular import dependencies. The pkg/utils/validator directory can be
removed by the follow up change (not in this change).
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Deploy the webhook in an e2e environment and performance a few manual tests: