change cert generate method and add pd and kv webhook #406

shuijing198799 · 2019-04-18T07:56:14Z

auto generator ca key crt file
add kv and pd webhook

shuijing198799 · 2019-04-18T07:56:38Z

@weekface @xiaojingchen PTLA

xiaojingchen

LGTM

xiaojingchen · 2019-04-18T09:52:56Z

@onlymellb PTAL the cert generate method

weekface · 2019-04-18T11:22:55Z

tests/pkg/apimachinery/certs.go

+func SetupServerCert(namespaceName, serviceName string) *CertContext {
+	certDir, err := ioutil.TempDir("", "test-e2e-server-cert")
+	if err != nil {
+		glog.Errorf("Failed to create a temp dir for cert generation %v", err)


You should return an error.

weekface · 2019-04-18T11:23:24Z

tests/pkg/apimachinery/certs.go

+	}
+	signingCert, err := cert.NewSelfSignedCACert(cert.Config{CommonName: "e2e-server-cert-ca"}, signingKey)
+	if err != nil {
+		glog.Errorf("Failed to create CA cert for apiserver %v", err)


weekface · 2019-04-18T11:23:49Z

tests/pkg/apimachinery/certs.go

+	}
+	caCertFile, err := ioutil.TempFile(certDir, "ca.crt")
+	if err != nil {
+		glog.Errorf("Failed to create a temp file for ca cert generation %v", err)


and all these should return a error.

weekface · 2019-04-18T11:26:36Z

tests/pkg/webhook/pods.go

+			glog.Infof("savely delete pod namespace %s name %s content %s", nameSpace, name, string(content))
+		}
+
+	} else if pod.Labels["app.kubernetes.io/component"] == "pd" {


Suggested change

} else if pod.Labels["app.kubernetes.io/component"] == "pd" {

} else if pod.Labels[label.ComponentLabelKey] == "pd" {

weekface · 2019-04-18T11:26:58Z

tests/pkg/webhook/pods.go

 		}

