From 2dc736edcc5d50fe3a78c3052f1c29a0dc527e5f Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 27 Jan 2020 13:29:22 -0800 Subject: [PATCH 01/27] add kubernetes service registration --- command/commands.go | 4 +- command/server.go | 2 +- go.mod | 1 + sdk/helper/certutil/certutil_test.go | 41 +++ sdk/helper/certutil/helpers.go | 58 ++++ .../kubernetes/client/client.go | 282 ++++++++++++++++++ .../kubernetes/client/client_test.go | 92 ++++++ .../kubernetes/client/cmd/kubeclient/main.go | 80 +++++ .../kubernetes/client/config.go | 67 +++++ .../kubernetes/retry_handler.go | 112 +++++++ .../kubernetes/retry_handler_test.go | 243 +++++++++++++++ .../kubernetes/service_registration.go | 238 +++++++++++++++ .../kubernetes/service_registration_test.go | 129 ++++++++ .../kubernetes/testing/README.md | 54 ++++ serviceregistration/kubernetes/testing/ca.crt | 18 ++ .../kubernetes/testing/resp-get-pod.json | 120 ++++++++ .../kubernetes/testing/resp-not-found.json | 13 + .../kubernetes/testing/resp-update-pod.json | 123 ++++++++ .../kubernetes/testing/testserver.go | 233 +++++++++++++++ serviceregistration/kubernetes/testing/token | 1 + .../vault/sdk/helper/certutil/helpers.go | 58 ++++ .../vault/sdk/helper/tlsutil/tlsutil.go | 4 +- 22 files changed, 1969 insertions(+), 4 deletions(-) create mode 100644 serviceregistration/kubernetes/client/client.go create mode 100644 serviceregistration/kubernetes/client/client_test.go create mode 100644 serviceregistration/kubernetes/client/cmd/kubeclient/main.go create mode 100644 serviceregistration/kubernetes/client/config.go create mode 100644 serviceregistration/kubernetes/retry_handler.go create mode 100644 serviceregistration/kubernetes/retry_handler_test.go create mode 100644 serviceregistration/kubernetes/service_registration.go create mode 100644 serviceregistration/kubernetes/service_registration_test.go create mode 100644 serviceregistration/kubernetes/testing/README.md create mode 100644 serviceregistration/kubernetes/testing/ca.crt create mode 100644 serviceregistration/kubernetes/testing/resp-get-pod.json create mode 100644 serviceregistration/kubernetes/testing/resp-not-found.json create mode 100644 serviceregistration/kubernetes/testing/resp-update-pod.json create mode 100644 serviceregistration/kubernetes/testing/testserver.go create mode 100644 serviceregistration/kubernetes/testing/token diff --git a/command/commands.go b/command/commands.go index ec48232bbfd7..49d9c2339403 100644 --- a/command/commands.go +++ b/command/commands.go @@ -66,6 +66,7 @@ import ( sr "github.com/hashicorp/vault/serviceregistration" csr "github.com/hashicorp/vault/serviceregistration/consul" + ksr "github.com/hashicorp/vault/serviceregistration/kubernetes" ) const ( @@ -161,7 +162,8 @@ var ( } serviceRegistrations = map[string]sr.Factory{ - "consul": csr.NewServiceRegistration, + "consul": csr.NewServiceRegistration, + "kubernetes": ksr.NewServiceRegistration, } ) diff --git a/command/server.go b/command/server.go index 9580245023ba..dc68044701ba 100644 --- a/command/server.go +++ b/command/server.go @@ -1279,7 +1279,7 @@ CLUSTER_SYNTHESIS_COMPLETE: // If ServiceRegistration is configured, then the backend must support HA isBackendHA := coreConfig.HAPhysical != nil && coreConfig.HAPhysical.HAEnabled() - if (coreConfig.ServiceRegistration != nil) && !isBackendHA { + if !c.flagDev && (coreConfig.ServiceRegistration != nil) && !isBackendHA { c.UI.Output("service_registration is configured, but storage does not support HA") return 1 } diff --git a/go.mod b/go.mod index 7a0973224d43..344ab61db2f3 100644 --- a/go.mod +++ b/go.mod @@ -25,6 +25,7 @@ require ( github.com/aws/aws-sdk-go v1.25.41 github.com/bitly/go-hostpool v0.0.0-20171023180738-a3a6125de932 // indirect github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect + github.com/cenkalti/backoff v2.2.1+incompatible github.com/chrismalek/oktasdk-go v0.0.0-20181212195951-3430665dfaa0 github.com/cockroachdb/apd v1.1.0 // indirect github.com/cockroachdb/cockroach-go v0.0.0-20181001143604-e0a95dfd547c diff --git a/sdk/helper/certutil/certutil_test.go b/sdk/helper/certutil/certutil_test.go index 1de9c4ee886a..cd8ac639e2fd 100644 --- a/sdk/helper/certutil/certutil_test.go +++ b/sdk/helper/certutil/certutil_test.go @@ -11,8 +11,10 @@ import ( "encoding/json" "encoding/pem" "fmt" + "io/ioutil" "math/big" mathrand "math/rand" + "os" "reflect" "strings" "sync" @@ -403,6 +405,45 @@ func TestTLSConfig(t *testing.T) { } } +func TestNewCertPool(t *testing.T) { + caExample := `-----BEGIN CERTIFICATE----- +MIIC5zCCAc+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p +a3ViZUNBMB4XDTE5MTIxMDIzMDUxOVoXDTI5MTIwODIzMDUxOVowFTETMBEGA1UE +AxMKbWluaWt1YmVDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANFi +/RIdMHd865X6JygTb9riX01DA3QnR+RoXDXNnj8D3LziLG2n8ItXMJvWbU3sxxyy +nX9HxJ0SIeexj1cYzdQBtJDjO1/PeuKc4CZ7zCukCAtHz8mC7BDPOU7F7pggpcQ0 +/t/pa2m22hmCu8aDF9WlUYHtJpYATnI/A5vz/VFLR9daxmkl59Qo3oHITj7vAzSx +/75r9cibpQyJ+FhiHOZHQWYY2JYw2g4v5hm5hg5SFM9yFcZ75ISI9ebyFFIl9iBY +zAk9jqv1mXvLr0Q39AVwMTamvGuap1oocjM9NIhQvaFL/DNqF1ouDQjCf5u2imLc +TraO1/2KO8fqwOZCOrMCAwEAAaNCMEAwDgYDVR0PAQH/BAQDAgKkMB0GA1UdJQQW +MBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3 +DQEBCwUAA4IBAQBtVZCwCPqUUUpIClAlE9nc2fo2bTs9gsjXRmqdQ5oaSomSLE93 +aJWYFuAhxPXtlApbLYZfW2m1sM3mTVQN60y0uE4e1jdSN1ErYQ9slJdYDAMaEmOh +iSexj+Nd1scUiMHV9lf3ps5J8sYeCpwZX3sPmw7lqZojTS12pANBDcigsaj5RRyN +9GyP3WkSQUsTpWlDb9Fd+KNdkCVw7nClIpBPA2KW4BQKw/rNSvOFD61mbzc89lo0 +Q9IFGQFFF8jO18lbyWqnRBGXcS4/G7jQ3S7C121d14YLUeAYOM7pJykI1g4CLx9y +vitin0L6nprauWkKO38XgM4T75qKZpqtiOcT +-----END CERTIFICATE----- +` + + tmpfile, err := ioutil.TempFile("", "ca.crt") + if err != nil { + t.Fatal(err) + } + defer os.Remove(tmpfile.Name()) + + if _, err := tmpfile.Write([]byte(caExample)); err != nil { + t.Fatal(err) + } + if err := tmpfile.Close(); err != nil { + t.Fatal(err) + } + + if _, err := NewCertPool(tmpfile.Name()); err != nil { + t.Fatal(err) + } +} + func refreshRSA8CertBundle() *CertBundle { initTest.Do(setCerts) return &CertBundle{ diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index 4a35f88dca5e..d53aa1cae209 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "io/ioutil" "math/big" "net" "net/url" @@ -804,3 +805,60 @@ func SignCertificate(data *CreationBundle) (*ParsedCertBundle, error) { return result, nil } + +func NewCertPool(pathToFile string) (*x509.CertPool, error) { + certs, err := certsFromFile(pathToFile) + if err != nil { + return nil, err + } + pool := x509.NewCertPool() + for _, cert := range certs { + pool.AddCert(cert) + } + return pool, nil +} + +// CertsFromFile returns the x509.Certificates contained in the given PEM-encoded file. +// Returns an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates +func certsFromFile(file string) ([]*x509.Certificate, error) { + pemBlock, err := ioutil.ReadFile(file) + if err != nil { + return nil, err + } + certs, err := parseCertsPEM(pemBlock) + if err != nil { + return nil, fmt.Errorf("error reading %s: %s", file, err) + } + return certs, nil +} + +// parseCertsPEM returns the x509.Certificates contained in the given PEM-encoded byte array +// Returns an error if a certificate could not be parsed, or if the data does not contain any certificates +func parseCertsPEM(pemCerts []byte) ([]*x509.Certificate, error) { + ok := false + certs := []*x509.Certificate{} + for len(pemCerts) > 0 { + var block *pem.Block + block, pemCerts = pem.Decode(pemCerts) + if block == nil { + break + } + // Only use PEM "CERTIFICATE" blocks without extra headers + if block.Type != "CERTIFICATE" || len(block.Headers) != 0 { + continue + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return certs, err + } + + certs = append(certs, cert) + ok = true + } + + if !ok { + return certs, errors.New("data does not contain any valid RSA or ECDSA certificates") + } + return certs, nil +} diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go new file mode 100644 index 000000000000..1eea1bc03444 --- /dev/null +++ b/serviceregistration/kubernetes/client/client.go @@ -0,0 +1,282 @@ +package client + +import ( + "bytes" + "crypto/tls" + "encoding/json" + "errors" + "fmt" + "io/ioutil" + "net/http" + "time" + + "github.com/cenkalti/backoff" + "github.com/hashicorp/go-cleanhttp" + "github.com/hashicorp/go-hclog" +) + +// maxRetries is the maximum number of times the client +// should retry. +const maxRetries = 10 + +var ( + ErrNamespaceUnset = errors.New(`"namespace" is unset`) + ErrPodNameUnset = errors.New(`"podName" is unset`) + ErrNotFound = errors.New("not found") + ErrNotInCluster = errors.New("unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined") +) + +// New instantiates a Client. The stopCh is used for exiting retry loops +// when closed. +func New(logger hclog.Logger, stopCh <-chan struct{}) (*Client, error) { + config, err := inClusterConfig() + if err != nil { + return nil, err + } + return &Client{ + logger: logger, + config: config, + stopCh: stopCh, + }, nil +} + +// Client is a minimal Kubernetes client. +type Client struct { + logger hclog.Logger + config *Config + stopCh <-chan struct{} +} + +// GetPod gets a pod from the Kubernetes API. +func (c *Client) GetPod(namespace, podName string) (*Pod, error) { + endpoint := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s", namespace, podName) + method := http.MethodGet + + // Validate that we received required parameters. + if namespace == "" { + return nil, ErrNamespaceUnset + } + if podName == "" { + return nil, ErrPodNameUnset + } + + req, err := http.NewRequest(method, c.config.Host+endpoint, nil) + if err != nil { + return nil, err + } + pod := &Pod{} + if err := c.do(req, pod); err != nil { + return nil, err + } + return pod, nil +} + +// PatchPod updates the pod's tags to the given ones. +// It does so non-destructively, or in other words, without tearing down +// the pod. +func (c *Client) PatchPod(namespace, podName string, patches ...*Patch) error { + endpoint := fmt.Sprintf("/api/v1/namespaces/%s/pods/%s", namespace, podName) + method := http.MethodPatch + + // Validate that we received required parameters. + if namespace == "" { + return ErrNamespaceUnset + } + if podName == "" { + return ErrPodNameUnset + } + if len(patches) == 0 { + // No work to perform. + return nil + } + + var jsonPatches []interface{} + for _, patch := range patches { + if patch.Operation == Unset { + return errors.New("patch operation must be set") + } + jsonPatches = append(jsonPatches, map[string]interface{}{ + "op": patch.Operation.String(), + "path": patch.Path, + "value": patch.Value, + }) + } + body, err := json.Marshal(jsonPatches) + if err != nil { + return err + } + req, err := http.NewRequest(method, c.config.Host+endpoint, bytes.NewReader(body)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json-patch+json") + return c.do(req, nil) +} + +// do executes the given request, retrying if necessary. +func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { + // Finish setting up a valid request. + req.Header.Set("Authorization", "Bearer "+c.config.BearerToken) + req.Header.Set("Accept", "application/json") + client := cleanhttp.DefaultClient() + client.Transport = &http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: c.config.CACertPool, + }, + } + + // Execute and retry the request. This exponential backoff comes + // with jitter already rolled in. + var lastErr error + b := backoff.NewExponentialBackOff() + for i := 0; i < maxRetries; i++ { + if i != 0 { + select { + case <-c.stopCh: + return nil + case <-time.NewTimer(b.NextBackOff()).C: + // Continue to the request. + } + } + shouldRetry, err := c.attemptRequest(client, req, ptrToReturnObj) + if !shouldRetry { + // The error may be nil or populated depending on whether the + // request was successful. + return err + } + lastErr = err + } + return lastErr +} + +// attemptRequest tries one single request. It's in its own function so each +// response body can be closed before returning, which would read awkwardly if +// executed in a loop. +func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToReturnObj interface{}) (shouldRetry bool, err error) { + // Preserve the original request body so it can be viewed for debugging if needed. + // Reading it empties it, so we need to re-add it afterwards. + var reqBody []byte + if req.Body != nil { + reqBody, _ = ioutil.ReadAll(req.Body) + reqBodyReader := bytes.NewReader(reqBody) + req.Body = ioutil.NopCloser(reqBodyReader) + } + + resp, err := client.Do(req) + if err != nil { + return false, err + } + defer func() { + if err := resp.Body.Close(); err != nil { + if c.logger.IsWarn() { + // Failing to close response bodies can present as a memory leak so it's + // important to surface it. + c.logger.Warn(fmt.Sprintf("unable to close response body: %s", err)) + } + } + }() + + // Check for success. + switch resp.StatusCode { + case 200, 201, 202: + // Pass. + case 401, 403: + // Perhaps the token from our bearer token file has been refreshed. + config, err := inClusterConfig() + if err != nil { + return false, err + } + if config.BearerToken == c.config.BearerToken { + // It's the same token. + return false, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + } + c.config = config + // Continue to try again, but return the error too in case the caller would rather read it out. + return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + case 404: + return false, ErrNotFound + case 500, 502, 503, 504: + // Could be transient. + return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + default: + // Unexpected. + return false, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + } + + // We only arrive here with success. + // If we're not supposed to read out the body, we have nothing further + // to do here. + if ptrToReturnObj == nil { + return false, nil + } + + // Attempt to read out the body into the given return object. + return false, json.NewDecoder(resp.Body).Decode(ptrToReturnObj) +} + +type Pod struct { + Metadata *Metadata `json:"metadata,omitempty"` +} + +type Metadata struct { + Name string `json:"name,omitempty"` + + // This map will be nil if no "labels" key was provided. + // It will be populated but have a length of zero if the + // key was provided, but no values. + Labels map[string]string `json:"labels,omitempty"` +} + +type PatchOperation int + +const ( + // When adding support for more PatchOperations in the future, + // DO NOT alphebetize them because it will change the underlying + // int representing a user's intent. If that's stored anywhere, + // it will cause storage reads to map to the incorrect operation. + Unset PatchOperation = iota + Add + Replace +) + +func Parse(s string) PatchOperation { + switch s { + case "add": + return Add + case "replace": + return Replace + default: + return Unset + } +} + +func (p PatchOperation) String() string { + switch p { + case Unset: + // This is an invalid choice, and will be shown on a patch + // where the PatchOperation is unset. That's because ints + // default to 0, and Unset corresponds to 0. + return "unset" + case Add: + return "add" + case Replace: + return "replace" + default: + // Should never arrive here. + return "" + } +} + +type Patch struct { + Operation PatchOperation + Path string + Value interface{} +} + +// sanitizedDebuggingInfo converts an http response to a string without +// including its headers to avoid leaking authorization +// headers. +func sanitizedDebuggingInfo(req *http.Request, reqBody []byte, resp *http.Response) string { + respBody, _ := ioutil.ReadAll(resp.Body) + return fmt.Sprintf("req method: %s, req url: %s, req body: %s, resp statuscode: %d, resp respBody: %s", req.Method, req.URL, reqBody, resp.StatusCode, respBody) +} diff --git a/serviceregistration/kubernetes/client/client_test.go b/serviceregistration/kubernetes/client/client_test.go new file mode 100644 index 000000000000..c36b5367bbb7 --- /dev/null +++ b/serviceregistration/kubernetes/client/client_test.go @@ -0,0 +1,92 @@ +package client + +import ( + "os" + "testing" + + "github.com/hashicorp/go-hclog" + kubetest "github.com/hashicorp/vault/serviceregistration/kubernetes/testing" +) + +func TestClient(t *testing.T) { + testState, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + + Scheme = testConf.ClientScheme + TokenFile = testConf.PathToTokenFile + RootCAFile = testConf.PathToRootCAFile + if err := os.Setenv(EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + client, err := New(hclog.Default(), make(chan struct{})) + if err != nil { + t.Fatal(err) + } + e := &env{ + client: client, + testState: testState, + } + e.TestGetPod(t) + e.TestGetPodNotFound(t) + e.TestUpdatePodTags(t) + e.TestUpdatePodTagsNotFound(t) +} + +type env struct { + client *Client + testState *kubetest.State +} + +func (e *env) TestGetPod(t *testing.T) { + pod, err := e.client.GetPod(kubetest.ExpectedNamespace, kubetest.ExpectedPodName) + if err != nil { + t.Fatal(err) + } + if pod.Metadata.Name != "shell-demo" { + t.Fatalf("expected %q but received %q", "shell-demo", pod.Metadata.Name) + } +} + +func (e *env) TestGetPodNotFound(t *testing.T) { + _, err := e.client.GetPod(kubetest.ExpectedNamespace, "no-exist") + if err == nil { + t.Fatal("expected error because pod is unfound") + } + if err != ErrNotFound { + t.Fatalf("expected %q but received %q", ErrNotFound, err) + } +} + +func (e *env) TestUpdatePodTags(t *testing.T) { + if err := e.client.PatchPod(kubetest.ExpectedNamespace, kubetest.ExpectedPodName, &Patch{ + Operation: Add, + Path: "/metadata/labels/fizz", + Value: "buzz", + }); err != nil { + t.Fatal(err) + } + if e.testState.NumPatches() != 1 { + t.Fatalf("expected 1 label but received %+v", e.testState) + } + if e.testState.Get("/metadata/labels/fizz")["value"] != "buzz" { + t.Fatalf("expected buzz but received %q", e.testState.Get("fizz")) + } +} + +func (e *env) TestUpdatePodTagsNotFound(t *testing.T) { + err := e.client.PatchPod(kubetest.ExpectedNamespace, "no-exist", &Patch{ + Operation: Add, + Path: "/metadata/labels/fizz", + Value: "buzz", + }) + if err == nil { + t.Fatal("expected error because pod is unfound") + } + if err != ErrNotFound { + t.Fatalf("expected %q but received %q", ErrNotFound, err) + } +} diff --git a/serviceregistration/kubernetes/client/cmd/kubeclient/main.go b/serviceregistration/kubernetes/client/cmd/kubeclient/main.go new file mode 100644 index 000000000000..536b63f22ee4 --- /dev/null +++ b/serviceregistration/kubernetes/client/cmd/kubeclient/main.go @@ -0,0 +1,80 @@ +package main + +// This code builds a minimal binary of the lightweight kubernetes +// client and exposes it for manual testing. +// The intention is that the binary can be built and dropped into +// a Kube environment like this: +// https://kubernetes.io/docs/tasks/debug-application-cluster/get-shell-running-container/ +// Then, commands can be run to test its API calls. +// The above commands are intended to be run inside an instance of +// minikube that has been started. +// After building this binary, place it in the container like this: +// $ kubectl cp kubeclient /shell-demo:/ +// At first you may get 403's, which can be resolved using this: +// https://github.com/fabric8io/fabric8/issues/6840#issuecomment-307560275 +// +// Example calls: +// ./kubeclient -call='get-pod' -namespace='default' -pod-name='shell-demo' +// ./kubeclient -call='patch-pod' -namespace='default' -pod-name='shell-demo' -patches='/metadata/labels/fizz:buzz,/metadata/labels/foo:bar' + +import ( + "encoding/json" + "flag" + "fmt" + "strings" + + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/serviceregistration/kubernetes/client" +) + +var callToMake string +var patchesToAdd string +var namespace string +var podName string + +func init() { + flag.StringVar(&callToMake, "call", "", `the call to make: 'get-pod' or 'patch-pod'`) + flag.StringVar(&patchesToAdd, "patches", "", `if call is "patch-pod", the patches to do like so: "/metadata/labels/fizz:buzz,/metadata/labels/foo:bar"`) + flag.StringVar(&namespace, "namespace", "", "the namespace to use") + flag.StringVar(&podName, "pod-name", "", "the pod name to use") +} + +func main() { + flag.Parse() + + c, err := client.New(hclog.Default(), make(chan struct{})) + if err != nil { + panic(err) + } + + switch callToMake { + case "get-pod": + pod, err := c.GetPod(namespace, podName) + if err != nil { + panic(err) + } + b, _ := json.Marshal(pod) + fmt.Printf("pod: %s\n", b) + return + case "patch-pod": + patchPairs := strings.Split(patchesToAdd, ",") + var patches []*client.Patch + for _, patchPair := range patchPairs { + fields := strings.Split(patchPair, ":") + if len(fields) != 2 { + panic(fmt.Errorf("unable to split %s from selectors provided of %s", fields, patchesToAdd)) + } + patches = append(patches, &client.Patch{ + Operation: client.Replace, + Path: fields[0], + Value: fields[1], + }) + } + if err := c.PatchPod(namespace, podName, patches...); err != nil { + panic(err) + } + return + default: + panic(fmt.Errorf(`unsupported call provided: %q`, callToMake)) + } +} diff --git a/serviceregistration/kubernetes/client/config.go b/serviceregistration/kubernetes/client/config.go new file mode 100644 index 000000000000..22b1226d6182 --- /dev/null +++ b/serviceregistration/kubernetes/client/config.go @@ -0,0 +1,67 @@ +package client + +import ( + "crypto/x509" + "io/ioutil" + "net" + "os" + + "github.com/hashicorp/vault/sdk/helper/certutil" +) + +const ( + // These environment variables aren't set by default. + // Vault may read them in if set through these environment variables. + // Example here: + // https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/ + EnvVarKubernetesNamespace = "VAULT_NAMESPACE" + EnvVarKubernetesPodName = "VAULT_POD_NAME" + + // The service host and port environment variables are + // set by default inside a Kubernetes environment. + EnvVarKubernetesServiceHost = "KUBERNETES_SERVICE_HOST" + EnvVarKubernetesServicePort = "KUBERNETES_SERVICE_PORT" +) + +var ( + // These are presented as variables so they can be updated + // to point at test fixtures if needed. They aren't passed + // into inClusterConfig to avoid dependency injection. + Scheme = "https://" + TokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" + RootCAFile = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" +) + +// inClusterConfig returns a config object which uses the service account +// kubernetes gives to services. It's intended for clients that expect to be +// running inside a service running on kubernetes. It will return ErrNotInCluster +// if called from a process not running in a kubernetes environment. +func inClusterConfig() (*Config, error) { + host, port := os.Getenv(EnvVarKubernetesServiceHost), os.Getenv(EnvVarKubernetesServicePort) + if len(host) == 0 || len(port) == 0 { + return nil, ErrNotInCluster + } + + token, err := ioutil.ReadFile(TokenFile) + if err != nil { + return nil, err + } + + pool, err := certutil.NewCertPool(RootCAFile) + if err != nil { + return nil, err + } + return &Config{ + Host: Scheme + net.JoinHostPort(host, port), + CACertPool: pool, + BearerToken: string(token), + BearerTokenFile: TokenFile, + }, nil +} + +type Config struct { + Host string + BearerToken string + BearerTokenFile string + CACertPool *x509.CertPool +} diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go new file mode 100644 index 000000000000..5eb835d200a7 --- /dev/null +++ b/serviceregistration/kubernetes/retry_handler.go @@ -0,0 +1,112 @@ +package kubernetes + +import ( + "fmt" + "strconv" + "sync" + "time" + + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/serviceregistration/kubernetes/client" +) + +// How often to retry sending a state update if it fails. +var retryFreq = 5 * time.Second + +// retryHandler executes retries. +// It is thread-safe. +type retryHandler struct { + // These don't need a mutex because they're never mutated. + logger hclog.Logger + namespace, podName string + client *client.Client + + // This gets mutated on multiple threads. + patchesToRetry []*client.Patch + patchesToRetryLock sync.Mutex +} + +// Run runs at an interval, checking if anything has failed and if so, +// attempting to send them again. +func (r *retryHandler) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { + // Make sure Vault will give us time to finish up here. + wait.Add(1) + defer wait.Done() + + retry := time.NewTicker(retryFreq) + for { + select { + case <-shutdownCh: + return + case <-retry.C: + r.retry() + } + } +} + +// Add adds a patch to be retried until it's either completed without +// error, or no longer needed. +func (r *retryHandler) Add(patch *client.Patch) error { + r.patchesToRetryLock.Lock() + defer r.patchesToRetryLock.Unlock() + + // - If the patch is a dupe, don't add it. + // - If the patch reverts another, remove them both. + // For example, perhaps we were already retrying "active = true", + // but this new patch tells us "active = false" again. + // - Otherwise, this is a new, unique patch, so add this patch to retries. + for i := 0; i < len(r.patchesToRetry); i++ { + prevPatch := r.patchesToRetry[i] + if patch.Path != prevPatch.Path { + continue + } + if patch.Operation != prevPatch.Operation { + continue + } + // These patches are operating upon the same thing. + // Let's look at what they're trying to do to determine + // the right action to take with the incoming patch. + patchValStr, ok := patch.Value.(string) + if !ok { + return fmt.Errorf("all patches must have bool values but received %+x", patch) + } + patchVal, err := strconv.ParseBool(patchValStr) + if err != nil { + return err + } + // This was already verified to be a bool string + // when it was added to the slice. + prevPatchVal, _ := strconv.ParseBool(prevPatch.Value.(string)) + if patchVal == prevPatchVal { + // We don't need to add the new patch because it already exists. + // Nothing further to do here. + return nil + } else { + // Rather than doing both an add and a subtract, or a true and a false, + // we need to just not act on both. This requires not adding the new patch, + // and removing the previous conflicting patch. + r.patchesToRetry = append(r.patchesToRetry[:i], r.patchesToRetry[i+1:]...) + return nil + } + } + r.patchesToRetry = append(r.patchesToRetry, patch) + return nil +} + +func (r *retryHandler) retry() { + r.patchesToRetryLock.Lock() + defer r.patchesToRetryLock.Unlock() + + if len(r.patchesToRetry) == 0 { + // Nothing to do here. + return + } + + if err := r.client.PatchPod(r.namespace, r.podName, r.patchesToRetry...); err != nil { + if r.logger.IsWarn() { + r.logger.Warn("unable to update state due to %s, will retry", err.Error()) + } + return + } + r.patchesToRetry = make([]*client.Patch, 0) +} diff --git a/serviceregistration/kubernetes/retry_handler_test.go b/serviceregistration/kubernetes/retry_handler_test.go new file mode 100644 index 000000000000..1be280ae7dcf --- /dev/null +++ b/serviceregistration/kubernetes/retry_handler_test.go @@ -0,0 +1,243 @@ +package kubernetes + +import ( + "os" + "sync" + "testing" + "time" + + "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/serviceregistration/kubernetes/client" + kubetest "github.com/hashicorp/vault/serviceregistration/kubernetes/testing" +) + +func TestRetryHandlerSimple(t *testing.T) { + if testing.Short() { + t.Skip("skipping because this test takes 10-15 seconds") + } + + testState, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + logger := hclog.NewNullLogger() + shutdownCh := make(chan struct{}) + wait := &sync.WaitGroup{} + testPatch := &client.Patch{ + Operation: client.Add, + Path: "patch-path", + Value: "true", + } + + c, err := client.New(logger, shutdownCh) + if err != nil { + t.Fatal(err) + } + + r := &retryHandler{ + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, + patchesToRetry: make([]*client.Patch, 0), + } + go r.Run(shutdownCh, wait) + + if testState.NumPatches() != 0 { + t.Fatal("expected no current patches") + } + if err := r.Add(testPatch); err != nil { + t.Fatal(err) + } + // Wait ample until the next try should have occurred. + <-time.NewTimer(retryFreq * 2).C + if testState.NumPatches() != 1 { + t.Fatal("expected 1 patch") + } +} + +func TestRetryHandlerAdd(t *testing.T) { + r := &retryHandler{ + logger: hclog.NewNullLogger(), + namespace: "some-namespace", + podName: "some-pod-name", + patchesToRetry: make([]*client.Patch, 0), + } + + testPatch1 := &client.Patch{ + Operation: client.Add, + Path: "one", + Value: "true", + } + testPatch2 := &client.Patch{ + Operation: client.Add, + Path: "two", + Value: "true", + } + testPatch3 := &client.Patch{ + Operation: client.Add, + Path: "three", + Value: "true", + } + testPatch4 := &client.Patch{ + Operation: client.Add, + Path: "four", + Value: "true", + } + + // Should be able to add all 4 patches. + if err := r.Add(testPatch1); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 1 { + t.Fatal("expected 1 patch") + } + + if err := r.Add(testPatch2); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 2 { + t.Fatal("expected 2 patches") + } + + if err := r.Add(testPatch3); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 3 { + t.Fatal("expected 3 patches") + } + + if err := r.Add(testPatch4); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") + } + + // Adding a dupe should result in no change. + if err := r.Add(testPatch4); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") + } + + // Adding a reversion should result in its twin being subtracted. + if err := r.Add(&client.Patch{ + Operation: client.Add, + Path: "four", + Value: "false", + }); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 3 { + t.Fatal("expected 3 patches") + } + + if err := r.Add(&client.Patch{ + Operation: client.Add, + Path: "three", + Value: "false", + }); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 2 { + t.Fatal("expected 2 patches") + } + + if err := r.Add(&client.Patch{ + Operation: client.Add, + Path: "two", + Value: "false", + }); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 1 { + t.Fatal("expected 1 patches") + } + + if err := r.Add(&client.Patch{ + Operation: client.Add, + Path: "one", + Value: "false", + }); err != nil { + t.Fatal(err) + } + if len(r.patchesToRetry) != 0 { + t.Fatal("expected 0 patches") + } +} + +// This is meant to be run with the -race flag on. +func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { + _, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + logger := hclog.NewNullLogger() + shutdownCh := make(chan struct{}) + wait := &sync.WaitGroup{} + testPatch := &client.Patch{ + Operation: client.Add, + Path: "patch-path", + Value: "true", + } + + c, err := client.New(logger, shutdownCh) + if err != nil { + t.Fatal(err) + } + + r := &retryHandler{ + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, + patchesToRetry: make([]*client.Patch, 0), + } + go r.Run(shutdownCh, wait) + + // Now hit it as quickly as possible to see if we can produce + // races or deadlocks. + start := make(chan struct{}) + done := make(chan bool) + numRoutines := 100 + for i := 0; i < numRoutines; i++ { + go func() { + <-start + if err := r.Add(testPatch); err != nil { + t.Fatal(err) + } + done <- true + }() + } + close(start) + + // Allow up to 5 seconds for everything to finish. + timer := time.NewTimer(5 * time.Second) + for i := 0; i < numRoutines; i++ { + select { + case <-timer.C: + t.Fatal("test took too long to complete, check for deadlock") + case <-done: + } + } +} diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go new file mode 100644 index 000000000000..dfb2ab931e0c --- /dev/null +++ b/serviceregistration/kubernetes/service_registration.go @@ -0,0 +1,238 @@ +package kubernetes + +import ( + "fmt" + "os" + "sync" + + "github.com/hashicorp/go-hclog" + sr "github.com/hashicorp/vault/serviceregistration" + "github.com/hashicorp/vault/serviceregistration/kubernetes/client" +) + +const ( + // Labels are placed in a pod's metadata. + labelVaultVersion = "vault-version" + labelActive = "vault-active" + labelSealed = "vault-sealed" + labelPerfStandby = "vault-perf-standby" + labelInitialized = "vault-initialized" + + // This is the path to where these labels are applied. + pathToLabels = "/metadata/labels/" +) + +func NewServiceRegistration(config map[string]string, logger hclog.Logger, state sr.State, _ string) (sr.ServiceRegistration, error) { + namespace, err := getRequiredField(logger, config, client.EnvVarKubernetesNamespace, "namespace") + if err != nil { + return nil, err + } + podName, err := getRequiredField(logger, config, client.EnvVarKubernetesPodName, "pod_name") + if err != nil { + return nil, err + } + return &serviceRegistration{ + logger: logger, + namespace: namespace, + podName: podName, + initialState: state, + retryHandler: &retryHandler{ + logger: logger, + namespace: namespace, + podName: podName, + patchesToRetry: make([]*client.Patch, 0), + }, + }, nil +} + +type serviceRegistration struct { + logger hclog.Logger + namespace, podName string + client *client.Client + initialState sr.State + retryHandler *retryHandler +} + +func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup) error { + c, err := client.New(r.logger, shutdownCh) + if err != nil { + return err + } + r.client = c + r.retryHandler.client = c + + // Verify that the pod exists and our configuration looks good. + pod, err := c.GetPod(r.namespace, r.podName) + if err != nil { + return err + } + + // Now to initially label our pod. + if pod.Metadata == nil { + // This should never happen IRL, just being defensive. + return fmt.Errorf("no pod metadata on %+v", pod) + } + if pod.Metadata.Labels == nil { + // Add the labels field, and the labels as part of that one call. + // The reason we must take a different approach to adding them is discussed here: + // https://stackoverflow.com/questions/57480205/error-while-applying-json-patch-to-kubernetes-custom-resource + if err := c.PatchPod(r.namespace, r.podName, &client.Patch{ + Operation: client.Add, + Path: "/metadata/labels", + Value: map[string]string{ + labelVaultVersion: r.initialState.VaultVersion, + labelActive: toString(r.initialState.IsActive), + labelSealed: toString(r.initialState.IsSealed), + labelPerfStandby: toString(r.initialState.IsPerformanceStandby), + labelInitialized: toString(r.initialState.IsInitialized), + }, + }); err != nil { + return err + } + } else { + // Create the labels through a patch to each individual field. + patches := []*client.Patch{ + { + Operation: client.Replace, + Path: pathToLabels + labelVaultVersion, + Value: r.initialState.VaultVersion, + }, + { + Operation: client.Replace, + Path: pathToLabels + labelActive, + Value: toString(r.initialState.IsActive), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelSealed, + Value: toString(r.initialState.IsSealed), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelPerfStandby, + Value: toString(r.initialState.IsPerformanceStandby), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelInitialized, + Value: toString(r.initialState.IsInitialized), + }, + } + if err := c.PatchPod(r.namespace, r.podName, patches...); err != nil { + return err + } + } + + // Run a service that retries errored-out notifications if they occur. + go r.retryHandler.Run(shutdownCh, wait) + + // Run a background goroutine to leave labels in the final state we'd like + // when Vault shuts down. + go r.onShutdown(shutdownCh, wait) + + return nil +} + +func (r *serviceRegistration) NotifyActiveStateChange(isActive bool) error { + return r.notifyOrRetry(&client.Patch{ + Operation: client.Replace, + Path: pathToLabels + labelActive, + Value: toString(isActive), + }) +} + +func (r *serviceRegistration) NotifySealedStateChange(isSealed bool) error { + return r.notifyOrRetry(&client.Patch{ + Operation: client.Replace, + Path: pathToLabels + labelSealed, + Value: toString(isSealed), + }) +} + +func (r *serviceRegistration) NotifyPerformanceStandbyStateChange(isStandby bool) error { + return r.notifyOrRetry(&client.Patch{ + Operation: client.Replace, + Path: pathToLabels + labelPerfStandby, + Value: toString(isStandby), + }) +} + +func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) error { + return r.notifyOrRetry(&client.Patch{ + Operation: client.Replace, + Path: pathToLabels + labelInitialized, + Value: toString(isInitialized), + }) +} + +func (r *serviceRegistration) onShutdown(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { + // Make sure Vault will give us time to finish up here. + wait.Add(1) + defer wait.Done() + + // Start running this when we receive a shutdown. + <-shutdownCh + + // Label the pod with the values we want to leave behind after shutdown. + patches := []*client.Patch{ + { + Operation: client.Replace, + Path: pathToLabels + labelActive, + Value: toString(false), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelSealed, + Value: toString(true), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelPerfStandby, + Value: toString(false), + }, + { + Operation: client.Replace, + Path: pathToLabels + labelInitialized, + Value: toString(false), + }, + } + if err := r.client.PatchPod(r.namespace, r.podName, patches...); err != nil { + if r.logger.IsError() { + r.logger.Error(fmt.Sprintf("unable to set final status on pod name %q in namespace %q on shutdown: %s", r.podName, r.namespace, err)) + } + return + } +} + +func (r *serviceRegistration) notifyOrRetry(patch *client.Patch) error { + if err := r.client.PatchPod(r.namespace, r.podName, patch); err != nil { + if r.logger.IsWarn() { + r.logger.Warn("unable to update state due to %s, will retry", err.Error()) + } + if err := r.retryHandler.Add(patch); err != nil { + return err + } + } + return nil +} + +func getRequiredField(logger hclog.Logger, config map[string]string, envVar, configParam string) (string, error) { + value := "" + switch { + case os.Getenv(envVar) != "": + value = os.Getenv(envVar) + case config[configParam] != "": + value = config[configParam] + default: + return "", fmt.Errorf(`%s must be provided via %q or the %q config parameter`, configParam, envVar, configParam) + } + if logger.IsDebug() { + logger.Debug(fmt.Sprintf("%q: %q", configParam, value)) + } + return value, nil +} + +// Converts a bool to "true" or "false". +func toString(b bool) string { + return fmt.Sprintf("%t", b) +} diff --git a/serviceregistration/kubernetes/service_registration_test.go b/serviceregistration/kubernetes/service_registration_test.go new file mode 100644 index 000000000000..3994c35b7b12 --- /dev/null +++ b/serviceregistration/kubernetes/service_registration_test.go @@ -0,0 +1,129 @@ +package kubernetes + +import ( + "os" + "sync" + "testing" + + "github.com/hashicorp/go-hclog" + sr "github.com/hashicorp/vault/serviceregistration" + "github.com/hashicorp/vault/serviceregistration/kubernetes/client" + kubetest "github.com/hashicorp/vault/serviceregistration/kubernetes/testing" +) + +var testVersion = "version 1" + +func TestServiceRegistration(t *testing.T) { + testState, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + if testState.NumPatches() != 0 { + t.Fatalf("expected 0 patches but have %d: %+v", testState.NumPatches(), testState) + } + shutdownCh := make(chan struct{}) + config := map[string]string{ + "namespace": kubetest.ExpectedNamespace, + "pod_name": kubetest.ExpectedPodName, + } + logger := hclog.NewNullLogger() + state := sr.State{ + VaultVersion: testVersion, + IsInitialized: true, + IsSealed: true, + IsActive: true, + IsPerformanceStandby: true, + } + reg, err := NewServiceRegistration(config, logger, state, "") + if err != nil { + t.Fatal(err) + } + if err := reg.Run(shutdownCh, &sync.WaitGroup{}); err != nil { + t.Fatal(err) + } + + // Test initial state. + if testState.NumPatches() != 5 { + t.Fatalf("expected 5 current labels but have %d: %+v", testState.NumPatches(), testState) + } + if testState.Get(pathToLabels + labelVaultVersion)["value"] != testVersion { + t.Fatalf("expected %q but received %q", testVersion, testState.Get(pathToLabels + labelVaultVersion)["value"]) + } + if testState.Get(pathToLabels + labelActive)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelActive)["value"]) + } + if testState.Get(pathToLabels + labelSealed)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelSealed)["value"]) + } + if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) + } + if testState.Get(pathToLabels + labelInitialized)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelInitialized)["value"]) + } + + // Test NotifyActiveStateChange. + if err := reg.NotifyActiveStateChange(false); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelActive)["value"] != toString(false) { + t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelActive)["value"]) + } + if err := reg.NotifyActiveStateChange(true); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelActive)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelActive)["value"]) + } + + // Test NotifySealedStateChange. + if err := reg.NotifySealedStateChange(false); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelSealed)["value"] != toString(false) { + t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelSealed)["value"]) + } + if err := reg.NotifySealedStateChange(true); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelSealed)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelSealed)["value"]) + } + + // Test NotifyPerformanceStandbyStateChange. + if err := reg.NotifyPerformanceStandbyStateChange(false); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(false) { + t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelPerfStandby)["value"]) + } + if err := reg.NotifyPerformanceStandbyStateChange(true); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) + } + + // Test NotifyInitializedStateChange. + if err := reg.NotifyInitializedStateChange(false); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelInitialized)["value"] != toString(false) { + t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelInitialized)["value"]) + } + if err := reg.NotifyInitializedStateChange(true); err != nil { + t.Fatal(err) + } + if testState.Get(pathToLabels + labelInitialized)["value"] != toString(true) { + t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelInitialized)["value"]) + } +} diff --git a/serviceregistration/kubernetes/testing/README.md b/serviceregistration/kubernetes/testing/README.md new file mode 100644 index 000000000000..7be3ead57da1 --- /dev/null +++ b/serviceregistration/kubernetes/testing/README.md @@ -0,0 +1,54 @@ +# How to Test Manually + +- `$ minikube start` +- In the Vault folder, `$ make dev` +- Create a file called `vault-test.yaml` with the following contents: + +``` +apiVersion: v1 +kind: Pod +metadata: + name: vault +spec: + containers: + - name: nginx + image: nginx + command: [ "sh", "-c"] + args: + - while true; do + echo -en '\n'; + printenv VAULT_POD_NAME VAULT_NAMESPACE; + sleep 10; + done; + env: + - name: VAULT_POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: VAULT_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + restartPolicy: Never +``` + +- Create the pod: `$ kubectl apply -f vault-test.yaml` +- View the full initial state of the pod: `$ kubectl get pod vault -o=yaml > initialstate.txt` +- Drop the Vault binary into the pod: `$ kubectl cp $(which vault) /vault:/` +- Drop to the shell within the pod: `$ kubectl exec -it vault -- /bin/bash` +- Install a text editor: `$ apt-get update`, `$ apt-get install nano` +- Write a test Vault config to `vault.config` like: + +``` +storage "inmem" {} +service_registration "kubernetes" {} +disable_mlock = true +ui = true +api_addr = "http://127.0.0.1:8200" +log_level = "debug" +``` + +- Run Vault: `$ ./vault server -config=vault.config -dev -dev-root-token-id=root` +- If 403's are received, you may need to grant RBAC, example here: https://github.com/fabric8io/fabric8/issues/6840#issuecomment-307560275 +- In a separate window outside the pod, view the resulting state of the pod: `$ kubectl get pod vault -o=yaml > currentstate.txt` +- View the differences: `$ diff initialstate.txt currentstate.txt` diff --git a/serviceregistration/kubernetes/testing/ca.crt b/serviceregistration/kubernetes/testing/ca.crt new file mode 100644 index 000000000000..077f48b86308 --- /dev/null +++ b/serviceregistration/kubernetes/testing/ca.crt @@ -0,0 +1,18 @@ +-----BEGIN CERTIFICATE----- +MIIC5zCCAc+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwptaW5p +a3ViZUNBMB4XDTE5MTIxMDIzMDUxOVoXDTI5MTIwODIzMDUxOVowFTETMBEGA1UE +AxMKbWluaWt1YmVDQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANFi +/RIdMHd865X6JygTb9riX01DA3QnR+RoXDXNnj8D3LziLG2n8ItXMJvWbU3sxxyy +nX9HxJ0SIeexj1cYzdQBtJDjO1/PeuKc4CZ7zCukCAtHz8mC7BDPOU7F7pggpcQ0 +/t/pa2m22hmCu8aDF9WlUYHtJpYATnI/A5vz/VFLR9daxmkl59Qo3oHITj7vAzSx +/75r9cibpQyJ+FhiHOZHQWYY2JYw2g4v5hm5hg5SFM9yFcZ75ISI9ebyFFIl9iBY +zAk9jqv1mXvLr0Q39AVwMTamvGuap1oocjM9NIhQvaFL/DNqF1ouDQjCf5u2imLc +TraO1/2KO8fqwOZCOrMCAwEAAaNCMEAwDgYDVR0PAQH/BAQDAgKkMB0GA1UdJQQW +MBQGCCsGAQUFBwMCBggrBgEFBQcDATAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3 +DQEBCwUAA4IBAQBtVZCwCPqUUUpIClAlE9nc2fo2bTs9gsjXRmqdQ5oaSomSLE93 +aJWYFuAhxPXtlApbLYZfW2m1sM3mTVQN60y0uE4e1jdSN1ErYQ9slJdYDAMaEmOh +iSexj+Nd1scUiMHV9lf3ps5J8sYeCpwZX3sPmw7lqZojTS12pANBDcigsaj5RRyN +9GyP3WkSQUsTpWlDb9Fd+KNdkCVw7nClIpBPA2KW4BQKw/rNSvOFD61mbzc89lo0 +Q9IFGQFFF8jO18lbyWqnRBGXcS4/G7jQ3S7C121d14YLUeAYOM7pJykI1g4CLx9y +vitin0L6nprauWkKO38XgM4T75qKZpqtiOcT +-----END CERTIFICATE----- diff --git a/serviceregistration/kubernetes/testing/resp-get-pod.json b/serviceregistration/kubernetes/testing/resp-get-pod.json new file mode 100644 index 000000000000..229eb1fbd3b2 --- /dev/null +++ b/serviceregistration/kubernetes/testing/resp-get-pod.json @@ -0,0 +1,120 @@ +{ + "kind": "Pod", + "apiVersion": "v1", + "metadata": { + "name": "shell-demo", + "labels": {"fizz": "buzz"}, + "namespace": "default", + "selfLink": "/api/v1/namespaces/default/pods/shell-demo", + "uid": "7ecb93ff-aa64-426d-b330-2c0b2c0957a2", + "resourceVersion": "87798", + "creationTimestamp": "2020-01-10T19:22:40Z", + "annotations": { + "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":{\"annotations\":{},\"name\":\"shell-demo\",\"namespace\":\"default\"},\"spec\":{\"containers\":[{\"image\":\"nginx\",\"name\":\"nginx\",\"volumeMounts\":[{\"mountPath\":\"/usr/share/nginx/html\",\"name\":\"shared-data\"}]}],\"dnsPolicy\":\"Default\",\"hostNetwork\":true,\"volumes\":[{\"emptyDir\":{},\"name\":\"shared-data\"}]}}\n" + } + }, + "spec": { + "volumes": [{ + "name": "shared-data", + "emptyDir": {} + }, { + "name": "default-token-5fjt9", + "secret": { + "secretName": "default-token-5fjt9", + "defaultMode": 420 + } + }], + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": {}, + "volumeMounts": [{ + "name": "shared-data", + "mountPath": "/usr/share/nginx/html" + }, { + "name": "default-token-5fjt9", + "readOnly": true, + "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount" + }], + "terminationMessagePath": "/dev/termination-log", + "terminationMessagePolicy": "File", + "imagePullPolicy": "Always" + }], + "restartPolicy": "Always", + "terminationGracePeriodSeconds": 30, + "dnsPolicy": "Default", + "serviceAccountName": "default", + "serviceAccount": "default", + "nodeName": "minikube", + "hostNetwork": true, + "securityContext": {}, + "schedulerName": "default-scheduler", + "tolerations": [{ + "key": "node.kubernetes.io/not-ready", + "operator": "Exists", + "effect": "NoExecute", + "tolerationSeconds": 300 + }, { + "key": "node.kubernetes.io/unreachable", + "operator": "Exists", + "effect": "NoExecute", + "tolerationSeconds": 300 + }], + "priority": 0, + "enableServiceLinks": true + }, + "status": { + "phase": "Running", + "conditions": [{ + "type": "Initialized", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T19:22:40Z" + }, { + "type": "Ready", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T20:20:55Z" + }, { + "type": "ContainersReady", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T20:20:55Z" + }, { + "type": "PodScheduled", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T19:22:40Z" + }], + "hostIP": "192.168.99.100", + "podIP": "192.168.99.100", + "podIPs": [{ + "ip": "192.168.99.100" + }], + "startTime": "2020-01-10T19:22:40Z", + "containerStatuses": [{ + "name": "nginx", + "state": { + "running": { + "startedAt": "2020-01-10T20:20:55Z" + } + }, + "lastState": { + "terminated": { + "exitCode": 0, + "reason": "Completed", + "startedAt": "2020-01-10T19:22:53Z", + "finishedAt": "2020-01-10T20:12:03Z", + "containerID": "docker://ed8bc068cd313ea5adb72780e8015ab09ecb61ea077e39304b4a3fe581f471c4" + } + }, + "ready": true, + "restartCount": 1, + "image": "nginx:latest", + "imageID": "docker-pullable://nginx@sha256:8aa7f6a9585d908a63e5e418dc5d14ae7467d2e36e1ab4f0d8f9d059a3d071ce", + "containerID": "docker://a8ee34466791bc6f082f271f40cdfc43625cea81831b1029b1e90b4f6949f6df", + "started": true + }], + "qosClass": "BestEffort" + } +} diff --git a/serviceregistration/kubernetes/testing/resp-not-found.json b/serviceregistration/kubernetes/testing/resp-not-found.json new file mode 100644 index 000000000000..800a9624f04f --- /dev/null +++ b/serviceregistration/kubernetes/testing/resp-not-found.json @@ -0,0 +1,13 @@ +{ + "kind": "Status", + "apiVersion": "v1", + "metadata": {}, + "status": "Failure", + "message": "pods \"shell-dem\" not found", + "reason": "NotFound", + "details": { + "name": "shell-dem", + "kind": "pods" + }, + "code": 404 +} diff --git a/serviceregistration/kubernetes/testing/resp-update-pod.json b/serviceregistration/kubernetes/testing/resp-update-pod.json new file mode 100644 index 000000000000..e808691f92f5 --- /dev/null +++ b/serviceregistration/kubernetes/testing/resp-update-pod.json @@ -0,0 +1,123 @@ +{ + "kind": "Pod", + "apiVersion": "v1", + "metadata": { + "name": "shell-demo", + "namespace": "default", + "selfLink": "/api/v1/namespaces/default/pods/shell-demo", + "uid": "7ecb93ff-aa64-426d-b330-2c0b2c0957a2", + "resourceVersion": "96433", + "creationTimestamp": "2020-01-10T19:22:40Z", + "labels": { + "fizz": "buzz", + "foo": "bar" + }, + "annotations": { + "kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"v1\",\"kind\":\"Pod\",\"metadata\":{\"annotations\":{},\"name\":\"shell-demo\",\"namespace\":\"default\"},\"spec\":{\"containers\":[{\"image\":\"nginx\",\"name\":\"nginx\",\"volumeMounts\":[{\"mountPath\":\"/usr/share/nginx/html\",\"name\":\"shared-data\"}]}],\"dnsPolicy\":\"Default\",\"hostNetwork\":true,\"volumes\":[{\"emptyDir\":{},\"name\":\"shared-data\"}]}}\n" + } + }, + "spec": { + "volumes": [{ + "name": "shared-data", + "emptyDir": {} + }, { + "name": "default-token-5fjt9", + "secret": { + "secretName": "default-token-5fjt9", + "defaultMode": 420 + } + }], + "containers": [{ + "name": "nginx", + "image": "nginx", + "resources": {}, + "volumeMounts": [{ + "name": "shared-data", + "mountPath": "/usr/share/nginx/html" + }, { + "name": "default-token-5fjt9", + "readOnly": true, + "mountPath": "/var/run/secrets/kubernetes.io/serviceaccount" + }], + "terminationMessagePath": "/dev/termination-log", + "terminationMessagePolicy": "File", + "imagePullPolicy": "Always" + }], + "restartPolicy": "Always", + "terminationGracePeriodSeconds": 30, + "dnsPolicy": "Default", + "serviceAccountName": "default", + "serviceAccount": "default", + "nodeName": "minikube", + "hostNetwork": true, + "securityContext": {}, + "schedulerName": "default-scheduler", + "tolerations": [{ + "key": "node.kubernetes.io/not-ready", + "operator": "Exists", + "effect": "NoExecute", + "tolerationSeconds": 300 + }, { + "key": "node.kubernetes.io/unreachable", + "operator": "Exists", + "effect": "NoExecute", + "tolerationSeconds": 300 + }], + "priority": 0, + "enableServiceLinks": true + }, + "status": { + "phase": "Running", + "conditions": [{ + "type": "Initialized", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T19:22:40Z" + }, { + "type": "Ready", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T20:20:55Z" + }, { + "type": "ContainersReady", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T20:20:55Z" + }, { + "type": "PodScheduled", + "status": "True", + "lastProbeTime": null, + "lastTransitionTime": "2020-01-10T19:22:40Z" + }], + "hostIP": "192.168.99.100", + "podIP": "192.168.99.100", + "podIPs": [{ + "ip": "192.168.99.100" + }], + "startTime": "2020-01-10T19:22:40Z", + "containerStatuses": [{ + "name": "nginx", + "state": { + "running": { + "startedAt": "2020-01-10T20:20:55Z" + } + }, + "lastState": { + "terminated": { + "exitCode": 0, + "reason": "Completed", + "startedAt": "2020-01-10T19:22:53Z", + "finishedAt": "2020-01-10T20:12:03Z", + "containerID": "docker://ed8bc068cd313ea5adb72780e8015ab09ecb61ea077e39304b4a3fe581f471c4" + } + }, + "ready": true, + "restartCount": 1, + "image": "nginx:latest", + "imageID": "docker-pullable://nginx@sha256:8aa7f6a9585d908a63e5e418dc5d14ae7467d2e36e1ab4f0d8f9d059a3d071ce", + "containerID": "docker://a8ee34466791bc6f082f271f40cdfc43625cea81831b1029b1e90b4f6949f6df", + "started": true + }], + "qosClass": "BestEffort" + } +} diff --git a/serviceregistration/kubernetes/testing/testserver.go b/serviceregistration/kubernetes/testing/testserver.go new file mode 100644 index 000000000000..d22af130e7a2 --- /dev/null +++ b/serviceregistration/kubernetes/testing/testserver.go @@ -0,0 +1,233 @@ +package testing + +import ( + "encoding/json" + "fmt" + "io/ioutil" + "net/http" + "net/http/httptest" + "os" + "path" + "strings" + "sync" + "testing" +) + +const ( + ExpectedNamespace = "default" + ExpectedPodName = "shell-demo" + + // File names of samples pulled from real life. + caCrtFile = "ca.crt" + respGetPod = "resp-get-pod.json" + respNotFound = "resp-not-found.json" + respUpdatePod = "resp-update-pod.json" + tokenFile = "token" +) + +var pathToFiles = func() string { + wd, _ := os.Getwd() + pathParts := strings.Split(wd, "vault") + return pathParts[0] + "vault/serviceregistration/kubernetes/testing/" +}() + +// Conf returns the info needed to configure the client to point at +// the test server. This must be done by the caller to avoid an import +// cycle between the client and the testserver. Example usage: +// +// client.Scheme = testConf.ClientScheme +// client.TokenFile = testConf.PathToTokenFile +// client.RootCAFile = testConf.PathToRootCAFile +// if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { +// t.Fatal(err) +// } +// if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { +// t.Fatal(err) +// } +type Conf struct { + ClientScheme, PathToTokenFile, PathToRootCAFile, ServiceHost, ServicePort string +} + +// Server returns an http test server that can be used to test +// Kubernetes client code. It returns its current patches as a map +// so the caller can check current state. Calling the closeFunc +// at the end closes the test server. Responses are provided using +// real responses that have been captured from the Kube API. +// testState is a map[string]map[string]interface{}. +func Server(t *testing.T) (testState *State, testConf *Conf, closeFunc func()) { + testState = &State{m: &sync.Map{}} + testConf = &Conf{ + ClientScheme: "http://", + } + + // We're going to have multiple close funcs to call. + var closers []func() + closeFunc = func() { + for _, closer := range closers { + closer() + } + } + + // Read in our sample files. + token, err := readFile(tokenFile) + if err != nil { + t.Fatal(err) + } + caCrt, err := readFile(caCrtFile) + if err != nil { + t.Fatal(err) + } + notFoundResponse, err := readFile(respNotFound) + if err != nil { + t.Fatal(err) + } + getPodResponse, err := readFile(respGetPod) + if err != nil { + t.Fatal(err) + } + updatePodTagsResponse, err := readFile(respUpdatePod) + if err != nil { + t.Fatal(err) + } + + // Plant our token in a place where it can be read for the config. + tmpToken, err := ioutil.TempFile("", "token") + if err != nil { + t.Fatal(err) + } + closers = append(closers, func() { + os.Remove(tmpToken.Name()) + }) + if _, err = tmpToken.WriteString(token); err != nil { + closeFunc() + t.Fatal(err) + } + if err := tmpToken.Close(); err != nil { + closeFunc() + t.Fatal(err) + } + testConf.PathToTokenFile = tmpToken.Name() + + tmpCACrt, err := ioutil.TempFile("", "ca.crt") + if err != nil { + closeFunc() + t.Fatal(err) + } + closers = append(closers, func() { + os.Remove(tmpCACrt.Name()) + }) + if _, err = tmpCACrt.WriteString(caCrt); err != nil { + closeFunc() + t.Fatal(err) + } + if err := tmpCACrt.Close(); err != nil { + closeFunc() + t.Fatal(err) + } + testConf.PathToRootCAFile = tmpCACrt.Name() + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + namespace, podName, err := parsePath(r.URL.Path) + if err != nil { + w.WriteHeader(400) + w.Write([]byte(fmt.Sprintf("unable to parse %s: %s", r.URL.Path, err.Error()))) + return + } + + switch { + case namespace != ExpectedNamespace, podName != ExpectedPodName: + w.WriteHeader(404) + w.Write([]byte(notFoundResponse)) + return + case r.Method == http.MethodGet: + w.WriteHeader(200) + w.Write([]byte(getPodResponse)) + return + case r.Method == http.MethodPatch: + var patches []interface{} + if err := json.NewDecoder(r.Body).Decode(&patches); err != nil { + w.WriteHeader(400) + w.Write([]byte(fmt.Sprintf("unable to decode patches %s: %s", r.URL.Path, err.Error()))) + return + } + for _, patch := range patches { + patchMap := patch.(map[string]interface{}) + p := patchMap["path"].(string) + testState.store(p, patchMap) + } + w.WriteHeader(200) + w.Write([]byte(updatePodTagsResponse)) + return + default: + w.WriteHeader(400) + w.Write([]byte(fmt.Sprintf("unexpected request method: %s", r.Method))) + } + })) + closers = append(closers, ts.Close) + + // ts.URL example: http://127.0.0.1:35681 + urlFields := strings.Split(ts.URL, "://") + if len(urlFields) != 2 { + closeFunc() + t.Fatal("received unexpected test url: " + ts.URL) + } + urlFields = strings.Split(urlFields[1], ":") + if len(urlFields) != 2 { + closeFunc() + t.Fatal("received unexpected test url: " + ts.URL) + } + testConf.ServiceHost = urlFields[0] + testConf.ServicePort = urlFields[1] + return testState, testConf, closeFunc +} + +type State struct { + m *sync.Map +} + +func (s *State) NumPatches() int { + l := 0 + f := func(key, value interface{}) bool { + l++ + return true + } + s.m.Range(f) + return l +} + +func (s *State) Get(key string) map[string]interface{} { + v, ok := s.m.Load(key) + if !ok { + return nil + } + patch, ok := v.(map[string]interface{}) + if !ok { + return nil + } + return patch +} + +func (s *State) store(k string, p map[string]interface{}) { + s.m.Store(k, p) +} + +// The path should be formatted like this: +// fmt.Sprintf("/api/v1/namespaces/%s/pods/%s", namespace, podName) +func parsePath(urlPath string) (namespace, podName string, err error) { + original := urlPath + podName = path.Base(urlPath) + urlPath = strings.TrimSuffix(urlPath, "/pods/"+podName) + namespace = path.Base(urlPath) + if original != fmt.Sprintf("/api/v1/namespaces/%s/pods/%s", namespace, podName) { + return "", "", fmt.Errorf("received unexpected path: %s", original) + } + return namespace, podName, nil +} + +func readFile(fileName string) (string, error) { + b, err := ioutil.ReadFile(pathToFiles + fileName) + if err != nil { + return "", err + } + return string(b), nil +} diff --git a/serviceregistration/kubernetes/testing/token b/serviceregistration/kubernetes/testing/token new file mode 100644 index 000000000000..42d4949f112f --- /dev/null +++ b/serviceregistration/kubernetes/testing/token @@ -0,0 +1 @@ +eyJhbGciOiJSUzI1NiIsImtpZCI6IjZVQU91ckJYcTZKRHQtWHpaOExib2EyUlFZQWZObms2d25mY3ZtVm1NNUUifQ.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6ImRlZmF1bHQtdG9rZW4tNWZqdDkiLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoiZGVmYXVsdCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6ImY0NGUyMDIxLTU2YWItNDEzNC1hMjMxLTBlMDJmNjhmNzJhNiIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZWZhdWx0OmRlZmF1bHQifQ.hgMbuT0hlxG04fDvI_Iyxtbwc8M-i3q3K7CqIGC_jYSjVlyezHN_0BeIB3rE0_M2xvbIs6chsWFZVsK_8Pj6ho7VT0x5PWy5n6KsqTBz8LPpjWpsaxpYQos0RzgA3KLnuzZE8Cl-v-PwWQK57jgbS4AdlXujQXdtLXJNwNAKI0pvCASA6UXP55_X845EsJkyT1J-bURSS3Le3g9A4pDoQ_MUv7hqa-p7yQEtFfYCkq1KKrUJZMRjmS4qda1rg-Em-dw9RFvQtPodRYF0DKT7A7qgmLUfIkuky3NnsQtvaUo8ZVtUiwIEfRdqw1oQIY4CSYz-wUl2xZa7n2QQBROE7w \ No newline at end of file diff --git a/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go b/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go index 4a35f88dca5e..d53aa1cae209 100644 --- a/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go +++ b/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "io/ioutil" "math/big" "net" "net/url" @@ -804,3 +805,60 @@ func SignCertificate(data *CreationBundle) (*ParsedCertBundle, error) { return result, nil } + +func NewCertPool(pathToFile string) (*x509.CertPool, error) { + certs, err := certsFromFile(pathToFile) + if err != nil { + return nil, err + } + pool := x509.NewCertPool() + for _, cert := range certs { + pool.AddCert(cert) + } + return pool, nil +} + +// CertsFromFile returns the x509.Certificates contained in the given PEM-encoded file. +// Returns an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates +func certsFromFile(file string) ([]*x509.Certificate, error) { + pemBlock, err := ioutil.ReadFile(file) + if err != nil { + return nil, err + } + certs, err := parseCertsPEM(pemBlock) + if err != nil { + return nil, fmt.Errorf("error reading %s: %s", file, err) + } + return certs, nil +} + +// parseCertsPEM returns the x509.Certificates contained in the given PEM-encoded byte array +// Returns an error if a certificate could not be parsed, or if the data does not contain any certificates +func parseCertsPEM(pemCerts []byte) ([]*x509.Certificate, error) { + ok := false + certs := []*x509.Certificate{} + for len(pemCerts) > 0 { + var block *pem.Block + block, pemCerts = pem.Decode(pemCerts) + if block == nil { + break + } + // Only use PEM "CERTIFICATE" blocks without extra headers + if block.Type != "CERTIFICATE" || len(block.Headers) != 0 { + continue + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return certs, err + } + + certs = append(certs, cert) + ok = true + } + + if !ok { + return certs, errors.New("data does not contain any valid RSA or ECDSA certificates") + } + return certs, nil +} diff --git a/vendor/github.com/hashicorp/vault/sdk/helper/tlsutil/tlsutil.go b/vendor/github.com/hashicorp/vault/sdk/helper/tlsutil/tlsutil.go index 918d4d2bc335..a17e10cda8b1 100644 --- a/vendor/github.com/hashicorp/vault/sdk/helper/tlsutil/tlsutil.go +++ b/vendor/github.com/hashicorp/vault/sdk/helper/tlsutil/tlsutil.go @@ -127,9 +127,9 @@ func SetupTLSConfig(conf map[string]string, address string) (*tls.Config, error) } insecureSkipVerify := false - tlsSkipVerify, ok := conf["tls_skip_verify"] + tlsSkipVerify := conf["tls_skip_verify"] - if ok && tlsSkipVerify != "" { + if tlsSkipVerify != "" { b, err := parseutil.ParseBool(tlsSkipVerify) if err != nil { return nil, errwrap.Wrapf("failed parsing tls_skip_verify parameter: {{err}}", err) From 4dc0d007c6af52fbf4d059fb9bb8b7464f1833cf Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Fri, 31 Jan 2020 15:53:37 -0800 Subject: [PATCH 02/27] rename env vars --- serviceregistration/kubernetes/client/config.go | 4 ++-- serviceregistration/kubernetes/testing/testserver.go | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/serviceregistration/kubernetes/client/config.go b/serviceregistration/kubernetes/client/config.go index 22b1226d6182..d2eb79404d93 100644 --- a/serviceregistration/kubernetes/client/config.go +++ b/serviceregistration/kubernetes/client/config.go @@ -14,8 +14,8 @@ const ( // Vault may read them in if set through these environment variables. // Example here: // https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/ - EnvVarKubernetesNamespace = "VAULT_NAMESPACE" - EnvVarKubernetesPodName = "VAULT_POD_NAME" + EnvVarKubernetesNamespace = "VAULT_K8S_NAMESPACE" + EnvVarKubernetesPodName = "VAULT_K8S_POD_NAME" // The service host and port environment variables are // set by default inside a Kubernetes environment. diff --git a/serviceregistration/kubernetes/testing/testserver.go b/serviceregistration/kubernetes/testing/testserver.go index d22af130e7a2..437c33a95847 100644 --- a/serviceregistration/kubernetes/testing/testserver.go +++ b/serviceregistration/kubernetes/testing/testserver.go @@ -49,11 +49,9 @@ type Conf struct { } // Server returns an http test server that can be used to test -// Kubernetes client code. It returns its current patches as a map -// so the caller can check current state. Calling the closeFunc -// at the end closes the test server. Responses are provided using -// real responses that have been captured from the Kube API. -// testState is a map[string]map[string]interface{}. +// Kubernetes client code. It also retains the current state, +// and a func to close the server and to clean up any temporary +// files. func Server(t *testing.T) (testState *State, testConf *Conf, closeFunc func()) { testState = &State{m: &sync.Map{}} testConf = &Conf{ From e75fe3b5a0e18f954173bd37242a5e5a7c92c586 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 3 Feb 2020 13:41:36 -0800 Subject: [PATCH 03/27] increase commentary around env vars --- serviceregistration/kubernetes/client/config.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/serviceregistration/kubernetes/client/config.go b/serviceregistration/kubernetes/client/config.go index d2eb79404d93..52ccaa4286ee 100644 --- a/serviceregistration/kubernetes/client/config.go +++ b/serviceregistration/kubernetes/client/config.go @@ -14,6 +14,12 @@ const ( // Vault may read them in if set through these environment variables. // Example here: // https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/ + // The client itself does nothing directly with these variables, it's + // up to the caller. However, they live here so they'll be consistently + // named should the client ever be reused. + // We generally recommend preferring environmental settings over configured + // ones, allowing settings from the Downward API to override hard-coded + // ones. EnvVarKubernetesNamespace = "VAULT_K8S_NAMESPACE" EnvVarKubernetesPodName = "VAULT_K8S_POD_NAME" From c211b87183fb94eec61b37f8c486202ef355ce52 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 4 Feb 2020 15:30:15 -0800 Subject: [PATCH 04/27] improve err and comments --- serviceregistration/kubernetes/client/client.go | 14 ++++++++++---- .../kubernetes/client/client_test.go | 8 ++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 1eea1bc03444..a66da2f971a2 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -22,7 +22,6 @@ const maxRetries = 10 var ( ErrNamespaceUnset = errors.New(`"namespace" is unset`) ErrPodNameUnset = errors.New(`"podName" is unset`) - ErrNotFound = errors.New("not found") ErrNotInCluster = errors.New("unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined") ) @@ -194,7 +193,7 @@ func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToRet // Continue to try again, but return the error too in case the caller would rather read it out. return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) case 404: - return false, ErrNotFound + return false, &ErrNotFound{debuggingInfo: sanitizedDebuggingInfo(req, reqBody, resp)} case 500, 502, 503, 504: // Could be transient. return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) @@ -273,9 +272,16 @@ type Patch struct { Value interface{} } +type ErrNotFound struct { + debuggingInfo string +} + +func (e *ErrNotFound) Error() string { + return e.debuggingInfo +} + // sanitizedDebuggingInfo converts an http response to a string without -// including its headers to avoid leaking authorization -// headers. +// including its headers to avoid leaking authorization headers. func sanitizedDebuggingInfo(req *http.Request, reqBody []byte, resp *http.Response) string { respBody, _ := ioutil.ReadAll(resp.Body) return fmt.Sprintf("req method: %s, req url: %s, req body: %s, resp statuscode: %d, resp respBody: %s", req.Method, req.URL, reqBody, resp.StatusCode, respBody) diff --git a/serviceregistration/kubernetes/client/client_test.go b/serviceregistration/kubernetes/client/client_test.go index c36b5367bbb7..877cac72504d 100644 --- a/serviceregistration/kubernetes/client/client_test.go +++ b/serviceregistration/kubernetes/client/client_test.go @@ -56,8 +56,8 @@ func (e *env) TestGetPodNotFound(t *testing.T) { if err == nil { t.Fatal("expected error because pod is unfound") } - if err != ErrNotFound { - t.Fatalf("expected %q but received %q", ErrNotFound, err) + if _, ok := err.(*ErrNotFound); !ok { + t.Fatalf("expected *ErrNotFound but received %T", err) } } @@ -86,7 +86,7 @@ func (e *env) TestUpdatePodTagsNotFound(t *testing.T) { if err == nil { t.Fatal("expected error because pod is unfound") } - if err != ErrNotFound { - t.Fatalf("expected %q but received %q", ErrNotFound, err) + if _, ok := err.(*ErrNotFound); !ok { + t.Fatalf("expected *ErrNotFound but received %T", err) } } From 708306b034b85b280fa408b87881452e5f39cc63 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 4 Feb 2020 15:34:28 -0800 Subject: [PATCH 05/27] strip onShutdown method --- .../kubernetes/service_registration.go | 43 ------------------- 1 file changed, 43 deletions(-) diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go index dfb2ab931e0c..a957f5555cdc 100644 --- a/serviceregistration/kubernetes/service_registration.go +++ b/serviceregistration/kubernetes/service_registration.go @@ -126,10 +126,6 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro // Run a service that retries errored-out notifications if they occur. go r.retryHandler.Run(shutdownCh, wait) - // Run a background goroutine to leave labels in the final state we'd like - // when Vault shuts down. - go r.onShutdown(shutdownCh, wait) - return nil } @@ -165,45 +161,6 @@ func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) e }) } -func (r *serviceRegistration) onShutdown(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { - // Make sure Vault will give us time to finish up here. - wait.Add(1) - defer wait.Done() - - // Start running this when we receive a shutdown. - <-shutdownCh - - // Label the pod with the values we want to leave behind after shutdown. - patches := []*client.Patch{ - { - Operation: client.Replace, - Path: pathToLabels + labelActive, - Value: toString(false), - }, - { - Operation: client.Replace, - Path: pathToLabels + labelSealed, - Value: toString(true), - }, - { - Operation: client.Replace, - Path: pathToLabels + labelPerfStandby, - Value: toString(false), - }, - { - Operation: client.Replace, - Path: pathToLabels + labelInitialized, - Value: toString(false), - }, - } - if err := r.client.PatchPod(r.namespace, r.podName, patches...); err != nil { - if r.logger.IsError() { - r.logger.Error(fmt.Sprintf("unable to set final status on pod name %q in namespace %q on shutdown: %s", r.podName, r.namespace, err)) - } - return - } -} - func (r *serviceRegistration) notifyOrRetry(patch *client.Patch) error { if err := r.client.PatchPod(r.namespace, r.podName, patch); err != nil { if r.logger.IsWarn() { From f38381ef5e8f1cabb3768ef1d5d3ff935c5f5ffb Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 10:51:44 -0800 Subject: [PATCH 06/27] use io.Reader in NewCertPool --- sdk/helper/certutil/certutil_test.go | 7 ++++-- sdk/helper/certutil/helpers.go | 23 ++++++------------- .../kubernetes/client/config.go | 7 +++++- 3 files changed, 18 insertions(+), 19 deletions(-) diff --git a/sdk/helper/certutil/certutil_test.go b/sdk/helper/certutil/certutil_test.go index cd8ac639e2fd..8250a056f372 100644 --- a/sdk/helper/certutil/certutil_test.go +++ b/sdk/helper/certutil/certutil_test.go @@ -438,8 +438,11 @@ vitin0L6nprauWkKO38XgM4T75qKZpqtiOcT if err := tmpfile.Close(); err != nil { t.Fatal(err) } - - if _, err := NewCertPool(tmpfile.Name()); err != nil { + fileBody, err := ioutil.ReadFile(tmpfile.Name()) + if err != nil { + t.Fatal(err) + } + if _, err := NewCertPool(bytes.NewReader(fileBody)); err != nil { t.Fatal(err) } } diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index d53aa1cae209..3bf6ae502b67 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "io" "io/ioutil" "math/big" "net" @@ -806,11 +807,15 @@ func SignCertificate(data *CreationBundle) (*ParsedCertBundle, error) { return result, nil } -func NewCertPool(pathToFile string) (*x509.CertPool, error) { - certs, err := certsFromFile(pathToFile) +func NewCertPool(reader io.Reader) (*x509.CertPool, error) { + pemBlock, err := ioutil.ReadAll(reader) if err != nil { return nil, err } + certs, err := parseCertsPEM(pemBlock) + if err != nil { + return nil, fmt.Errorf("error reading certs: %s", err) + } pool := x509.NewCertPool() for _, cert := range certs { pool.AddCert(cert) @@ -818,20 +823,6 @@ func NewCertPool(pathToFile string) (*x509.CertPool, error) { return pool, nil } -// CertsFromFile returns the x509.Certificates contained in the given PEM-encoded file. -// Returns an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates -func certsFromFile(file string) ([]*x509.Certificate, error) { - pemBlock, err := ioutil.ReadFile(file) - if err != nil { - return nil, err - } - certs, err := parseCertsPEM(pemBlock) - if err != nil { - return nil, fmt.Errorf("error reading %s: %s", file, err) - } - return certs, nil -} - // parseCertsPEM returns the x509.Certificates contained in the given PEM-encoded byte array // Returns an error if a certificate could not be parsed, or if the data does not contain any certificates func parseCertsPEM(pemCerts []byte) ([]*x509.Certificate, error) { diff --git a/serviceregistration/kubernetes/client/config.go b/serviceregistration/kubernetes/client/config.go index 52ccaa4286ee..a61b679f1f25 100644 --- a/serviceregistration/kubernetes/client/config.go +++ b/serviceregistration/kubernetes/client/config.go @@ -1,6 +1,7 @@ package client import ( + "bytes" "crypto/x509" "io/ioutil" "net" @@ -53,7 +54,11 @@ func inClusterConfig() (*Config, error) { return nil, err } - pool, err := certutil.NewCertPool(RootCAFile) + caBytes, err := ioutil.ReadFile(RootCAFile) + if err != nil { + return nil, err + } + pool, err := certutil.NewCertPool(bytes.NewReader(caBytes)) if err != nil { return nil, err } From c116c5b57f5b17c0087aea3190d6187a6148eda0 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 11:08:52 -0800 Subject: [PATCH 07/27] minor changes from feedback --- .../kubernetes/client/client.go | 8 ++++-- .../kubernetes/client/config.go | 26 ++++++++++++++++--- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index a66da2f971a2..206cd2589979 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -39,7 +39,11 @@ func New(logger hclog.Logger, stopCh <-chan struct{}) (*Client, error) { }, nil } -// Client is a minimal Kubernetes client. +// Client is a minimal Kubernetes client. We rolled our own because the existing +// Kubernetes client-go library available externally has a high number of dependencies +// and we thought it wasn't worth it for only two API calls. If at some point they break +// the client into smaller modules, or if we add quite a few methods to this client, it may +// be worthwhile to revisit that decision. type Client struct { logger hclog.Logger config *Config @@ -177,7 +181,7 @@ func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToRet // Check for success. switch resp.StatusCode { - case 200, 201, 202: + case 200, 201, 202, 204: // Pass. case 401, 403: // Perhaps the token from our bearer token file has been refreshed. diff --git a/serviceregistration/kubernetes/client/config.go b/serviceregistration/kubernetes/client/config.go index a61b679f1f25..4e6a0f45848c 100644 --- a/serviceregistration/kubernetes/client/config.go +++ b/serviceregistration/kubernetes/client/config.go @@ -43,6 +43,8 @@ var ( // kubernetes gives to services. It's intended for clients that expect to be // running inside a service running on kubernetes. It will return ErrNotInCluster // if called from a process not running in a kubernetes environment. +// inClusterConfig is based on this: +// https://github.com/kubernetes/client-go/blob/a56922badea0f2a91771411eaa1173c9e9243908/rest/config.go#L451 func inClusterConfig() (*Config, error) { host, port := os.Getenv(EnvVarKubernetesServiceHost), os.Getenv(EnvVarKubernetesServicePort) if len(host) == 0 || len(port) == 0 { @@ -70,9 +72,27 @@ func inClusterConfig() (*Config, error) { }, nil } +// This config is based on the one returned here: +// https://github.com/kubernetes/client-go/blob/a56922badea0f2a91771411eaa1173c9e9243908/rest/config.go#L451 +// It is pared down to the absolute minimum fields used by this code. +// The CACertPool is promoted to the top level from being originally on the TLSClientConfig +// because it is the only parameter of the TLSClientConfig used by this code. +// Also, it made more sense to simply reuse the pool rather than holding raw values +// and parsing it repeatedly. type Config struct { - Host string - BearerToken string + CACertPool *x509.CertPool + + // Host must be a host string, a host:port pair, or a URL to the base of the apiserver. + // If a URL is given then the (optional) Path of that URL represents a prefix that must + // be appended to all request URIs used to access the apiserver. This allows a frontend + // proxy to easily relocate all of the apiserver endpoints. + Host string + + // Server requires Bearer authentication. This client will not attempt to use + // refresh tokens for an OAuth2 flow. + BearerToken string + + // Path to a file containing a BearerToken. + // If set, checks for a new token in the case of authorization errors. BearerTokenFile string - CACertPool *x509.CertPool } From 38723df4c63f2ff82722012ee36f22f2717bc5f3 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 11:14:02 -0800 Subject: [PATCH 08/27] Update serviceregistration/kubernetes/client/client.go Co-Authored-By: Jim Kalafut --- serviceregistration/kubernetes/client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 206cd2589979..9ae5a9158692 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -93,7 +93,7 @@ func (c *Client) PatchPod(namespace, podName string, patches ...*Patch) error { return nil } - var jsonPatches []interface{} + var jsonPatches []map[string]interface{} for _, patch := range patches { if patch.Operation == Unset { return errors.New("patch operation must be set") From 27ec6f40225328dbc9e86da8f214162f002451b8 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 11:17:29 -0800 Subject: [PATCH 09/27] Update serviceregistration/kubernetes/retry_handler.go Co-Authored-By: Jim Kalafut --- serviceregistration/kubernetes/retry_handler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go index 5eb835d200a7..778b0bad0590 100644 --- a/serviceregistration/kubernetes/retry_handler.go +++ b/serviceregistration/kubernetes/retry_handler.go @@ -108,5 +108,5 @@ func (r *retryHandler) retry() { } return } - r.patchesToRetry = make([]*client.Patch, 0) + r.patchesToRetry = nil } From 84cd8ece1a2083c3f214cb3e9f49efa20e7432bd Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 11:19:08 -0800 Subject: [PATCH 10/27] leave patchesToRetry nil --- .../kubernetes/retry_handler_test.go | 25 ++++++++----------- .../kubernetes/service_registration.go | 7 +++--- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/serviceregistration/kubernetes/retry_handler_test.go b/serviceregistration/kubernetes/retry_handler_test.go index 1be280ae7dcf..3c6f8c9fa609 100644 --- a/serviceregistration/kubernetes/retry_handler_test.go +++ b/serviceregistration/kubernetes/retry_handler_test.go @@ -44,11 +44,10 @@ func TestRetryHandlerSimple(t *testing.T) { } r := &retryHandler{ - logger: logger, - namespace: kubetest.ExpectedNamespace, - podName: kubetest.ExpectedPodName, - client: c, - patchesToRetry: make([]*client.Patch, 0), + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, } go r.Run(shutdownCh, wait) @@ -67,10 +66,9 @@ func TestRetryHandlerSimple(t *testing.T) { func TestRetryHandlerAdd(t *testing.T) { r := &retryHandler{ - logger: hclog.NewNullLogger(), - namespace: "some-namespace", - podName: "some-pod-name", - patchesToRetry: make([]*client.Patch, 0), + logger: hclog.NewNullLogger(), + namespace: "some-namespace", + podName: "some-pod-name", } testPatch1 := &client.Patch{ @@ -207,11 +205,10 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { } r := &retryHandler{ - logger: logger, - namespace: kubetest.ExpectedNamespace, - podName: kubetest.ExpectedPodName, - client: c, - patchesToRetry: make([]*client.Patch, 0), + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, } go r.Run(shutdownCh, wait) diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go index a957f5555cdc..173e6079df2a 100644 --- a/serviceregistration/kubernetes/service_registration.go +++ b/serviceregistration/kubernetes/service_registration.go @@ -37,10 +37,9 @@ func NewServiceRegistration(config map[string]string, logger hclog.Logger, state podName: podName, initialState: state, retryHandler: &retryHandler{ - logger: logger, - namespace: namespace, - podName: podName, - patchesToRetry: make([]*client.Patch, 0), + logger: logger, + namespace: namespace, + podName: podName, }, }, nil } From 74c438ebaad33111ceb5f6e08f34856b529dfab5 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 11:41:41 -0800 Subject: [PATCH 11/27] simplify PatchOperation enum to string --- .../kubernetes/client/client.go | 42 +++---------------- .../kubernetes/client/client_test.go | 5 ++- 2 files changed, 9 insertions(+), 38 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 9ae5a9158692..634e90679ddd 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -99,7 +99,7 @@ func (c *Client) PatchPod(namespace, podName string, patches ...*Patch) error { return errors.New("patch operation must be set") } jsonPatches = append(jsonPatches, map[string]interface{}{ - "op": patch.Operation.String(), + "op": patch.Operation, "path": patch.Path, "value": patch.Value, }) @@ -230,46 +230,14 @@ type Metadata struct { Labels map[string]string `json:"labels,omitempty"` } -type PatchOperation int +type PatchOperation string const ( - // When adding support for more PatchOperations in the future, - // DO NOT alphebetize them because it will change the underlying - // int representing a user's intent. If that's stored anywhere, - // it will cause storage reads to map to the incorrect operation. - Unset PatchOperation = iota - Add - Replace + Unset PatchOperation = "unset" + Add = "add" + Replace = "replace" ) -func Parse(s string) PatchOperation { - switch s { - case "add": - return Add - case "replace": - return Replace - default: - return Unset - } -} - -func (p PatchOperation) String() string { - switch p { - case Unset: - // This is an invalid choice, and will be shown on a patch - // where the PatchOperation is unset. That's because ints - // default to 0, and Unset corresponds to 0. - return "unset" - case Add: - return "add" - case Replace: - return "replace" - default: - // Should never arrive here. - return "" - } -} - type Patch struct { Operation PatchOperation Path string diff --git a/serviceregistration/kubernetes/client/client_test.go b/serviceregistration/kubernetes/client/client_test.go index 877cac72504d..7196baaa8c7e 100644 --- a/serviceregistration/kubernetes/client/client_test.go +++ b/serviceregistration/kubernetes/client/client_test.go @@ -73,7 +73,10 @@ func (e *env) TestUpdatePodTags(t *testing.T) { t.Fatalf("expected 1 label but received %+v", e.testState) } if e.testState.Get("/metadata/labels/fizz")["value"] != "buzz" { - t.Fatalf("expected buzz but received %q", e.testState.Get("fizz")) + t.Fatalf("expected buzz but received %q", e.testState.Get("fizz")["value"]) + } + if e.testState.Get("/metadata/labels/fizz")["op"] != "add" { + t.Fatalf("expected add but received %q", e.testState.Get("fizz")["op"]) } } From b00a0c420174d2557d789ba1e9924aa89e73f51a Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 13:26:27 -0800 Subject: [PATCH 12/27] use retryable client for exp backoff and jitter --- .../kubernetes/client/client.go | 140 +++++++++--------- 1 file changed, 66 insertions(+), 74 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 634e90679ddd..0473d1096c75 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -2,6 +2,7 @@ package client import ( "bytes" + "context" "crypto/tls" "encoding/json" "errors" @@ -10,14 +11,17 @@ import ( "net/http" "time" - "github.com/cenkalti/backoff" "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-retryablehttp" ) -// maxRetries is the maximum number of times the client -// should retry. -const maxRetries = 10 +const ( + // Retry configuration + retryWaitMin = 500 * time.Millisecond + retryWaitMax = 30 * time.Second + retryMax = 10 +) var ( ErrNamespaceUnset = errors.New(`"namespace" is unset`) @@ -118,44 +122,6 @@ func (c *Client) PatchPod(namespace, podName string, patches ...*Patch) error { // do executes the given request, retrying if necessary. func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { - // Finish setting up a valid request. - req.Header.Set("Authorization", "Bearer "+c.config.BearerToken) - req.Header.Set("Accept", "application/json") - client := cleanhttp.DefaultClient() - client.Transport = &http.Transport{ - TLSClientConfig: &tls.Config{ - RootCAs: c.config.CACertPool, - }, - } - - // Execute and retry the request. This exponential backoff comes - // with jitter already rolled in. - var lastErr error - b := backoff.NewExponentialBackOff() - for i := 0; i < maxRetries; i++ { - if i != 0 { - select { - case <-c.stopCh: - return nil - case <-time.NewTimer(b.NextBackOff()).C: - // Continue to the request. - } - } - shouldRetry, err := c.attemptRequest(client, req, ptrToReturnObj) - if !shouldRetry { - // The error may be nil or populated depending on whether the - // request was successful. - return err - } - lastErr = err - } - return lastErr -} - -// attemptRequest tries one single request. It's in its own function so each -// response body can be closed before returning, which would read awkwardly if -// executed in a loop. -func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToReturnObj interface{}) (shouldRetry bool, err error) { // Preserve the original request body so it can be viewed for debugging if needed. // Reading it empties it, so we need to re-add it afterwards. var reqBody []byte @@ -165,9 +131,33 @@ func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToRet req.Body = ioutil.NopCloser(reqBodyReader) } - resp, err := client.Do(req) + // Finish setting up a valid request. + retryableReq, err := retryablehttp.FromRequest(req) if err != nil { - return false, err + return err + } + retryableReq.Header.Set("Authorization", "Bearer "+c.config.BearerToken) + retryableReq.Header.Set("Accept", "application/json") + + client := &retryablehttp.Client{ + HTTPClient: cleanhttp.DefaultClient(), + RetryWaitMin: retryWaitMin, + RetryWaitMax: retryWaitMax, + RetryMax: retryMax, + CheckRetry: c.getCheckRetry(req, reqBody), + Backoff: retryablehttp.DefaultBackoff, + } + client.HTTPClient.Transport = &http.Transport{ + TLSClientConfig: &tls.Config{ + RootCAs: c.config.CACertPool, + }, + } + + // Execute and retry the request. This client comes with exponential backoff and + // jitter already rolled in. + resp, err := client.Do(retryableReq) + if err != nil { + return err } defer func() { if err := resp.Body.Close(); err != nil { @@ -179,42 +169,44 @@ func (c *Client) attemptRequest(client *http.Client, req *http.Request, ptrToRet } }() - // Check for success. - switch resp.StatusCode { - case 200, 201, 202, 204: - // Pass. - case 401, 403: - // Perhaps the token from our bearer token file has been refreshed. - config, err := inClusterConfig() - if err != nil { - return false, err - } - if config.BearerToken == c.config.BearerToken { - // It's the same token. - return false, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) - } - c.config = config - // Continue to try again, but return the error too in case the caller would rather read it out. - return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) - case 404: - return false, &ErrNotFound{debuggingInfo: sanitizedDebuggingInfo(req, reqBody, resp)} - case 500, 502, 503, 504: - // Could be transient. - return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) - default: - // Unexpected. - return false, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) - } - - // We only arrive here with success. // If we're not supposed to read out the body, we have nothing further // to do here. if ptrToReturnObj == nil { - return false, nil + return nil } // Attempt to read out the body into the given return object. - return false, json.NewDecoder(resp.Body).Decode(ptrToReturnObj) + return json.NewDecoder(resp.Body).Decode(ptrToReturnObj) +} + +func (c *Client) getCheckRetry(req *http.Request, reqBody []byte) retryablehttp.CheckRetry { + return func(ctx context.Context, resp *http.Response, err error) (bool, error) { + switch resp.StatusCode { + case 200, 201, 202, 204: + // Success. + return false, nil + case 401, 403: + // Perhaps the token from our bearer token file has been refreshed. + config, err := inClusterConfig() + if err != nil { + return false, err + } + if config.BearerToken == c.config.BearerToken { + // It's the same token. + return false, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + } + c.config = config + // Continue to try again, but return the error too in case the caller would rather read it out. + return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + case 404: + return false, &ErrNotFound{debuggingInfo: sanitizedDebuggingInfo(req, reqBody, resp)} + case 500, 502, 503, 504: + // Could be transient. + return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + } + // Unexpected. + return false, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + } } type Pod struct { From 283ba7f1a443aa1e41ceb26098d9d051b76c0379 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 13:46:54 -0800 Subject: [PATCH 13/27] add ability to respond to application stops --- .../kubernetes/client/client.go | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 0473d1096c75..c1cc39c1d4ef 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -9,6 +9,7 @@ import ( "fmt" "io/ioutil" "net/http" + "sync" "time" "github.com/hashicorp/go-cleanhttp" @@ -136,6 +137,7 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { if err != nil { return err } + retryableReq.WithContext(newContext(c.stopCh)) retryableReq.Header.Set("Authorization", "Bearer "+c.config.BearerToken) retryableReq.Header.Set("Accept", "application/json") @@ -250,3 +252,45 @@ func sanitizedDebuggingInfo(req *http.Request, reqBody []byte, resp *http.Respon respBody, _ := ioutil.ReadAll(resp.Body) return fmt.Sprintf("req method: %s, req url: %s, req body: %s, resp statuscode: %d, resp respBody: %s", req.Method, req.URL, reqBody, resp.StatusCode, respBody) } + +// stopChContext allows us to cause requests to stop retrying if we receive +// a stop from the caller. +func newContext(stopCh <-chan struct{}) context.Context { + ctx := &stopChContext{ + stopCh: stopCh, + } + go ctx.monitorStopCh() + return ctx +} + +type stopChContext struct { + stopCh <-chan struct{} + errLock sync.RWMutex + err error +} + +func (c *stopChContext) Deadline() (deadline time.Time, ok bool) { + // Return false to indicate that no deadline is set. + return time.Time{}, false +} + +func (c *stopChContext) Done() <-chan struct{} { + return c.stopCh +} + +func (c *stopChContext) Err() error { + c.errLock.RLock() + defer c.errLock.RUnlock() + return c.err +} + +func (c *stopChContext) Value(key interface{}) interface{} { + return nil +} + +func (c *stopChContext) monitorStopCh() { + <-c.stopCh + c.errLock.Lock() + defer c.errLock.Unlock() + c.err = errors.New("stop channel has been closed") +} From e92f7168f5c75884ed67d0371498927f411afc8c Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 13:59:41 -0800 Subject: [PATCH 14/27] replace toString with FormatBool --- .../kubernetes/service_registration.go | 30 +++++------- .../kubernetes/service_registration_test.go | 49 ++++++++++--------- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go index 173e6079df2a..3df880442177 100644 --- a/serviceregistration/kubernetes/service_registration.go +++ b/serviceregistration/kubernetes/service_registration.go @@ -3,6 +3,7 @@ package kubernetes import ( "fmt" "os" + "strconv" "sync" "github.com/hashicorp/go-hclog" @@ -80,10 +81,10 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro Path: "/metadata/labels", Value: map[string]string{ labelVaultVersion: r.initialState.VaultVersion, - labelActive: toString(r.initialState.IsActive), - labelSealed: toString(r.initialState.IsSealed), - labelPerfStandby: toString(r.initialState.IsPerformanceStandby), - labelInitialized: toString(r.initialState.IsInitialized), + labelActive: strconv.FormatBool(r.initialState.IsActive), + labelSealed: strconv.FormatBool(r.initialState.IsSealed), + labelPerfStandby: strconv.FormatBool(r.initialState.IsPerformanceStandby), + labelInitialized: strconv.FormatBool(r.initialState.IsInitialized), }, }); err != nil { return err @@ -99,22 +100,22 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro { Operation: client.Replace, Path: pathToLabels + labelActive, - Value: toString(r.initialState.IsActive), + Value: strconv.FormatBool(r.initialState.IsActive), }, { Operation: client.Replace, Path: pathToLabels + labelSealed, - Value: toString(r.initialState.IsSealed), + Value: strconv.FormatBool(r.initialState.IsSealed), }, { Operation: client.Replace, Path: pathToLabels + labelPerfStandby, - Value: toString(r.initialState.IsPerformanceStandby), + Value: strconv.FormatBool(r.initialState.IsPerformanceStandby), }, { Operation: client.Replace, Path: pathToLabels + labelInitialized, - Value: toString(r.initialState.IsInitialized), + Value: strconv.FormatBool(r.initialState.IsInitialized), }, } if err := c.PatchPod(r.namespace, r.podName, patches...); err != nil { @@ -132,7 +133,7 @@ func (r *serviceRegistration) NotifyActiveStateChange(isActive bool) error { return r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelActive, - Value: toString(isActive), + Value: strconv.FormatBool(isActive), }) } @@ -140,7 +141,7 @@ func (r *serviceRegistration) NotifySealedStateChange(isSealed bool) error { return r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelSealed, - Value: toString(isSealed), + Value: strconv.FormatBool(isSealed), }) } @@ -148,7 +149,7 @@ func (r *serviceRegistration) NotifyPerformanceStandbyStateChange(isStandby bool return r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelPerfStandby, - Value: toString(isStandby), + Value: strconv.FormatBool(isStandby), }) } @@ -156,7 +157,7 @@ func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) e return r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelInitialized, - Value: toString(isInitialized), + Value: strconv.FormatBool(isInitialized), }) } @@ -187,8 +188,3 @@ func getRequiredField(logger hclog.Logger, config map[string]string, envVar, con } return value, nil } - -// Converts a bool to "true" or "false". -func toString(b bool) string { - return fmt.Sprintf("%t", b) -} diff --git a/serviceregistration/kubernetes/service_registration_test.go b/serviceregistration/kubernetes/service_registration_test.go index 3994c35b7b12..da6d35b0fd41 100644 --- a/serviceregistration/kubernetes/service_registration_test.go +++ b/serviceregistration/kubernetes/service_registration_test.go @@ -2,6 +2,7 @@ package kubernetes import ( "os" + "strconv" "sync" "testing" @@ -58,72 +59,72 @@ func TestServiceRegistration(t *testing.T) { if testState.Get(pathToLabels + labelVaultVersion)["value"] != testVersion { t.Fatalf("expected %q but received %q", testVersion, testState.Get(pathToLabels + labelVaultVersion)["value"]) } - if testState.Get(pathToLabels + labelActive)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelActive)["value"]) + if testState.Get(pathToLabels + labelActive)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelActive)["value"]) } - if testState.Get(pathToLabels + labelSealed)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelSealed)["value"]) + if testState.Get(pathToLabels + labelSealed)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelSealed)["value"]) } - if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) + if testState.Get(pathToLabels + labelPerfStandby)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) } - if testState.Get(pathToLabels + labelInitialized)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelInitialized)["value"]) + if testState.Get(pathToLabels + labelInitialized)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelInitialized)["value"]) } // Test NotifyActiveStateChange. if err := reg.NotifyActiveStateChange(false); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelActive)["value"] != toString(false) { - t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelActive)["value"]) + if testState.Get(pathToLabels + labelActive)["value"] != strconv.FormatBool(false) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(false), testState.Get(pathToLabels + labelActive)["value"]) } if err := reg.NotifyActiveStateChange(true); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelActive)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelActive)["value"]) + if testState.Get(pathToLabels + labelActive)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelActive)["value"]) } // Test NotifySealedStateChange. if err := reg.NotifySealedStateChange(false); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelSealed)["value"] != toString(false) { - t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelSealed)["value"]) + if testState.Get(pathToLabels + labelSealed)["value"] != strconv.FormatBool(false) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(false), testState.Get(pathToLabels + labelSealed)["value"]) } if err := reg.NotifySealedStateChange(true); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelSealed)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelSealed)["value"]) + if testState.Get(pathToLabels + labelSealed)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelSealed)["value"]) } // Test NotifyPerformanceStandbyStateChange. if err := reg.NotifyPerformanceStandbyStateChange(false); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(false) { - t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelPerfStandby)["value"]) + if testState.Get(pathToLabels + labelPerfStandby)["value"] != strconv.FormatBool(false) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(false), testState.Get(pathToLabels + labelPerfStandby)["value"]) } if err := reg.NotifyPerformanceStandbyStateChange(true); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelPerfStandby)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) + if testState.Get(pathToLabels + labelPerfStandby)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelPerfStandby)["value"]) } // Test NotifyInitializedStateChange. if err := reg.NotifyInitializedStateChange(false); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelInitialized)["value"] != toString(false) { - t.Fatalf("expected %q but received %q", toString(false), testState.Get(pathToLabels + labelInitialized)["value"]) + if testState.Get(pathToLabels + labelInitialized)["value"] != strconv.FormatBool(false) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(false), testState.Get(pathToLabels + labelInitialized)["value"]) } if err := reg.NotifyInitializedStateChange(true); err != nil { t.Fatal(err) } - if testState.Get(pathToLabels + labelInitialized)["value"] != toString(true) { - t.Fatalf("expected %q but received %q", toString(true), testState.Get(pathToLabels + labelInitialized)["value"]) + if testState.Get(pathToLabels + labelInitialized)["value"] != strconv.FormatBool(true) { + t.Fatalf("expected %q but received %q", strconv.FormatBool(true), testState.Get(pathToLabels + labelInitialized)["value"]) } } From 511de335b2e25f6f10b44dc7d33bf846d88d28b3 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 14:01:21 -0800 Subject: [PATCH 15/27] update vendored files --- vendor/github.com/hashicorp/vault/api/go.sum | 1 + .../vault/sdk/helper/certutil/helpers.go | 23 ++++++------------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/vendor/github.com/hashicorp/vault/api/go.sum b/vendor/github.com/hashicorp/vault/api/go.sum index 431b6d658c5a..dce1069176dd 100644 --- a/vendor/github.com/hashicorp/vault/api/go.sum +++ b/vendor/github.com/hashicorp/vault/api/go.sum @@ -72,6 +72,7 @@ github.com/hashicorp/go-hclog v0.9.2/go.mod h1:5CU+agLiy3J7N7QjHK5d05KxGsuXiQLrj github.com/hashicorp/go-hclog v0.10.0/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= github.com/hashicorp/go-hclog v0.10.1 h1:uyt/l0dWjJ879yiAu+T7FG3/6QX+zwm4bQ8P7XsYt3o= github.com/hashicorp/go-hclog v0.10.1/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= +github.com/hashicorp/go-hclog v0.12.0/go.mod h1:whpDNt7SSdeAju8AWKIWsul05p54N/39EeqMAyrmvFQ= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-kms-wrapping v0.0.0-20191129225826-634facde9f88/go.mod h1:Pm+Umb/6Gij6ZG534L7QDyvkauaOQWGb+arj9aFjCE0= github.com/hashicorp/go-multierror v1.0.0 h1:iVjPR7a6H0tWELX5NxNe7bYopibicUzc7uPribsnS6o= diff --git a/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go b/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go index d53aa1cae209..3bf6ae502b67 100644 --- a/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go +++ b/vendor/github.com/hashicorp/vault/sdk/helper/certutil/helpers.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "io" "io/ioutil" "math/big" "net" @@ -806,11 +807,15 @@ func SignCertificate(data *CreationBundle) (*ParsedCertBundle, error) { return result, nil } -func NewCertPool(pathToFile string) (*x509.CertPool, error) { - certs, err := certsFromFile(pathToFile) +func NewCertPool(reader io.Reader) (*x509.CertPool, error) { + pemBlock, err := ioutil.ReadAll(reader) if err != nil { return nil, err } + certs, err := parseCertsPEM(pemBlock) + if err != nil { + return nil, fmt.Errorf("error reading certs: %s", err) + } pool := x509.NewCertPool() for _, cert := range certs { pool.AddCert(cert) @@ -818,20 +823,6 @@ func NewCertPool(pathToFile string) (*x509.CertPool, error) { return pool, nil } -// CertsFromFile returns the x509.Certificates contained in the given PEM-encoded file. -// Returns an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates -func certsFromFile(file string) ([]*x509.Certificate, error) { - pemBlock, err := ioutil.ReadFile(file) - if err != nil { - return nil, err - } - certs, err := parseCertsPEM(pemBlock) - if err != nil { - return nil, fmt.Errorf("error reading %s: %s", file, err) - } - return certs, nil -} - // parseCertsPEM returns the x509.Certificates contained in the given PEM-encoded byte array // Returns an error if a certificate could not be parsed, or if the data does not contain any certificates func parseCertsPEM(pemCerts []byte) ([]*x509.Certificate, error) { From 03573a8cd1579050a9f9e9ec39e1ebbd2723eb78 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 14:03:12 -0800 Subject: [PATCH 16/27] stop ticker in retryHandler --- serviceregistration/kubernetes/retry_handler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go index 778b0bad0590..79067268dbfa 100644 --- a/serviceregistration/kubernetes/retry_handler.go +++ b/serviceregistration/kubernetes/retry_handler.go @@ -34,6 +34,7 @@ func (r *retryHandler) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { defer wait.Done() retry := time.NewTicker(retryFreq) + defer retry.Stop() for { select { case <-shutdownCh: From a89bc1f228fb5780247caeafdb411a3620895036 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 5 Feb 2020 14:13:20 -0800 Subject: [PATCH 17/27] update how test fails --- serviceregistration/kubernetes/retry_handler_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/serviceregistration/kubernetes/retry_handler_test.go b/serviceregistration/kubernetes/retry_handler_test.go index 3c6f8c9fa609..7e262644e454 100644 --- a/serviceregistration/kubernetes/retry_handler_test.go +++ b/serviceregistration/kubernetes/retry_handler_test.go @@ -221,7 +221,8 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { go func() { <-start if err := r.Add(testPatch); err != nil { - t.Fatal(err) + t.Log(err) + t.Fail() } done <- true }() From 662c272086c54a05515a44ec79fc3882c2900965 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Thu, 6 Feb 2020 11:19:26 -0800 Subject: [PATCH 18/27] strip req and resp body from error responses --- .../kubernetes/client/client.go | 33 +++++++------------ 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index c1cc39c1d4ef..1a7996ee9014 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -7,7 +7,6 @@ import ( "encoding/json" "errors" "fmt" - "io/ioutil" "net/http" "sync" "time" @@ -123,15 +122,6 @@ func (c *Client) PatchPod(namespace, podName string, patches ...*Patch) error { // do executes the given request, retrying if necessary. func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { - // Preserve the original request body so it can be viewed for debugging if needed. - // Reading it empties it, so we need to re-add it afterwards. - var reqBody []byte - if req.Body != nil { - reqBody, _ = ioutil.ReadAll(req.Body) - reqBodyReader := bytes.NewReader(reqBody) - req.Body = ioutil.NopCloser(reqBodyReader) - } - // Finish setting up a valid request. retryableReq, err := retryablehttp.FromRequest(req) if err != nil { @@ -146,7 +136,7 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { RetryWaitMin: retryWaitMin, RetryWaitMax: retryWaitMax, RetryMax: retryMax, - CheckRetry: c.getCheckRetry(req, reqBody), + CheckRetry: c.getCheckRetry(req), Backoff: retryablehttp.DefaultBackoff, } client.HTTPClient.Transport = &http.Transport{ @@ -181,7 +171,7 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { return json.NewDecoder(resp.Body).Decode(ptrToReturnObj) } -func (c *Client) getCheckRetry(req *http.Request, reqBody []byte) retryablehttp.CheckRetry { +func (c *Client) getCheckRetry(req *http.Request) retryablehttp.CheckRetry { return func(ctx context.Context, resp *http.Response, err error) (bool, error) { switch resp.StatusCode { case 200, 201, 202, 204: @@ -195,19 +185,19 @@ func (c *Client) getCheckRetry(req *http.Request, reqBody []byte) retryablehttp. } if config.BearerToken == c.config.BearerToken { // It's the same token. - return false, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + return false, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, resp.StatusCode)) } c.config = config // Continue to try again, but return the error too in case the caller would rather read it out. - return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + return true, fmt.Errorf("bad status code: %s", sanitizedDebuggingInfo(req, resp.StatusCode)) case 404: - return false, &ErrNotFound{debuggingInfo: sanitizedDebuggingInfo(req, reqBody, resp)} + return false, &ErrNotFound{debuggingInfo: sanitizedDebuggingInfo(req, resp.StatusCode)} case 500, 502, 503, 504: // Could be transient. - return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + return true, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, resp.StatusCode)) } // Unexpected. - return false, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, reqBody, resp)) + return false, fmt.Errorf("unexpected status code: %s", sanitizedDebuggingInfo(req, resp.StatusCode)) } } @@ -246,11 +236,10 @@ func (e *ErrNotFound) Error() string { return e.debuggingInfo } -// sanitizedDebuggingInfo converts an http response to a string without -// including its headers to avoid leaking authorization headers. -func sanitizedDebuggingInfo(req *http.Request, reqBody []byte, resp *http.Response) string { - respBody, _ := ioutil.ReadAll(resp.Body) - return fmt.Sprintf("req method: %s, req url: %s, req body: %s, resp statuscode: %d, resp respBody: %s", req.Method, req.URL, reqBody, resp.StatusCode, respBody) +// sanitizedDebuggingInfo provides a returnable string that can be used for debugging. This is intentionally somewhat vague +// because we don't want to leak secrets that may be in a request or response body. +func sanitizedDebuggingInfo(req *http.Request, respStatus int) string { + return fmt.Sprintf("req method: %s, req url: %s, resp statuscode: %d", req.Method, req.URL, respStatus) } // stopChContext allows us to cause requests to stop retrying if we receive From 04bc9951b16c4a54c4dfb90f23e53611200b4a73 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 10 Feb 2020 15:59:57 -0800 Subject: [PATCH 19/27] use a map to de-dupe patches to same path --- .../kubernetes/retry_handler.go | 62 +++--------- .../kubernetes/retry_handler_test.go | 94 ++++++++----------- .../kubernetes/service_registration.go | 26 ++--- 3 files changed, 65 insertions(+), 117 deletions(-) diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go index 79067268dbfa..936f6c495574 100644 --- a/serviceregistration/kubernetes/retry_handler.go +++ b/serviceregistration/kubernetes/retry_handler.go @@ -1,8 +1,6 @@ package kubernetes import ( - "fmt" - "strconv" "sync" "time" @@ -22,7 +20,9 @@ type retryHandler struct { client *client.Client // This gets mutated on multiple threads. - patchesToRetry []*client.Patch + // The map holds the path to the label being updated. It will only either + // not hold a particular label, or hold _the last_ state we were aware of. + patchesToRetry map[string]*client.Patch patchesToRetryLock sync.Mutex } @@ -47,51 +47,10 @@ func (r *retryHandler) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { // Add adds a patch to be retried until it's either completed without // error, or no longer needed. -func (r *retryHandler) Add(patch *client.Patch) error { +func (r *retryHandler) Add(patch *client.Patch) { r.patchesToRetryLock.Lock() defer r.patchesToRetryLock.Unlock() - - // - If the patch is a dupe, don't add it. - // - If the patch reverts another, remove them both. - // For example, perhaps we were already retrying "active = true", - // but this new patch tells us "active = false" again. - // - Otherwise, this is a new, unique patch, so add this patch to retries. - for i := 0; i < len(r.patchesToRetry); i++ { - prevPatch := r.patchesToRetry[i] - if patch.Path != prevPatch.Path { - continue - } - if patch.Operation != prevPatch.Operation { - continue - } - // These patches are operating upon the same thing. - // Let's look at what they're trying to do to determine - // the right action to take with the incoming patch. - patchValStr, ok := patch.Value.(string) - if !ok { - return fmt.Errorf("all patches must have bool values but received %+x", patch) - } - patchVal, err := strconv.ParseBool(patchValStr) - if err != nil { - return err - } - // This was already verified to be a bool string - // when it was added to the slice. - prevPatchVal, _ := strconv.ParseBool(prevPatch.Value.(string)) - if patchVal == prevPatchVal { - // We don't need to add the new patch because it already exists. - // Nothing further to do here. - return nil - } else { - // Rather than doing both an add and a subtract, or a true and a false, - // we need to just not act on both. This requires not adding the new patch, - // and removing the previous conflicting patch. - r.patchesToRetry = append(r.patchesToRetry[:i], r.patchesToRetry[i+1:]...) - return nil - } - } - r.patchesToRetry = append(r.patchesToRetry, patch) - return nil + r.patchesToRetry[patch.Path] = patch } func (r *retryHandler) retry() { @@ -103,11 +62,18 @@ func (r *retryHandler) retry() { return } - if err := r.client.PatchPod(r.namespace, r.podName, r.patchesToRetry...); err != nil { + patches := make([]*client.Patch, len(r.patchesToRetry)) + i := 0 + for _, patch := range r.patchesToRetry { + patches[i] = patch + i++ + } + + if err := r.client.PatchPod(r.namespace, r.podName, patches...); err != nil { if r.logger.IsWarn() { r.logger.Warn("unable to update state due to %s, will retry", err.Error()) } return } - r.patchesToRetry = nil + r.patchesToRetry = make(map[string]*client.Patch) } diff --git a/serviceregistration/kubernetes/retry_handler_test.go b/serviceregistration/kubernetes/retry_handler_test.go index 7e262644e454..6af949dfa0e4 100644 --- a/serviceregistration/kubernetes/retry_handler_test.go +++ b/serviceregistration/kubernetes/retry_handler_test.go @@ -44,19 +44,18 @@ func TestRetryHandlerSimple(t *testing.T) { } r := &retryHandler{ - logger: logger, - namespace: kubetest.ExpectedNamespace, - podName: kubetest.ExpectedPodName, - client: c, + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, + patchesToRetry: make(map[string]*client.Patch), } go r.Run(shutdownCh, wait) if testState.NumPatches() != 0 { t.Fatal("expected no current patches") } - if err := r.Add(testPatch); err != nil { - t.Fatal(err) - } + r.Add(testPatch) // Wait ample until the next try should have occurred. <-time.NewTimer(retryFreq * 2).C if testState.NumPatches() != 1 { @@ -66,9 +65,10 @@ func TestRetryHandlerSimple(t *testing.T) { func TestRetryHandlerAdd(t *testing.T) { r := &retryHandler{ - logger: hclog.NewNullLogger(), - namespace: "some-namespace", - podName: "some-pod-name", + logger: hclog.NewNullLogger(), + namespace: "some-namespace", + podName: "some-pod-name", + patchesToRetry: make(map[string]*client.Patch), } testPatch1 := &client.Patch{ @@ -93,85 +93,67 @@ func TestRetryHandlerAdd(t *testing.T) { } // Should be able to add all 4 patches. - if err := r.Add(testPatch1); err != nil { - t.Fatal(err) - } + r.Add(testPatch1) if len(r.patchesToRetry) != 1 { t.Fatal("expected 1 patch") } - if err := r.Add(testPatch2); err != nil { - t.Fatal(err) - } + r.Add(testPatch2) if len(r.patchesToRetry) != 2 { t.Fatal("expected 2 patches") } - if err := r.Add(testPatch3); err != nil { - t.Fatal(err) - } + r.Add(testPatch3) if len(r.patchesToRetry) != 3 { t.Fatal("expected 3 patches") } - if err := r.Add(testPatch4); err != nil { - t.Fatal(err) - } + r.Add(testPatch4) if len(r.patchesToRetry) != 4 { t.Fatal("expected 4 patches") } // Adding a dupe should result in no change. - if err := r.Add(testPatch4); err != nil { - t.Fatal(err) - } + r.Add(testPatch4) if len(r.patchesToRetry) != 4 { t.Fatal("expected 4 patches") } // Adding a reversion should result in its twin being subtracted. - if err := r.Add(&client.Patch{ + r.Add(&client.Patch{ Operation: client.Add, Path: "four", Value: "false", - }); err != nil { - t.Fatal(err) - } - if len(r.patchesToRetry) != 3 { - t.Fatal("expected 3 patches") + }) + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") } - if err := r.Add(&client.Patch{ + r.Add(&client.Patch{ Operation: client.Add, Path: "three", Value: "false", - }); err != nil { - t.Fatal(err) - } - if len(r.patchesToRetry) != 2 { - t.Fatal("expected 2 patches") + }) + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") } - if err := r.Add(&client.Patch{ + r.Add(&client.Patch{ Operation: client.Add, Path: "two", Value: "false", - }); err != nil { - t.Fatal(err) - } - if len(r.patchesToRetry) != 1 { - t.Fatal("expected 1 patches") + }) + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") } - if err := r.Add(&client.Patch{ + r.Add(&client.Patch{ Operation: client.Add, Path: "one", Value: "false", - }); err != nil { - t.Fatal(err) - } - if len(r.patchesToRetry) != 0 { - t.Fatal("expected 0 patches") + }) + if len(r.patchesToRetry) != 4 { + t.Fatal("expected 4 patches") } } @@ -205,10 +187,11 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { } r := &retryHandler{ - logger: logger, - namespace: kubetest.ExpectedNamespace, - podName: kubetest.ExpectedPodName, - client: c, + logger: logger, + namespace: kubetest.ExpectedNamespace, + podName: kubetest.ExpectedPodName, + client: c, + patchesToRetry: make(map[string]*client.Patch), } go r.Run(shutdownCh, wait) @@ -220,10 +203,7 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { for i := 0; i < numRoutines; i++ { go func() { <-start - if err := r.Add(testPatch); err != nil { - t.Log(err) - t.Fail() - } + r.Add(testPatch) done <- true }() } diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go index 3df880442177..99d080e6e082 100644 --- a/serviceregistration/kubernetes/service_registration.go +++ b/serviceregistration/kubernetes/service_registration.go @@ -38,9 +38,10 @@ func NewServiceRegistration(config map[string]string, logger hclog.Logger, state podName: podName, initialState: state, retryHandler: &retryHandler{ - logger: logger, - namespace: namespace, - podName: podName, + logger: logger, + namespace: namespace, + podName: podName, + patchesToRetry: make(map[string]*client.Patch), }, }, nil } @@ -130,47 +131,48 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro } func (r *serviceRegistration) NotifyActiveStateChange(isActive bool) error { - return r.notifyOrRetry(&client.Patch{ + r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelActive, Value: strconv.FormatBool(isActive), }) + return nil } func (r *serviceRegistration) NotifySealedStateChange(isSealed bool) error { - return r.notifyOrRetry(&client.Patch{ + r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelSealed, Value: strconv.FormatBool(isSealed), }) + return nil } func (r *serviceRegistration) NotifyPerformanceStandbyStateChange(isStandby bool) error { - return r.notifyOrRetry(&client.Patch{ + r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelPerfStandby, Value: strconv.FormatBool(isStandby), }) + return nil } func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) error { - return r.notifyOrRetry(&client.Patch{ + r.notifyOrRetry(&client.Patch{ Operation: client.Replace, Path: pathToLabels + labelInitialized, Value: strconv.FormatBool(isInitialized), }) + return nil } -func (r *serviceRegistration) notifyOrRetry(patch *client.Patch) error { +func (r *serviceRegistration) notifyOrRetry(patch *client.Patch) { if err := r.client.PatchPod(r.namespace, r.podName, patch); err != nil { if r.logger.IsWarn() { r.logger.Warn("unable to update state due to %s, will retry", err.Error()) } - if err := r.retryHandler.Add(patch); err != nil { - return err - } + r.retryHandler.Add(patch) } - return nil } func getRequiredField(logger hclog.Logger, config map[string]string, envVar, configParam string) (string, error) { From 630f180f87b9746129605e9ae7ca10628d5d17e5 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 10 Feb 2020 16:15:06 -0800 Subject: [PATCH 20/27] use simpler context --- .../kubernetes/client/client.go | 55 ++++--------------- 1 file changed, 11 insertions(+), 44 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 1a7996ee9014..62e50371f893 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "net/http" - "sync" "time" "github.com/hashicorp/go-cleanhttp" @@ -127,7 +126,17 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { if err != nil { return err } - retryableReq.WithContext(newContext(c.stopCh)) + + // Build a context that will call the cancelFunc when we receive + // a stop from our stopChan. This allows us to exit from our retry + // loop during a shutdown, rather than hanging. + ctx, cancelFunc := context.WithCancel(context.Background()) + go func(stopCh <-chan struct{}) { + <-stopCh + cancelFunc() + }(c.stopCh) + retryableReq.WithContext(ctx) + retryableReq.Header.Set("Authorization", "Bearer "+c.config.BearerToken) retryableReq.Header.Set("Accept", "application/json") @@ -241,45 +250,3 @@ func (e *ErrNotFound) Error() string { func sanitizedDebuggingInfo(req *http.Request, respStatus int) string { return fmt.Sprintf("req method: %s, req url: %s, resp statuscode: %d", req.Method, req.URL, respStatus) } - -// stopChContext allows us to cause requests to stop retrying if we receive -// a stop from the caller. -func newContext(stopCh <-chan struct{}) context.Context { - ctx := &stopChContext{ - stopCh: stopCh, - } - go ctx.monitorStopCh() - return ctx -} - -type stopChContext struct { - stopCh <-chan struct{} - errLock sync.RWMutex - err error -} - -func (c *stopChContext) Deadline() (deadline time.Time, ok bool) { - // Return false to indicate that no deadline is set. - return time.Time{}, false -} - -func (c *stopChContext) Done() <-chan struct{} { - return c.stopCh -} - -func (c *stopChContext) Err() error { - c.errLock.RLock() - defer c.errLock.RUnlock() - return c.err -} - -func (c *stopChContext) Value(key interface{}) interface{} { - return nil -} - -func (c *stopChContext) monitorStopCh() { - <-c.stopCh - c.errLock.Lock() - defer c.errLock.Unlock() - c.err = errors.New("stop channel has been closed") -} From 59b3db7b72355c7bea2449e69874e548a0a1633e Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Mon, 10 Feb 2020 16:20:05 -0800 Subject: [PATCH 21/27] update certutil test --- sdk/helper/certutil/certutil_test.go | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/sdk/helper/certutil/certutil_test.go b/sdk/helper/certutil/certutil_test.go index 8250a056f372..b42837214125 100644 --- a/sdk/helper/certutil/certutil_test.go +++ b/sdk/helper/certutil/certutil_test.go @@ -11,10 +11,8 @@ import ( "encoding/json" "encoding/pem" "fmt" - "io/ioutil" "math/big" mathrand "math/rand" - "os" "reflect" "strings" "sync" @@ -425,24 +423,7 @@ Q9IFGQFFF8jO18lbyWqnRBGXcS4/G7jQ3S7C121d14YLUeAYOM7pJykI1g4CLx9y vitin0L6nprauWkKO38XgM4T75qKZpqtiOcT -----END CERTIFICATE----- ` - - tmpfile, err := ioutil.TempFile("", "ca.crt") - if err != nil { - t.Fatal(err) - } - defer os.Remove(tmpfile.Name()) - - if _, err := tmpfile.Write([]byte(caExample)); err != nil { - t.Fatal(err) - } - if err := tmpfile.Close(); err != nil { - t.Fatal(err) - } - fileBody, err := ioutil.ReadFile(tmpfile.Name()) - if err != nil { - t.Fatal(err) - } - if _, err := NewCertPool(bytes.NewReader(fileBody)); err != nil { + if _, err := NewCertPool(bytes.NewReader([]byte(caExample))); err != nil { t.Fatal(err) } } From e2a0a7b07bb746bf7fb49b79f3aae3b354067d71 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 11 Feb 2020 15:30:36 -0800 Subject: [PATCH 22/27] fix env vars in manual testing doc --- serviceregistration/kubernetes/testing/README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/serviceregistration/kubernetes/testing/README.md b/serviceregistration/kubernetes/testing/README.md index 7be3ead57da1..7eaf3f527631 100644 --- a/serviceregistration/kubernetes/testing/README.md +++ b/serviceregistration/kubernetes/testing/README.md @@ -17,15 +17,15 @@ spec: args: - while true; do echo -en '\n'; - printenv VAULT_POD_NAME VAULT_NAMESPACE; + printenv VAULT_K8S_POD_NAME VAULT_K8S_NAMESPACE; sleep 10; done; env: - - name: VAULT_POD_NAME + - name: VAULT_K8S_POD_NAME valueFrom: fieldRef: fieldPath: metadata.name - - name: VAULT_NAMESPACE + - name: VAULT_K8S_NAMESPACE valueFrom: fieldRef: fieldPath: metadata.namespace From 255e113bf869bc4b00823143f83c72186d6af74c Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 11 Feb 2020 15:32:35 -0800 Subject: [PATCH 23/27] Update serviceregistration/kubernetes/testing/README.md Co-Authored-By: Theron Voran --- serviceregistration/kubernetes/testing/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serviceregistration/kubernetes/testing/README.md b/serviceregistration/kubernetes/testing/README.md index 7eaf3f527631..61a22e680dcc 100644 --- a/serviceregistration/kubernetes/testing/README.md +++ b/serviceregistration/kubernetes/testing/README.md @@ -1,7 +1,7 @@ # How to Test Manually - `$ minikube start` -- In the Vault folder, `$ make dev` +- In the Vault folder, `$ make dev XC_ARCH=amd64 XC_OS=linux XC_OSARCH=linux/amd64` - Create a file called `vault-test.yaml` with the following contents: ``` From aa1b492f3a8cba598b91d1e714eb1c7b14a3cde2 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Tue, 11 Feb 2020 15:34:47 -0800 Subject: [PATCH 24/27] Update serviceregistration/kubernetes/testing/README.md Co-Authored-By: Theron Voran --- serviceregistration/kubernetes/testing/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serviceregistration/kubernetes/testing/README.md b/serviceregistration/kubernetes/testing/README.md index 61a22e680dcc..940415b6b436 100644 --- a/serviceregistration/kubernetes/testing/README.md +++ b/serviceregistration/kubernetes/testing/README.md @@ -34,7 +34,7 @@ spec: - Create the pod: `$ kubectl apply -f vault-test.yaml` - View the full initial state of the pod: `$ kubectl get pod vault -o=yaml > initialstate.txt` -- Drop the Vault binary into the pod: `$ kubectl cp $(which vault) /vault:/` +- Drop the Vault binary into the pod: `$ kubectl cp bin/vault /vault:/` - Drop to the shell within the pod: `$ kubectl exec -it vault -- /bin/bash` - Install a text editor: `$ apt-get update`, `$ apt-get install nano` - Write a test Vault config to `vault.config` like: From 14ba35f67fa794007cd2ae3c104444525effe2d8 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 12 Feb 2020 12:59:37 -0800 Subject: [PATCH 25/27] retry initial state if it fails --- .../kubernetes/client/client.go | 17 +- .../kubernetes/retry_handler.go | 109 ++++++-- .../kubernetes/retry_handler_test.go | 261 ++++++++++++++++-- .../kubernetes/service_registration.go | 37 +-- .../kubernetes/testing/testserver.go | 22 +- 5 files changed, 365 insertions(+), 81 deletions(-) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 62e50371f893..33c4ac38cfe2 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -15,14 +15,13 @@ import ( "github.com/hashicorp/go-retryablehttp" ) -const ( +var ( // Retry configuration - retryWaitMin = 500 * time.Millisecond - retryWaitMax = 30 * time.Second - retryMax = 10 -) + RetryWaitMin = 500 * time.Millisecond + RetryWaitMax = 30 * time.Second + RetryMax = 10 -var ( + // Standard errs ErrNamespaceUnset = errors.New(`"namespace" is unset`) ErrPodNameUnset = errors.New(`"podName" is unset`) ErrNotInCluster = errors.New("unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined") @@ -142,9 +141,9 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { client := &retryablehttp.Client{ HTTPClient: cleanhttp.DefaultClient(), - RetryWaitMin: retryWaitMin, - RetryWaitMax: retryWaitMax, - RetryMax: retryMax, + RetryWaitMin: RetryWaitMin, + RetryWaitMax: RetryWaitMax, + RetryMax: RetryMax, CheckRetry: c.getCheckRetry(req), Backoff: retryablehttp.DefaultBackoff, } diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go index 936f6c495574..c02b492c67c6 100644 --- a/serviceregistration/kubernetes/retry_handler.go +++ b/serviceregistration/kubernetes/retry_handler.go @@ -17,48 +17,99 @@ type retryHandler struct { // These don't need a mutex because they're never mutated. logger hclog.Logger namespace, podName string - client *client.Client - // This gets mutated on multiple threads. + // To synchronize setInitialState and patchesToRetry. + lock sync.Mutex + + // setInitialState will be nil if this has been done successfully. + // It must be done before any patches are retried. + setInitialState func() error + // The map holds the path to the label being updated. It will only either // not hold a particular label, or hold _the last_ state we were aware of. - patchesToRetry map[string]*client.Patch - patchesToRetryLock sync.Mutex + // These should only be updated after initial state has been set. + patchesToRetry map[string]*client.Patch } -// Run runs at an interval, checking if anything has failed and if so, -// attempting to send them again. -func (r *retryHandler) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup) { - // Make sure Vault will give us time to finish up here. - wait.Add(1) - defer wait.Done() - - retry := time.NewTicker(retryFreq) - defer retry.Stop() - for { - select { - case <-shutdownCh: - return - case <-retry.C: - r.retry() +func (r *retryHandler) SetInitialState(setInitialState func() error) { + r.lock.Lock() + defer r.lock.Unlock() + if err := setInitialState(); err != nil { + if r.logger.IsWarn() { + r.logger.Warn("unable to set initial state due to %s, will retry", err.Error()) } + r.setInitialState = setInitialState } } -// Add adds a patch to be retried until it's either completed without +// Run must be called for retries to be started. +func (r *retryHandler) Run(shutdownCh <-chan struct{}, wait *sync.WaitGroup, c *client.Client) { + // Run this in a go func so this call doesn't block. + go func() { + // Make sure Vault will give us time to finish up here. + wait.Add(1) + defer wait.Done() + + retry := time.NewTicker(retryFreq) + defer retry.Stop() + for { + select { + case <-shutdownCh: + return + case <-retry.C: + r.retry(c) + } + } + }() +} + +// Notify adds a patch to be retried until it's either completed without // error, or no longer needed. -func (r *retryHandler) Add(patch *client.Patch) { - r.patchesToRetryLock.Lock() - defer r.patchesToRetryLock.Unlock() - r.patchesToRetry[patch.Path] = patch +func (r *retryHandler) Notify(c *client.Client, patch *client.Patch) { + r.lock.Lock() + defer r.lock.Unlock() + + // Initial state must be set first, or subsequent notifications we've + // received could get smashed by a late-arriving initial state. + // We will store this to retry it when appropriate. + if r.setInitialState != nil { + if r.logger.IsWarn() { + r.logger.Warn("cannot notify of present state because initial state is unset", patch.Path) + } + r.patchesToRetry[patch.Path] = patch + return + } + + // Initial state has been sent, so it's OK to attempt a patch immediately. + if err := c.PatchPod(r.namespace, r.podName, patch); err != nil { + if r.logger.IsWarn() { + r.logger.Warn("unable to update state due to %s, will retry", patch.Path, err.Error()) + } + r.patchesToRetry[patch.Path] = patch + } } -func (r *retryHandler) retry() { - r.patchesToRetryLock.Lock() - defer r.patchesToRetryLock.Unlock() +func (r *retryHandler) retry(c *client.Client) { + r.lock.Lock() + defer r.lock.Unlock() + + // Initial state must be set first, or subsequent notifications we've + // received could get smashed by a late-arriving initial state. + if r.setInitialState != nil { + if err := r.setInitialState(); err != nil { + if r.logger.IsWarn() { + r.logger.Warn("unable to set initial state due to %s, will retry", err.Error()) + } + // On failure, we leave the initial state func populated for + // the next retry. + return + } + // On success, we set it to nil and allow the logic to continue. + r.setInitialState = nil + } if len(r.patchesToRetry) == 0 { - // Nothing to do here. + // Nothing further to do here. return } @@ -69,7 +120,7 @@ func (r *retryHandler) retry() { i++ } - if err := r.client.PatchPod(r.namespace, r.podName, patches...); err != nil { + if err := c.PatchPod(r.namespace, r.podName, patches...); err != nil { if r.logger.IsWarn() { r.logger.Warn("unable to update state due to %s, will retry", err.Error()) } diff --git a/serviceregistration/kubernetes/retry_handler_test.go b/serviceregistration/kubernetes/retry_handler_test.go index 6af949dfa0e4..441cbaf14935 100644 --- a/serviceregistration/kubernetes/retry_handler_test.go +++ b/serviceregistration/kubernetes/retry_handler_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/hashicorp/go-hclog" + sr "github.com/hashicorp/vault/serviceregistration" "github.com/hashicorp/vault/serviceregistration/kubernetes/client" kubetest "github.com/hashicorp/vault/serviceregistration/kubernetes/testing" ) @@ -47,15 +48,14 @@ func TestRetryHandlerSimple(t *testing.T) { logger: logger, namespace: kubetest.ExpectedNamespace, podName: kubetest.ExpectedPodName, - client: c, patchesToRetry: make(map[string]*client.Patch), } - go r.Run(shutdownCh, wait) + r.Run(shutdownCh, wait, c) if testState.NumPatches() != 0 { t.Fatal("expected no current patches") } - r.Add(testPatch) + r.Notify(c, testPatch) // Wait ample until the next try should have occurred. <-time.NewTimer(retryFreq * 2).C if testState.NumPatches() != 1 { @@ -64,6 +64,26 @@ func TestRetryHandlerSimple(t *testing.T) { } func TestRetryHandlerAdd(t *testing.T) { + _, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + logger := hclog.NewNullLogger() + shutdownCh := make(chan struct{}) + c, err := client.New(logger, shutdownCh) + if err != nil { + t.Fatal(err) + } + r := &retryHandler{ logger: hclog.NewNullLogger(), namespace: "some-namespace", @@ -93,34 +113,34 @@ func TestRetryHandlerAdd(t *testing.T) { } // Should be able to add all 4 patches. - r.Add(testPatch1) + r.Notify(c, testPatch1) if len(r.patchesToRetry) != 1 { t.Fatal("expected 1 patch") } - r.Add(testPatch2) + r.Notify(c, testPatch2) if len(r.patchesToRetry) != 2 { t.Fatal("expected 2 patches") } - r.Add(testPatch3) + r.Notify(c, testPatch3) if len(r.patchesToRetry) != 3 { t.Fatal("expected 3 patches") } - r.Add(testPatch4) + r.Notify(c, testPatch4) if len(r.patchesToRetry) != 4 { t.Fatal("expected 4 patches") } // Adding a dupe should result in no change. - r.Add(testPatch4) + r.Notify(c, testPatch4) if len(r.patchesToRetry) != 4 { t.Fatal("expected 4 patches") } // Adding a reversion should result in its twin being subtracted. - r.Add(&client.Patch{ + r.Notify(c, &client.Patch{ Operation: client.Add, Path: "four", Value: "false", @@ -129,7 +149,7 @@ func TestRetryHandlerAdd(t *testing.T) { t.Fatal("expected 4 patches") } - r.Add(&client.Patch{ + r.Notify(c, &client.Patch{ Operation: client.Add, Path: "three", Value: "false", @@ -138,7 +158,7 @@ func TestRetryHandlerAdd(t *testing.T) { t.Fatal("expected 4 patches") } - r.Add(&client.Patch{ + r.Notify(c, &client.Patch{ Operation: client.Add, Path: "two", Value: "false", @@ -147,7 +167,7 @@ func TestRetryHandlerAdd(t *testing.T) { t.Fatal("expected 4 patches") } - r.Add(&client.Patch{ + r.Notify(c, &client.Patch{ Operation: client.Add, Path: "one", Value: "false", @@ -190,10 +210,8 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { logger: logger, namespace: kubetest.ExpectedNamespace, podName: kubetest.ExpectedPodName, - client: c, patchesToRetry: make(map[string]*client.Patch), } - go r.Run(shutdownCh, wait) // Now hit it as quickly as possible to see if we can produce // races or deadlocks. @@ -203,7 +221,20 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { for i := 0; i < numRoutines; i++ { go func() { <-start - r.Add(testPatch) + r.Notify(c, testPatch) + done <- true + }() + go func() { + <-start + r.Run(shutdownCh, wait, c) + done <- true + }() + go func() { + <-start + r.SetInitialState(func() error { + c.GetPod(kubetest.ExpectedNamespace, kubetest.ExpectedPodName) + return nil + }) done <- true }() } @@ -211,7 +242,7 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { // Allow up to 5 seconds for everything to finish. timer := time.NewTimer(5 * time.Second) - for i := 0; i < numRoutines; i++ { + for i := 0; i < numRoutines*3; i++ { select { case <-timer.C: t.Fatal("test took too long to complete, check for deadlock") @@ -219,3 +250,201 @@ func TestRetryHandlerRacesAndDeadlocks(t *testing.T) { } } } + +// In this test, the API server sends bad responses for 5 seconds, +// then sends good responses, and we make sure we get the expected behavior. +func TestRetryHandlerAPIConnectivityProblemsInitialState(t *testing.T) { + if testing.Short() { + t.Skip() + } + + testState, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + kubetest.ReturnGatewayTimeouts.Store(true) + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + client.RetryMax = 0 + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + shutdownCh := make(chan struct{}) + wait := &sync.WaitGroup{} + reg, err := NewServiceRegistration(map[string]string{ + "namespace": kubetest.ExpectedNamespace, + "pod_name": kubetest.ExpectedPodName, + }, hclog.NewNullLogger(), sr.State{ + VaultVersion: "vault-version", + IsInitialized: true, + IsSealed: true, + IsActive: true, + IsPerformanceStandby: true, + }, "") + if err != nil { + t.Fatal(err) + } + if err := reg.Run(shutdownCh, wait); err != nil { + t.Fatal(err) + } + + // At this point, since the initial state can't be set, + // remotely we should have false for all these labels. + patch := testState.Get(pathToLabels + labelVaultVersion) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelActive) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelSealed) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelPerfStandby) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelInitialized) + if patch != nil { + t.Fatal("expected no value") + } + + kubetest.ReturnGatewayTimeouts.Store(false) + + // Now we need to wait to give the retry handler + // a chance to update these values. + time.Sleep(retryFreq + time.Second) + val := testState.Get(pathToLabels + labelVaultVersion)["value"] + if val != "vault-version" { + t.Fatal("expected vault-version") + } + val = testState.Get(pathToLabels + labelActive)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelSealed)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelPerfStandby)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelInitialized)["value"] + if val != "true" { + t.Fatal("expected true") + } +} + +// In this test, the API server sends bad responses for 5 seconds, +// then sends good responses, and we make sure we get the expected behavior. +func TestRetryHandlerAPIConnectivityProblemsNotifications(t *testing.T) { + if testing.Short() { + t.Skip() + } + + testState, testConf, closeFunc := kubetest.Server(t) + defer closeFunc() + kubetest.ReturnGatewayTimeouts.Store(true) + + client.Scheme = testConf.ClientScheme + client.TokenFile = testConf.PathToTokenFile + client.RootCAFile = testConf.PathToRootCAFile + client.RetryMax = 0 + if err := os.Setenv(client.EnvVarKubernetesServiceHost, testConf.ServiceHost); err != nil { + t.Fatal(err) + } + if err := os.Setenv(client.EnvVarKubernetesServicePort, testConf.ServicePort); err != nil { + t.Fatal(err) + } + + shutdownCh := make(chan struct{}) + wait := &sync.WaitGroup{} + reg, err := NewServiceRegistration(map[string]string{ + "namespace": kubetest.ExpectedNamespace, + "pod_name": kubetest.ExpectedPodName, + }, hclog.NewNullLogger(), sr.State{ + VaultVersion: "vault-version", + IsInitialized: false, + IsSealed: false, + IsActive: false, + IsPerformanceStandby: false, + }, "") + if err != nil { + t.Fatal(err) + } + if err := reg.Run(shutdownCh, wait); err != nil { + t.Fatal(err) + } + + if err := reg.NotifyActiveStateChange(true); err != nil { + t.Fatal(err) + } + if err := reg.NotifyInitializedStateChange(true); err != nil { + t.Fatal(err) + } + if err := reg.NotifyPerformanceStandbyStateChange(true); err != nil { + t.Fatal(err) + } + if err := reg.NotifySealedStateChange(true); err != nil { + t.Fatal(err) + } + + // At this point, since the initial state can't be set, + // remotely we should have false for all these labels. + patch := testState.Get(pathToLabels + labelVaultVersion) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelActive) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelSealed) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelPerfStandby) + if patch != nil { + t.Fatal("expected no value") + } + patch = testState.Get(pathToLabels + labelInitialized) + if patch != nil { + t.Fatal("expected no value") + } + + kubetest.ReturnGatewayTimeouts.Store(false) + + // Now we need to wait to give the retry handler + // a chance to update these values. + time.Sleep(retryFreq + time.Second) + + // They should be "true" if the Notifications were set after the + // initial state. + val := testState.Get(pathToLabels + labelVaultVersion)["value"] + if val != "vault-version" { + t.Fatal("expected vault-version") + } + val = testState.Get(pathToLabels + labelActive)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelSealed)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelPerfStandby)["value"] + if val != "true" { + t.Fatal("expected true") + } + val = testState.Get(pathToLabels + labelInitialized)["value"] + if val != "true" { + t.Fatal("expected true") + } +} diff --git a/serviceregistration/kubernetes/service_registration.go b/serviceregistration/kubernetes/service_registration.go index 99d080e6e082..0fbdd659897f 100644 --- a/serviceregistration/kubernetes/service_registration.go +++ b/serviceregistration/kubernetes/service_registration.go @@ -60,10 +60,16 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro return err } r.client = c - r.retryHandler.client = c + // Now that we've populated the client, we can begin using the retry handler. + r.retryHandler.SetInitialState(r.setInitialState) + r.retryHandler.Run(shutdownCh, wait, c) + return nil +} + +func (r *serviceRegistration) setInitialState() error { // Verify that the pod exists and our configuration looks good. - pod, err := c.GetPod(r.namespace, r.podName) + pod, err := r.client.GetPod(r.namespace, r.podName) if err != nil { return err } @@ -74,10 +80,10 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro return fmt.Errorf("no pod metadata on %+v", pod) } if pod.Metadata.Labels == nil { - // Add the labels field, and the labels as part of that one call. + // Notify the labels field, and the labels as part of that one call. // The reason we must take a different approach to adding them is discussed here: // https://stackoverflow.com/questions/57480205/error-while-applying-json-patch-to-kubernetes-custom-resource - if err := c.PatchPod(r.namespace, r.podName, &client.Patch{ + if err := r.client.PatchPod(r.namespace, r.podName, &client.Patch{ Operation: client.Add, Path: "/metadata/labels", Value: map[string]string{ @@ -119,19 +125,15 @@ func (r *serviceRegistration) Run(shutdownCh <-chan struct{}, wait *sync.WaitGro Value: strconv.FormatBool(r.initialState.IsInitialized), }, } - if err := c.PatchPod(r.namespace, r.podName, patches...); err != nil { + if err := r.client.PatchPod(r.namespace, r.podName, patches...); err != nil { return err } } - - // Run a service that retries errored-out notifications if they occur. - go r.retryHandler.Run(shutdownCh, wait) - return nil } func (r *serviceRegistration) NotifyActiveStateChange(isActive bool) error { - r.notifyOrRetry(&client.Patch{ + r.retryHandler.Notify(r.client, &client.Patch{ Operation: client.Replace, Path: pathToLabels + labelActive, Value: strconv.FormatBool(isActive), @@ -140,7 +142,7 @@ func (r *serviceRegistration) NotifyActiveStateChange(isActive bool) error { } func (r *serviceRegistration) NotifySealedStateChange(isSealed bool) error { - r.notifyOrRetry(&client.Patch{ + r.retryHandler.Notify(r.client, &client.Patch{ Operation: client.Replace, Path: pathToLabels + labelSealed, Value: strconv.FormatBool(isSealed), @@ -149,7 +151,7 @@ func (r *serviceRegistration) NotifySealedStateChange(isSealed bool) error { } func (r *serviceRegistration) NotifyPerformanceStandbyStateChange(isStandby bool) error { - r.notifyOrRetry(&client.Patch{ + r.retryHandler.Notify(r.client, &client.Patch{ Operation: client.Replace, Path: pathToLabels + labelPerfStandby, Value: strconv.FormatBool(isStandby), @@ -158,7 +160,7 @@ func (r *serviceRegistration) NotifyPerformanceStandbyStateChange(isStandby bool } func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) error { - r.notifyOrRetry(&client.Patch{ + r.retryHandler.Notify(r.client, &client.Patch{ Operation: client.Replace, Path: pathToLabels + labelInitialized, Value: strconv.FormatBool(isInitialized), @@ -166,15 +168,6 @@ func (r *serviceRegistration) NotifyInitializedStateChange(isInitialized bool) e return nil } -func (r *serviceRegistration) notifyOrRetry(patch *client.Patch) { - if err := r.client.PatchPod(r.namespace, r.podName, patch); err != nil { - if r.logger.IsWarn() { - r.logger.Warn("unable to update state due to %s, will retry", err.Error()) - } - r.retryHandler.Add(patch) - } -} - func getRequiredField(logger hclog.Logger, config map[string]string, envVar, configParam string) (string, error) { value := "" switch { diff --git a/serviceregistration/kubernetes/testing/testserver.go b/serviceregistration/kubernetes/testing/testserver.go index 437c33a95847..0955afed86da 100644 --- a/serviceregistration/kubernetes/testing/testserver.go +++ b/serviceregistration/kubernetes/testing/testserver.go @@ -11,6 +11,8 @@ import ( "strings" "sync" "testing" + + "go.uber.org/atomic" ) const ( @@ -25,11 +27,17 @@ const ( tokenFile = "token" ) -var pathToFiles = func() string { - wd, _ := os.Getwd() - pathParts := strings.Split(wd, "vault") - return pathParts[0] + "vault/serviceregistration/kubernetes/testing/" -}() +var ( + // ReturnGatewayTimeouts toggles whether the test server should return, + // well, gateway timeouts... + ReturnGatewayTimeouts = atomic.NewBool(false) + + pathToFiles = func() string { + wd, _ := os.Getwd() + pathParts := strings.Split(wd, "vault") + return pathParts[0] + "vault/serviceregistration/kubernetes/testing/" + }() +) // Conf returns the info needed to configure the client to point at // the test server. This must be done by the caller to avoid an import @@ -125,6 +133,10 @@ func Server(t *testing.T) (testState *State, testConf *Conf, closeFunc func()) { testConf.PathToRootCAFile = tmpCACrt.Name() ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if ReturnGatewayTimeouts.Load() { + w.WriteHeader(504) + return + } namespace, podName, err := parsePath(r.URL.Path) if err != nil { w.WriteHeader(400) From 8b62a65ad43499a53bc91e243f2c969e2a88896a Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 12 Feb 2020 13:05:19 -0800 Subject: [PATCH 26/27] improve logging in retry handler --- serviceregistration/kubernetes/retry_handler.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/serviceregistration/kubernetes/retry_handler.go b/serviceregistration/kubernetes/retry_handler.go index c02b492c67c6..67381bc734d5 100644 --- a/serviceregistration/kubernetes/retry_handler.go +++ b/serviceregistration/kubernetes/retry_handler.go @@ -1,6 +1,7 @@ package kubernetes import ( + "fmt" "sync" "time" @@ -36,7 +37,7 @@ func (r *retryHandler) SetInitialState(setInitialState func() error) { defer r.lock.Unlock() if err := setInitialState(); err != nil { if r.logger.IsWarn() { - r.logger.Warn("unable to set initial state due to %s, will retry", err.Error()) + r.logger.Warn(fmt.Sprintf("unable to set initial state due to %s, will retry", err.Error())) } r.setInitialState = setInitialState } @@ -74,7 +75,7 @@ func (r *retryHandler) Notify(c *client.Client, patch *client.Patch) { // We will store this to retry it when appropriate. if r.setInitialState != nil { if r.logger.IsWarn() { - r.logger.Warn("cannot notify of present state because initial state is unset", patch.Path) + r.logger.Warn(fmt.Sprintf("cannot notify of present state for %s because initial state is unset", patch.Path)) } r.patchesToRetry[patch.Path] = patch return @@ -83,7 +84,7 @@ func (r *retryHandler) Notify(c *client.Client, patch *client.Patch) { // Initial state has been sent, so it's OK to attempt a patch immediately. if err := c.PatchPod(r.namespace, r.podName, patch); err != nil { if r.logger.IsWarn() { - r.logger.Warn("unable to update state due to %s, will retry", patch.Path, err.Error()) + r.logger.Warn(fmt.Sprintf("unable to update state for %s due to %s, will retry", patch.Path, err.Error())) } r.patchesToRetry[patch.Path] = patch } @@ -98,7 +99,7 @@ func (r *retryHandler) retry(c *client.Client) { if r.setInitialState != nil { if err := r.setInitialState(); err != nil { if r.logger.IsWarn() { - r.logger.Warn("unable to set initial state due to %s, will retry", err.Error()) + r.logger.Warn(fmt.Sprintf("unable to set initial state due to %s, will retry", err.Error())) } // On failure, we leave the initial state func populated for // the next retry. @@ -122,7 +123,7 @@ func (r *retryHandler) retry(c *client.Client) { if err := c.PatchPod(r.namespace, r.podName, patches...); err != nil { if r.logger.IsWarn() { - r.logger.Warn("unable to update state due to %s, will retry", err.Error()) + r.logger.Warn(fmt.Sprintf("unable to update state for due to %s, will retry", err.Error())) } return } From 5eb05679a665fb528c86daaa0c48ef56a3972081 Mon Sep 17 00:00:00 2001 From: Becca Petrin Date: Wed, 12 Feb 2020 13:15:58 -0800 Subject: [PATCH 27/27] protect against nil response --- serviceregistration/kubernetes/client/client.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/serviceregistration/kubernetes/client/client.go b/serviceregistration/kubernetes/client/client.go index 33c4ac38cfe2..3c5bbf587862 100644 --- a/serviceregistration/kubernetes/client/client.go +++ b/serviceregistration/kubernetes/client/client.go @@ -181,6 +181,9 @@ func (c *Client) do(req *http.Request, ptrToReturnObj interface{}) error { func (c *Client) getCheckRetry(req *http.Request) retryablehttp.CheckRetry { return func(ctx context.Context, resp *http.Response, err error) (bool, error) { + if resp == nil { + return true, fmt.Errorf("nil response: %s", req.URL.RequestURI()) + } switch resp.StatusCode { case 200, 201, 202, 204: // Success.