Fix some webhook error (#1963)

* fix webhook error
pingcap · Mar 18, 2020 · 21e650c · 21e650c
1 parent 936542b
commit 21e650c
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 8 deletions.
diff --git a/charts/tidb-operator/templates/admission/admission-webhook-deployment.yaml b/charts/tidb-operator/templates/admission/admission-webhook-deployment.yaml
@@ -53,6 +53,11 @@ spec:
               scheme: HTTPS
             initialDelaySeconds: 5
             timeoutSeconds: 5
+          env:
+          - name: NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace
           {{- if eq .Values.admissionWebhook.apiservice.insecureSkipTLSVerify false  }}
           volumeMounts:
             - mountPath: /var/serving-cert

diff --git a/cmd/admission-webhook/main.go b/cmd/admission-webhook/main.go
@@ -15,13 +15,15 @@ package main
 
 import (
 	"flag"
+	"fmt"
 	"os"
 	"time"
 
 	"github.com/openshift/generic-admission-server/pkg/cmd"
 	"github.com/pingcap/tidb-operator/pkg/features"
 	"github.com/pingcap/tidb-operator/pkg/version"
 	"github.com/pingcap/tidb-operator/pkg/webhook"
+	"github.com/pingcap/tidb-operator/pkg/webhook/pod"
 	"k8s.io/component-base/logs"
 	"k8s.io/klog"
 )
@@ -59,5 +61,11 @@ func main() {
 		ExtraServiceAccounts:     extraServiceAccounts,
 		EvictRegionLeaderTimeout: evictRegionLeaderTimeout,
 	}
+	ns := os.Getenv("NAMESPACE")
+	if len(ns) < 1 {
+		klog.Fatal("ENV NAMESPACE should be set.")
+	}
+	pod.AstsControllerServiceAccounts = fmt.Sprintf("system:serviceaccount:%s:advanced-statefulset-controller", ns)
+
 	cmd.RunAdmissionServer(ah)
 }
diff --git a/pkg/webhook/pod/pods.go b/pkg/webhook/pod/pods.go
@@ -47,8 +47,11 @@ type PodAdmissionControl struct {
 }
 
 const (
-	stsControllerServiceAccounts  = "system:serviceaccount:kube-system:statefulset-controller"
-	astsControllerServiceAccounts = "system:serviceaccount:pingcap:advanced-statefulset-controller"
+	stsControllerServiceAccounts = "system:serviceaccount:kube-system:statefulset-controller"
+)
+
+var (
+	AstsControllerServiceAccounts string
 )
 
 func NewPodAdmissionControl(kubeCli kubernetes.Interface, operatorCli versioned.Interface, PdControl pdapi.PDControlInterface, extraServiceAccounts []string, evictRegionLeaderTimeout time.Duration) *PodAdmissionControl {
@@ -58,7 +61,7 @@ func NewPodAdmissionControl(kubeCli kubernetes.Interface, operatorCli versioned.
 		serviceAccounts.Insert(sa)
 	}
 	if features.DefaultFeatureGate.Enabled(features.AdvancedStatefulSet) {
-		serviceAccounts.Insert(astsControllerServiceAccounts)
+		serviceAccounts.Insert(AstsControllerServiceAccounts)
 	}
 	EvictLeaderTimeout = evictRegionLeaderTimeout
 	return &PodAdmissionControl{

diff --git a/pkg/webhook/pod/tikv_deleter.go b/pkg/webhook/pod/tikv_deleter.go
@@ -100,9 +100,9 @@ func (pc *PodAdmissionControl) admitDeleteTiKVPods(payload *admitPayload) *admis
 	case v1alpha1.TiKVStateTombstone:
 		return pc.admitDeleteUselessTiKVPod(payload)
 	case v1alpha1.TiKVStateOffline:
-		return pc.admitDeleteOfflineTiKVPod()
+		return pc.rejectDeleteTiKVPod()
 	case v1alpha1.TiKVStateDown:
-		return pc.admitDeleteUselessTiKVPod(payload)
+		return pc.admitDeleteDownTikvPod(payload)
 	case v1alpha1.TiKVStateUp:
 		return pc.admitDeleteUpTiKVPod(payload, storeInfo, storesInfo)
 	default:
@@ -147,7 +147,7 @@ func (pc *PodAdmissionControl) admitDeleteUselessTiKVPod(payload *admitPayload)
 	return util.ARSuccess()
 }
 
-func (pc *PodAdmissionControl) admitDeleteOfflineTiKVPod() *admission.AdmissionResponse {
+func (pc *PodAdmissionControl) rejectDeleteTiKVPod() *admission.AdmissionResponse {
 	return &admission.AdmissionResponse{
 		Allowed: false,
 	}
@@ -217,3 +217,19 @@ func (pc *PodAdmissionControl) admitDeleteUpTiKVPodDuringUpgrading(payload *admi
 
 	return util.ARSuccess()
 }
+
+// When the target tikv's store is DOWN, we would not pass the deleting request during scale-in, otherwise it would cause
+// the duplicated id problem for the newly tikv pod.
+// Users should offline the target tikv into tombstone first, then scale-in it.
+// In other cases, we would admit to delete the down tikv pod like upgrading.
+func (pc *PodAdmissionControl) admitDeleteDownTikvPod(payload *admitPayload) *admission.AdmissionResponse {
+
+	isInOrdinal, err := operatorUtils.IsPodOrdinalNotExceedReplicas(payload.pod, payload.ownerStatefulSet)
+	if err != nil {
+		return util.ARFail(err)
+	}
+	if !isInOrdinal {
+		return pc.rejectDeleteTiKVPod()
+	}
+	return util.ARSuccess()
+}
diff --git a/pkg/webhook/pod/tikv_deleter_test.go b/pkg/webhook/pod/tikv_deleter_test.go
@@ -212,7 +212,7 @@ func TestTiKVDeleterDelete(t *testing.T) {
 			UpdatePVCErr:   false,
 			PVCNotFound:    false,
 			expectFn: func(g *GomegaWithT, response *admission.AdmissionResponse) {
-				g.Expect(response.Allowed).Should(Equal(true))
+				g.Expect(response.Allowed).Should(Equal(false))
 			},
 		},
 		{
@@ -236,7 +236,7 @@ func TestTiKVDeleterDelete(t *testing.T) {
 			UpdatePVCErr:   true,
 			PVCNotFound:    true,
 			expectFn: func(g *GomegaWithT, response *admission.AdmissionResponse) {
-				g.Expect(response.Allowed).Should(Equal(true))
+				g.Expect(response.Allowed).Should(Equal(false))
 			},
 		},
 		{