From dfa74989be50ae295f6a4584c3cb3513e31874f0 Mon Sep 17 00:00:00 2001 From: weekface Date: Mon, 18 May 2020 21:36:24 +0800 Subject: [PATCH] remove lock on tidbControl --- pkg/controller/tidb_control.go | 154 ++++++------- pkg/controller/tidb_control_test.go | 339 ++++++++++++++++++++++++++++ 2 files changed, 414 insertions(+), 79 deletions(-) create mode 100644 pkg/controller/tidb_control_test.go diff --git a/pkg/controller/tidb_control.go b/pkg/controller/tidb_control.go index 0bbc611465..8cd974c306 100644 --- a/pkg/controller/tidb_control.go +++ b/pkg/controller/tidb_control.go @@ -18,17 +18,17 @@ import ( "crypto/x509" "encoding/json" "fmt" + "io/ioutil" + "net/http" + "time" + "github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1" "github.com/pingcap/tidb-operator/pkg/httputil" "github.com/pingcap/tidb-operator/pkg/util" "github.com/pingcap/tidb/config" - "io/ioutil" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" - "net/http" - "sync" - "time" ) const ( @@ -54,89 +54,41 @@ type TiDBControlInterface interface { // defaultTiDBControl is default implementation of TiDBControlInterface. type defaultTiDBControl struct { - // Now, all controllers use this single tidbConrol instance, - // we should add a mutex to avoid two controllers update the httpClient.Transport at same time. - mutex sync.Mutex - // TODO use httpClients cache instead, like pkg/pdapi/pdapi.go does. - httpClient *http.Client - kubeCli kubernetes.Interface + kubeCli kubernetes.Interface + // for unit test only + testURL string } // NewDefaultTiDBControl returns a defaultTiDBControl instance -func NewDefaultTiDBControl(kubeCli kubernetes.Interface) TiDBControlInterface { - return &defaultTiDBControl{httpClient: &http.Client{Timeout: timeout}, kubeCli: kubeCli} -} - -func (tdc *defaultTiDBControl) useTLSHTTPClient(tc *v1alpha1.TidbCluster) error { - if !tc.IsTLSClusterEnabled() { - return nil - } - - tcName := tc.Name - ns := tc.Namespace - secretName := util.ClusterClientTLSSecretName(tcName) - secret, err := tdc.kubeCli.CoreV1().Secrets(ns).Get(secretName, metav1.GetOptions{}) - if err != nil { - return err - } - - clientCert, certExists := secret.Data[v1.TLSCertKey] - clientKey, keyExists := secret.Data[v1.TLSPrivateKeyKey] - if !certExists || !keyExists { - return fmt.Errorf("cert or key does not exist in secret %s/%s", ns, secretName) - } - - tlsCert, err := tls.X509KeyPair(clientCert, clientKey) - if err != nil { - return fmt.Errorf("unable to load certificates from secret %s/%s: %v", ns, secretName, err) - } - - rootCAs := x509.NewCertPool() - rootCAs.AppendCertsFromPEM(secret.Data[v1.ServiceAccountRootCAKey]) - config := &tls.Config{ - RootCAs: rootCAs, - Certificates: []tls.Certificate{tlsCert}, - } - tdc.httpClient.Transport = &http.Transport{TLSClientConfig: config} - return nil +func NewDefaultTiDBControl(kubeCli kubernetes.Interface) *defaultTiDBControl { + return &defaultTiDBControl{kubeCli: kubeCli} } func (tdc *defaultTiDBControl) GetHealth(tc *v1alpha1.TidbCluster, ordinal int32) (bool, error) { - tdc.mutex.Lock() - defer tdc.mutex.Unlock() - - tcName := tc.GetName() - ns := tc.GetNamespace() - scheme := tc.Scheme() - - if err := tdc.useTLSHTTPClient(tc); err != nil { + httpClient, err := tdc.getHTTPClient(tc) + if err != nil { return false, err } - hostName := fmt.Sprintf("%s-%d", TiDBMemberName(tcName), ordinal) - url := fmt.Sprintf("%s://%s.%s.%s:10080/status", scheme, hostName, TiDBPeerMemberName(tcName), ns) - _, err := tdc.getBodyOK(url) + baseURL := tdc.getBaseURL(tc, ordinal) + url := fmt.Sprintf("%s/status", baseURL) + _, err = getBodyOK(httpClient, url) return err == nil, nil } func (tdc *defaultTiDBControl) GetInfo(tc *v1alpha1.TidbCluster, ordinal int32) (*DBInfo, error) { - tdc.mutex.Lock() - defer tdc.mutex.Unlock() - - tcName := tc.GetName() - ns := tc.GetNamespace() - scheme := tc.Scheme() - if err := tdc.useTLSHTTPClient(tc); err != nil { + httpClient, err := tdc.getHTTPClient(tc) + if err != nil { return nil, err } - hostName := fmt.Sprintf("%s-%d", TiDBMemberName(tcName), ordinal) - url := fmt.Sprintf("%s://%s.%s.%s:10080/info", scheme, hostName, TiDBPeerMemberName(tcName), ns) + baseURL := tdc.getBaseURL(tc, ordinal) + url := fmt.Sprintf("%s/info", baseURL) req, err := http.NewRequest("POST", url, nil) if err != nil { return nil, err } - res, err := tdc.httpClient.Do(req) + res, err := httpClient.Do(req) if err != nil { return nil, err } @@ -158,23 +110,18 @@ func (tdc *defaultTiDBControl) GetInfo(tc *v1alpha1.TidbCluster, ordinal int32) } func (tdc *defaultTiDBControl) GetSettings(tc *v1alpha1.TidbCluster, ordinal int32) (*config.Config, error) { - tdc.mutex.Lock() - defer tdc.mutex.Unlock() - - tcName := tc.GetName() - ns := tc.GetNamespace() - scheme := tc.Scheme() - if err := tdc.useTLSHTTPClient(tc); err != nil { + httpClient, err := tdc.getHTTPClient(tc) + if err != nil { return nil, err } - hostName := fmt.Sprintf("%s-%d", TiDBMemberName(tcName), ordinal) - url := fmt.Sprintf("%s://%s.%s.%s:10080/settings", scheme, hostName, TiDBPeerMemberName(tcName), ns) + baseURL := tdc.getBaseURL(tc, ordinal) + url := fmt.Sprintf("%s/settings", baseURL) req, err := http.NewRequest("GET", url, nil) if err != nil { return nil, err } - res, err := tdc.httpClient.Do(req) + res, err := httpClient.Do(req) if err != nil { return nil, err } @@ -195,8 +142,8 @@ func (tdc *defaultTiDBControl) GetSettings(tc *v1alpha1.TidbCluster, ordinal int return &info, nil } -func (tdc *defaultTiDBControl) getBodyOK(apiURL string) ([]byte, error) { - res, err := tdc.httpClient.Get(apiURL) +func getBodyOK(httpClient *http.Client, apiURL string) ([]byte, error) { + res, err := httpClient.Get(apiURL) if err != nil { return nil, err } @@ -213,6 +160,55 @@ func (tdc *defaultTiDBControl) getBodyOK(apiURL string) ([]byte, error) { return body, err } +func (tdc *defaultTiDBControl) getHTTPClient(tc *v1alpha1.TidbCluster) (*http.Client, error) { + httpClient := &http.Client{Timeout: timeout} + if !tc.IsTLSClusterEnabled() { + return httpClient, nil + } + + tcName := tc.Name + ns := tc.Namespace + secretName := util.ClusterClientTLSSecretName(tcName) + secret, err := tdc.kubeCli.CoreV1().Secrets(ns).Get(secretName, metav1.GetOptions{}) + if err != nil { + return nil, err + } + + clientCert, certExists := secret.Data[v1.TLSCertKey] + clientKey, keyExists := secret.Data[v1.TLSPrivateKeyKey] + if !certExists || !keyExists { + return nil, fmt.Errorf("cert or key does not exist in secret %s/%s", ns, secretName) + } + + tlsCert, err := tls.X509KeyPair(clientCert, clientKey) + if err != nil { + return nil, fmt.Errorf("unable to load certificates from secret %s/%s: %v", ns, secretName, err) + } + + rootCAs := x509.NewCertPool() + rootCAs.AppendCertsFromPEM(secret.Data[v1.ServiceAccountRootCAKey]) + config := &tls.Config{ + RootCAs: rootCAs, + Certificates: []tls.Certificate{tlsCert}, + } + httpClient.Transport = &http.Transport{TLSClientConfig: config} + + return httpClient, nil +} + +func (tdc *defaultTiDBControl) getBaseURL(tc *v1alpha1.TidbCluster, ordinal int32) string { + if tdc.testURL != "" { + return tdc.testURL + } + + tcName := tc.GetName() + ns := tc.GetNamespace() + scheme := tc.Scheme() + hostName := fmt.Sprintf("%s-%d", TiDBMemberName(tcName), ordinal) + + return fmt.Sprintf("%s://%s.%s.%s:10080", scheme, hostName, TiDBPeerMemberName(tcName), ns) +} + // FakeTiDBControl is a fake implementation of TiDBControlInterface. type FakeTiDBControl struct { healthInfo map[string]bool diff --git a/pkg/controller/tidb_control_test.go b/pkg/controller/tidb_control_test.go new file mode 100644 index 0000000000..96faf54286 --- /dev/null +++ b/pkg/controller/tidb_control_test.go @@ -0,0 +1,339 @@ +// Copyright 2020 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// See the License for the specific language governing permissions and +// limitations under the License. + +package controller + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + . "github.com/onsi/gomega" + "github.com/pingcap/tidb-operator/pkg/apis/pingcap/v1alpha1" + "github.com/pingcap/tidb/config" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/fake" + core "k8s.io/client-go/testing" +) + +const ( + ContentTypeJSON string = "application/json" + caData string = ` +-----BEGIN CERTIFICATE----- +MIIDrDCCApSgAwIBAgIUbd5rgQbs0yXA0lrdMeDG3ot1xhswDQYJKoZIhvcNAQEL +BQAwXDELMAkGA1UEBhMCVVMxEDAOBgNVBAgTB0JlaWppbmcxCzAJBgNVBAcTAkNB +MRAwDgYDVQQKEwdQaW5nQ0FQMQ0wCwYDVQQLEwRUaURCMQ0wCwYDVQQDEwRUaURC +MB4XDTIwMDUxODEzMDQwMFoXDTI1MDUxNzEzMDQwMFowXDELMAkGA1UEBhMCVVMx +EDAOBgNVBAgTB0JlaWppbmcxCzAJBgNVBAcTAkNBMRAwDgYDVQQKEwdQaW5nQ0FQ +MQ0wCwYDVQQLEwRUaURCMQ0wCwYDVQQDEwRUaURCMIIBIjANBgkqhkiG9w0BAQEF +AAOCAQ8AMIIBCgKCAQEA5hNTcyql8kNccZqPnbphPlfAREwzZ44FOLnTZKwFgAAL +fnoZrTnCupCZZTmkKFwJrpe9h20e/YzFRHe08XYRLC5FtOx/GcbKPdfXTrQeH/yP +nRAwJiL9+uuyjNAOBFl/D42kwaSRlhRCyGB2l9E2/LxHQIsC0o644jVf8R7wI3Or +DT58LaY/Ls5t++scffPqCrDPvtBycgSnXgxiN79zG86MjsWi37hzZ3bMtm5M3IL7 +0MmeVPYaAa0DrUJEITMl+HOrZJgdtni6HIwhmmlJcyGvKgsJMEBFD3nbbZeHuA0b +YEuM4SPneyt0pfd+5PpKvWyyMxv22agu18GI2eWBLQIDAQABo2YwZDAOBgNVHQ8B +Af8EBAMCAQYwEgYDVR0TAQH/BAgwBgEB/wIBAjAdBgNVHQ4EFgQUZZ3644otjnxA +yR5qJQqc55JABsUwHwYDVR0jBBgwFoAUZZ3644otjnxAyR5qJQqc55JABsUwDQYJ +KoZIhvcNAQELBQADggEBAJFM6gzsZHKvEN/fjAs3hcawT/Rvm1m0HF5fQMNz9qFz +wJty8Z9l2NN2jqHvV20gByMV/CMsecvmPM6my01c1SEewnMEJ427Qlmw9cjT0sZY +N25C9IQQsiPtEPuhIBla/EEVgJI97BsKhDlZv+M4Y6rXZM7gDz4233jG0pG0b21i +aDToY4Yuv23b/2HvmC2vbthqQ79k8CkObIc0zHCtneu14cTtDiXZb/oWB118UUoh +6YL9bweBSkti8srFUi/jzH+UbsTkCfzYlY+xe2b3A2lgTkbA8SdSxzfGyYWCiDg3 +AVtszHCdFeRCInTt0md3rZb7AR9onqkCmRdOVXLXuVc= +-----END CERTIFICATE----- +` + certData string = ` +-----BEGIN CERTIFICATE----- +MIIDBzCCAe+gAwIBAgIUJRIgZmjB0oCRS26oScdxQLqJCoUwDQYJKoZIhvcNAQEL +BQAwXDELMAkGA1UEBhMCVVMxEDAOBgNVBAgTB0JlaWppbmcxCzAJBgNVBAcTAkNB +MRAwDgYDVQQKEwdQaW5nQ0FQMQ0wCwYDVQQLEwRUaURCMQ0wCwYDVQQDEwRUaURC +MB4XDTIwMDUxODEzMDQwMFoXDTIxMDUxODEzMDQwMFowSDELMAkGA1UEBhMCVVMx +FjAUBgNVBAgTDVNhbiBGcmFuY2lzY28xCzAJBgNVBAcTAkNBMRQwEgYDVQQDEwtl +eGFtcGxlLm5ldDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABMvDGev6J0DIaCqK +Fb4yrxRxlURbfk64yQm4bAgMemV23U4JrDOz/JLJctmhr7krAz5bljKbf3XD8fja +ZgF1e6SjgZ8wgZwwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsGAQUFBwMC +MAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFFT0RT/TccSXVGLom1mDYEpiQFOTMB8G +A1UdIwQYMBaAFGWd+uOKLY58QMkeaiUKnOeSQAbFMCcGA1UdEQQgMB6CC2V4YW1w +bGUubmV0gg93d3cuZXhhbXBsZS5uZXQwDQYJKoZIhvcNAQELBQADggEBADoX65Sb +0GWwiak/x4SNVJRx/Ktj3Hl9nNKsvbGh/3BMgBO6P8M7lrwmtIg5TJtO2lnYOjVr +yk0wi1lGEkbWkCPPXhYx8d9aYcNBLz6CAbSKzG6OX6OCUnKogVf4D44+6b6VgfqP +Ja4yJOe6C1pe9dDMh7VAcZyKFUcEkp4Klheh96PY6seDrzT/kRCTYA7X9tfIEFTO +Qcu9ZVAsypVgOUc1pZJGPED1oItx24V2ZX9E1SM/8tQcRt2s/3ah+LWQV8lpVXCi +o8O7UTMyQ7MUrPusaqsG/QuvppbdahOLzkVc0E5jUOL/dgSxsdOqc7EIxd94Cg65 +cQQSTMrQTbQLo5c= +-----END CERTIFICATE----- +` + keyData string = ` +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIFbUNEYv0ujI3dTLbnb5lTBfRxwst3lMROmRV2tN7NTroAoGCCqGSM49 +AwEHoUQDQgAEy8MZ6/onQMhoKooVvjKvFHGVRFt+TrjJCbhsCAx6ZXbdTgmsM7P8 +ksly2aGvuSsDPluWMpt/dcPx+NpmAXV7pA== +-----END EC PRIVATE KEY----- +` +) + +func getClientServer(h func(http.ResponseWriter, *http.Request)) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(h)) +} + +func TestHealth(t *testing.T) { + g := NewGomegaWithT(t) + + cases := []struct { + caseName string + path string + method string + failed bool + healthExpected bool + }{ + { + caseName: "GetHealth health", + path: "/status", + method: "GET", + failed: false, + healthExpected: true, + }, + { + caseName: "GetHealth not health", + path: "/status", + method: "GET", + failed: true, + healthExpected: false, + }, + } + + for _, c := range cases { + svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + g.Expect(request.Method).To(Equal(c.method), "check method") + g.Expect(request.URL.Path).To(Equal(c.path), "check url") + + w.Header().Set("Content-Type", ContentTypeJSON) + if c.failed { + w.WriteHeader(http.StatusInternalServerError) + } else { + w.WriteHeader(http.StatusOK) + } + }) + defer svc.Close() + + fakeClient := &fake.Clientset{} + tc := getTidbCluster() + + control := NewDefaultTiDBControl(fakeClient) + control.testURL = svc.URL + result, err := control.GetHealth(tc, 0) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(result).To(Equal(c.healthExpected)) + } +} + +func TestInfo(t *testing.T) { + g := NewGomegaWithT(t) + + cases := []struct { + caseName string + path string + method string + resp DBInfo + failed bool + expected *DBInfo + }{ + { + caseName: "GetInfo is not owner", + path: "/info", + method: "POST", + resp: DBInfo{IsOwner: false}, + failed: false, + expected: &DBInfo{IsOwner: false}, + }, + { + caseName: "GetInfo is owner", + path: "/info", + method: "POST", + resp: DBInfo{IsOwner: true}, + failed: false, + expected: &DBInfo{IsOwner: true}, + }, + { + caseName: "GetInfo failed", + path: "/info", + method: "POST", + resp: DBInfo{IsOwner: true}, + failed: true, + }, + } + + for _, c := range cases { + svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + g.Expect(request.Method).To(Equal(c.method), "check method") + g.Expect(request.URL.Path).To(Equal(c.path), "check url") + + w.Header().Set("Content-Type", ContentTypeJSON) + data, err := json.Marshal(c.resp) + g.Expect(err).NotTo(HaveOccurred()) + + if c.failed { + w.WriteHeader(http.StatusInternalServerError) + } else { + w.Write(data) + } + }) + defer svc.Close() + + fakeClient := &fake.Clientset{} + control := NewDefaultTiDBControl(fakeClient) + control.testURL = svc.URL + tc := getTidbCluster() + result, err := control.GetInfo(tc, 0) + if c.failed { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(result).To(Equal(c.expected)) + } + } +} + +func TestSettings(t *testing.T) { + g := NewGomegaWithT(t) + + cases := []struct { + caseName string + path string + method string + failed bool + resp config.Config + expected *config.Config + }{ + { + caseName: "GetSettings", + path: "/settings", + method: "GET", + failed: false, + resp: config.Config{Host: "host1", Port: 1}, + expected: &config.Config{Host: "host1", Port: 1}, + }, + { + caseName: "GetSettings", + path: "/settings", + method: "GET", + failed: true, + resp: config.Config{Host: "host2", Port: 2}, + expected: nil, + }, + } + + for _, c := range cases { + svc := getClientServer(func(w http.ResponseWriter, request *http.Request) { + g.Expect(request.Method).To(Equal(c.method), "check method") + g.Expect(request.URL.Path).To(Equal(c.path), "check url") + + w.Header().Set("Content-Type", ContentTypeJSON) + if c.failed { + w.WriteHeader(http.StatusInternalServerError) + } else { + data, err := json.Marshal(c.resp) + g.Expect(err).NotTo(HaveOccurred()) + w.Write(data) + } + }) + defer svc.Close() + + fakeClient := &fake.Clientset{} + control := NewDefaultTiDBControl(fakeClient) + control.testURL = svc.URL + tc := getTidbCluster() + result, err := control.GetSettings(tc, 0) + if c.failed { + g.Expect(err).To(HaveOccurred()) + } + g.Expect(result).To(Equal(c.expected)) + } +} + +func TestGetHTTPClient(t *testing.T) { + g := NewGomegaWithT(t) + + cases := []struct { + caseName string + updateTC func(*v1alpha1.TidbCluster) + expected func(*http.Client) + }{ + { + caseName: "getHTTPClient tls not enabled", + updateTC: func(cluster *v1alpha1.TidbCluster) {}, + expected: func(client *http.Client) { + g.Expect(client.Transport).To(BeNil()) + }, + }, + { + caseName: "getHTTPClient tls enabled", + updateTC: func(cluster *v1alpha1.TidbCluster) { + cluster.Spec.TLSCluster = &v1alpha1.TLSCluster{ + Enabled: true, + } + }, + expected: func(client *http.Client) { + g.Expect(client.Transport).NotTo(BeNil()) + }, + }, + } + + for _, c := range cases { + fakeClient := &fake.Clientset{} + fakeSecret(fakeClient) + control := NewDefaultTiDBControl(fakeClient) + tc := getTidbCluster() + c.updateTC(tc) + httpClient, err := control.getHTTPClient(tc) + g.Expect(err).NotTo(HaveOccurred()) + c.expected(httpClient) + } +} + +func getTidbCluster() *v1alpha1.TidbCluster { + return &v1alpha1.TidbCluster{ + TypeMeta: metav1.TypeMeta{ + Kind: "TidbCluster", + APIVersion: "pingcap.com/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "demo", + Namespace: corev1.NamespaceDefault, + UID: types.UID("test"), + }, + Spec: v1alpha1.TidbClusterSpec{ + TiDB: v1alpha1.TiDBSpec{}, + }, + } +} + +func fakeSecret(fakeClient *fake.Clientset) { + fakeClient.AddReactor("get", "secrets", func(action core.Action) (bool, runtime.Object, error) { + return true, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "ns", + }, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte(certData), + corev1.TLSPrivateKeyKey: []byte(keyData), + corev1.ServiceAccountRootCAKey: []byte(caData), + }, + }, nil + }) +}