Using admission server in e2e test instead of applying `webhook.yaml` #1377

Yisaer · 2019-12-19T11:35:06Z

What problem does this PR solve?

Using the admission webhook server in e2e instead of applying manifests/webhook.yaml.
fix some admission webhook template bugs and add update statefulset webhook configuration in admission webhook template.
Upgrading Operator from disable webhook to enable webhook wouldn't restart tidb-controller-manager until we add webhook switch in it.
moving statefulset webhook configuration into admission webhook template and add a switch for each hook.

What is changed?

go code
chart template
e2e test

Yisaer · 2019-12-19T11:35:19Z

/run-e2e-in-kind

Yisaer · 2019-12-19T12:16:46Z

/run-e2e-in-kind

Yisaer · 2019-12-19T12:44:48Z

/run-e2e-in-kind

Yisaer · 2019-12-19T13:12:01Z

tests/webhook.go

+	}
+	klog.Infof("success to execute helm operator upgrade")
+
+	//// ensure pods unchanged when upgrading operator


Currently, Upgrade Operator from disable webhook to enable webhook would only create a new deployment admission webhook. After we add webhook-enabled switch in tidb-controller-manager, it would be necessary to check whether upgrading Operator would cause tidbcluster restarted as tidb-controller-manager would restart.

why should the check be commented out?

For now, upgrading Operator from disable admission to enable admission won't restart the tidb-controller-manager so it is not necessary to check the tidbcluster.

After we add admission-webhook switch in tidb-controller-manager, the checking would be necessary.

Yisaer · 2019-12-19T13:48:51Z

/run-e2e-in-kind

Yisaer · 2019-12-20T02:43:23Z

/run-e2e-in-kind

cofyc · 2019-12-20T03:21:04Z

tests/webhook.go

+	cmd := fmt.Sprintf(`helm upgrade %s %s --set-string %s `,
+		info.ReleaseName,
+		oa.operatorChartPath(info.Tag),
+		setString)


add webhook settings in OperatorCfg and use UpgradeOperator?

is it possible to configure caBundle via --set-string? if it is, we can configure caBundle in OperatorConfig and update OperatorConfig.OperatorHelmSetString to use it if it exists, e.g.

configure webhook configurations in OperatorConfig

(if < 1.13) get caBundle from kube-system.extension-apiserver-authentication

(if < 1.13) configure caBundle in OperatorConfig

UpgradeOperator(OperatorConfig)

I set cabundle by --set-string at first and encounter a multi-line problem as the cabundle content is multi-line. I used -f to avoid this problem.

Now, I add these webhook enable switch into OperatorConfig and UpgradeOperator would call SwitchAdmissionWebhook to update the admission webhook .

you can fix the multi-line problem by escaping the value of the --set-string flag.

The current code is fragile because it does not escape the shell arguments. another way is pass command and its arguments to exec.Command (don't use sh, then no need to escape shell arguments).

updated, for now, the switch of each hook and admission server and the cabundle if managed by OperatorConfig.

cofyc · 2019-12-20T03:23:33Z

charts/tidb-operator/values.yaml

+    statefulSets: false
+    ## pods hook would check requests for creating and deleting tidbcluster's pods
+    ## if enabled it, the pods of tidbcluster would safely created or deleted by webhook instead of controller
+    pods: false


because we will always deploy webhook eventually because many features must be implemented in webhook (defaulting & validating, etc). Maybe we can have only one gate hooksEnabled?

cc @aylei

I am ok with these fine-grained control, but I think the default value is better to be true. The UX I expect is that when I enable webhook by setting admission.create=true, I get all the admissions out-of-box, and could disable some of them based on my customized requirement.

updated, the podsWebhook is true in default while the stsWebhook is false as stsWebhook is a special feature.

aylei · 2019-12-20T03:24:30Z

charts/tidb-operator/templates/admission/admission-webhook-deployment.yaml

@@ -1,4 +1,4 @@
-{{- if eq .Values.admissionWebhook.create true }}
+{{- if .Values.admissionWebhook.create }}


why we have to change the criteria

Cause {{- if eq .Values.admissionWebhook.create true }} didn't work well when I configure the setting by helm upgrade <name> <path> --set-string key=value which would casue a comparing type error, so I use {{- if .Values.admissionWebhook.create }} to avoid it.

By the way, {{- if .Values.key.create }} is more common used in our templates. It is better to unify the compare style.

This is should be our helm BP

aylei · 2019-12-20T03:28:12Z

charts/tidb-operator/values.yaml

+    statefulSets: false
+    ## pods hook would check requests for creating and deleting tidbcluster's pods
+    ## if enabled it, the pods of tidbcluster would safely created or deleted by webhook instead of controller
+    pods: false


I am ok with these fine-grained control, but I think the default value is better to be true. The UX I expect is that when I enable webhook by setting admission.create=true, I get all the admissions out-of-box, and could disable some of them based on my customized requirement.

tests/webhook.go

aylei · 2019-12-20T03:33:29Z

tests/webhook.go

+		}
+	}
+
+	klog.Info(cmd)


This log helps to print the command we would execute, so it didn't need any extra info except itself.

tests/webhook.go

aylei · 2019-12-20T03:37:14Z

tests/webhook.go

+	}
+	klog.Infof("success to execute helm operator upgrade")
+
+	//// ensure pods unchanged when upgrading operator


why should the check be commented out?

tests/webhook.go

Yisaer · 2019-12-20T04:00:44Z

/run-e2e-in-test

Yisaer · 2019-12-20T05:15:20Z

/run-e2e-test

Yisaer · 2019-12-20T06:14:33Z

