add validation for cluster api and remove validating webhook

[carlory]

What type of PR is this?

/kind feature

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

add validation for cluster API

[karmada-bot]

Welcome @carlory! It looks like this is your first PR to karmada-io/karmada 🎉

[XiShanYongYe-Chang]

Hi @carlory , thanks for your contribution. :)

[carlory]

/cc @kevin-wangzefeng @RainbowMango

[RainbowMango]

Thanks @carlory I'll review it ASAP.

[carlory]

/cc @kevin-wangzefeng @RainbowMango

[RainbowMango]

Got it @carlory.

[carlory]

add validation for impersonatorSecretRef
/cc @RainbowMango @XiShanYongYe-Chang

[carlory]

/cc @RainbowMango @XiShanYongYe-Chang

remove validating webhook

[XiShanYongYe-Chang]

Hi @carlory, I'll review it now.

[XiShanYongYe-Chang]

Thanks, @carlory. It looks good to me.

[RainbowMango]

Not an objection. Just a question.

The validation functionality in webhook still works as expected, right?
If it is the case, I'd suggest doing this after the coming new release.

[RainbowMango]

We should remove the configuration from karmada init:

karmada/pkg/karmadactl/cmdinit/karmada/webhook_configuration.go

Lines 83 to 98 in 1448754

	return fmt.Sprintf(`apiVersion: admissionregistration.k8s.io/v1
	kind: ValidatingWebhookConfiguration
	metadata:
	name: validating-config
	labels:
	app: validating-config
	webhooks:
	- name: cluster.karmada.io
	rules:
	- operations: ["CREATE", "UPDATE"]
	apiGroups: ["cluster.karmada.io"]
	apiVersions: ["*"]
	resources: ["clusters"]
	scope: "Cluster"
	clientConfig:
	url: https://karmada-webhook.%s.svc:443/validate-cluster

cc @lonelyCZ @prodanlabs

[lonelyCZ]

We should remove the configuration from karmada init

Ok, I will do it.

[lonelyCZ]

This can be removed in this pr. Thanks @carlory

[carlory]

/cc @RainbowMango @XiShanYongYe-Chang

[XiShanYongYe-Chang]

thanks!
/lgtm

[RainbowMango]

/assign

[RainbowMango]

Generally looks good to me.

1. I suggest just putting validation.go to pkg/apis/cluster directory as it is only for internel types.
2. One main effect for moving the validation from webhook to AA is if there is a bug, it'll block people registering cluster(They can't disable the validation anymore). So, I suggest more unit test for the validations.

[RainbowMango]

Suggested change

	if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {
	if uniqueTaints[currTaint.Effect].Has(currTaint.Key) {

This will be enough, right?
Don't know if len(sets.String) will work.

[carlory]

@RainbowMango len(uniqueTaints[currTaint.Effect]) > 0 is necessary to avoid panic

[RainbowMango]

Really? Do you mean if we invoke Has() method on an empty sets.String{} will panic?

[carlory]

1. I suggest just putting validation.go to pkg/apis/cluster directory as it is only for internel types.

@RainbowMango

In kubernetes/kubernetes and openshift/origin repo, the validation files are placed in a separate directory.

I recommend keeping the same code style as them, what do you think?

[RainbowMango]

I recommend keeping the same code style as them, what do you think?

OK for me.

[carlory]

Really? Do you mean if we invoke Has() method on an empty sets.String{} will panic?

It may not have been initialized when we invoke Has() method.

// ValidateClusterTaints tests if given taints have valid data.
func ValidateClusterTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorList {
	allErrors := field.ErrorList{}

	uniqueTaints := map[corev1.TaintEffect]sets.String{}
	for i, currTaint := range taints {
             ...
		// validate if taint is unique by <key, effect>
		if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {
                      ....
		}

		// add taint to existingTaints for uniqueness check
		if len(uniqueTaints[currTaint.Effect]) == 0 {
			uniqueTaints[currTaint.Effect] = sets.String{}
		}
		uniqueTaints[currTaint.Effect].Insert(currTaint.Key)
	}
	return allErrors
}

[carlory]

ValidateClusterTaints is lifted from kubernetes validateNodeTaints

[RainbowMango]

For code lifted from k/k, I suggest moving it to a separated file(like kubevalidation.go) and keeping the file header.
If there is a bug we should help fix it in k/k then port it back to Karmada. We can ignore code redundant issues for porting code.

[RainbowMango]

But, I'm still interest if the len(uniqueTaints[currTaint.Effect]) > 0 could be removed, though.
Because, the sets has been initilized by uniqueTaints := map[corev1.TaintEffect]sets.String{}.

[RainbowMango]

Can we move utilvalidation.ValidateClusterName to here?

[RainbowMango]

@carlory Please cc me again if it is ready for review. Thanks for doing this, appreciate it!!

[carlory]

/cc @RainbowMango @XiShanYongYe-Chang

It's ready for review.

[XiShanYongYe-Chang]

LGTM, can you squash some commits.

[carlory]

@XiShanYongYe-Chang done.

[RainbowMango]

/approve
Leave LGTM to @XiShanYongYe-Chang

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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 [RainbowMango]

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

[XiShanYongYe-Chang]

/lgtm