-		if info.IsOwner {
-			glog.Errorf("tidb is ddl owner, can't be deleted namespace %s name %s", nameSpace, name)
+	} else if pod.Labels["app.kubernetes.io/component"] == "tikv" {


weekface · 2019-04-18T11:27:47Z

tests/pkg/webhook/pods.go

+		podIP := tc.Status.PD.Leader.ClientURL
+		for _, store := range tc.Status.TiKV.Stores {
+			if store.PodName == name {
+				storeID = store.ID


Suggested change

storeID = store.ID

storeID = store.ID

break

weekface · 2019-04-19T02:33:25Z

tests/cmd/e2e/main.go

@@ -40,7 +40,10 @@ func main() {

 	cli, kubeCli := client.NewCliOrDie()

-	context := apimachinery.SetupServerCert(os.Getenv("NAMESPACE"), "webhook-service")
+	context, err := apimachinery.SetupServerCert(os.Getenv("NAMESPACE"), "webhook-service")


merge this function into StartValidatingAdmissionWebhookServerOrDie.

weekface · 2019-04-19T02:34:20Z

tests/pkg/apimachinery/certs.go

 	}
 	caCertFile, err := ioutil.TempFile(certDir, "ca.crt")
 	if err != nil {
 		glog.Errorf("Failed to create a temp file for ca cert generation %v", err)
+		return nil, err
 	}
 	if err := ioutil.WriteFile(caCertFile.Name(), cert.EncodeCertPEM(signingCert), 0644); err != nil {
 		glog.Errorf("Failed to write CA cert %v", err)


return error here.

this should be fixed also

shuijing198799 · 2019-04-19T05:51:24Z

/run-e2e-tests

xiaojingchen · 2019-04-22T06:36:42Z

tests/pkg/webhook/pods.go

+
+	}
+
+	if kvLeaders, err = getAllKVLeaders(tc, httpClient); err != nil {


the getAllKVLeaders should be called outside admitPods, best in the init phase of the webhook server.

weekface · 2019-04-22T03:33:33Z

tests/pkg/webhook/pods.go

+	ret = make(map[string]int)
+	podIP := tc.Status.PD.Leader.ClientURL
+	for _, store := range tc.Status.TiKV.Stores {
+		url := fmt.Sprintf("%s/pd/api/v1/store/%s", podIP, store.ID)


why not use GetStore:

tidb-operator/pkg/controller/pd_control.go

Line 292 in ebd499e

func (pc *pdClient) GetStore(storeID uint64) (*StoreInfo, error) {

get store have no pod name。

what does it mean?

use GetStore is better,it only needs storeID and less codes

… to record the number

xiaojingchen

LGTM

weekface · 2019-04-25T10:09:38Z

pkg/controller/tidb_control.go

@@ -89,6 +96,37 @@ func (tdc *defaultTiDBControl) ResignDDLOwner(tc *v1alpha1.TidbCluster, ordinal
 	return false, err2
 }

+func (tdc *defaultTiDBControl) GetInfo(tc *v1alpha1.TidbCluster, ordinal int32) (*dbInfo, error) {


Open an issue to remember to add unit tests to cover this method.

weekface · 2019-04-25T10:15:16Z

tests/pkg/webhook/pods.go

 	if err != nil {
-		reviewResponse.Allowed = false


Why delete this line? reviewResponse.Allowed default is true.

weekface · 2019-04-25T10:15:29Z

tests/pkg/webhook/pods.go

 	if err != nil {
-		reviewResponse.Allowed = false


weekface · 2019-04-25T10:16:04Z

tests/pkg/webhook/pods.go

 		if err != nil {
-			glog.Errorf("fail to read response %v", err)
+			glog.Errorf("fail to convert string to int while deleting TiDB err %v", err)
 			return &reviewResponse


weekface · 2019-04-25T10:16:11Z

tests/pkg/webhook/pods.go

 		if err != nil {
-			glog.Errorf("unmarshal failed,namespace %s name %s error:%v", nameSpace, name, err)
+			glog.Errorf("fail to get tidb info error:%v", err)


weekface · 2019-04-25T10:17:08Z

tests/pkg/webhook/pods.go

 		if err != nil {
-			glog.Errorf("fail to read response %v", err)
-			return &reviewResponse


weekface · 2019-04-25T10:17:14Z

tests/pkg/webhook/pods.go

-		err = json.Unmarshal(content, leader)
-		if err != nil {
-			glog.Errorf("unmarshal failed,namespace %s name %s error:%v", nameSpace, name, err)
+			glog.Errorf("fail to get pd leader %v", err)


weekface · 2019-04-25T10:17:31Z

tests/pkg/webhook/pods.go

+				storeID, err = strconv.ParseUint(store.ID, 10, 64)
+				if err != nil {
+					glog.Errorf("fail to convert string to int while deleting PD err %v", err)
+					return &reviewResponse


weekface · 2019-04-25T10:18:53Z

tests/pkg/webhook/pods.go

-			glog.Errorf("fail to read response %v", err)
+		// Fail to get store in stores
+		if storeID == 0 {
+			glog.Errorf("fail to find store in TIKV.Stores podname %s", name)


weekface · 2019-04-25T10:18:59Z

tests/pkg/webhook/pods.go

 		if err != nil {
-			glog.Errorf("unmarshal failed,namespace %s name %s error:%v", nameSpace, name, err)
+			glog.Errorf("fail to read storeID %d response %v", storeID, err)


shuijing198799 · 2019-04-25T12:51:31Z

/run-e2e-tests

shuijing198799 · 2019-04-26T06:33:41Z

/run-e2e-tests

shuijing198799 · 2019-04-26T06:49:06Z

/run-e2e-tests

shuijing198799 · 2019-04-26T09:22:54Z

/run-e2e-tests

shuijing198799 · 2019-04-28T02:38:48Z

/run-e2e-tests

shuijing198799 · 2019-04-28T06:42:22Z

/run-e2e-tests

change cert generate method and add pd and kv webhook

333f221

xiaojingchen reviewed Apr 18, 2019

View reviewed changes

weekface reviewed Apr 18, 2019

View reviewed changes

comment fix

9c3c6d3

weekface reviewed Apr 19, 2019

View reviewed changes

merge setupcert into validatingwebhook function

21e1fe9

shuijing198799 requested review from xiaojingchen and weekface April 19, 2019 06:35

shuijing198799 added 2 commits April 19, 2019 16:15

fix comment

7cdc33a

save tikv leader count and compare

d6f9478

xiaojingchen reviewed Apr 22, 2019

View reviewed changes

weekface reviewed Apr 22, 2019

View reviewed changes

weekface added the test/stability stability tests label Apr 24, 2019

shuijing198799 added 2 commits April 24, 2019 20:03

use pdclient and tidbcontroller instead of raw httpclient and use map…

38d645b

… to record the number

conflit resolve

7349ed1

xiaojingchen reviewed Apr 24, 2019

View reviewed changes

weekface reviewed Apr 25, 2019

View reviewed changes

all illegel condition return false

48aab80

shuijing198799 mentioned this pull request Apr 25, 2019

add unit test to tidb_controller GetInfo Interface #406 #429

Closed

if tidb is deleting, allow the delete operation in webhook

71162db

forget to format

41436ab

Merge branch 'master' into webhook-teseet

055761e

weekface approved these changes Apr 29, 2019

View reviewed changes

weekface merged commit 0bfd6dc into pingcap:master Apr 29, 2019

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

en: remove extra lines (#406)

09ddbb3

	} else if pod.Labels["app.kubernetes.io/component"] == "pd" {
	} else if pod.Labels[label.ComponentLabelKey] == "pd" {


		}

		if kvLeaders, err = getAllKVLeaders(tc, httpClient); err != nil {

change cert generate method and add pd and kv webhook #406

change cert generate method and add pd and kv webhook #406

Conversation

shuijing198799 commented Apr 18, 2019

shuijing198799 commented Apr 18, 2019

xiaojingchen left a comment

Choose a reason for hiding this comment

xiaojingchen commented Apr 18, 2019

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

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

shuijing198799 commented Apr 19, 2019

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

xiaojingchen left a 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

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

shuijing198799 commented Apr 25, 2019

shuijing198799 commented Apr 26, 2019

shuijing198799 commented Apr 26, 2019

shuijing198799 commented Apr 26, 2019

shuijing198799 commented Apr 28, 2019

shuijing198799 commented Apr 28, 2019