From ccaf8180086159903322a555ec062b128b0973dc Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 9 Mar 2021 17:52:03 -0600 Subject: [PATCH 01/20] Add basic tests and service registration polling --- connect-inject/container_init.go | 25 ++- subcommand/connect-init/command.go | 119 ++++++++++--- subcommand/connect-init/command_test.go | 227 ++++++++++++++++++++++-- 3 files changed, 335 insertions(+), 36 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 22fcb1d65b..9b53e0af9c 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -19,9 +19,10 @@ const ( ) type initContainerCommandData struct { - ServiceName string - ProxyServiceName string - ServicePort int32 + ServiceName string + ServiceAccountName string + ProxyServiceName string + ServicePort int32 // ServiceProtocol is the protocol for the service-defaults config // that will be written if WriteServiceDefaults is true. ServiceProtocol string @@ -95,6 +96,7 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co ConsulCACert: h.ConsulCACert, MetaKeyPodName: MetaKeyPodName, MetaKeyKubeNS: MetaKeyKubeNS, + ServiceAccountName: pod.Spec.ServiceAccountName, } if data.ServiceName == "" { // Assertion, since we call defaultAnnotations above and do @@ -323,6 +325,23 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co // and the connect-proxy service should come after the "main" service // because its alias health check depends on the main service to exist. const initContainerCommandTpl = ` +<<<<<<< Updated upstream +======= +{{- if .AuthMethod }} +consul-k8s connect-init -method="{{ .AuthMethod }}" \ + -service-account-name="{{ .ServiceAccountName }}" \ + {{- if.ConsulNamespace }} + {{- if .NamespaceMirroringEnabled }} + {{- /* If namespace mirroring is enabled, the auth method is + defined in the default namespace */}} + -namespace="default" \ + {{- else }} + -namespace="{{ .ConsulNamespace }}" \ + {{- end }} + {{- end }} + -meta="pod=${POD_NAMESPACE}/${POD_NAME}" +{{- end }} +>>>>>>> Stashed changes {{- if .ConsulCACert}} export CONSUL_HTTP_ADDR="https://${HOST_IP}:8501" export CONSUL_GRPC_ADDR="https://${HOST_IP}:8502" diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 4c15ba1371..5121cfbe41 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -3,6 +3,7 @@ package connectinit import ( "flag" "fmt" + "io/ioutil" "sync" "time" @@ -16,15 +17,22 @@ import ( const bearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" const tokenSinkFile = "/consul/connect-inject/acl-token" +const proxyIDFile = "/consul/connect-inject/proxyid" const numLoginRetries = 3 +const serviceRegistrationPollingRetries = 6 // This maps to 60 seconds type Command struct { UI cli.Ui - flagACLAuthMethod string // Auth Method to use for ACLs, if enabled - flagMeta map[string]string // Flag for metadata to consul login. - flagBearerTokenFile string // Location of the bearer token. - flagTokenSinkFile string // Location to write the output token. + flagACLAuthMethod string // Auth Method to use for ACLs, if enabled. + flagMeta map[string]string // Flag for metadata to consul login. + flagBearerTokenFile string // Location of the bearer token. + flagTokenSinkFile string // Location to write the output token. + flagProxyIDFile string // Location to write the output proxyID. + flagPodName string // Pod name. + flagPodNamespace string // Pod namespace. + flagServiceAccountName string // ServiceAccountName for this service. + flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. flagSet *flag.FlagSet http *flags.HTTPFlags @@ -37,8 +45,7 @@ type Command struct { func (c *Command) init() { c.flagSet = flag.NewFlagSet("", flag.ContinueOnError) - c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "", - "Name of the auth method to login to.") + c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "", "Name of the auth method to login to.") c.flagSet.Var((*flags.FlagMapValue)(&c.flagMeta), "meta", "Metadata to set on the token, formatted as key=value. This flag may be specified multiple "+ "times to set multiple meta fields.") @@ -47,9 +54,16 @@ func (c *Command) init() { "Default is /var/run/secrets/kubernetes.io/serviceaccount/token.") c.flagSet.StringVar(&c.flagTokenSinkFile, "token-sink-file", tokenSinkFile, "The most recent token's SecretID is kept up to date in this file. Default is /consul/connect-inject/acl-token.") + c.flagSet.StringVar(&c.flagProxyIDFile, "proxyid-file", proxyIDFile, "Location to write the output proxyid file.") + c.flagSet.StringVar(&c.flagPodName, "pod-name", "", "Name of the pod.") + c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") + c.flagSet.StringVar(&c.flagServiceAccountName, "service-account-name", "", "The service account name for this service.") - c.http = &flags.HTTPFlags{} + // TODO: we dont need this if we can mock the login bits + c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, + "The service account name for this service.") + c.http = &flags.HTTPFlags{} flags.Merge(c.flagSet, c.http.Flags()) c.help = flags.Usage(help, c.flagSet) } @@ -60,17 +74,14 @@ func (c *Command) Run(args []string) int { if err := c.flagSet.Parse(args); err != nil { return 1 } - - // Validate flags. - if c.flagACLAuthMethod == "" { - c.UI.Error("-acl-auth-method must be set") + if c.flagPodName == "" { + c.UI.Error("-pod-name must be set") return 1 } - if c.flagMeta == nil { - c.UI.Error("-meta must be set") + if c.flagPodNamespace == "" { + c.UI.Error("-pod-namespace must be set") return 1 } - // TODO: Add namespace support if c.consulClient == nil { cfg := api.DefaultConfig() @@ -81,19 +92,85 @@ func (c *Command) Run(args []string) int { return 1 } } + // First do the ACL Login, if necessary. + if c.flagACLAuthMethod != "" { + // Validate flags related to ACL login. + if c.flagMeta == nil { + c.UI.Error("-meta must be set") + return 1 + } + if c.flagServiceAccountName == "" { + c.UI.Error("-service-account-name must be set") + return 1 + } + err = backoff.Retry(func() error { + err := common.ConsulLogin(c.consulClient, c.flagBearerTokenFile, c.flagACLAuthMethod, c.flagTokenSinkFile, c.flagMeta) + if err != nil { + c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) + } + return err + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), uint64(numLoginRetries))) + if err != nil { + c.UI.Error(fmt.Sprintf("Hit maximum retries for consul login: %s", err)) + return 1 + } + c.UI.Info("Consul login complete") + } + if c.flagSkipServiceRegistrationPolling { + return 0 + } + // Now wait for the service to be registered. Do this by querying the Agent for a service + // which maps to this one. + // In the case of ACLs this will match the serviceAccountName, we query on this. + // If ACLs are disabled we query all services and search through + // the list for a service with `meta["pod-name"]` that matches this pod. + data := "" err = backoff.Retry(func() error { - err := common.ConsulLogin(c.consulClient, c.flagBearerTokenFile, c.flagACLAuthMethod, c.flagTokenSinkFile, c.flagMeta) - if err != nil { - c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) + if c.flagACLAuthMethod == "" { + // TODO: can we filter this request somehow? + filter := fmt.Sprintf("Kind != `%s` and Meta.pod-name == %s and Meta.k8s-namespace == %s", "connect-proxy", c.flagPodName, c.flagPodNamespace) + serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) + if err != nil { + c.UI.Error(fmt.Sprintf("Unable to get agent services: %s", err)) + return err + } + for _, y := range serviceList { + // TODO: in theory we've already filtered enough.. can we just return? + if y.Kind != "connect-proxy" && y.Meta["pod-name"] == c.flagPodName && y.Meta["k8s-namespace"] == c.flagPodNamespace { + c.UI.Info(fmt.Sprintf("Registered pod has been detected: %s", y.Meta["pod-name"])) + data = fmt.Sprintf("%s-%s-%s", c.flagPodName, y.ID, "sidecar-proxy") + return nil + } + } + return fmt.Errorf("Unable to find registered service") + } else { + // If ACLs are enabled we don't have permission to go through the list of all services + svc, _, err := c.consulClient.Agent().Service(c.flagServiceAccountName, &api.QueryOptions{}) + if err != nil { + c.UI.Error(fmt.Sprintf("Unable to write proxyid out: %s", err)) + return err + } else { + if svc == nil { + c.UI.Info(fmt.Sprintf("unable to fetch registered service for %v", c.flagServiceAccountName)) + return fmt.Errorf("Unable to find registered service") + } + data = fmt.Sprintf("%s-%s-%s", c.flagPodName, svc.ID, "sidecar-proxy") + } + return nil } - return err - }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), numLoginRetries)) + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), serviceRegistrationPollingRetries)) + if err != nil { + c.UI.Error("Timed out waiting for service registration") + return 1 + } + // Write the proxyid to the shared volume. + err = ioutil.WriteFile(c.flagProxyIDFile, []byte(data), 0444) if err != nil { - c.UI.Error(fmt.Sprintf("Hit maximum retries for consul login: %s", err)) + c.UI.Error(fmt.Sprintf("Unable to write proxyid out: %s", err)) return 1 } - c.UI.Info("Consul login complete") + c.UI.Info("Bootstrapping completed") return 0 } diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 22caf0fd8f..7fc9bdd65a 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -1,6 +1,7 @@ package connectinit import ( + "fmt" "github.com/hashicorp/consul-k8s/consul" "github.com/hashicorp/consul-k8s/subcommand/common" "github.com/hashicorp/consul/api" @@ -13,18 +14,117 @@ import ( "testing" ) +// Tests: +// ACLS disabled (polling on/off) +// ACLS enabled (polling on/off) +// TODO: when endpoints controller is ready "polling" as a flag will be removed and this will just be a test of the command +// passing with ACLs enabled or disabled. +// TestRun_LoginAndPolling tests basic happy path of combinations of ACL/Polling +func TestRun_LoginAndPolling(t *testing.T) { + cases := []struct { + name string + flags []string + secure bool + polling bool + }{ + { + name: "acls enabled, registration polling enabled", + flags: []string{ + "-pod-name", testPodName, + "-pod-namespace", testPodNamespace, + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-service-account-name", testServiceAccountName}, + secure: true, + polling: true, + }, + { + name: "acls enabled, registration polling disabled", + flags: []string{ + "-pod-name", testPodName, + "-pod-namespace", testPodNamespace, + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-service-account-name", testServiceAccountName}, + secure: true, + polling: false, + }, + { + name: "acls disabled, registration polling enabled", + flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace}, + secure: false, + polling: true, + }, + { + name: "acls disabled, registration polling disabled", + flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace}, + secure: false, + polling: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + extraFlags := []string{} + bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") + proxyFile := common.WriteTempFile(t, "") + tokenFile := common.WriteTempFile(t, "") + extraFlags = append(extraFlags, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-proxyid-file", proxyFile) + + // Start the mock Consul server. + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if c.secure { + // ACL login request + if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { + w.Write([]byte(testLoginResponse)) + } + // Agent service get, this is used when ACLs are enabled + if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { + w.Write([]byte(testServiceGetResponse)) + } + } else { + // Agent services list request, used when ACLs are disabled + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(testServiceListResponse)) + } + } + })) + defer consulServer.Close() + serverURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + clientConfig := &api.Config{Address: serverURL.String()} + client, err := consul.NewClient(clientConfig) + require.NoError(t, err) + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: client, + } + c.flags = append(c.flags, extraFlags...) + c.flags = append(c.flags, fmt.Sprintf("-skip-service-registration-polling=%t", !c.polling)) + code := cmd.Run(c.flags) + require.Equal(t, 0, code) + }) + } +} + func TestRun_FlagValidation(t *testing.T) { cases := []struct { flags []string expErr string }{ { - flags: []string{"-acl-auth-method", testAuthMethod}, + flags: []string{}, + expErr: "-pod-name must be set", + }, + { + flags: []string{"-pod-name", testPodName}, + expErr: "-pod-namespace must be set", + }, + { + flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod}, expErr: "-meta must be set", }, { - flags: []string{"-meta", "pod=default/foo"}, - expErr: "-acl-auth-method must be set", + flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", "pod=default/foo"}, + expErr: "-service-account-name must be set", }, } for _, c := range cases { @@ -46,12 +146,14 @@ func TestRun_RetryACLLoginFails(t *testing.T) { cmd := Command{ UI: ui, } - code := cmd.Run([]string{"-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) + code := cmd.Run([]string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-skip-service-registration-polling", "-service-account-name", testServiceAccountName}) require.Equal(t, 1, code) require.Contains(t, ui.ErrorWriter.String(), "Hit maximum retries for consul login") } -func TestRun_withRetries(t *testing.T) { +func TestRun_LoginwithRetries(t *testing.T) { cases := []struct { Description string TestRetry bool @@ -83,7 +185,7 @@ func TestRun_withRetries(t *testing.T) { if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { counter++ if !c.TestRetry || (c.TestRetry && c.LoginAttemptsCount == counter) { - w.Write([]byte(loginResponse)) + w.Write([]byte(testLoginResponse)) } } })) @@ -100,10 +202,13 @@ func TestRun_withRetries(t *testing.T) { UI: ui, consulClient: client, } - code := cmd.Run([]string{"-bearer-token-file", bearerTokenFile, - "-token-sink-file", tokenFile, - "-meta", "host=foo", - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) + code := cmd.Run([]string{ + "-pod-name", testPodName, + "-pod-namespace", testPodNamespace, + "-token-sink-file", tokenFile, "-bearer-token-file", bearerTokenFile, + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-service-account-name", testServiceAccountName, + "-skip-service-registration-polling"}) require.Equal(t, c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. require.Equal(t, c.LoginAttemptsCount, counter) @@ -115,10 +220,44 @@ func TestRun_withRetries(t *testing.T) { } } +const testPodNamespace = "default" +const testPodName = "counting" +const testServiceAccountName = "counting" +const testPodMeta = "pod=default/counting" const testAuthMethod = "consul-k8s-auth-method" +const testServiceGetResponse = `{ + "ID": "counting-counting", + "Service": "counting", + "Tags": [], + "Meta": { + "k8s-namespace": "default", + "pod-name": "counting" + }, + "Port": 9001, + "Address": "10.32.3.22", + "TaggedAddresses": { + "lan_ipv4": { + "Address": "10.32.3.22", + "Port": 9001 + }, + "wan_ipv4": { + "Address": "10.32.3.22", + "Port": 9001 + } + }, + "Weights": { + "Passing": 1, + "Warning": 1 + }, + "EnableTagOverride": false, + "ContentHash": "43efce0313d03c9", + "Datacenter": "dc1" +} +` + // sample response from https://consul.io/api-docs/acl#sample-response -const loginResponse = `{ +const testLoginResponse = `{ "AccessorID": "926e2bd2-b344-d91b-0c83-ae89f372cd9b", "SecretID": "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", "Description": "token created via login", @@ -141,4 +280,68 @@ const loginResponse = `{ "ModifyIndex": 36 }` -const testPodMeta = "pod=default/podName" +const testServiceListResponse = `{ + "counting-counting": { + "ID": "counting-counting", + "Service": "counting", + "Tags": [], + "Meta": { + "k8s-namespace": "default", + "pod-name": "counting" + }, + "Port": 9001, + "Address": "10.32.3.26", + "TaggedAddresses": { + "lan_ipv4": { + "Address": "10.32.3.26", + "Port": 9001 + }, + "wan_ipv4": { + "Address": "10.32.3.26", + "Port": 9001 + } + }, + "Weights": { + "Passing": 1, + "Warning": 1 + }, + "EnableTagOverride": false, + "Datacenter": "dc1" + }, + "counting-counting-sidecar-proxy": { + "Kind": "connect-proxy", + "ID": "counting-counting-sidecar-proxy", + "Service": "counting-sidecar-proxy", + "Tags": [], + "Meta": { + "k8s-namespace": "default", + "pod-name": "counting" + }, + "Port": 20000, + "Address": "10.32.3.26", + "TaggedAddresses": { + "lan_ipv4": { + "Address": "10.32.3.26", + "Port": 20000 + }, + "wan_ipv4": { + "Address": "10.32.3.26", + "Port": 20000 + } + }, + "Weights": { + "Passing": 1, + "Warning": 1 + }, + "EnableTagOverride": false, + "Proxy": { + "DestinationServiceName": "counting", + "DestinationServiceID": "counting-counting", + "LocalServiceAddress": "127.0.0.1", + "LocalServicePort": 9001, + "MeshGateway": {}, + "Expose": {} + }, + "Datacenter": "dc1" + } +}` From 35b837e70219e52b42aa64dc00177b830334f1f2 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 9 Mar 2021 18:07:47 -0600 Subject: [PATCH 02/20] fix merge mess --- connect-inject/container_init.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 9b53e0af9c..6a1f0da0ee 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -325,23 +325,6 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co // and the connect-proxy service should come after the "main" service // because its alias health check depends on the main service to exist. const initContainerCommandTpl = ` -<<<<<<< Updated upstream -======= -{{- if .AuthMethod }} -consul-k8s connect-init -method="{{ .AuthMethod }}" \ - -service-account-name="{{ .ServiceAccountName }}" \ - {{- if.ConsulNamespace }} - {{- if .NamespaceMirroringEnabled }} - {{- /* If namespace mirroring is enabled, the auth method is - defined in the default namespace */}} - -namespace="default" \ - {{- else }} - -namespace="{{ .ConsulNamespace }}" \ - {{- end }} - {{- end }} - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" -{{- end }} ->>>>>>> Stashed changes {{- if .ConsulCACert}} export CONSUL_HTTP_ADDR="https://${HOST_IP}:8501" export CONSUL_GRPC_ADDR="https://${HOST_IP}:8502" From 50a168b9b465883fa912a65f2cc02c97692a6af0 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Wed, 10 Mar 2021 12:37:03 -0600 Subject: [PATCH 03/20] add some more unit tests --- subcommand/connect-init/command.go | 3 +- subcommand/connect-init/command_test.go | 150 +++++++++++++++++++++++- 2 files changed, 146 insertions(+), 7 deletions(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 5121cfbe41..3fd6087afe 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -128,11 +128,10 @@ func (c *Command) Run(args []string) int { data := "" err = backoff.Retry(func() error { if c.flagACLAuthMethod == "" { - // TODO: can we filter this request somehow? filter := fmt.Sprintf("Kind != `%s` and Meta.pod-name == %s and Meta.k8s-namespace == %s", "connect-proxy", c.flagPodName, c.flagPodNamespace) serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) if err != nil { - c.UI.Error(fmt.Sprintf("Unable to get agent services: %s", err)) + c.UI.Error(fmt.Sprintf("Unable to get agent service: %s", err)) return err } for _, y := range serviceList { diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 7fc9bdd65a..9ad04e2732 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -2,16 +2,18 @@ package connectinit import ( "fmt" - "github.com/hashicorp/consul-k8s/consul" - "github.com/hashicorp/consul-k8s/subcommand/common" - "github.com/hashicorp/consul/api" - "github.com/mitchellh/cli" - "github.com/stretchr/testify/require" "io/ioutil" + "math/rand" "net/http" "net/http/httptest" "net/url" "testing" + + "github.com/hashicorp/consul-k8s/consul" + "github.com/hashicorp/consul-k8s/subcommand/common" + "github.com/hashicorp/consul/api" + "github.com/mitchellh/cli" + "github.com/stretchr/testify/require" ) // Tests: @@ -101,6 +103,144 @@ func TestRun_LoginAndPolling(t *testing.T) { c.flags = append(c.flags, fmt.Sprintf("-skip-service-registration-polling=%t", !c.polling)) code := cmd.Run(c.flags) require.Equal(t, 0, code) + // TODO: parse cmd.writer() and require.Contains() it + }) + } +} + +// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid +func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { + cases := []struct { + name string + secure bool + loginresponse string + getlistresponse string + getserviceresponse string + expErr string + }{ + { + name: "acls enabled, acl response invalid", + secure: true, + loginresponse: "", + expErr: "Hit maximum retries for consul login", + }, + { + name: "acls enabled, get service response invalid", + secure: true, + loginresponse: testLoginResponse, + getserviceresponse: "", + expErr: "Timed out waiting for service registration", + }, + { + name: "acls disabled, get list response invalid", + secure: false, + getlistresponse: "", + expErr: "Timed out waiting for service registration", + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + proxyFile := common.WriteTempFile(t, "") + flags := []string{ + "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-meta", + testPodMeta, "-proxyid-file", proxyFile, "-skip-service-registration-polling=false"} + if c.secure { + bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") + tokenFile := common.WriteTempFile(t, "") + flags = append(flags, []string{"-acl-auth-method", testAuthMethod, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-service-account-name", testServiceAccountName}...) + } + + // Start the mock Consul server. + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if c.secure { + // ACL login request + if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { + w.Write([]byte(c.loginresponse)) + } + // Agent service get, this is used when ACLs are enabled + if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { + w.Write([]byte(c.getserviceresponse)) + } + } else { + // Agent services list request, used when ACLs are disabled + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(c.getlistresponse)) + } + } + })) + defer consulServer.Close() + serverURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + clientConfig := &api.Config{Address: serverURL.String()} + client, err := consul.NewClient(clientConfig) + require.NoError(t, err) + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: client, + } + code := cmd.Run(flags) + fmt.Printf("========== here: %v\n", ui.ErrorWriter.String()) + fmt.Printf("========== here: %v\n", ui.OutputWriter.String()) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), c.expErr) + }) + } +} + +// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid +func TestRun_ServiceRegistrationFailsWithBadTproxyIDFile(t *testing.T) { + cases := []struct { + secure bool + }{ + {secure: true}, + {secure: false}, + } + for _, c := range cases { + t.Run(fmt.Sprintf("Secure: %t", c.secure), func(t *testing.T) { + randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) + flags := []string{ + "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-meta", + testPodMeta, "-proxyid-file", randFileName, "-skip-service-registration-polling=false"} + if c.secure { + bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") + tokenFile := common.WriteTempFile(t, "") + flags = append(flags, []string{"-acl-auth-method", testAuthMethod, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-service-account-name", testServiceAccountName}...) + } + + // Start the mock Consul server. + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if c.secure { + // ACL login request + if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { + w.Write([]byte(testLoginResponse)) + } + // Agent service get, this is used when ACLs are enabled + if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { + w.Write([]byte(testServiceGetResponse)) + } + } else { + // Agent services list request, used when ACLs are disabled + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(testServiceListResponse)) + } + } + })) + defer consulServer.Close() + serverURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) + clientConfig := &api.Config{Address: serverURL.String()} + client, err := consul.NewClient(clientConfig) + require.NoError(t, err) + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: client, + } + expErr := fmt.Sprintf("Unable to write proxyid out: open %s: no such file or directory\n", randFileName) + code := cmd.Run(flags) + require.Equal(t, 1, code) + require.Equal(t, expErr, ui.ErrorWriter.String()) }) } } From afca1af1d736259cbca8842ee0b076fb0f421530 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Wed, 10 Mar 2021 17:27:11 -0600 Subject: [PATCH 04/20] plumb through to init_container --- connect-inject/container_init.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 6a1f0da0ee..46e6476016 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -338,6 +338,8 @@ export CONSUL_GRPC_ADDR="${HOST_IP}:8502" {{- end}} {{- if .AuthMethod }} consul-k8s connect-init -acl-auth-method="{{ .AuthMethod }}" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -service-account-name={{ .ServiceAccountName }} \ {{- if .ConsulNamespace }} {{- if .NamespaceMirroringEnabled }} {{- /* If namespace mirroring is enabled, the auth method is @@ -348,6 +350,9 @@ consul-k8s connect-init -acl-auth-method="{{ .AuthMethod }}" \ {{- end }} {{- end }} -meta="pod=${POD_NAMESPACE}/${POD_NAME}" +{{- else }} +consul-k8s connect-init \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} {{- end }} # Register the service. The HCL is stored in the volume so that From 5598cb14316408e0055e6c27ca618f41603c9c34 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Fri, 12 Mar 2021 11:57:52 -0600 Subject: [PATCH 05/20] fix tests --- connect-inject/container_init_test.go | 12 +++++ subcommand/connect-init/command.go | 2 +- subcommand/connect-init/command_test.go | 68 +++++++++++-------------- 3 files changed, 43 insertions(+), 39 deletions(-) diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index ae25de2f59..b4d6e4deb8 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -54,6 +54,8 @@ func TestHandlerContainerInit(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" +consul-k8s connect-init \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -736,6 +738,8 @@ func TestHandlerContainerInit_namespacesEnabled(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" +consul-k8s connect-init \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -808,6 +812,8 @@ EOF `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" +consul-k8s connect-init \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -882,6 +888,8 @@ EOF export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -acl-auth-method="auth-method" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -service-account-name=web \ -namespace="non-default" \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}" @@ -961,6 +969,8 @@ EOF export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -acl-auth-method="auth-method" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -service-account-name=web \ -namespace="default" \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}" @@ -1144,6 +1154,8 @@ func TestHandlerContainerInit_authMethod(t *testing.T) { actual := strings.Join(container.Command, " ") require.Contains(actual, ` consul-k8s connect-init -acl-auth-method="release-name-consul-k8s-auth-method" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -service-account-name=foo \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}"`) require.Contains(actual, ` /consul/connect-inject/consul services register \ diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 3fd6087afe..21f354c68e 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -59,7 +59,7 @@ func (c *Command) init() { c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") c.flagSet.StringVar(&c.flagServiceAccountName, "service-account-name", "", "The service account name for this service.") - // TODO: we dont need this if we can mock the login bits + // TODO: when the endpoints controller manages service registration this can be removed. For now it preserves back compatibility. c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, "The service account name for this service.") diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 9ad04e2732..702825c0f3 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -16,12 +16,9 @@ import ( "github.com/stretchr/testify/require" ) -// Tests: -// ACLS disabled (polling on/off) -// ACLS enabled (polling on/off) -// TODO: when endpoints controller is ready "polling" as a flag will be removed and this will just be a test of the command +// TODO: when endpoints controller is ready the polling flag will be removed and this will just be a test of the command // passing with ACLs enabled or disabled. -// TestRun_LoginAndPolling tests basic happy path of combinations of ACL/Polling +// TestRun_LoginAndPolling tests basic happy path of combinations of ACLs/RegistrationPolling. func TestRun_LoginAndPolling(t *testing.T) { cases := []struct { name string @@ -64,11 +61,11 @@ func TestRun_LoginAndPolling(t *testing.T) { } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - extraFlags := []string{} bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") proxyFile := common.WriteTempFile(t, "") tokenFile := common.WriteTempFile(t, "") - extraFlags = append(extraFlags, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-proxyid-file", proxyFile) + extraFlags := []string{"-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, + "-proxyid-file", proxyFile, fmt.Sprintf("-skip-service-registration-polling=%t", !c.polling)} // Start the mock Consul server. consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -100,42 +97,40 @@ func TestRun_LoginAndPolling(t *testing.T) { consulClient: client, } c.flags = append(c.flags, extraFlags...) - c.flags = append(c.flags, fmt.Sprintf("-skip-service-registration-polling=%t", !c.polling)) code := cmd.Run(c.flags) require.Equal(t, 0, code) - // TODO: parse cmd.writer() and require.Contains() it }) } } -// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid +// TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid error responses. func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { cases := []struct { - name string - secure bool - loginresponse string - getlistresponse string - getserviceresponse string - expErr string + name string + secure bool + loginResponse string + getServicesListResponse string + getServiceResponse string + expErr string }{ { - name: "acls enabled, acl response invalid", + name: "acls enabled, acl login response invalid", secure: true, - loginresponse: "", + loginResponse: "", expErr: "Hit maximum retries for consul login", }, { name: "acls enabled, get service response invalid", secure: true, - loginresponse: testLoginResponse, - getserviceresponse: "", + loginResponse: testLoginResponse, + getServiceResponse: "", expErr: "Timed out waiting for service registration", }, { - name: "acls disabled, get list response invalid", - secure: false, - getlistresponse: "", - expErr: "Timed out waiting for service registration", + name: "acls disabled, get list response invalid", + secure: false, + getServicesListResponse: "", + expErr: "Timed out waiting for service registration", }, } for _, c := range cases { @@ -149,22 +144,21 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { tokenFile := common.WriteTempFile(t, "") flags = append(flags, []string{"-acl-auth-method", testAuthMethod, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-service-account-name", testServiceAccountName}...) } - // Start the mock Consul server. consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if c.secure { - // ACL login request + // ACL login request. if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { - w.Write([]byte(c.loginresponse)) + w.Write([]byte(c.loginResponse)) } - // Agent service get, this is used when ACLs are enabled + // Agent service get. if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { - w.Write([]byte(c.getserviceresponse)) + w.Write([]byte(c.getServiceResponse)) } } else { - // Agent services list request, used when ACLs are disabled + // Agent services list request, used when ACLs are disabled. if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { - w.Write([]byte(c.getlistresponse)) + w.Write([]byte(c.getServicesListResponse)) } } })) @@ -180,15 +174,13 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { consulClient: client, } code := cmd.Run(flags) - fmt.Printf("========== here: %v\n", ui.ErrorWriter.String()) - fmt.Printf("========== here: %v\n", ui.OutputWriter.String()) require.Equal(t, 1, code) require.Contains(t, ui.ErrorWriter.String(), c.expErr) }) } } -// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid +// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid. func TestRun_ServiceRegistrationFailsWithBadTproxyIDFile(t *testing.T) { cases := []struct { secure bool @@ -211,16 +203,16 @@ func TestRun_ServiceRegistrationFailsWithBadTproxyIDFile(t *testing.T) { // Start the mock Consul server. consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if c.secure { - // ACL login request + // ACL login request. if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { w.Write([]byte(testLoginResponse)) } - // Agent service get, this is used when ACLs are enabled + // Agent service get. if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { w.Write([]byte(testServiceGetResponse)) } } else { - // Agent services list request, used when ACLs are disabled + // Agent services list request, used when ACLs are disabled. if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { w.Write([]byte(testServiceListResponse)) } @@ -280,7 +272,7 @@ func TestRun_FlagValidation(t *testing.T) { } } -// TestRun_RetryACLLoginFails tests that after retries the command fails +// TestRun_RetryACLLoginFails tests that after retries the command fails. func TestRun_RetryACLLoginFails(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ From 2af63d91ff6969de68eb12371e36411361347818 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Fri, 12 Mar 2021 12:00:39 -0600 Subject: [PATCH 06/20] fix typo --- subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 21f354c68e..40067d95df 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -61,7 +61,7 @@ func (c *Command) init() { // TODO: when the endpoints controller manages service registration this can be removed. For now it preserves back compatibility. c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, - "The service account name for this service.") + "Flag to preserve backward compatibility with service registration.") c.http = &flags.HTTPFlags{} flags.Merge(c.flagSet, c.http.Flags()) From 8aced8d487bf36cab0d40ad3ea2c0d8e08535f00 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Mon, 15 Mar 2021 17:56:34 -0500 Subject: [PATCH 07/20] refactor tests and command to use new apis and less flags --- connect-inject/container_init.go | 8 +- connect-inject/container_init_test.go | 15 +- subcommand/connect-init/command.go | 92 ++--- subcommand/connect-init/command_test.go | 514 +++++++++++++----------- 4 files changed, 327 insertions(+), 302 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 46e6476016..fdb4c0cf70 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -336,10 +336,9 @@ EOF export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" {{- end}} -{{- if .AuthMethod }} consul-k8s connect-init -acl-auth-method="{{ .AuthMethod }}" \ -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -service-account-name={{ .ServiceAccountName }} \ + {{- if .AuthMethod }} {{- if .ConsulNamespace }} {{- if .NamespaceMirroringEnabled }} {{- /* If namespace mirroring is enabled, the auth method is @@ -350,10 +349,7 @@ consul-k8s connect-init -acl-auth-method="{{ .AuthMethod }}" \ {{- end }} {{- end }} -meta="pod=${POD_NAMESPACE}/${POD_NAME}" -{{- else }} -consul-k8s connect-init \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} -{{- end }} + {{- end }} # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index b4d6e4deb8..fa5ecba84f 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -54,8 +54,8 @@ func TestHandlerContainerInit(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} +consul-k8s connect-init -acl-auth-method="" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -738,8 +738,8 @@ func TestHandlerContainerInit_namespacesEnabled(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} +consul-k8s connect-init -acl-auth-method="" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -812,8 +812,8 @@ EOF `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} +consul-k8s connect-init -acl-auth-method="" \ + -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -889,7 +889,6 @@ export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -acl-auth-method="auth-method" \ -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -service-account-name=web \ -namespace="non-default" \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}" @@ -970,7 +969,6 @@ export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -acl-auth-method="auth-method" \ -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -service-account-name=web \ -namespace="default" \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}" @@ -1155,7 +1153,6 @@ func TestHandlerContainerInit_authMethod(t *testing.T) { require.Contains(actual, ` consul-k8s connect-init -acl-auth-method="release-name-consul-k8s-auth-method" \ -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -service-account-name=foo \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}"`) require.Contains(actual, ` /consul/connect-inject/consul services register \ diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 40067d95df..18105256b9 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -19,19 +19,15 @@ const bearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" const tokenSinkFile = "/consul/connect-inject/acl-token" const proxyIDFile = "/consul/connect-inject/proxyid" const numLoginRetries = 3 -const serviceRegistrationPollingRetries = 6 // This maps to 60 seconds +const serviceRegistrationPollingRetries = 60 // This maps to 60 seconds type Command struct { UI cli.Ui flagACLAuthMethod string // Auth Method to use for ACLs, if enabled. flagMeta map[string]string // Flag for metadata to consul login. - flagBearerTokenFile string // Location of the bearer token. - flagTokenSinkFile string // Location to write the output token. - flagProxyIDFile string // Location to write the output proxyID. flagPodName string // Pod name. flagPodNamespace string // Pod namespace. - flagServiceAccountName string // ServiceAccountName for this service. flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. flagSet *flag.FlagSet @@ -39,6 +35,11 @@ type Command struct { consulClient *api.Client + BearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. + TokenSinkFile string // Location to write the output token. Default is /consul/connect-inject/acl-token. + ProxyIDFile string // Location to write the output proxyID. Default is /consul/connect-inject/proxyid. + ServiceRegistrationPollingAttempts int // Number of times to attempt service registration retry + once sync.Once help string } @@ -49,20 +50,26 @@ func (c *Command) init() { c.flagSet.Var((*flags.FlagMapValue)(&c.flagMeta), "meta", "Metadata to set on the token, formatted as key=value. This flag may be specified multiple "+ "times to set multiple meta fields.") - c.flagSet.StringVar(&c.flagBearerTokenFile, "bearer-token-file", bearerTokenFile, - "Path to a file containing a secret bearer token to use with this auth method. "+ - "Default is /var/run/secrets/kubernetes.io/serviceaccount/token.") - c.flagSet.StringVar(&c.flagTokenSinkFile, "token-sink-file", tokenSinkFile, - "The most recent token's SecretID is kept up to date in this file. Default is /consul/connect-inject/acl-token.") - c.flagSet.StringVar(&c.flagProxyIDFile, "proxyid-file", proxyIDFile, "Location to write the output proxyid file.") c.flagSet.StringVar(&c.flagPodName, "pod-name", "", "Name of the pod.") c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") - c.flagSet.StringVar(&c.flagServiceAccountName, "service-account-name", "", "The service account name for this service.") // TODO: when the endpoints controller manages service registration this can be removed. For now it preserves back compatibility. c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, "Flag to preserve backward compatibility with service registration.") + if c.BearerTokenFile == "" { + c.BearerTokenFile = bearerTokenFile + } + if c.TokenSinkFile == "" { + c.TokenSinkFile = tokenSinkFile + } + if c.ProxyIDFile == "" { + c.ProxyIDFile = proxyIDFile + } + if c.ServiceRegistrationPollingAttempts == 0 { + c.ServiceRegistrationPollingAttempts = serviceRegistrationPollingRetries + } + c.http = &flags.HTTPFlags{} flags.Merge(c.flagSet, c.http.Flags()) c.help = flags.Usage(help, c.flagSet) @@ -99,12 +106,8 @@ func (c *Command) Run(args []string) int { c.UI.Error("-meta must be set") return 1 } - if c.flagServiceAccountName == "" { - c.UI.Error("-service-account-name must be set") - return 1 - } err = backoff.Retry(func() error { - err := common.ConsulLogin(c.consulClient, c.flagBearerTokenFile, c.flagACLAuthMethod, c.flagTokenSinkFile, c.flagMeta) + err := common.ConsulLogin(c.consulClient, c.BearerTokenFile, c.flagACLAuthMethod, c.TokenSinkFile, c.flagMeta) if err != nil { c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) } @@ -121,55 +124,40 @@ func (c *Command) Run(args []string) int { } // Now wait for the service to be registered. Do this by querying the Agent for a service - // which maps to this one. - // In the case of ACLs this will match the serviceAccountName, we query on this. - // If ACLs are disabled we query all services and search through - // the list for a service with `meta["pod-name"]` that matches this pod. + // which maps to this pod+namespace. data := "" err = backoff.Retry(func() error { - if c.flagACLAuthMethod == "" { - filter := fmt.Sprintf("Kind != `%s` and Meta.pod-name == %s and Meta.k8s-namespace == %s", "connect-proxy", c.flagPodName, c.flagPodNamespace) - serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) - if err != nil { - c.UI.Error(fmt.Sprintf("Unable to get agent service: %s", err)) - return err - } - for _, y := range serviceList { - // TODO: in theory we've already filtered enough.. can we just return? - if y.Kind != "connect-proxy" && y.Meta["pod-name"] == c.flagPodName && y.Meta["k8s-namespace"] == c.flagPodNamespace { - c.UI.Info(fmt.Sprintf("Registered pod has been detected: %s", y.Meta["pod-name"])) - data = fmt.Sprintf("%s-%s-%s", c.flagPodName, y.ID, "sidecar-proxy") - return nil - } - } + filter := fmt.Sprintf("Meta[\"pod-name\"] == %s and Meta[\"k8s-namespace\"] == %s", c.flagPodName, c.flagPodNamespace) + serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) + if err != nil { + c.UI.Error(fmt.Sprintf("Unable to get agent service: %s", err)) + return err + } + // Wait for the service and the connect-proxy service to be registered. + if len(serviceList) != 2 { return fmt.Errorf("Unable to find registered service") - } else { - // If ACLs are enabled we don't have permission to go through the list of all services - svc, _, err := c.consulClient.Agent().Service(c.flagServiceAccountName, &api.QueryOptions{}) - if err != nil { - c.UI.Error(fmt.Sprintf("Unable to write proxyid out: %s", err)) - return err - } else { - if svc == nil { - c.UI.Info(fmt.Sprintf("unable to fetch registered service for %v", c.flagServiceAccountName)) - return fmt.Errorf("Unable to find registered service") - } - data = fmt.Sprintf("%s-%s-%s", c.flagPodName, svc.ID, "sidecar-proxy") + } + for _, y := range serviceList { + c.UI.Info(fmt.Sprintf("Registered pod has been detected: %s", y.Meta["pod-name"])) + if y.Kind == "connect-proxy" { + // This is the proxy service ID + data = y.ID + return nil } - return nil } - }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), serviceRegistrationPollingRetries)) + return fmt.Errorf("Unable to find registered service") + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), uint64(c.ServiceRegistrationPollingAttempts))) if err != nil { c.UI.Error("Timed out waiting for service registration") return 1 } // Write the proxyid to the shared volume. - err = ioutil.WriteFile(c.flagProxyIDFile, []byte(data), 0444) + err = ioutil.WriteFile(c.ProxyIDFile, []byte(data), 0444) if err != nil { c.UI.Error(fmt.Sprintf("Unable to write proxyid out: %s", err)) return 1 } - c.UI.Info("Bootstrapping completed") + c.UI.Info("Service registration completed") return 0 } diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 702825c0f3..3948bdb44b 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -8,265 +8,280 @@ import ( "net/http/httptest" "net/url" "testing" + "time" "github.com/hashicorp/consul-k8s/consul" "github.com/hashicorp/consul-k8s/subcommand/common" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil" "github.com/mitchellh/cli" "github.com/stretchr/testify/require" ) -// TODO: when endpoints controller is ready the polling flag will be removed and this will just be a test of the command -// passing with ACLs enabled or disabled. -// TestRun_LoginAndPolling tests basic happy path of combinations of ACLs/RegistrationPolling. -func TestRun_LoginAndPolling(t *testing.T) { +// NOTES: Test coverage works as follows: +// 1. All tests which are specific to 'login' are in subcommand/common/common_test.go +// 2. The rest are here: +// ACLS enabled happy path // done +// ACLs disabled happy path (uses a valid agent) // done +// ACLs disabled invalid response on service get // covered by ACLs enabled test bc its the same API call +// invalid proxyid file // done +// ACLS enabled fails due to invalid server responses // done +// ACLS enabled fails due to IO (invalid bearer, tokenfile) // covered by common_test.go +// test that retries work for service polling + +// TestRun_RetryServicePolling starts the command and does not register the consul services +// for 2 seconds and then asserts that the proxyid file is written correctly. +func TestRun_RetryServicePolling(t *testing.T) { + require := require.New(t) + proxyFile := common.WriteTempFile(t, "") + + // Start Consul server. + server, err := testutil.NewTestServerConfigT(t, nil) + defer server.Stop() + require.NoError(err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) + require.NoError(err) + + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: consulClient, + ProxyIDFile: proxyFile, + ServiceRegistrationPollingAttempts: 10, + } + // Start the command asynchronously, later registering the services. + exitChan := runCommandAsynchronously(&cmd, defaultTestFlags) + // Wait a moment. + time.Sleep(time.Second * 2) + // Register Consul services. + testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} + for _, svc := range testConsulServices { + require.NoError(consulClient.Agent().ServiceRegister(&svc)) + } + + // Assert that it exits cleanly or timeout. + select { + case exitCode := <-exitChan: + require.Equal(0, exitCode) + case <-time.After(time.Second * 10): + // Fail if the stopCh was not caught. + require.Fail("timeout waiting for command to exit") + } + + // Validate contents of proxyFile. + data, err := ioutil.ReadFile(proxyFile) + require.NoError(err) + require.Contains("counting-counting-sidecar-proxy", string(data)) +} + +func TestRun_FlagValidation(t *testing.T) { cases := []struct { - name string - flags []string - secure bool - polling bool + flags []string + expErr string }{ { - name: "acls enabled, registration polling enabled", - flags: []string{ - "-pod-name", testPodName, - "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, - "-service-account-name", testServiceAccountName}, - secure: true, - polling: true, - }, - { - name: "acls enabled, registration polling disabled", - flags: []string{ - "-pod-name", testPodName, - "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, - "-service-account-name", testServiceAccountName}, - secure: true, - polling: false, + flags: []string{}, + expErr: "-pod-name must be set", }, { - name: "acls disabled, registration polling enabled", - flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace}, - secure: false, - polling: true, + flags: []string{"-pod-name", testPodName}, + expErr: "-pod-namespace must be set", }, { - name: "acls disabled, registration polling disabled", - flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace}, - secure: false, - polling: false, + flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod}, + expErr: "-meta must be set", }, } for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") - proxyFile := common.WriteTempFile(t, "") - tokenFile := common.WriteTempFile(t, "") - extraFlags := []string{"-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, - "-proxyid-file", proxyFile, fmt.Sprintf("-skip-service-registration-polling=%t", !c.polling)} - - // Start the mock Consul server. - consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if c.secure { - // ACL login request - if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { - w.Write([]byte(testLoginResponse)) - } - // Agent service get, this is used when ACLs are enabled - if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { - w.Write([]byte(testServiceGetResponse)) - } - } else { - // Agent services list request, used when ACLs are disabled - if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { - w.Write([]byte(testServiceListResponse)) - } - } - })) - defer consulServer.Close() - serverURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) - clientConfig := &api.Config{Address: serverURL.String()} - client, err := consul.NewClient(clientConfig) - require.NoError(t, err) + t.Run(c.expErr, func(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ - UI: ui, - consulClient: client, + UI: ui, } - c.flags = append(c.flags, extraFlags...) code := cmd.Run(c.flags) - require.Equal(t, 0, code) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), c.expErr) }) } } +// This test mocks ACL login and the service list response (/v1/agent/services), +// the later of which is also validated by an actual agent in TestRun_happyPathNoACLs(). +func TestRun_happyPathACLs(t *testing.T) { + require := require.New(t) + + bearerFile := common.WriteTempFile(t, "bearerTokenFile") + proxyFile := common.WriteTempFile(t, "") + tokenFile := common.WriteTempFile(t, "") + + // Start the mock Consul server. + consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // ACL login request. + if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { + w.Write([]byte(testLoginResponse)) + } + // Get list of Agent Services. + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(testServiceListResponse)) + } + })) + defer consulServer.Close() + serverURL, err := url.Parse(consulServer.URL) + require.NoError(err) + clientConfig := &api.Config{Address: serverURL.String()} + client, err := consul.NewClient(clientConfig) + require.NoError(err) + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: client, + BearerTokenFile: bearerFile, + TokenSinkFile: tokenFile, + ProxyIDFile: proxyFile, + } + flags := []string{"-pod-name", testPodName, + "-pod-namespace", testPodNamespace, + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-skip-service-registration-polling=false"} + // Run the command. + code := cmd.Run(flags) + require.Equal(0, code) + + // Validate contents of proxyFile. + data, err := ioutil.ReadFile(proxyFile) + require.NoError(err) + require.Contains("counting-counting-sidecar-proxy", string(data)) +} + +// This test validates happy path without ACLs : wait on proxy+service to be registered and write out proxyid file +func TestRun_happyPathNoACLs(t *testing.T) { + t.Parallel() + require := require.New(t) + // This is the output file for the proxyid. + proxyFile := common.WriteTempFile(t, "") + + // Start Consul server. + server, err := testutil.NewTestServerConfigT(t, nil) + defer server.Stop() + require.NoError(err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) + require.NoError(err) + + // Register Consul services. + testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} + for _, svc := range testConsulServices { + require.NoError(consulClient.Agent().ServiceRegister(&svc)) + } + + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: consulClient, + ProxyIDFile: proxyFile, + } + code := cmd.Run(defaultTestFlags) + require.Equal(0, code) + + // Validate contents of proxyFile. + data, err := ioutil.ReadFile(proxyFile) + require.NoError(err) + require.Contains("counting-counting-sidecar-proxy", string(data)) +} + +// This functions as coverage for both ACL and non-ACL codepaths of this error case. +func TestRun_invalidProxyFile(t *testing.T) { + require := require.New(t) + // This is the output file for the proxyid. + randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) + + // Start Consul server. + server, err := testutil.NewTestServerConfigT(t, nil) + defer server.Stop() + require.NoError(err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) + require.NoError(err) + + // Register Consul services. + testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} + for _, svc := range testConsulServices { + require.NoError(consulClient.Agent().ServiceRegister(&svc)) + } + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: consulClient, + ProxyIDFile: randFileName, + ServiceRegistrationPollingAttempts: 3, + } + expErr := fmt.Sprintf("Unable to write proxyid out: open %s: no such file or directory\n", randFileName) + code := cmd.Run(defaultTestFlags) + require.Equal(1, code) + require.Equal(expErr, ui.ErrorWriter.String()) +} + // TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid error responses. func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { cases := []struct { name string - secure bool loginResponse string getServicesListResponse string - getServiceResponse string expErr string }{ { name: "acls enabled, acl login response invalid", - secure: true, loginResponse: "", expErr: "Hit maximum retries for consul login", }, { - name: "acls enabled, get service response invalid", - secure: true, - loginResponse: testLoginResponse, - getServiceResponse: "", - expErr: "Timed out waiting for service registration", - }, - { - name: "acls disabled, get list response invalid", - secure: false, + name: "acls enabled, get service response invalid", + loginResponse: testLoginResponse, getServicesListResponse: "", expErr: "Timed out waiting for service registration", }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - proxyFile := common.WriteTempFile(t, "") - flags := []string{ - "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-meta", - testPodMeta, "-proxyid-file", proxyFile, "-skip-service-registration-polling=false"} - if c.secure { - bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") - tokenFile := common.WriteTempFile(t, "") - flags = append(flags, []string{"-acl-auth-method", testAuthMethod, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-service-account-name", testServiceAccountName}...) - } + bearerFile := common.WriteTempFile(t, "bearerTokenFile") + tokenFile := common.WriteTempFile(t, "") + // Start the mock Consul server. consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if c.secure { - // ACL login request. - if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { - w.Write([]byte(c.loginResponse)) - } - // Agent service get. - if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { - w.Write([]byte(c.getServiceResponse)) - } - } else { - // Agent services list request, used when ACLs are disabled. - if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { - w.Write([]byte(c.getServicesListResponse)) - } + // ACL login request. + if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { + w.Write([]byte(c.loginResponse)) + } + // Agent Services get. + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(c.getServicesListResponse)) } })) defer consulServer.Close() + // Setup the Client. serverURL, err := url.Parse(consulServer.URL) require.NoError(t, err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) require.NoError(t, err) + + // Setup the Command. ui := cli.NewMockUi() cmd := Command{ - UI: ui, - consulClient: client, + UI: ui, + consulClient: client, + BearerTokenFile: bearerFile, + TokenSinkFile: tokenFile, + ServiceRegistrationPollingAttempts: 2, } - code := cmd.Run(flags) - require.Equal(t, 1, code) - require.Contains(t, ui.ErrorWriter.String(), c.expErr) - }) - } -} -// TestRun_ServiceRegistrationFailsWithBadTproxyIDFile passes in an invalid output file for the proxyid. -func TestRun_ServiceRegistrationFailsWithBadTproxyIDFile(t *testing.T) { - cases := []struct { - secure bool - }{ - {secure: true}, - {secure: false}, - } - for _, c := range cases { - t.Run(fmt.Sprintf("Secure: %t", c.secure), func(t *testing.T) { - randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) flags := []string{ - "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-meta", - testPodMeta, "-proxyid-file", randFileName, "-skip-service-registration-polling=false"} - if c.secure { - bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") - tokenFile := common.WriteTempFile(t, "") - flags = append(flags, []string{"-acl-auth-method", testAuthMethod, "-bearer-token-file", bearerTokenFile, "-token-sink-file", tokenFile, "-service-account-name", testServiceAccountName}...) - } - - // Start the mock Consul server. - consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if c.secure { - // ACL login request. - if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { - w.Write([]byte(testLoginResponse)) - } - // Agent service get. - if r != nil && r.URL.Path == fmt.Sprintf("/v1/agent/service/%s", testServiceAccountName) && r.Method == "GET" { - w.Write([]byte(testServiceGetResponse)) - } - } else { - // Agent services list request, used when ACLs are disabled. - if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { - w.Write([]byte(testServiceListResponse)) - } - } - })) - defer consulServer.Close() - serverURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) - clientConfig := &api.Config{Address: serverURL.String()} - client, err := consul.NewClient(clientConfig) - require.NoError(t, err) - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, - consulClient: client, - } - expErr := fmt.Sprintf("Unable to write proxyid out: open %s: no such file or directory\n", randFileName) + "-pod-name", testPodName, "-pod-namespace", testPodNamespace, + "-meta", testPodMeta, "-acl-auth-method", testAuthMethod, + "-skip-service-registration-polling=false"} code := cmd.Run(flags) require.Equal(t, 1, code) - require.Equal(t, expErr, ui.ErrorWriter.String()) - }) - } -} - -func TestRun_FlagValidation(t *testing.T) { - cases := []struct { - flags []string - expErr string - }{ - { - flags: []string{}, - expErr: "-pod-name must be set", - }, - { - flags: []string{"-pod-name", testPodName}, - expErr: "-pod-namespace must be set", - }, - { - flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod}, - expErr: "-meta must be set", - }, - { - flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", "pod=default/foo"}, - expErr: "-service-account-name must be set", - }, - } - for _, c := range cases { - t.Run(c.expErr, func(t *testing.T) { - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, - } - code := cmd.Run(c.flags) - require.Equal(t, 1, code) require.Contains(t, ui.ErrorWriter.String(), c.expErr) }) } @@ -279,12 +294,12 @@ func TestRun_RetryACLLoginFails(t *testing.T) { UI: ui, } code := cmd.Run([]string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, - "-skip-service-registration-polling", "-service-account-name", testServiceAccountName}) + "-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) require.Equal(t, 1, code) require.Contains(t, ui.ErrorWriter.String(), "Hit maximum retries for consul login") } +// Tests ACL Login with Retries func TestRun_LoginwithRetries(t *testing.T) { cases := []struct { Description string @@ -308,18 +323,24 @@ func TestRun_LoginwithRetries(t *testing.T) { for _, c := range cases { t.Run(c.Description, func(t *testing.T) { // Create a fake input bearer token file and an output file. - bearerTokenFile := common.WriteTempFile(t, "bearerTokenFile") + bearerFile := common.WriteTempFile(t, "bearerTokenFile") tokenFile := common.WriteTempFile(t, "") + proxyFile := common.WriteTempFile(t, "") // Start the mock Consul server. counter := 0 consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // ACL Login if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { counter++ if !c.TestRetry || (c.TestRetry && c.LoginAttemptsCount == counter) { w.Write([]byte(testLoginResponse)) } } + // Agent Services get. + if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { + w.Write([]byte(testServiceListResponse)) + } })) defer consulServer.Close() @@ -331,16 +352,17 @@ func TestRun_LoginwithRetries(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ - UI: ui, - consulClient: client, + UI: ui, + consulClient: client, + TokenSinkFile: tokenFile, + BearerTokenFile: bearerFile, + ProxyIDFile: proxyFile, } code := cmd.Run([]string{ "-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-token-sink-file", tokenFile, "-bearer-token-file", bearerTokenFile, "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, - "-service-account-name", testServiceAccountName, - "-skip-service-registration-polling"}) + "-skip-service-registration-polling=false"}) require.Equal(t, c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. require.Equal(t, c.LoginAttemptsCount, counter) @@ -348,46 +370,19 @@ func TestRun_LoginwithRetries(t *testing.T) { data, err := ioutil.ReadFile(tokenFile) require.NoError(t, err) require.Contains(t, "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", string(data)) + // Validate contents of proxyFile. + proxydata, err := ioutil.ReadFile(proxyFile) + require.NoError(t, err) + require.Contains(t, "counting-counting-sidecar-proxy", string(proxydata)) }) } } const testPodNamespace = "default" const testPodName = "counting" -const testServiceAccountName = "counting" const testPodMeta = "pod=default/counting" const testAuthMethod = "consul-k8s-auth-method" -const testServiceGetResponse = `{ - "ID": "counting-counting", - "Service": "counting", - "Tags": [], - "Meta": { - "k8s-namespace": "default", - "pod-name": "counting" - }, - "Port": 9001, - "Address": "10.32.3.22", - "TaggedAddresses": { - "lan_ipv4": { - "Address": "10.32.3.22", - "Port": 9001 - }, - "wan_ipv4": { - "Address": "10.32.3.22", - "Port": 9001 - } - }, - "Weights": { - "Passing": 1, - "Warning": 1 - }, - "EnableTagOverride": false, - "ContentHash": "43efce0313d03c9", - "Datacenter": "dc1" -} -` - // sample response from https://consul.io/api-docs/acl#sample-response const testLoginResponse = `{ "AccessorID": "926e2bd2-b344-d91b-0c83-ae89f372cd9b", @@ -477,3 +472,52 @@ const testServiceListResponse = `{ "Datacenter": "dc1" } }` +const metaKeyPodName = "pod-name" +const metaKeyKubeNS = "k8s-namespace" + +var ( + consulCountingSvc = api.AgentServiceRegistration{ + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + } + consulCountingSvcSidecar = api.AgentServiceRegistration{ + ID: "counting-counting-sidecar-proxy", + Name: "counting-sidecar-proxy", + Kind: "connect-proxy", + Proxy: &api.AgentServiceConnectProxyConfig{ + DestinationServiceName: "foo", + DestinationServiceID: "foo", + Config: nil, + Upstreams: nil, + }, + Port: 9999, + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + } + defaultTestFlags = []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-skip-service-registration-polling=false"} +) + +// This function starts the command asynchronously and returns a non-blocking chan. +// When finished, the command will send its exit code to the channel. +// Note that it's the responsibility of the caller to terminate the command by calling stopCommand, +// otherwise it can run forever. +func runCommandAsynchronously(cmd *Command, args []string) chan int { + // We have to run cmd.init() to ensure that the channel the command is + // using to watch for os interrupts is initialized. If we don't do this, + // then if stopCommand is called immediately, it will block forever + // because it calls interrupt() which will attempt to send on a nil channel. + cmd.init() + exitChan := make(chan int, 1) + go func() { + exitChan <- cmd.Run(args) + }() + return exitChan +} From 9e15d0b6c263c445095d86ec1c4e4e13e5a64942 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Mon, 15 Mar 2021 17:58:16 -0500 Subject: [PATCH 08/20] fixes --- connect-inject/container_init.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index fdb4c0cf70..4f071b1e00 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -19,10 +19,9 @@ const ( ) type initContainerCommandData struct { - ServiceName string - ServiceAccountName string - ProxyServiceName string - ServicePort int32 + ServiceName string + ProxyServiceName string + ServicePort int32 // ServiceProtocol is the protocol for the service-defaults config // that will be written if WriteServiceDefaults is true. ServiceProtocol string @@ -96,7 +95,6 @@ func (h *Handler) containerInit(pod *corev1.Pod, k8sNamespace string) (corev1.Co ConsulCACert: h.ConsulCACert, MetaKeyPodName: MetaKeyPodName, MetaKeyKubeNS: MetaKeyKubeNS, - ServiceAccountName: pod.Spec.ServiceAccountName, } if data.ServiceName == "" { // Assertion, since we call defaultAnnotations above and do From 6561b177214d791c45e6df917fbd2ad5831bab61 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 16 Mar 2021 10:53:31 -0500 Subject: [PATCH 09/20] test cleanup --- subcommand/connect-init/command.go | 46 +++---- subcommand/connect-init/command_test.go | 152 +++++++++++++----------- 2 files changed, 107 insertions(+), 91 deletions(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 18105256b9..a158364fe3 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -15,11 +15,12 @@ import ( "github.com/mitchellh/cli" ) -const bearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" -const tokenSinkFile = "/consul/connect-inject/acl-token" -const proxyIDFile = "/consul/connect-inject/proxyid" -const numLoginRetries = 3 -const serviceRegistrationPollingRetries = 60 // This maps to 60 seconds +const defaultBearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" +const defaultTokenSinkFile = "/consul/connect-inject/acl-token" +const defaultProxyIDFile = "/consul/connect-inject/proxyid" + +const numLoginRetries = 3 // The number of times to attempt ACL Login. +const defaultServicePollingRetries = 60 // The number of times to attempt to read this service. (60s) type Command struct { UI cli.Ui @@ -30,16 +31,15 @@ type Command struct { flagPodNamespace string // Pod namespace. flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. - flagSet *flag.FlagSet - http *flags.HTTPFlags + BearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. + TokenSinkFile string // Location to write the output token. Default is defaultTokenSinkFile. + ProxyIDFile string // Location to write the output proxyID. Default is defaultProxyIDFile. + ServiceRegistrationPollingAttempts int // Number of times to poll for this service to be registered. + flagSet *flag.FlagSet + http *flags.HTTPFlags consulClient *api.Client - BearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. - TokenSinkFile string // Location to write the output token. Default is /consul/connect-inject/acl-token. - ProxyIDFile string // Location to write the output proxyID. Default is /consul/connect-inject/proxyid. - ServiceRegistrationPollingAttempts int // Number of times to attempt service registration retry - once sync.Once help string } @@ -58,16 +58,16 @@ func (c *Command) init() { "Flag to preserve backward compatibility with service registration.") if c.BearerTokenFile == "" { - c.BearerTokenFile = bearerTokenFile + c.BearerTokenFile = defaultBearerTokenFile } if c.TokenSinkFile == "" { - c.TokenSinkFile = tokenSinkFile + c.TokenSinkFile = defaultTokenSinkFile } if c.ProxyIDFile == "" { - c.ProxyIDFile = proxyIDFile + c.ProxyIDFile = defaultProxyIDFile } if c.ServiceRegistrationPollingAttempts == 0 { - c.ServiceRegistrationPollingAttempts = serviceRegistrationPollingRetries + c.ServiceRegistrationPollingAttempts = defaultServicePollingRetries } c.http = &flags.HTTPFlags{} @@ -130,22 +130,26 @@ func (c *Command) Run(args []string) int { filter := fmt.Sprintf("Meta[\"pod-name\"] == %s and Meta[\"k8s-namespace\"] == %s", c.flagPodName, c.flagPodNamespace) serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) if err != nil { - c.UI.Error(fmt.Sprintf("Unable to get agent service: %s", err)) + c.UI.Error(fmt.Sprintf("Unable to get Agent services: %s", err)) return err } // Wait for the service and the connect-proxy service to be registered. if len(serviceList) != 2 { - return fmt.Errorf("Unable to find registered service") + c.UI.Info("Unable to find registered services; Retrying") + return fmt.Errorf("Did not find correct number of services: %d", len(serviceList)) } for _, y := range serviceList { - c.UI.Info(fmt.Sprintf("Registered pod has been detected: %s", y.Meta["pod-name"])) + c.UI.Info(fmt.Sprintf("Registered service has been detected: %s", y.Service)) if y.Kind == "connect-proxy" { - // This is the proxy service ID + // This is the proxy service ID. data = y.ID return nil } } - return fmt.Errorf("Unable to find registered service") + // In theory we can't reach this point unless we have 2 services registered against + // this pod and neither are the connect-proxy, we don't support this case anyway but it + // is necessary to return from the function. + return fmt.Errorf("Unable to find registered connect-proxy service") }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), uint64(c.ServiceRegistrationPollingAttempts))) if err != nil { c.UI.Error("Timed out waiting for service registration") diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 3948bdb44b..96010d6578 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -29,53 +29,9 @@ import ( // ACLS enabled fails due to IO (invalid bearer, tokenfile) // covered by common_test.go // test that retries work for service polling -// TestRun_RetryServicePolling starts the command and does not register the consul services -// for 2 seconds and then asserts that the proxyid file is written correctly. -func TestRun_RetryServicePolling(t *testing.T) { - require := require.New(t) - proxyFile := common.WriteTempFile(t, "") - - // Start Consul server. - server, err := testutil.NewTestServerConfigT(t, nil) - defer server.Stop() - require.NoError(err) - server.WaitForLeader(t) - consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) - require.NoError(err) - - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, - consulClient: consulClient, - ProxyIDFile: proxyFile, - ServiceRegistrationPollingAttempts: 10, - } - // Start the command asynchronously, later registering the services. - exitChan := runCommandAsynchronously(&cmd, defaultTestFlags) - // Wait a moment. - time.Sleep(time.Second * 2) - // Register Consul services. - testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} - for _, svc := range testConsulServices { - require.NoError(consulClient.Agent().ServiceRegister(&svc)) - } - - // Assert that it exits cleanly or timeout. - select { - case exitCode := <-exitChan: - require.Equal(0, exitCode) - case <-time.After(time.Second * 10): - // Fail if the stopCh was not caught. - require.Fail("timeout waiting for command to exit") - } - - // Validate contents of proxyFile. - data, err := ioutil.ReadFile(proxyFile) - require.NoError(err) - require.Contains("counting-counting-sidecar-proxy", string(data)) -} - func TestRun_FlagValidation(t *testing.T) { + t.Parallel() + require := require.New(t) cases := []struct { flags []string expErr string @@ -100,8 +56,8 @@ func TestRun_FlagValidation(t *testing.T) { UI: ui, } code := cmd.Run(c.flags) - require.Equal(t, 1, code) - require.Contains(t, ui.ErrorWriter.String(), c.expErr) + require.Equal(1, code) + require.Contains(ui.ErrorWriter.String(), c.expErr) }) } } @@ -151,7 +107,7 @@ func TestRun_happyPathACLs(t *testing.T) { // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) require.NoError(err) - require.Contains("counting-counting-sidecar-proxy", string(data)) + require.Contains(string(data), "counting-counting-sidecar-proxy") } // This test validates happy path without ACLs : wait on proxy+service to be registered and write out proxyid file @@ -187,11 +143,63 @@ func TestRun_happyPathNoACLs(t *testing.T) { // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) require.NoError(err) - require.Contains("counting-counting-sidecar-proxy", string(data)) + require.Contains(string(data), "counting-counting-sidecar-proxy") } -// This functions as coverage for both ACL and non-ACL codepaths of this error case. +// TestRun_RetryServicePolling starts the command and does not register the consul service +// for 2 seconds and then asserts that the proxyid file gets written correctly. +func TestRun_RetryServicePolling(t *testing.T) { + t.Parallel() + require := require.New(t) + proxyFile := common.WriteTempFile(t, "") + + // Start Consul server. + server, err := testutil.NewTestServerConfigT(t, nil) + defer server.Stop() + require.NoError(err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) + require.NoError(err) + + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + consulClient: consulClient, + ProxyIDFile: proxyFile, + ServiceRegistrationPollingAttempts: 10, + } + // Start the command asynchronously, later registering the services. + exitChan := runCommandAsynchronously(&cmd, defaultTestFlags) + // Wait a moment. + time.Sleep(time.Second * 1) + // Register Consul services. + testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} + for _, svc := range testConsulServices { + require.NoError(consulClient.Agent().ServiceRegister(&svc)) + time.Sleep(time.Second * 2) + } + + // Assert that it exits cleanly or timeout. + select { + case exitCode := <-exitChan: + require.Equal(0, exitCode) + case <-time.After(time.Second * 10): + // Fail if the stopCh was not caught. + require.Fail("timeout waiting for command to exit") + } + // Validate that we hit the retry logic when proxy service is not registered yet. + require.Contains(ui.OutputWriter.String(), "Unable to find registered services; Retrying") + + // Validate contents of proxyFile. + data, err := ioutil.ReadFile(proxyFile) + require.NoError(err) + require.Contains(string(data), "counting-counting-sidecar-proxy") +} + +// TestRun_invalidProxyFile validates that we correctly fail in case the proxyid file +// is not writable. This functions as coverage for both ACL and non-ACL codepaths. func TestRun_invalidProxyFile(t *testing.T) { + t.Parallel() require := require.New(t) // This is the output file for the proxyid. randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) @@ -222,8 +230,9 @@ func TestRun_invalidProxyFile(t *testing.T) { require.Equal(expErr, ui.ErrorWriter.String()) } -// TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid error responses. +// TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid server responses. func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { + require := require.New(t) cases := []struct { name string loginResponse string @@ -261,10 +270,10 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { defer consulServer.Close() // Setup the Client. serverURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) + require.NoError(err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) - require.NoError(t, err) + require.NoError(err) // Setup the Command. ui := cli.NewMockUi() @@ -281,26 +290,29 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { "-meta", testPodMeta, "-acl-auth-method", testAuthMethod, "-skip-service-registration-polling=false"} code := cmd.Run(flags) - require.Equal(t, 1, code) - require.Contains(t, ui.ErrorWriter.String(), c.expErr) + require.Equal(1, code) + require.Contains(ui.ErrorWriter.String(), c.expErr) }) } } -// TestRun_RetryACLLoginFails tests that after retries the command fails. +// TestRun_RetryACLLoginFails tests that after retry the command fails. func TestRun_RetryACLLoginFails(t *testing.T) { + t.Parallel() + require := require.New(t) ui := cli.NewMockUi() cmd := Command{ UI: ui, } code := cmd.Run([]string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) - require.Equal(t, 1, code) - require.Contains(t, ui.ErrorWriter.String(), "Hit maximum retries for consul login") + require.Equal(1, code) + require.Contains(ui.ErrorWriter.String(), "Hit maximum retries for consul login") } -// Tests ACL Login with Retries +// Tests ACL Login with Retries. func TestRun_LoginwithRetries(t *testing.T) { + require := require.New(t) cases := []struct { Description string TestRetry bool @@ -310,7 +322,7 @@ func TestRun_LoginwithRetries(t *testing.T) { { Description: "Login succeeds without retries", TestRetry: false, - LoginAttemptsCount: 1, // 1 because we dont actually retry + LoginAttemptsCount: 1, // 1 because we dont actually retry. ExpCode: 0, }, { @@ -330,7 +342,7 @@ func TestRun_LoginwithRetries(t *testing.T) { // Start the mock Consul server. counter := 0 consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // ACL Login + // ACL Login. if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { counter++ if !c.TestRetry || (c.TestRetry && c.LoginAttemptsCount == counter) { @@ -345,10 +357,10 @@ func TestRun_LoginwithRetries(t *testing.T) { defer consulServer.Close() serverURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) + require.NoError(err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) - require.NoError(t, err) + require.NoError(err) ui := cli.NewMockUi() cmd := Command{ @@ -363,17 +375,17 @@ func TestRun_LoginwithRetries(t *testing.T) { "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, "-skip-service-registration-polling=false"}) - require.Equal(t, c.ExpCode, code) + require.Equal(c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. - require.Equal(t, c.LoginAttemptsCount, counter) + require.Equal(c.LoginAttemptsCount, counter) // Validate that the token was written to disk if we succeeded. data, err := ioutil.ReadFile(tokenFile) - require.NoError(t, err) - require.Contains(t, "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", string(data)) + require.NoError(err) + require.Contains(string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586") // Validate contents of proxyFile. proxydata, err := ioutil.ReadFile(proxyFile) - require.NoError(t, err) - require.Contains(t, "counting-counting-sidecar-proxy", string(proxydata)) + require.NoError(err) + require.Contains(string(proxydata), "counting-counting-sidecar-proxy") }) } } From 9a477b349a879dc79101f7473db37e60eb754054 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 16 Mar 2021 11:25:18 -0500 Subject: [PATCH 10/20] remove parallel to test something --- subcommand/connect-init/command_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 96010d6578..87affbf290 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -112,7 +112,6 @@ func TestRun_happyPathACLs(t *testing.T) { // This test validates happy path without ACLs : wait on proxy+service to be registered and write out proxyid file func TestRun_happyPathNoACLs(t *testing.T) { - t.Parallel() require := require.New(t) // This is the output file for the proxyid. proxyFile := common.WriteTempFile(t, "") @@ -149,7 +148,6 @@ func TestRun_happyPathNoACLs(t *testing.T) { // TestRun_RetryServicePolling starts the command and does not register the consul service // for 2 seconds and then asserts that the proxyid file gets written correctly. func TestRun_RetryServicePolling(t *testing.T) { - t.Parallel() require := require.New(t) proxyFile := common.WriteTempFile(t, "") @@ -199,7 +197,6 @@ func TestRun_RetryServicePolling(t *testing.T) { // TestRun_invalidProxyFile validates that we correctly fail in case the proxyid file // is not writable. This functions as coverage for both ACL and non-ACL codepaths. func TestRun_invalidProxyFile(t *testing.T) { - t.Parallel() require := require.New(t) // This is the output file for the proxyid. randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) From b48497920bbf36da7b014c04cb5e803f04cb03df Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 16 Mar 2021 11:45:20 -0500 Subject: [PATCH 11/20] remove require.New --- subcommand/connect-init/command_test.go | 92 ++++++++++++------------- 1 file changed, 43 insertions(+), 49 deletions(-) diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 87affbf290..74197f716a 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -31,7 +31,6 @@ import ( func TestRun_FlagValidation(t *testing.T) { t.Parallel() - require := require.New(t) cases := []struct { flags []string expErr string @@ -56,8 +55,8 @@ func TestRun_FlagValidation(t *testing.T) { UI: ui, } code := cmd.Run(c.flags) - require.Equal(1, code) - require.Contains(ui.ErrorWriter.String(), c.expErr) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), c.expErr) }) } } @@ -65,8 +64,6 @@ func TestRun_FlagValidation(t *testing.T) { // This test mocks ACL login and the service list response (/v1/agent/services), // the later of which is also validated by an actual agent in TestRun_happyPathNoACLs(). func TestRun_happyPathACLs(t *testing.T) { - require := require.New(t) - bearerFile := common.WriteTempFile(t, "bearerTokenFile") proxyFile := common.WriteTempFile(t, "") tokenFile := common.WriteTempFile(t, "") @@ -84,10 +81,10 @@ func TestRun_happyPathACLs(t *testing.T) { })) defer consulServer.Close() serverURL, err := url.Parse(consulServer.URL) - require.NoError(err) + require.NoError(t, err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) - require.NoError(err) + require.NoError(t, err) ui := cli.NewMockUi() cmd := Command{ UI: ui, @@ -102,32 +99,32 @@ func TestRun_happyPathACLs(t *testing.T) { "-skip-service-registration-polling=false"} // Run the command. code := cmd.Run(flags) - require.Equal(0, code) + require.Equal(t, 0, code) // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) - require.NoError(err) - require.Contains(string(data), "counting-counting-sidecar-proxy") + require.NoError(t, err) + require.Contains(t, string(data), "counting-counting-sidecar-proxy") } // This test validates happy path without ACLs : wait on proxy+service to be registered and write out proxyid file func TestRun_happyPathNoACLs(t *testing.T) { - require := require.New(t) + t.Parallel() // This is the output file for the proxyid. proxyFile := common.WriteTempFile(t, "") // Start Consul server. server, err := testutil.NewTestServerConfigT(t, nil) defer server.Stop() - require.NoError(err) + require.NoError(t, err) server.WaitForLeader(t) consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) - require.NoError(err) + require.NoError(t, err) // Register Consul services. testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} for _, svc := range testConsulServices { - require.NoError(consulClient.Agent().ServiceRegister(&svc)) + require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) } ui := cli.NewMockUi() @@ -137,27 +134,27 @@ func TestRun_happyPathNoACLs(t *testing.T) { ProxyIDFile: proxyFile, } code := cmd.Run(defaultTestFlags) - require.Equal(0, code) + require.Equal(t, 0, code) // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) - require.NoError(err) - require.Contains(string(data), "counting-counting-sidecar-proxy") + require.NoError(t, err) + require.Contains(t, string(data), "counting-counting-sidecar-proxy") } // TestRun_RetryServicePolling starts the command and does not register the consul service // for 2 seconds and then asserts that the proxyid file gets written correctly. func TestRun_RetryServicePolling(t *testing.T) { - require := require.New(t) + t.Parallel() proxyFile := common.WriteTempFile(t, "") // Start Consul server. server, err := testutil.NewTestServerConfigT(t, nil) defer server.Stop() - require.NoError(err) + require.NoError(t, err) server.WaitForLeader(t) consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) - require.NoError(err) + require.NoError(t, err) ui := cli.NewMockUi() cmd := Command{ @@ -173,46 +170,46 @@ func TestRun_RetryServicePolling(t *testing.T) { // Register Consul services. testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} for _, svc := range testConsulServices { - require.NoError(consulClient.Agent().ServiceRegister(&svc)) + require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) time.Sleep(time.Second * 2) } // Assert that it exits cleanly or timeout. select { case exitCode := <-exitChan: - require.Equal(0, exitCode) + require.Equal(t, 0, exitCode) case <-time.After(time.Second * 10): // Fail if the stopCh was not caught. - require.Fail("timeout waiting for command to exit") + require.Fail(t, "timeout waiting for command to exit") } // Validate that we hit the retry logic when proxy service is not registered yet. - require.Contains(ui.OutputWriter.String(), "Unable to find registered services; Retrying") + require.Contains(t, ui.OutputWriter.String(), "Unable to find registered services; Retrying") // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) - require.NoError(err) - require.Contains(string(data), "counting-counting-sidecar-proxy") + require.NoError(t, err) + require.Contains(t, string(data), "counting-counting-sidecar-proxy") } // TestRun_invalidProxyFile validates that we correctly fail in case the proxyid file // is not writable. This functions as coverage for both ACL and non-ACL codepaths. func TestRun_invalidProxyFile(t *testing.T) { - require := require.New(t) + t.Parallel() // This is the output file for the proxyid. randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) // Start Consul server. server, err := testutil.NewTestServerConfigT(t, nil) defer server.Stop() - require.NoError(err) + require.NoError(t, err) server.WaitForLeader(t) consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) - require.NoError(err) + require.NoError(t, err) // Register Consul services. testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} for _, svc := range testConsulServices { - require.NoError(consulClient.Agent().ServiceRegister(&svc)) + require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) } ui := cli.NewMockUi() cmd := Command{ @@ -223,13 +220,12 @@ func TestRun_invalidProxyFile(t *testing.T) { } expErr := fmt.Sprintf("Unable to write proxyid out: open %s: no such file or directory\n", randFileName) code := cmd.Run(defaultTestFlags) - require.Equal(1, code) - require.Equal(expErr, ui.ErrorWriter.String()) + require.Equal(t, 1, code) + require.Equal(t, expErr, ui.ErrorWriter.String()) } // TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid server responses. func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { - require := require.New(t) cases := []struct { name string loginResponse string @@ -267,10 +263,10 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { defer consulServer.Close() // Setup the Client. serverURL, err := url.Parse(consulServer.URL) - require.NoError(err) + require.NoError(t, err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) - require.NoError(err) + require.NoError(t, err) // Setup the Command. ui := cli.NewMockUi() @@ -287,8 +283,8 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { "-meta", testPodMeta, "-acl-auth-method", testAuthMethod, "-skip-service-registration-polling=false"} code := cmd.Run(flags) - require.Equal(1, code) - require.Contains(ui.ErrorWriter.String(), c.expErr) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), c.expErr) }) } } @@ -296,20 +292,18 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { // TestRun_RetryACLLoginFails tests that after retry the command fails. func TestRun_RetryACLLoginFails(t *testing.T) { t.Parallel() - require := require.New(t) ui := cli.NewMockUi() cmd := Command{ UI: ui, } code := cmd.Run([]string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) - require.Equal(1, code) - require.Contains(ui.ErrorWriter.String(), "Hit maximum retries for consul login") + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), "Hit maximum retries for consul login") } // Tests ACL Login with Retries. func TestRun_LoginwithRetries(t *testing.T) { - require := require.New(t) cases := []struct { Description string TestRetry bool @@ -354,10 +348,10 @@ func TestRun_LoginwithRetries(t *testing.T) { defer consulServer.Close() serverURL, err := url.Parse(consulServer.URL) - require.NoError(err) + require.NoError(t, err) clientConfig := &api.Config{Address: serverURL.String()} client, err := consul.NewClient(clientConfig) - require.NoError(err) + require.NoError(t, err) ui := cli.NewMockUi() cmd := Command{ @@ -372,17 +366,17 @@ func TestRun_LoginwithRetries(t *testing.T) { "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, "-skip-service-registration-polling=false"}) - require.Equal(c.ExpCode, code) + require.Equal(t, c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. - require.Equal(c.LoginAttemptsCount, counter) + require.Equal(t, c.LoginAttemptsCount, counter) // Validate that the token was written to disk if we succeeded. data, err := ioutil.ReadFile(tokenFile) - require.NoError(err) - require.Contains(string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586") + require.NoError(t, err) + require.Contains(t, string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586") // Validate contents of proxyFile. proxydata, err := ioutil.ReadFile(proxyFile) - require.NoError(err) - require.Contains(string(proxydata), "counting-counting-sidecar-proxy") + require.NoError(t, err) + require.Contains(t, string(proxydata), "counting-counting-sidecar-proxy") }) } } From 691a6ee16f0114ac3899e860a88a42f2b7ca9cdc Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Tue, 16 Mar 2021 17:14:17 -0500 Subject: [PATCH 12/20] review comments --- connect-inject/container_init.go | 10 ++-- connect-inject/container_init_test.go | 29 +++++------ subcommand/connect-init/command.go | 58 ++++++++++----------- subcommand/connect-init/command_test.go | 69 +++++++++---------------- 4 files changed, 71 insertions(+), 95 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index 4f071b1e00..c64da8bca4 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -334,19 +334,19 @@ EOF export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" {{- end}} -consul-k8s connect-init -acl-auth-method="{{ .AuthMethod }}" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ {{- if .AuthMethod }} + -acl-auth-method="{{ .AuthMethod }}" \ + -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ {{- if .ConsulNamespace }} {{- if .NamespaceMirroringEnabled }} {{- /* If namespace mirroring is enabled, the auth method is defined in the default namespace */}} - -namespace="default" \ + -namespace="default" {{- else }} - -namespace="{{ .ConsulNamespace }}" \ + -namespace="{{ .ConsulNamespace }}" {{- end }} {{- end }} - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" {{- end }} # Register the service. The HCL is stored in the volume so that diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index fa5ecba84f..488625bc40 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -54,8 +54,7 @@ func TestHandlerContainerInit(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init -acl-auth-method="" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -738,8 +737,7 @@ func TestHandlerContainerInit_namespacesEnabled(t *testing.T) { `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init -acl-auth-method="" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -812,8 +810,7 @@ EOF `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init -acl-auth-method="" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -887,10 +884,10 @@ EOF `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init -acl-auth-method="auth-method" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -namespace="non-default" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -acl-auth-method="auth-method" \ + -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ + -namespace="non-default" # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -967,10 +964,10 @@ EOF `/bin/sh -ec export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" -consul-k8s connect-init -acl-auth-method="auth-method" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -namespace="default" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -acl-auth-method="auth-method" \ + -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ + -namespace="default" # Register the service. The HCL is stored in the volume so that # the preStop hook can access it to deregister the service. @@ -1151,8 +1148,8 @@ func TestHandlerContainerInit_authMethod(t *testing.T) { require.NoError(err) actual := strings.Join(container.Command, " ") require.Contains(actual, ` -consul-k8s connect-init -acl-auth-method="release-name-consul-k8s-auth-method" \ - -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ +consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ + -acl-auth-method="release-name-consul-k8s-auth-method" \ -meta="pod=${POD_NAMESPACE}/${POD_NAME}"`) require.Contains(actual, ` /consul/connect-inject/consul services register \ diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index a158364fe3..9cd5d17e0e 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -19,8 +19,8 @@ const defaultBearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/to const defaultTokenSinkFile = "/consul/connect-inject/acl-token" const defaultProxyIDFile = "/consul/connect-inject/proxyid" -const numLoginRetries = 3 // The number of times to attempt ACL Login. -const defaultServicePollingRetries = 60 // The number of times to attempt to read this service. (60s) +const numLoginRetries = 3 // The number of times to attempt ACL Login. +const defaultServicePollingRetries = uint64(60) // The number of times to attempt to read this service. (60s) type Command struct { UI cli.Ui @@ -31,10 +31,10 @@ type Command struct { flagPodNamespace string // Pod namespace. flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. - BearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. - TokenSinkFile string // Location to write the output token. Default is defaultTokenSinkFile. - ProxyIDFile string // Location to write the output proxyID. Default is defaultProxyIDFile. - ServiceRegistrationPollingAttempts int // Number of times to poll for this service to be registered. + bearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. + tokenSinkFile string // Location to write the output token. Default is defaultTokenSinkFile. + proxyIDFile string // Location to write the output proxyID. Default is defaultProxyIDFile. + serviceRegistrationPollingAttempts uint64 // Number of times to poll for this service to be registered. flagSet *flag.FlagSet http *flags.HTTPFlags @@ -57,17 +57,17 @@ func (c *Command) init() { c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, "Flag to preserve backward compatibility with service registration.") - if c.BearerTokenFile == "" { - c.BearerTokenFile = defaultBearerTokenFile + if c.bearerTokenFile == "" { + c.bearerTokenFile = defaultBearerTokenFile } - if c.TokenSinkFile == "" { - c.TokenSinkFile = defaultTokenSinkFile + if c.tokenSinkFile == "" { + c.tokenSinkFile = defaultTokenSinkFile } - if c.ProxyIDFile == "" { - c.ProxyIDFile = defaultProxyIDFile + if c.proxyIDFile == "" { + c.proxyIDFile = defaultProxyIDFile } - if c.ServiceRegistrationPollingAttempts == 0 { - c.ServiceRegistrationPollingAttempts = defaultServicePollingRetries + if c.serviceRegistrationPollingAttempts == 0 { + c.serviceRegistrationPollingAttempts = defaultServicePollingRetries } c.http = &flags.HTTPFlags{} @@ -107,12 +107,12 @@ func (c *Command) Run(args []string) int { return 1 } err = backoff.Retry(func() error { - err := common.ConsulLogin(c.consulClient, c.BearerTokenFile, c.flagACLAuthMethod, c.TokenSinkFile, c.flagMeta) + err := common.ConsulLogin(c.consulClient, c.bearerTokenFile, c.flagACLAuthMethod, c.tokenSinkFile, c.flagMeta) if err != nil { c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) } return err - }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), uint64(numLoginRetries))) + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), numLoginRetries)) if err != nil { c.UI.Error(fmt.Sprintf("Hit maximum retries for consul login: %s", err)) return 1 @@ -125,7 +125,7 @@ func (c *Command) Run(args []string) int { // Now wait for the service to be registered. Do this by querying the Agent for a service // which maps to this pod+namespace. - data := "" + var proxyID string err = backoff.Retry(func() error { filter := fmt.Sprintf("Meta[\"pod-name\"] == %s and Meta[\"k8s-namespace\"] == %s", c.flagPodName, c.flagPodNamespace) serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) @@ -135,33 +135,33 @@ func (c *Command) Run(args []string) int { } // Wait for the service and the connect-proxy service to be registered. if len(serviceList) != 2 { - c.UI.Info("Unable to find registered services; Retrying") - return fmt.Errorf("Did not find correct number of services: %d", len(serviceList)) + c.UI.Info("Unable to find registered services; retrying") + return fmt.Errorf("did not find correct number of services: %d", len(serviceList)) } - for _, y := range serviceList { - c.UI.Info(fmt.Sprintf("Registered service has been detected: %s", y.Service)) - if y.Kind == "connect-proxy" { + for _, svc := range serviceList { + c.UI.Info(fmt.Sprintf("Registered service has been detected: %s", svc.Service)) + if svc.Kind == api.ServiceKindConnectProxy { // This is the proxy service ID. - data = y.ID + proxyID = svc.ID return nil } } // In theory we can't reach this point unless we have 2 services registered against - // this pod and neither are the connect-proxy, we don't support this case anyway but it + // this pod and neither are the connect-proxy. We don't support this case anyway, but it // is necessary to return from the function. - return fmt.Errorf("Unable to find registered connect-proxy service") - }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), uint64(c.ServiceRegistrationPollingAttempts))) + return fmt.Errorf("unable to find registered connect-proxy service") + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), c.serviceRegistrationPollingAttempts)) if err != nil { c.UI.Error("Timed out waiting for service registration") return 1 } // Write the proxyid to the shared volume. - err = ioutil.WriteFile(c.ProxyIDFile, []byte(data), 0444) + err = ioutil.WriteFile(c.proxyIDFile, []byte(proxyID), 0444) if err != nil { - c.UI.Error(fmt.Sprintf("Unable to write proxyid out: %s", err)) + c.UI.Error(fmt.Sprintf("unable to write proxy ID to file: %s", err)) return 1 } - c.UI.Info("Service registration completed") + c.UI.Info("connect initialization completed") return 0 } diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 74197f716a..43b63cfb36 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -18,17 +18,6 @@ import ( "github.com/stretchr/testify/require" ) -// NOTES: Test coverage works as follows: -// 1. All tests which are specific to 'login' are in subcommand/common/common_test.go -// 2. The rest are here: -// ACLS enabled happy path // done -// ACLs disabled happy path (uses a valid agent) // done -// ACLs disabled invalid response on service get // covered by ACLs enabled test bc its the same API call -// invalid proxyid file // done -// ACLS enabled fails due to invalid server responses // done -// ACLS enabled fails due to IO (invalid bearer, tokenfile) // covered by common_test.go -// test that retries work for service polling - func TestRun_FlagValidation(t *testing.T) { t.Parallel() cases := []struct { @@ -64,6 +53,7 @@ func TestRun_FlagValidation(t *testing.T) { // This test mocks ACL login and the service list response (/v1/agent/services), // the later of which is also validated by an actual agent in TestRun_happyPathNoACLs(). func TestRun_happyPathACLs(t *testing.T) { + t.Parallel() bearerFile := common.WriteTempFile(t, "bearerTokenFile") proxyFile := common.WriteTempFile(t, "") tokenFile := common.WriteTempFile(t, "") @@ -89,9 +79,9 @@ func TestRun_happyPathACLs(t *testing.T) { cmd := Command{ UI: ui, consulClient: client, - BearerTokenFile: bearerFile, - TokenSinkFile: tokenFile, - ProxyIDFile: proxyFile, + bearerTokenFile: bearerFile, + tokenSinkFile: tokenFile, + proxyIDFile: proxyFile, } flags := []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, @@ -131,7 +121,7 @@ func TestRun_happyPathNoACLs(t *testing.T) { cmd := Command{ UI: ui, consulClient: consulClient, - ProxyIDFile: proxyFile, + proxyIDFile: proxyFile, } code := cmd.Run(defaultTestFlags) require.Equal(t, 0, code) @@ -160,8 +150,8 @@ func TestRun_RetryServicePolling(t *testing.T) { cmd := Command{ UI: ui, consulClient: consulClient, - ProxyIDFile: proxyFile, - ServiceRegistrationPollingAttempts: 10, + proxyIDFile: proxyFile, + serviceRegistrationPollingAttempts: 10, } // Start the command asynchronously, later registering the services. exitChan := runCommandAsynchronously(&cmd, defaultTestFlags) @@ -183,7 +173,7 @@ func TestRun_RetryServicePolling(t *testing.T) { require.Fail(t, "timeout waiting for command to exit") } // Validate that we hit the retry logic when proxy service is not registered yet. - require.Contains(t, ui.OutputWriter.String(), "Unable to find registered services; Retrying") + require.Contains(t, ui.OutputWriter.String(), "Unable to find registered services; retrying") // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) @@ -215,17 +205,18 @@ func TestRun_invalidProxyFile(t *testing.T) { cmd := Command{ UI: ui, consulClient: consulClient, - ProxyIDFile: randFileName, - ServiceRegistrationPollingAttempts: 3, + proxyIDFile: randFileName, + serviceRegistrationPollingAttempts: 3, } - expErr := fmt.Sprintf("Unable to write proxyid out: open %s: no such file or directory\n", randFileName) + expErr := fmt.Sprintf("unable to write proxy ID to file: open %s: no such file or directory\n", randFileName) code := cmd.Run(defaultTestFlags) require.Equal(t, 1, code) require.Equal(t, expErr, ui.ErrorWriter.String()) } -// TestRun_ServiceRegistrationFailsWithBadServerResponses tests error handling with invalid server responses. -func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { +// TestRun_FailsWithBadServerResponses tests error handling with invalid server responses. +func TestRun_FailsWithBadServerResponses(t *testing.T) { + t.Parallel() cases := []struct { name string loginResponse string @@ -273,9 +264,9 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { cmd := Command{ UI: ui, consulClient: client, - BearerTokenFile: bearerFile, - TokenSinkFile: tokenFile, - ServiceRegistrationPollingAttempts: 2, + bearerTokenFile: bearerFile, + tokenSinkFile: tokenFile, + serviceRegistrationPollingAttempts: 2, } flags := []string{ @@ -289,21 +280,9 @@ func TestRun_ServiceRegistrationFailsWithBadServerResponses(t *testing.T) { } } -// TestRun_RetryACLLoginFails tests that after retry the command fails. -func TestRun_RetryACLLoginFails(t *testing.T) { - t.Parallel() - ui := cli.NewMockUi() - cmd := Command{ - UI: ui, - } - code := cmd.Run([]string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta}) - require.Equal(t, 1, code) - require.Contains(t, ui.ErrorWriter.String(), "Hit maximum retries for consul login") -} - // Tests ACL Login with Retries. func TestRun_LoginwithRetries(t *testing.T) { + t.Parallel() cases := []struct { Description string TestRetry bool @@ -357,9 +336,9 @@ func TestRun_LoginwithRetries(t *testing.T) { cmd := Command{ UI: ui, consulClient: client, - TokenSinkFile: tokenFile, - BearerTokenFile: bearerFile, - ProxyIDFile: proxyFile, + tokenSinkFile: tokenFile, + bearerTokenFile: bearerFile, + proxyIDFile: proxyFile, } code := cmd.Run([]string{ "-pod-name", testPodName, @@ -370,13 +349,13 @@ func TestRun_LoginwithRetries(t *testing.T) { // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. require.Equal(t, c.LoginAttemptsCount, counter) // Validate that the token was written to disk if we succeeded. - data, err := ioutil.ReadFile(tokenFile) + tokenData, err := ioutil.ReadFile(tokenFile) require.NoError(t, err) - require.Contains(t, string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586") + require.Equal(t, "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", string(tokenData)) // Validate contents of proxyFile. proxydata, err := ioutil.ReadFile(proxyFile) require.NoError(t, err) - require.Contains(t, string(proxydata), "counting-counting-sidecar-proxy") + require.Equal(t, "counting-counting-sidecar-proxy", string(proxydata)) }) } } From 1c0958d26602b194ea7eef5a61f20a0ae8a126e8 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Wed, 17 Mar 2021 09:07:40 -0500 Subject: [PATCH 13/20] review comment --- subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 9cd5d17e0e..238f4a3275 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -155,7 +155,7 @@ func (c *Command) Run(args []string) int { c.UI.Error("Timed out waiting for service registration") return 1 } - // Write the proxyid to the shared volume. + // Write the proxyid to the shared volume so `consul connect envoy` can use it for bootstrapping. err = ioutil.WriteFile(c.proxyIDFile, []byte(proxyID), 0444) if err != nil { c.UI.Error(fmt.Sprintf("unable to write proxy ID to file: %s", err)) From de8adf6c0248790df3be3d090ebc5f31245ea820 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Wed, 17 Mar 2021 10:45:22 -0500 Subject: [PATCH 14/20] remove -meta flag --- connect-inject/container_init.go | 1 - connect-inject/container_init_test.go | 5 +---- subcommand/connect-init/command.go | 21 +++++++-------------- subcommand/connect-init/command_test.go | 10 +++------- 4 files changed, 11 insertions(+), 26 deletions(-) diff --git a/connect-inject/container_init.go b/connect-inject/container_init.go index c64da8bca4..ae9a627348 100644 --- a/connect-inject/container_init.go +++ b/connect-inject/container_init.go @@ -337,7 +337,6 @@ export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ {{- if .AuthMethod }} -acl-auth-method="{{ .AuthMethod }}" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ {{- if .ConsulNamespace }} {{- if .NamespaceMirroringEnabled }} {{- /* If namespace mirroring is enabled, the auth method is diff --git a/connect-inject/container_init_test.go b/connect-inject/container_init_test.go index 488625bc40..e3b99796f6 100644 --- a/connect-inject/container_init_test.go +++ b/connect-inject/container_init_test.go @@ -886,7 +886,6 @@ export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -acl-auth-method="auth-method" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ -namespace="non-default" # Register the service. The HCL is stored in the volume so that @@ -966,7 +965,6 @@ export CONSUL_HTTP_ADDR="${HOST_IP}:8500" export CONSUL_GRPC_ADDR="${HOST_IP}:8502" consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ -acl-auth-method="auth-method" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}" \ -namespace="default" # Register the service. The HCL is stored in the volume so that @@ -1149,8 +1147,7 @@ func TestHandlerContainerInit_authMethod(t *testing.T) { actual := strings.Join(container.Command, " ") require.Contains(actual, ` consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ - -acl-auth-method="release-name-consul-k8s-auth-method" \ - -meta="pod=${POD_NAMESPACE}/${POD_NAME}"`) + -acl-auth-method="release-name-consul-k8s-auth-method"`) require.Contains(actual, ` /consul/connect-inject/consul services register \ -token-file="/consul/connect-inject/acl-token" \ diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 238f4a3275..88ef4a7037 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -25,11 +25,10 @@ const defaultServicePollingRetries = uint64(60) // The number of times to attemp type Command struct { UI cli.Ui - flagACLAuthMethod string // Auth Method to use for ACLs, if enabled. - flagMeta map[string]string // Flag for metadata to consul login. - flagPodName string // Pod name. - flagPodNamespace string // Pod namespace. - flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. + flagACLAuthMethod string // Auth Method to use for ACLs, if enabled. + flagPodName string // Pod name. + flagPodNamespace string // Pod namespace. + flagSkipServiceRegistrationPolling bool // Whether or not to skip service registration. bearerTokenFile string // Location of the bearer token. Default is /var/run/secrets/kubernetes.io/serviceaccount/token. tokenSinkFile string // Location to write the output token. Default is defaultTokenSinkFile. @@ -47,9 +46,6 @@ type Command struct { func (c *Command) init() { c.flagSet = flag.NewFlagSet("", flag.ContinueOnError) c.flagSet.StringVar(&c.flagACLAuthMethod, "acl-auth-method", "", "Name of the auth method to login to.") - c.flagSet.Var((*flags.FlagMapValue)(&c.flagMeta), "meta", - "Metadata to set on the token, formatted as key=value. This flag may be specified multiple "+ - "times to set multiple meta fields.") c.flagSet.StringVar(&c.flagPodName, "pod-name", "", "Name of the pod.") c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") @@ -101,13 +97,10 @@ func (c *Command) Run(args []string) int { } // First do the ACL Login, if necessary. if c.flagACLAuthMethod != "" { - // Validate flags related to ACL login. - if c.flagMeta == nil { - c.UI.Error("-meta must be set") - return 1 - } + // loginMeta is the default metadata that we pass to the consul login API. + loginMeta := map[string]string{"pod-name": fmt.Sprintf("%s/%s", c.flagPodNamespace, c.flagPodName)} err = backoff.Retry(func() error { - err := common.ConsulLogin(c.consulClient, c.bearerTokenFile, c.flagACLAuthMethod, c.tokenSinkFile, c.flagMeta) + err := common.ConsulLogin(c.consulClient, c.bearerTokenFile, c.flagACLAuthMethod, c.tokenSinkFile, loginMeta) if err != nil { c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) } diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 43b63cfb36..c87ac169b3 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -32,10 +32,6 @@ func TestRun_FlagValidation(t *testing.T) { flags: []string{"-pod-name", testPodName}, expErr: "-pod-namespace must be set", }, - { - flags: []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod}, - expErr: "-meta must be set", - }, } for _, c := range cases { t.Run(c.expErr, func(t *testing.T) { @@ -85,7 +81,7 @@ func TestRun_happyPathACLs(t *testing.T) { } flags := []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-acl-auth-method", testAuthMethod, "-skip-service-registration-polling=false"} // Run the command. code := cmd.Run(flags) @@ -271,7 +267,7 @@ func TestRun_FailsWithBadServerResponses(t *testing.T) { flags := []string{ "-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-meta", testPodMeta, "-acl-auth-method", testAuthMethod, + "-acl-auth-method", testAuthMethod, "-skip-service-registration-polling=false"} code := cmd.Run(flags) require.Equal(t, 1, code) @@ -343,7 +339,7 @@ func TestRun_LoginwithRetries(t *testing.T) { code := cmd.Run([]string{ "-pod-name", testPodName, "-pod-namespace", testPodNamespace, - "-acl-auth-method", testAuthMethod, "-meta", testPodMeta, + "-acl-auth-method", testAuthMethod, "-skip-service-registration-polling=false"}) require.Equal(t, c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. From 423d4261b0c3a390512394648b49b6c4d7e1d962 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Wed, 17 Mar 2021 17:41:35 -0500 Subject: [PATCH 15/20] change how the test is run --- subcommand/connect-init/command_test.go | 54 ++++++++----------------- 1 file changed, 16 insertions(+), 38 deletions(-) diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index c87ac169b3..de19f3b374 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -128,7 +128,7 @@ func TestRun_happyPathNoACLs(t *testing.T) { require.Contains(t, string(data), "counting-counting-sidecar-proxy") } -// TestRun_RetryServicePolling starts the command and does not register the consul service +// TestRun_RetryServicePolling runs the command but does not register the consul service // for 2 seconds and then asserts that the proxyid file gets written correctly. func TestRun_RetryServicePolling(t *testing.T) { t.Parallel() @@ -142,6 +142,18 @@ func TestRun_RetryServicePolling(t *testing.T) { consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) require.NoError(t, err) + // Start the consul service registration in a go func and delay it so that it runs + // after the cmd.Run() starts. + go func() { + // Wait a moment. + time.Sleep(time.Second * 1) + // Register counting service. + require.NoError(t, consulClient.Agent().ServiceRegister(&consulCountingSvc)) + time.Sleep(time.Second * 2) + // Register proxy sidecar service. + require.NoError(t, consulClient.Agent().ServiceRegister(&consulCountingSvcSidecar)) + }() + ui := cli.NewMockUi() cmd := Command{ UI: ui, @@ -149,26 +161,9 @@ func TestRun_RetryServicePolling(t *testing.T) { proxyIDFile: proxyFile, serviceRegistrationPollingAttempts: 10, } - // Start the command asynchronously, later registering the services. - exitChan := runCommandAsynchronously(&cmd, defaultTestFlags) - // Wait a moment. - time.Sleep(time.Second * 1) - // Register Consul services. - testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} - for _, svc := range testConsulServices { - require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) - time.Sleep(time.Second * 2) - } - - // Assert that it exits cleanly or timeout. - select { - case exitCode := <-exitChan: - require.Equal(t, 0, exitCode) - case <-time.After(time.Second * 10): - // Fail if the stopCh was not caught. - require.Fail(t, "timeout waiting for command to exit") - } - // Validate that we hit the retry logic when proxy service is not registered yet. + code := cmd.Run(defaultTestFlags) + require.Equal(t, 0, code) + // Validate that we hit the retry logic when the service was registered but the proxy service is not registered yet. require.Contains(t, ui.OutputWriter.String(), "Unable to find registered services; retrying") // Validate contents of proxyFile. @@ -482,20 +477,3 @@ var ( } defaultTestFlags = []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-skip-service-registration-polling=false"} ) - -// This function starts the command asynchronously and returns a non-blocking chan. -// When finished, the command will send its exit code to the channel. -// Note that it's the responsibility of the caller to terminate the command by calling stopCommand, -// otherwise it can run forever. -func runCommandAsynchronously(cmd *Command, args []string) chan int { - // We have to run cmd.init() to ensure that the channel the command is - // using to watch for os interrupts is initialized. If we don't do this, - // then if stopCommand is called immediately, it will block forever - // because it calls interrupt() which will attempt to send on a nil channel. - cmd.init() - exitChan := make(chan int, 1) - go func() { - exitChan <- cmd.Run(args) - }() - return exitChan -} From 222261ff07e341bef178432daac234dd9fab64fd Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Thu, 18 Mar 2021 16:07:46 -0500 Subject: [PATCH 16/20] fixes for consul client --- subcommand/common/test_util.go | 2 +- subcommand/connect-init/command.go | 31 ++-- subcommand/connect-init/command_test.go | 207 ++++++++++++++++++------ 3 files changed, 174 insertions(+), 66 deletions(-) diff --git a/subcommand/common/test_util.go b/subcommand/common/test_util.go index fdb02f0278..5278acfcad 100644 --- a/subcommand/common/test_util.go +++ b/subcommand/common/test_util.go @@ -58,7 +58,7 @@ func GenerateServerCerts(t *testing.T) (string, string, string, func()) { // name. It will remove the file once the test completes. func WriteTempFile(t *testing.T, contents string) string { t.Helper() - file, err := ioutil.TempFile("", contents) + file, err := ioutil.TempFile("", "testName") require.NoError(t, err) _, err = file.WriteString(contents) require.NoError(t, err) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 88ef4a7037..c435815cbd 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -50,7 +50,7 @@ func (c *Command) init() { c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") // TODO: when the endpoints controller manages service registration this can be removed. For now it preserves back compatibility. - c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, + c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", false, "Flag to preserve backward compatibility with service registration.") if c.bearerTokenFile == "" { @@ -85,16 +85,15 @@ func (c *Command) Run(args []string) int { c.UI.Error("-pod-namespace must be set") return 1 } - // TODO: Add namespace support - if c.consulClient == nil { - cfg := api.DefaultConfig() - c.http.MergeOntoConfig(cfg) - c.consulClient, err = consul.NewClient(cfg) - if err != nil { - c.UI.Error(fmt.Sprintf("Unable to get client connection: %s", err)) - return 1 - } + + cfg := api.DefaultConfig() + c.http.MergeOntoConfig(cfg) + c.consulClient, err = consul.NewClient(cfg) + if err != nil { + c.UI.Error(fmt.Sprintf("Unable to get client connection: %s", err)) + return 1 } + // First do the ACL Login, if necessary. if c.flagACLAuthMethod != "" { // loginMeta is the default metadata that we pass to the consul login API. @@ -110,6 +109,14 @@ func (c *Command) Run(args []string) int { c.UI.Error(fmt.Sprintf("Hit maximum retries for consul login: %s", err)) return 1 } + // Now update the client so that it will read the ACL token we just fetched. + cfg.TokenFile = c.tokenSinkFile + c.http.MergeOntoConfig(cfg) + c.consulClient, err = consul.NewClient(cfg) + if err != nil { + c.UI.Error(fmt.Sprintf("Unable to update client connection: %s", err)) + return 1 + } c.UI.Info("Consul login complete") } if c.flagSkipServiceRegistrationPolling { @@ -128,7 +135,7 @@ func (c *Command) Run(args []string) int { } // Wait for the service and the connect-proxy service to be registered. if len(serviceList) != 2 { - c.UI.Info("Unable to find registered services; retrying") + c.UI.Info(fmt.Sprintf("Unable to find registered services; retrying, %d", len(serviceList))) return fmt.Errorf("did not find correct number of services: %d", len(serviceList)) } for _, svc := range serviceList { @@ -148,7 +155,7 @@ func (c *Command) Run(args []string) int { c.UI.Error("Timed out waiting for service registration") return 1 } - // Write the proxyid to the shared volume so `consul connect envoy` can use it for bootstrapping. + // Write the proxy ID to the shared volume so `consul connect envoy` can use it for bootstrapping. err = ioutil.WriteFile(c.proxyIDFile, []byte(proxyID), 0444) if err != nil { c.UI.Error(fmt.Sprintf("unable to write proxy ID to file: %s", err)) diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index de19f3b374..115ac5b6db 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/hashicorp/consul-k8s/consul" "github.com/hashicorp/consul-k8s/subcommand/common" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/sdk/testutil" @@ -46,47 +45,92 @@ func TestRun_FlagValidation(t *testing.T) { } } -// This test mocks ACL login and the service list response (/v1/agent/services), -// the later of which is also validated by an actual agent in TestRun_happyPathNoACLs(). -func TestRun_happyPathACLs(t *testing.T) { +// TestRun_HappyPathACLs bootstraps and starts a consul server using a mock +// kubernetes server to provide responses for setting up the consul AuthMethod +// then validates that the command runs end to end succesfully. +func TestRun_HappyPathACLs(t *testing.T) { t.Parallel() - bearerFile := common.WriteTempFile(t, "bearerTokenFile") + bearerFile := common.WriteTempFile(t, serviceAccountJWTToken) proxyFile := common.WriteTempFile(t, "") tokenFile := common.WriteTempFile(t, "") - // Start the mock Consul server. - consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // ACL login request. - if r != nil && r.URL.Path == "/v1/acl/login" && r.Method == "POST" { - w.Write([]byte(testLoginResponse)) + // Start Consul server with ACLs enabled and default deny policy. + var masterToken = "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586" + server, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.ACL.Enabled = true + c.ACL.DefaultPolicy = "deny" + c.ACL.Tokens.Master = masterToken + }) + defer server.Stop() + require.NoError(t, err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr, Token: masterToken}) + require.NoError(t, err) + + // Start the mock k8s server. + k8sMockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + if r != nil && r.URL.Path == "/apis/authentication.k8s.io/v1/tokenreviews" && r.Method == "POST" { + w.Write([]byte(tokenReviewFoundResponse)) } - // Get list of Agent Services. - if r != nil && r.URL.Path == "/v1/agent/services" && r.Method == "GET" { - w.Write([]byte(testServiceListResponse)) + if r != nil && r.URL.Path == "/api/v1/namespaces/default/serviceaccounts/counting" && r.Method == "GET" { + w.Write([]byte(readServiceAccountFound)) } })) - defer consulServer.Close() - serverURL, err := url.Parse(consulServer.URL) + defer k8sMockServer.Close() + + // Set up Consul's auth method. + authMethodTmpl := api.ACLAuthMethod{ + Name: testAuthMethod, + Description: "Kubernetes Auth Method", + Type: "kubernetes", + Config: map[string]interface{}{ + "Host": k8sMockServer.URL, + "CACert": serviceAccountCACert, + "ServiceAccountJWT": serviceAccountJWTToken, + }, + } + _, _, err = consulClient.ACL().AuthMethodCreate(&authMethodTmpl, nil) require.NoError(t, err) - clientConfig := &api.Config{Address: serverURL.String()} - client, err := consul.NewClient(clientConfig) + + // Create the binding rule. + aclBindingRule := api.ACLBindingRule{ + Description: "Kubernetes binding rule", + AuthMethod: testAuthMethod, + BindType: api.BindingRuleBindTypeService, + BindName: "${serviceaccount.name}", + Selector: "serviceaccount.name!=default", + } + _, _, err = consulClient.ACL().BindingRuleCreate(&aclBindingRule, nil) require.NoError(t, err) + + // Register Consul services. + testConsulServices := []api.AgentServiceRegistration{consulCountingSvc, consulCountingSvcSidecar} + for _, svc := range testConsulServices { + require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) + } + ui := cli.NewMockUi() cmd := Command{ - UI: ui, - consulClient: client, - bearerTokenFile: bearerFile, - tokenSinkFile: tokenFile, - proxyIDFile: proxyFile, + UI: ui, + bearerTokenFile: bearerFile, + tokenSinkFile: tokenFile, + proxyIDFile: proxyFile, + serviceRegistrationPollingAttempts: 3, } flags := []string{"-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, + "-http-addr", server.HTTPAddr, "-skip-service-registration-polling=false"} // Run the command. code := cmd.Run(flags) - require.Equal(t, 0, code) + require.Equal(t, 0, code, ui.ErrorWriter.String()) + // Validate the ACL token was written. + tokenData, err := ioutil.ReadFile(tokenFile) + require.NoError(t, err) + require.NotEmpty(t, tokenData) // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) require.NoError(t, err) @@ -115,11 +159,12 @@ func TestRun_happyPathNoACLs(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ - UI: ui, - consulClient: consulClient, - proxyIDFile: proxyFile, + UI: ui, + proxyIDFile: proxyFile, } - code := cmd.Run(defaultTestFlags) + flags := []string{"-http-addr", server.HTTPAddr} + flags = append(flags, defaultTestFlags...) + code := cmd.Run(flags) require.Equal(t, 0, code) // Validate contents of proxyFile. @@ -157,11 +202,12 @@ func TestRun_RetryServicePolling(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ UI: ui, - consulClient: consulClient, proxyIDFile: proxyFile, serviceRegistrationPollingAttempts: 10, } - code := cmd.Run(defaultTestFlags) + flags := []string{"-http-addr", server.HTTPAddr} + flags = append(flags, defaultTestFlags...) + code := cmd.Run(flags) require.Equal(t, 0, code) // Validate that we hit the retry logic when the service was registered but the proxy service is not registered yet. require.Contains(t, ui.OutputWriter.String(), "Unable to find registered services; retrying") @@ -195,12 +241,13 @@ func TestRun_invalidProxyFile(t *testing.T) { ui := cli.NewMockUi() cmd := Command{ UI: ui, - consulClient: consulClient, proxyIDFile: randFileName, serviceRegistrationPollingAttempts: 3, } expErr := fmt.Sprintf("unable to write proxy ID to file: open %s: no such file or directory\n", randFileName) - code := cmd.Run(defaultTestFlags) + flags := []string{"-http-addr", server.HTTPAddr} + flags = append(flags, defaultTestFlags...) + code := cmd.Run(flags) require.Equal(t, 1, code) require.Equal(t, expErr, ui.ErrorWriter.String()) } @@ -243,27 +290,22 @@ func TestRun_FailsWithBadServerResponses(t *testing.T) { } })) defer consulServer.Close() - // Setup the Client. - serverURL, err := url.Parse(consulServer.URL) - require.NoError(t, err) - clientConfig := &api.Config{Address: serverURL.String()} - client, err := consul.NewClient(clientConfig) - require.NoError(t, err) // Setup the Command. ui := cli.NewMockUi() cmd := Command{ UI: ui, - consulClient: client, bearerTokenFile: bearerFile, tokenSinkFile: tokenFile, serviceRegistrationPollingAttempts: 2, } + serverURL, err := url.Parse(consulServer.URL) + require.NoError(t, err) flags := []string{ "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, - "-skip-service-registration-polling=false"} + "-skip-service-registration-polling=false", "-http-addr", serverURL.String()} code := cmd.Run(flags) require.Equal(t, 1, code) require.Contains(t, ui.ErrorWriter.String(), c.expErr) @@ -319,14 +361,10 @@ func TestRun_LoginwithRetries(t *testing.T) { serverURL, err := url.Parse(consulServer.URL) require.NoError(t, err) - clientConfig := &api.Config{Address: serverURL.String()} - client, err := consul.NewClient(clientConfig) - require.NoError(t, err) ui := cli.NewMockUi() cmd := Command{ UI: ui, - consulClient: client, tokenSinkFile: tokenFile, bearerTokenFile: bearerFile, proxyIDFile: proxyFile, @@ -335,7 +373,8 @@ func TestRun_LoginwithRetries(t *testing.T) { "-pod-name", testPodName, "-pod-namespace", testPodNamespace, "-acl-auth-method", testAuthMethod, - "-skip-service-registration-polling=false"}) + "-skip-service-registration-polling=false", + "-http-addr", serverURL.String()}) require.Equal(t, c.ExpCode, code) // Cmd will return 1 after numACLLoginRetries, so bound LoginAttemptsCount if we exceeded it. require.Equal(t, c.LoginAttemptsCount, counter) @@ -351,13 +390,76 @@ func TestRun_LoginwithRetries(t *testing.T) { } } -const testPodNamespace = "default" -const testPodName = "counting" -const testPodMeta = "pod=default/counting" -const testAuthMethod = "consul-k8s-auth-method" +const ( + metaKeyPodName = "pod-name" + metaKeyKubeNS = "k8s-namespace" + testPodNamespace = "default" + testPodName = "counting" + testAuthMethod = "consul-k8s-auth-method" + + serviceAccountJWTToken = `eyJhbGciOiJSUzI1NiIsImtpZCI6IiJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6ImtoYWtpLWFyYWNobmlkLWNvbnN1bC1jb25uZWN0LWluamVjdG9yLWF1dGhtZXRob2Qtc3ZjLWFjY29obmRidiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJraGFraS1hcmFjaG5pZC1jb25zdWwtY29ubmVjdC1pbmplY3Rvci1hdXRobWV0aG9kLXN2Yy1hY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZXJ2aWNlLWFjY291bnQudWlkIjoiN2U5NWUxMjktZTQ3My0xMWU5LThmYWEtNDIwMTBhODAwMTIyIiwic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmRlZmF1bHQ6a2hha2ktYXJhY2huaWQtY29uc3VsLWNvbm5lY3QtaW5qZWN0b3ItYXV0aG1ldGhvZC1zdmMtYWNjb3VudCJ9.Yi63MMtzh5MBWKKd3a7dzCJjTITE15ikFy_Tnpdk_AwdwA9J4AMSGEeHN5vWtCuuFjo_lMJqBBPHkK2AqbnoFUj9m5CopWyqICJQlvEOP4fUQ-Rc0W1P_JjU1rZERHG39b5TMLgKPQguyhaiZEJ6CjVtm9wUTagrgiuqYV2iUqLuF6SYNm6SrKtkPS-lqIO-u7C06wVk5m5uqwIVQNpZSIC_5Ls5aLmyZU3nHvH-V7E3HmBhVyZAB76jgKB0TyVX1IOskt9PDFarNtU3suZyCjvqC-UJA6sYeySe4dBNKsKlSZ6YuxUUmn1Rgv32YMdImnsWg8khf-zJvqgWk7B5EA` + serviceAccountCACert = `-----BEGIN CERTIFICATE----- +MIIDCzCCAfOgAwIBAgIQKzs7Njl9Hs6Xc8EXou25hzANBgkqhkiG9w0BAQsFADAv +MS0wKwYDVQQDEyQ1OWU2ZGM0MS0yMDhmLTQwOTUtYTI4OS0xZmM3MDBhYzFjYzgw +HhcNMTkwNjA3MTAxNzMxWhcNMjQwNjA1MTExNzMxWjAvMS0wKwYDVQQDEyQ1OWU2 +ZGM0MS0yMDhmLTQwOTUtYTI4OS0xZmM3MDBhYzFjYzgwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQDZjHzwqofzTpGpc0MdICS7euvfujUKE3PC/apfDAgB +4jzEFKA78/9+KUGw/c/0SHeSQhN+a8gwlHRnAz1NJcfOIXy4dweUuOkAiFxH8pht +ECwkeNO7z8DoV8ceminCRHGjaRmoMxpZ7g2pZAJNZePxi3y1aNkFAXe9gSUSdjRZ +RXYka7wh2AO9k2dlGFAYB+t3vWwJ6twjG0TtKQrhYM9Od1/oN0E01LzBcZuxkN1k +8gfIHy7bOFCBM2WTEDW/0aAvcAPrO8DLqDJ+6Mjc3r7+zlzl8aQspb0S08pVzki5 +Dz//83kyu0phJuij5eB88V7UfPXxXF/EtV6fvrL7MN4fAgMBAAGjIzAhMA4GA1Ud +DwEB/wQEAwICBDAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQBv +QsaG6qlcaRktJ0zGhxxJ52NnRV2GcIYPeN3Zv2VXe3ML3Vd6G32PV7lIOhjx3KmA +/uMh6NhqBzsekkTz0PuC3wJyM2OGonVQisFlqx9sFQ3fU2mIGXCa3wC8e/qP8BHS +w7/VeA7lzmj3TQRE/W0U0ZGeoAxn9b6JtT0iMucYvP0hXKTPBWlnzIijamU50r2Y +7ia065Ug2xUN5FLX/vxOA3y4rjpkjWoVQcu1p8TZrVoM3dsGFWp10fDMRiAHTvOH +Z23jGuk6rn9DUHC2xPj3wCTmd8SGEJoV31noJV5dVeQ90wusXz3vTG7ficKnvHFS +xtr5PSwH1DusYfVaGH2O +-----END CERTIFICATE-----` -// sample response from https://consul.io/api-docs/acl#sample-response -const testLoginResponse = `{ + readServiceAccountFound = `{ + "kind": "ServiceAccount", + "apiVersion": "v1", + "metadata": { + "name": "counting", + "namespace": "default", + "selfLink": "/api/v1/namespaces/default/serviceaccounts/counting", + "uid": "9ff51ff4-557e-11e9-9687-48e6c8b8ecb5", + "resourceVersion": "2101", + "creationTimestamp": "2019-04-02T19:36:34Z" + }, + "secrets": [ + { + "name": "counting-token-m9cvn" + } + ] +}` + + tokenReviewFoundResponse = `{ + "kind": "TokenReview", + "apiVersion": "authentication.k8s.io/v1", + "metadata": { + "creationTimestamp": null + }, + "spec": { + "token": "eyJhbGciOiJSUzI1NiIsImtpZCI6IiJ9.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJkZWZhdWx0Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9zZWNyZXQubmFtZSI6ImRlbW8tdG9rZW4tbTljdm4iLCJrdWJlcm5ldGVzLmlvL3NlcnZpY2VhY2NvdW50L3NlcnZpY2UtYWNjb3VudC5uYW1lIjoiZGVtbyIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6IjlmZjUxZmY0LTU1N2UtMTFlOS05Njg3LTQ4ZTZjOGI4ZWNiNSIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDpkZWZhdWx0OmRlbW8ifQ.UJEphtrN261gy9WCl4ZKjm2PRDLDkc3Xg9VcDGfzyroOqFQ6sog5dVAb9voc5Nc0-H5b1yGwxDViEMucwKvZpA5pi7VEx_OskK-KTWXSmafM0Xg_AvzpU9Ed5TSRno-OhXaAraxdjXoC4myh1ay2DMeHUusJg_ibqcYJrWx-6MO1bH_ObORtAKhoST_8fzkqNAlZmsQ87FinQvYN5mzDXYukl-eeRdBgQUBkWvEb-Ju6cc0-QE4sUQ4IH_fs0fUyX_xc0om0SZGWLP909FTz4V8LxV8kr6L7irxROiS1jn3Fvyc9ur1PamVf3JOPPrOyfmKbaGRiWJM32b3buQw7cg" + }, + "status": { + "authenticated": true, + "user": { + "username": "system:serviceaccount:default:counting", + "uid": "9ff51ff4-557e-11e9-9687-48e6c8b8ecb5", + "groups": [ + "system:serviceaccounts", + "system:serviceaccounts:default", + "system:authenticated" + ] + } + } +}` + // sample response from https://consul.io/api-docs/acl#sample-response + testLoginResponse = `{ "AccessorID": "926e2bd2-b344-d91b-0c83-ae89f372cd9b", "SecretID": "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", "Description": "token created via login", @@ -380,7 +482,7 @@ const testLoginResponse = `{ "ModifyIndex": 36 }` -const testServiceListResponse = `{ + testServiceListResponse = `{ "counting-counting": { "ID": "counting-counting", "Service": "counting", @@ -445,8 +547,7 @@ const testServiceListResponse = `{ "Datacenter": "dc1" } }` -const metaKeyPodName = "pod-name" -const metaKeyKubeNS = "k8s-namespace" +) var ( consulCountingSvc = api.AgentServiceRegistration{ From 620e971fe3c3e298c3f71eb52b81529a5ad8e683 Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Thu, 18 Mar 2021 16:10:47 -0500 Subject: [PATCH 17/20] typo fix --- subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index c435815cbd..dfa2547402 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -50,7 +50,7 @@ func (c *Command) init() { c.flagSet.StringVar(&c.flagPodNamespace, "pod-namespace", "", "Name of the pod namespace.") // TODO: when the endpoints controller manages service registration this can be removed. For now it preserves back compatibility. - c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", false, + c.flagSet.BoolVar(&c.flagSkipServiceRegistrationPolling, "skip-service-registration-polling", true, "Flag to preserve backward compatibility with service registration.") if c.bearerTokenFile == "" { From ce7206d9da3aefa1885123d0a27b99bfb7d52079 Mon Sep 17 00:00:00 2001 From: Iryna Shustava Date: Thu, 18 Mar 2021 20:52:50 -0700 Subject: [PATCH 18/20] Small fixes; more tests --- subcommand/connect-init/command.go | 38 +++-- subcommand/connect-init/command_test.go | 214 +++++++++++++++++++++++- 2 files changed, 225 insertions(+), 27 deletions(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index dfa2547402..6d76e124eb 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -15,12 +15,16 @@ import ( "github.com/mitchellh/cli" ) -const defaultBearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" -const defaultTokenSinkFile = "/consul/connect-inject/acl-token" -const defaultProxyIDFile = "/consul/connect-inject/proxyid" - -const numLoginRetries = 3 // The number of times to attempt ACL Login. -const defaultServicePollingRetries = uint64(60) // The number of times to attempt to read this service. (60s) +const ( + defaultBearerTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" + defaultTokenSinkFile = "/consul/connect-inject/acl-token" + defaultProxyIDFile = "/consul/connect-inject/proxyid" + + // The number of times to attempt ACL Login. + numLoginRetries = 3 + // The number of times to attempt to read this service (60s). + defaultServicePollingRetries = 60 +) type Command struct { UI cli.Ui @@ -35,9 +39,8 @@ type Command struct { proxyIDFile string // Location to write the output proxyID. Default is defaultProxyIDFile. serviceRegistrationPollingAttempts uint64 // Number of times to poll for this service to be registered. - flagSet *flag.FlagSet - http *flags.HTTPFlags - consulClient *api.Client + flagSet *flag.FlagSet + http *flags.HTTPFlags once sync.Once help string @@ -88,7 +91,7 @@ func (c *Command) Run(args []string) int { cfg := api.DefaultConfig() c.http.MergeOntoConfig(cfg) - c.consulClient, err = consul.NewClient(cfg) + consulClient, err := consul.NewClient(cfg) if err != nil { c.UI.Error(fmt.Sprintf("Unable to get client connection: %s", err)) return 1 @@ -97,9 +100,9 @@ func (c *Command) Run(args []string) int { // First do the ACL Login, if necessary. if c.flagACLAuthMethod != "" { // loginMeta is the default metadata that we pass to the consul login API. - loginMeta := map[string]string{"pod-name": fmt.Sprintf("%s/%s", c.flagPodNamespace, c.flagPodName)} + loginMeta := map[string]string{"pod": fmt.Sprintf("%s/%s", c.flagPodNamespace, c.flagPodName)} err = backoff.Retry(func() error { - err := common.ConsulLogin(c.consulClient, c.bearerTokenFile, c.flagACLAuthMethod, c.tokenSinkFile, loginMeta) + err := common.ConsulLogin(consulClient, c.bearerTokenFile, c.flagACLAuthMethod, c.tokenSinkFile, loginMeta) if err != nil { c.UI.Error(fmt.Sprintf("Consul login failed; retrying: %s", err)) } @@ -111,8 +114,7 @@ func (c *Command) Run(args []string) int { } // Now update the client so that it will read the ACL token we just fetched. cfg.TokenFile = c.tokenSinkFile - c.http.MergeOntoConfig(cfg) - c.consulClient, err = consul.NewClient(cfg) + consulClient, err = consul.NewClient(cfg) if err != nil { c.UI.Error(fmt.Sprintf("Unable to update client connection: %s", err)) return 1 @@ -128,7 +130,7 @@ func (c *Command) Run(args []string) int { var proxyID string err = backoff.Retry(func() error { filter := fmt.Sprintf("Meta[\"pod-name\"] == %s and Meta[\"k8s-namespace\"] == %s", c.flagPodName, c.flagPodNamespace) - serviceList, err := c.consulClient.Agent().ServicesWithFilter(filter) + serviceList, err := consulClient.Agent().ServicesWithFilter(filter) if err != nil { c.UI.Error(fmt.Sprintf("Unable to get Agent services: %s", err)) return err @@ -152,16 +154,16 @@ func (c *Command) Run(args []string) int { return fmt.Errorf("unable to find registered connect-proxy service") }, backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), c.serviceRegistrationPollingAttempts)) if err != nil { - c.UI.Error("Timed out waiting for service registration") + c.UI.Error(fmt.Sprintf("Timed out waiting for service registration: %v", err)) return 1 } // Write the proxy ID to the shared volume so `consul connect envoy` can use it for bootstrapping. err = ioutil.WriteFile(c.proxyIDFile, []byte(proxyID), 0444) if err != nil { - c.UI.Error(fmt.Sprintf("unable to write proxy ID to file: %s", err)) + c.UI.Error(fmt.Sprintf("Unable to write proxy ID to file: %s", err)) return 1 } - c.UI.Info("connect initialization completed") + c.UI.Info("Connect initialization completed") return 0 } diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 115ac5b6db..19bce3c354 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -45,10 +45,10 @@ func TestRun_FlagValidation(t *testing.T) { } } -// TestRun_HappyPathACLs bootstraps and starts a consul server using a mock +// TestRun_ServicePollingWithACLs bootstraps and starts a consul server using a mock // kubernetes server to provide responses for setting up the consul AuthMethod -// then validates that the command runs end to end succesfully. -func TestRun_HappyPathACLs(t *testing.T) { +// then validates that the command runs end to end successfully. +func TestRun_ServicePollingWithACLs(t *testing.T) { t.Parallel() bearerFile := common.WriteTempFile(t, serviceAccountJWTToken) proxyFile := common.WriteTempFile(t, "") @@ -131,14 +131,23 @@ func TestRun_HappyPathACLs(t *testing.T) { tokenData, err := ioutil.ReadFile(tokenFile) require.NoError(t, err) require.NotEmpty(t, tokenData) + + // Check that the token has the metadata with pod name and pod namespace. + consulClient, + err = api.NewClient(&api.Config{Address: server.HTTPAddr, Token: string(tokenData)}) + require.NoError(t, err) + token, _, err := consulClient.ACL().TokenReadSelf(nil) + require.NoError(t, err) + require.Equal(t, token.Description, "token created via login: {\"pod\":\"default/counting\"}") + // Validate contents of proxyFile. data, err := ioutil.ReadFile(proxyFile) require.NoError(t, err) require.Contains(t, string(data), "counting-counting-sidecar-proxy") } -// This test validates happy path without ACLs : wait on proxy+service to be registered and write out proxyid file -func TestRun_happyPathNoACLs(t *testing.T) { +// This test validates service polling works in a happy case scenario. +func TestRun_ServicePollingOnly(t *testing.T) { t.Parallel() // This is the output file for the proxyid. proxyFile := common.WriteTempFile(t, "") @@ -173,6 +182,193 @@ func TestRun_happyPathNoACLs(t *testing.T) { require.Contains(t, string(data), "counting-counting-sidecar-proxy") } +// TestRun_ServicePollingErrors tests that when registered services could not be found, +// we error out. +func TestRun_ServicePollingErrors(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + services []api.AgentServiceRegistration + expError string + }{ + { + name: "only service is registered; proxy service is missing", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + }, + }, + expError: "Timed out waiting for service registration: did not find correct number of services: 1", + }, + { + name: "only proxy is registered; service is missing", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting-sidecar-proxy", + Name: "counting-sidecar-proxy", + Kind: "connect-proxy", + Proxy: &api.AgentServiceConnectProxyConfig{ + DestinationServiceName: "counting", + DestinationServiceID: "counting-counting", + }, + Port: 9999, + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + }, + }, + expError: "Timed out waiting for service registration: did not find correct number of services: 1", + }, + { + name: "service and proxy without pod-name and k8s-namespace meta", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + }, + { + ID: "counting-counting-sidecar-proxy", + Name: "counting-sidecar-proxy", + Kind: "connect-proxy", + Proxy: &api.AgentServiceConnectProxyConfig{ + DestinationServiceName: "counting", + DestinationServiceID: "counting-counting", + }, + Port: 9999, + Address: "127.0.0.1", + }, + }, + expError: "Timed out waiting for service registration: did not find correct number of services: 0", + }, + { + name: "service and proxy with pod-name meta but without k8s-namespace meta", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + }, + }, + { + ID: "counting-counting-sidecar-proxy", + Name: "counting-sidecar-proxy", + Kind: "connect-proxy", + Proxy: &api.AgentServiceConnectProxyConfig{ + DestinationServiceName: "counting", + DestinationServiceID: "counting-counting", + }, + Port: 9999, + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + }, + }, + }, + expError: "Timed out waiting for service registration: did not find correct number of services: 0", + }, + { + name: "service and proxy with k8s-namespace meta but pod-name meta", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyKubeNS: "default", + }, + }, + { + ID: "counting-counting-sidecar-proxy", + Name: "counting-sidecar-proxy", + Kind: "connect-proxy", + Proxy: &api.AgentServiceConnectProxyConfig{ + DestinationServiceName: "counting", + DestinationServiceID: "counting-counting", + }, + Port: 9999, + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyKubeNS: "default", + }, + }, + }, + expError: "Timed out waiting for service registration: did not find correct number of services: 0", + }, + { + name: "both services are non-proxy services", + services: []api.AgentServiceRegistration{ + { + ID: "counting-counting", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + }, + { + ID: "counting-counting-1", + Name: "counting", + Address: "127.0.0.1", + Meta: map[string]string{ + metaKeyPodName: "counting", + metaKeyKubeNS: "default", + }, + }, + }, + expError: "Timed out waiting for service registration: unable to find registered connect-proxy service", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + proxyFile := common.WriteTempFile(t, "") + + // Start Consul server. + server, err := testutil.NewTestServerConfigT(t, nil) + defer server.Stop() + require.NoError(t, err) + server.WaitForLeader(t) + consulClient, err := api.NewClient(&api.Config{Address: server.HTTPAddr}) + require.NoError(t, err) + + // Register Consul services. + for _, svc := range c.services { + require.NoError(t, consulClient.Agent().ServiceRegister(&svc)) + } + + ui := cli.NewMockUi() + cmd := Command{ + UI: ui, + proxyIDFile: proxyFile, + serviceRegistrationPollingAttempts: 1, + } + flags := []string{ + "-http-addr", server.HTTPAddr, + "-pod-name", testPodName, + "-pod-namespace", testPodNamespace, + "-skip-service-registration-polling=false", + } + + code := cmd.Run(flags) + require.Equal(t, 1, code) + require.Contains(t, ui.ErrorWriter.String(), c.expError) + }) + } +} + // TestRun_RetryServicePolling runs the command but does not register the consul service // for 2 seconds and then asserts that the proxyid file gets written correctly. func TestRun_RetryServicePolling(t *testing.T) { @@ -218,9 +414,9 @@ func TestRun_RetryServicePolling(t *testing.T) { require.Contains(t, string(data), "counting-counting-sidecar-proxy") } -// TestRun_invalidProxyFile validates that we correctly fail in case the proxyid file +// TestRun_InvalidProxyFile validates that we correctly fail in case the proxyid file // is not writable. This functions as coverage for both ACL and non-ACL codepaths. -func TestRun_invalidProxyFile(t *testing.T) { +func TestRun_InvalidProxyFile(t *testing.T) { t.Parallel() // This is the output file for the proxyid. randFileName := fmt.Sprintf("/foo/%d/%d", rand.Int(), rand.Int()) @@ -244,7 +440,7 @@ func TestRun_invalidProxyFile(t *testing.T) { proxyIDFile: randFileName, serviceRegistrationPollingAttempts: 3, } - expErr := fmt.Sprintf("unable to write proxy ID to file: open %s: no such file or directory\n", randFileName) + expErr := fmt.Sprintf("Unable to write proxy ID to file: open %s: no such file or directory\n", randFileName) flags := []string{"-http-addr", server.HTTPAddr} flags = append(flags, defaultTestFlags...) code := cmd.Run(flags) @@ -314,7 +510,7 @@ func TestRun_FailsWithBadServerResponses(t *testing.T) { } // Tests ACL Login with Retries. -func TestRun_LoginwithRetries(t *testing.T) { +func TestRun_LoginWithRetries(t *testing.T) { t.Parallel() cases := []struct { Description string From 1ec6066540c00d91c00b448241c5d0e6d545265d Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Fri, 19 Mar 2021 09:20:38 -0500 Subject: [PATCH 19/20] fix wording --- subcommand/connect-init/command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subcommand/connect-init/command.go b/subcommand/connect-init/command.go index 6d76e124eb..6f9717bc97 100644 --- a/subcommand/connect-init/command.go +++ b/subcommand/connect-init/command.go @@ -137,7 +137,7 @@ func (c *Command) Run(args []string) int { } // Wait for the service and the connect-proxy service to be registered. if len(serviceList) != 2 { - c.UI.Info(fmt.Sprintf("Unable to find registered services; retrying, %d", len(serviceList))) + c.UI.Info("Unable to find registered services; retrying") return fmt.Errorf("did not find correct number of services: %d", len(serviceList)) } for _, svc := range serviceList { From 215142275fa847e601f782355b3e05dbcda8120a Mon Sep 17 00:00:00 2001 From: Kyle Schochenmaier Date: Fri, 19 Mar 2021 12:43:30 -0500 Subject: [PATCH 20/20] Update subcommand/connect-init/command_test.go Co-authored-by: Iryna Shustava --- subcommand/connect-init/command_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/subcommand/connect-init/command_test.go b/subcommand/connect-init/command_test.go index 19bce3c354..922d151496 100644 --- a/subcommand/connect-init/command_test.go +++ b/subcommand/connect-init/command_test.go @@ -133,8 +133,7 @@ func TestRun_ServicePollingWithACLs(t *testing.T) { require.NotEmpty(t, tokenData) // Check that the token has the metadata with pod name and pod namespace. - consulClient, - err = api.NewClient(&api.Config{Address: server.HTTPAddr, Token: string(tokenData)}) + consulClient, err = api.NewClient(&api.Config{Address: server.HTTPAddr, Token: string(tokenData)}) require.NoError(t, err) token, _, err := consulClient.ACL().TokenReadSelf(nil) require.NoError(t, err)