/run-e2e-in-kind

cofyc

LGTM

aylei

LGTM

aylei · 2019-12-20T07:22:19Z

charts/tidb-operator/templates/admission/admission-webhook-deployment.yaml

@@ -1,4 +1,4 @@
-{{- if eq .Values.admissionWebhook.create true }}
+{{- if .Values.admissionWebhook.create }}


This is should be our helm BP

Yisaer · 2019-12-20T07:26:19Z

/run-e2e-in-kind

Song Gao added 5 commits December 19, 2019 19:20

Update admission-webhook-registration.yaml

15aa554

Update values.yaml

c84a786

add webhook

56af7cd

add into e2e

3eb21f7

use helm 2.14.1

43a7400

fix

bf35855

fix upgrade

8f1fb56

Update webhook.go

b0af8d2

Yisaer requested review from cofyc, weekface, tennix, aylei and DanielZhangQD December 19, 2019 13:05

Yisaer added area/webhook Related to webhook test/e2e e2e test labels Dec 19, 2019

Yisaer changed the title ~~[WIP] Use admission server in e2e~~ Using admission server in e2e test instead of applying webhook.yaml Dec 19, 2019

Yisaer commented Dec 19, 2019

View reviewed changes

Song Gao and others added 2 commits December 19, 2019 21:47

fix e2e

4e3fa4d

Merge branch 'master' into use_admission_server_in_e2e

ef357f6

Yisaer closed this Dec 19, 2019

Yisaer reopened this Dec 19, 2019

fix

94c3bba

cofyc reviewed Dec 20, 2019

View reviewed changes

aylei reviewed Dec 20, 2019

View reviewed changes

Song Gao added 3 commits December 20, 2019 11:37

embed webhook into operator cfg

2e3603c

Update webhook.go

611f710

Update values.yaml

c2ec2ea

Yisaer requested review from cofyc and aylei December 20, 2019 04:03

Song Gao added 3 commits December 20, 2019 14:06

move cabundle into operatorConfig

9266413

Update tidbcluster.go

4aeabe3

Update tidbcluster.go

943ff43

This comment has been minimized.

Sign in to view

Update actions.go

c8647c7

Yisaer added the status/PTAL PR needs to be reviewed label Dec 20, 2019

cofyc approved these changes Dec 20, 2019

View reviewed changes

aylei approved these changes Dec 20, 2019

View reviewed changes

Merge branch 'master' into use_admission_server_in_e2e

50ac82d

Yisaer merged commit f046d12 into pingcap:master Dec 20, 2019

This was referenced Dec 20, 2019

Using a more elegant way for patch-e2e.sh #1358

Closed

support pausing rolling-update of TidbCluster #1381

Closed

yahonda pushed a commit that referenced this pull request Dec 27, 2021

fix link of v1.2.3 release note (#1377)

f24205e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using admission server in e2e test instead of applying `webhook.yaml` #1377

Using admission server in e2e test instead of applying `webhook.yaml` #1377

Yisaer commented Dec 19, 2019 •

edited

Loading

Yisaer commented Dec 19, 2019

Yisaer commented Dec 19, 2019

Yisaer commented Dec 19, 2019

Yisaer Dec 19, 2019

aylei Dec 20, 2019

Yisaer Dec 20, 2019

Yisaer commented Dec 19, 2019

Yisaer commented Dec 20, 2019

cofyc Dec 20, 2019

cofyc Dec 20, 2019 •

edited

Loading

Yisaer Dec 20, 2019

Yisaer Dec 20, 2019 •

edited

Loading

cofyc Dec 20, 2019

cofyc Dec 20, 2019

Yisaer Dec 20, 2019

cofyc Dec 20, 2019

aylei Dec 20, 2019

Yisaer Dec 20, 2019

aylei Dec 20, 2019

Yisaer Dec 20, 2019 •

edited

Loading

aylei Dec 20, 2019

aylei Dec 20, 2019

aylei Dec 20, 2019

Yisaer Dec 20, 2019 •

edited

Loading

aylei Dec 20, 2019

Yisaer commented Dec 20, 2019

Yisaer commented Dec 20, 2019

This comment has been minimized.

Yisaer commented Dec 20, 2019

cofyc left a comment

aylei left a comment

aylei Dec 20, 2019

Yisaer commented Dec 20, 2019

		@@ -1,4 +1,4 @@
		{{- if eq .Values.admissionWebhook.create true }}
		{{- if .Values.admissionWebhook.create }}

Using admission server in e2e test instead of applying webhook.yaml #1377

Using admission server in e2e test instead of applying webhook.yaml #1377

Conversation

Yisaer commented Dec 19, 2019 • edited Loading

What problem does this PR solve?

What is changed?

Yisaer commented Dec 19, 2019

Yisaer commented Dec 19, 2019

Yisaer commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Dec 19, 2019

Yisaer commented Dec 20, 2019

Choose a reason for hiding this comment

cofyc Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Dec 20, 2019

Yisaer commented Dec 20, 2019

This comment has been minimized.

Yisaer commented Dec 20, 2019

cofyc left a comment

Choose a reason for hiding this comment

aylei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Dec 20, 2019

Using admission server in e2e test instead of applying `webhook.yaml` #1377

Using admission server in e2e test instead of applying `webhook.yaml` #1377

Yisaer commented Dec 19, 2019 •

edited

Loading

cofyc Dec 20, 2019 •

edited

Loading

Yisaer Dec 20, 2019 •

edited

Loading

Yisaer Dec 20, 2019 •

edited

Loading

Yisaer Dec 20, 2019 •

edited

Loading