-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Webhook for IMC #442
Conversation
a02e4aa
to
da4a788
Compare
pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go
Outdated
Show resolved
Hide resolved
c35de6a
to
7cae41b
Compare
klog.V(2).InfoS("user is not allowed to update IMC status", "user", userInfo.Username, "groups", userInfo.Groups, "kind", imcKind, "namespacedName", namespacedName) | ||
return admission.Denied(fmt.Sprintf(imcStatusUpdateNotAllowedFormat, userInfo.Username, userInfo.Groups, namespacedName)) | ||
} | ||
return ValidateUserForFleetResource(currentIMC.Kind, namespacedName, whiteListedUsers, userInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we pass the label changes from MC to imc? Label change doesn't change the spec generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now we don't pass the label changes from MC to IMC. Agreed label changes won't affect generation
@@ -64,7 +65,7 @@ func GetClusterClient(cluster *Cluster) { | |||
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Rest Mapper") | |||
|
|||
cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme}) | |||
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client") | |||
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Impersonate Kube Client") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the GetImpersonateClientConfig is not configurable so how can we test different cases (like user belong to some group/name)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it's being used to simulate a test user who is not part of the system:masters group to make requests to the api server. AFAIK we need to create new Impersonate clients if we want to test scenarios where user has a different name and belongs to different groups
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we need that. Can you add a TODO?
return hubCluster.KubeClient.Create(ctx, mc) | ||
}, PollTimeout, PollInterval).Should(gomega.Succeed(), "Failed to wait for member cluster %s to be created in %s cluster", mc.Name, hubCluster.ClusterName) | ||
|
||
imc := &fleetv1alpha1.InternalMemberCluster{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this used ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fleet/test/e2e/webhook_test.go
Line 710 in 7cae41b
imc := fleetv1alpha1.InternalMemberCluster{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused, the line you pasted declared a new variable.
6cc082b
to
01bf445
Compare
Description of your changes
Fixes #
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer