Webhook to check name + namespace exceeds 63 characters #117

yiraeChristineKim · 2023-06-16T16:44:30Z

Description: No validation if name + namespace exceeds 63 characters and Policy applied via CLI or Gitops now

After fix: Error from server (Forbidden): error when creating "policy.yaml": admission webhook "policy.open-cluster-management.io.webhook" denied the request: the combined length of namespace and policy name (namespaceName.policyName) should not exceed 63 characters

This error message shows on terminal when policy name + namespace exceeds 63 characters

Ref: https://issues.redhat.com/browse/ACM-4394

build/common/scripts/setup-webhook.sh

deploy/manager/manager.yaml

build/common/scripts/setup-webhook.sh

JustinKuli · 2023-06-20T18:36:01Z

deploy/manager/manager.yaml

+  labels: 
+    app.kubernetes.io/component: manager
+    app.kubernetes.io/created-by: webhook
+    app.kubernetes.io/instance: controller-manager
+    app.kubernetes.io/managed-by: kustomize
+    app.kubernetes.io/name: deployment
+    app.kubernetes.io/part-of: webhook
+    control-plane: controller-manager


Do we need to add any of these labels for the webhook to work? If so, let's make them a bit more specific to just our controller, if possible. Eg app.kubernetes.io/name: governance-policy-propagator

seems like woking well without label

JustinKuli · 2023-06-20T18:39:10Z

test/e2e/case17_policy_webhook_test.go

+		Expect(err).ShouldNot(HaveOccurred())
+	})
+	Describe("Test name + namespace over 63", func() {
+		It("Should the error message is presented", func() {


Can we add another test for when the error doesn't happen? Since this is the only test running right now when the webhook is enabled, I think?

I tried to add update case but namespace and name cannot be modified... I tried to add long namespace and short name but namespace itself has limitation so... would you give some idea?!

Makefile

mprahl · 2023-07-19T13:47:26Z

api/v1/policy_webhook.go

+
+var (
+	// log is for logging in this package.
+	policylog = logf.Log.WithName("policy-resource")


Suggested change

policylog = logf.Log.WithName("policy-resource")

policylog = logf.Log.WithName("policy-validating-webhook")

mprahl · 2023-07-19T13:54:37Z

deploy/webhook.yaml

@@ -0,0 +1,61 @@
+apiVersion: v1


Note that when this is added to ACM, you'll want to use OpenShift's mechanism of generating certificates instead of Cert Manager:
https://docs.openshift.com/container-platform/4.13/security/certificates/service-serving-certificate.html#add-service-certificate_service-serving-certificate
https://docs.openshift.com/container-platform/4.13/security/certificates/service-serving-certificate.html#add-service-certificate-validating-webhook_service-serving-certificate

No action is needed in this PR though.

mprahl · 2023-07-19T13:55:20Z

main.go


 	pflag.StringVar(&metricsAddr, "metrics-bind-address", ":8383", "The address the metric endpoint binds to.")
 	pflag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
 	pflag.BoolVar(&enableLeaderElection, "leader-elect", true,
 		"Enable leader election for controller manager. "+
 			"Enabling this will ensure there is only one active controller manager.")
+	pflag.BoolVar(&enableWebhooks, "enable-webhooks", true,
+		"Enable webhook for validating namespace + name less than 63 characters")


Suggested change

"Enable webhook for validating namespace + name less than 63 characters")

"Enable the policy validating webhook")

mprahl · 2023-07-19T13:58:16Z

test/e2e/case17_policy_webhook_test.go

+			Expect(utf8.RuneCountInString(testNamespace+"."+case17PolicyReplicatedName) +
+				utf8.RuneCountInString("managed1")).Should(BeNumerically(">", 63))


Suggested change

Expect(utf8.RuneCountInString(testNamespace+"."+case17PolicyReplicatedName) +

utf8.RuneCountInString("managed1")).Should(BeNumerically(">", 63))

Expect(utf8.RuneCountInString("managed1."+testNamespace+"."+case17PolicyReplicatedName)).Should(BeNumerically(">", 63))

mprahl

This is looking really good! Just a few minor comments.

mprahl · 2023-07-19T14:45:03Z

@JustinKuli did you want to take another look at this?

- The 63 character limit validation for policy name is not working while creating a Policy via CLI. - Policy creation - Policy name + namespace name can’t go longer than 63 From CLI, it possible to create a policy with a name like “policy-check-setupjob-setupjob-operator-installer” with no issue; but, while wanting to edit it, we can’t. - Getting this error: “The combined length of namespace and policy nmabe (namespaceName.policyName) should not exceed 63 characters (see attached screeshot) Ref: https://issues.redhat.com/browse/ACM-3558 Signed-off-by: Yi Rae Kim <[email protected]>

mprahl · 2023-07-19T15:27:37Z

deploy/webhook.yaml

+  ports:
+  - port: 443
+    protocol: TCP
+    targetPort: 9443


Do you want to use the name (i.e. webhook-http) here instead of the port number?

I think targetport is enough

why I change to webhook-http, http duplicated so there was kind test error

mprahl

/hold in case @JustinKuli wants to review it again

JustinKuli

LGTM

JustinKuli · 2023-07-19T17:28:39Z

/unhold

openshift-ci · 2023-07-19T17:28:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, mprahl, yiraeChristineKim

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:

~~OWNERS~~ [JustinKuli,mprahl,yiraeChristineKim]

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

openshift-ci bot requested review from dhaiducek and mprahl June 16, 2023 16:44

openshift-ci bot added dco-signoff: yes approved labels Jun 16, 2023

yiraeChristineKim force-pushed the ACM-3558 branch 2 times, most recently from be17816 to d37ce21 Compare June 16, 2023 16:51

yiraeChristineKim commented Jun 16, 2023

View reviewed changes

build/common/scripts/setup-webhook.sh Outdated Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-3558 branch 2 times, most recently from 2cfe113 to 76261c0 Compare June 16, 2023 17:01

yiraeChristineKim removed request for mprahl and dhaiducek June 16, 2023 17:16

yiraeChristineKim force-pushed the ACM-3558 branch 4 times, most recently from 1dc429a to c6bfb1d Compare June 16, 2023 20:47

openshift-merge-robot added the needs-rebase label Jun 19, 2023

yiraeChristineKim force-pushed the ACM-3558 branch from 0d814e7 to 7203213 Compare June 19, 2023 13:54

openshift-merge-robot removed the needs-rebase label Jun 19, 2023

yiraeChristineKim force-pushed the ACM-3558 branch 2 times, most recently from 9f492e2 to f8e9617 Compare June 19, 2023 14:55

yiraeChristineKim requested review from dhaiducek, mprahl, JustinKuli and gparvin June 19, 2023 14:55

yiraeChristineKim force-pushed the ACM-3558 branch from f8e9617 to 6a4f78a Compare June 19, 2023 15:07

JustinKuli requested changes Jun 20, 2023

View reviewed changes

openshift-ci bot assigned JustinKuli Jun 20, 2023

yiraeChristineKim force-pushed the ACM-3558 branch 3 times, most recently from e0378df to 6e8dd3f Compare June 26, 2023 15:20

yiraeChristineKim force-pushed the ACM-3558 branch 2 times, most recently from 6dab275 to 3d1fdd0 Compare July 19, 2023 13:07

yiraeChristineKim requested a review from mprahl July 19, 2023 13:42

mprahl reviewed Jul 19, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

yiraeChristineKim force-pushed the ACM-3558 branch from 3d1fdd0 to fa4d861 Compare July 19, 2023 13:47

mprahl reviewed Jul 19, 2023

View reviewed changes

yiraeChristineKim force-pushed the ACM-3558 branch from fa4d861 to 57df98c Compare July 19, 2023 14:05

mprahl previously approved these changes Jul 19, 2023

View reviewed changes

openshift-ci bot assigned mprahl Jul 19, 2023

openshift-ci bot added the lgtm label Jul 19, 2023

yiraeChristineKim dismissed mprahl’s stale review via 6de2be4 July 19, 2023 15:21

yiraeChristineKim force-pushed the ACM-3558 branch from 57df98c to 6de2be4 Compare July 19, 2023 15:21

openshift-ci bot removed the lgtm label Jul 19, 2023

mprahl reviewed Jul 19, 2023

View reviewed changes

yiraeChristineKim requested a review from mprahl July 19, 2023 15:56

mprahl approved these changes Jul 19, 2023

View reviewed changes

openshift-ci bot added do-not-merge/hold lgtm labels Jul 19, 2023

JustinKuli approved these changes Jul 19, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold label Jul 19, 2023

openshift-merge-robot merged commit c93f938 into open-cluster-management-io:main Jul 19, 2023

magic-mirror-bot bot mentioned this pull request Jul 19, 2023

😿 Failed to sync the upstream PRs: #117 stolostron/governance-policy-propagator#418

Closed

	policylog = logf.Log.WithName("policy-resource")
	policylog = logf.Log.WithName("policy-validating-webhook")

	"Enable webhook for validating namespace + name less than 63 characters")
	"Enable the policy validating webhook")

		Expect(utf8.RuneCountInString(testNamespace+"."+case17PolicyReplicatedName) +
		utf8.RuneCountInString("managed1")).Should(BeNumerically(">", 63))

	Expect(utf8.RuneCountInString(testNamespace+"."+case17PolicyReplicatedName) +
	utf8.RuneCountInString("managed1")).Should(BeNumerically(">", 63))
	Expect(utf8.RuneCountInString("managed1."+testNamespace+"."+case17PolicyReplicatedName)).Should(BeNumerically(">", 63))

Webhook to check name + namespace exceeds 63 characters #117

Webhook to check name + namespace exceeds 63 characters #117

Uh oh!

Conversation

yiraeChristineKim commented Jun 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl left a comment

Choose a reason for hiding this comment

Uh oh!

mprahl commented Jul 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprahl left a comment

Choose a reason for hiding this comment

Uh oh!

JustinKuli left a comment

Choose a reason for hiding this comment

Uh oh!

JustinKuli commented Jul 19, 2023

Uh oh!

openshift-ci bot commented Jul 19, 2023

Uh oh!

Uh oh!