Add connect-init command and consul login api #446

kschoche · 2021-02-24T15:51:55Z

Changes proposed in this PR:

Create a new function for the consul login command which uses only Consul client API calls.
Add connect-init command and refactor the init container and its tests to use it to do ACL retrieval for the connect-inject init container.

How I've tested this PR:
Unit tests added and passed.
Manual test by deploying kschoche/consul-k8s-dev with ACLs enabled and deploy a connect injected application.

How I expect reviewers to test this PR:
Manual test.

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

Add consul-init command and plumb it through init container

kschoche · 2021-03-03T23:50:35Z

connect-inject/container_init.go

+  -meta="pod=${POD_NAMESPACE}/${POD_NAME}"
+{{- /* The acl token file needs to be read by the consul-sidecar which runs
+       as non-root user consul-k8s. TODO: will this be necessary anymore? */}}
+{{- end }}


When the endpoints controller is ready we will remove everything past this in the command tmpl

kschoche · 2021-03-03T23:51:31Z

connect-inject/container_init.go

@@ -458,7 +453,7 @@ chmod 444 /consul/connect-inject/acl-token
  /consul/connect-inject/service.hcl

 # Generate the envoy bootstrap code
-/bin/consul connect envoy \
+/consul/connect-inject/consul connect envoy \


Since we are now using consul-k8s image instead of consul the path will be from the shared vol instead of /bin/.. update all the tests accordingly.

subcommand/common/common.go

subcommand/common/common_test.go

kschoche · 2021-03-03T23:52:55Z

subcommand/common/common_test.go

+			Method: r.Method,
+			Path:   r.URL.Path,
+		})
+		b := "{\n  \"AccessorID\": \"926e2bd2-b344-d91b-0c83-ae89f372cd9b\",\n  \"SecretID\": \"b78d37c7-0ca7-5f4d-99ee-6d9975ce4586\",\n  \"Description\": \"token created via login\",\n  \"Roles\": [\n    {\n      \"ID\": \"3356c67c-5535-403a-ad79-c1d5f9df8fc7\",\n      \"Name\": \"demo\"\n    }\n  ],\n  \"ServiceIdentities\": [\n    {\n      \"ServiceName\": \"example\"\n    }\n  ],\n  \"Local\": true,\n  \"AuthMethod\": \"minikube\",\n  \"CreateTime\": \"2019-04-29T10:08:08.404370762-05:00\",\n  \"Hash\": \"nLimyD+7l6miiHEBmN/tvCelAmE/SbIXxcnTzG3pbGY=\",\n  \"CreateIndex\": 36,\n  \"ModifyIndex\": 36\n}"


This is a sample API output from consul.io that we are mocking.

This is a nifty way to stub out the server and assert on the api call! I love what you've done here.

subcommand/consul-init/command.go

connect-inject/cleanup_resource_ent_test.go

lkysow

This is looking good! I think the biggest thing I'm wondering is that right now this command is only run when ACLs are enabled but the plan is for this command to eventually run when ACLs are disabled so does it make sense to write it right now so it essentially no-ops if ACLs are disabled?

Because otherwise we're writing tests that -auth-method is a required flag but then later it will be run without that flag (if ACLs are disabled).

subcommand/common/common.go

commands.go

connect-inject/container_init.go

subcommand/common/common.go

lkysow · 2021-03-04T23:07:50Z

subcommand/consul-init/command.go

+	ErrCh := make(chan error)
+	ExitCh := make(chan bool)
+	meta := map[string]string{"pod": strings.Split(c.flagMeta, "=")[1]}
+	go func() {


This loop ignore any sigterms right now. I'm also not sure it needs to run in a go routine? Would something like this work:

retries := 0 for { err = common.ConsulLogin(c.consulClient, c.flagBearerTokenFile, c.flagACLAuthMethod, c.flagTokenSinkFile, meta) if err == nil { break } retries++ if retries == c.numACLLoginRetries { c.UI.Error("hit maximum retries for consul login") return 1 } c.UI.Error(fmt.Sprintf("consul login failed; retrying: %s", err)) select { case <-time.After(1 * time.Second): // retry loop case sig := <-c.sigCh: c.UI.Info(fmt.Sprintf("%s received, shutting down", sig)) return 0 } }

What do you think about using backoff.Retry (example here). I think this package will already handle a lot the signals and make the logic much cleaner. You'd need to pass in backoff.WithMaxRetries(backoff.NewConstantBackOff(1*time.Second), 3) as the second argument so that it stops after 3 tries.

@ishustava great idea, I like it and its much cleaner looking I'm not sure I understand how it handles signals. In get-consul-client-ca I don't see it either and we're not testing that command for any signal handling. Could you can show me? 🙇🏽

What is the reason to have signal handling in this command?

In get-consul-client-ca, we don't handle interrupt signals because it doesn't start any goroutines, and so if an interrupt or a term signal is sent, it doesn't need to drain or stop any other processes that it's running. We also don't do it in other similar commands, e.g server-acl-init, probably for the same reason.

We do loop for the login so wouldn't it be nice if you could delete the pod and it would exit quickly if it was in its init?

I'm fine with not having it too if it's deemed too complicated.

We do loop for the login so wouldn't it be nice if you could delete the pod and it would exit quickly if it was in its init

If you delete a pod, then it should still exit quickly. If you have a program that doesn't handle interrupts and you send it an interrupt, it should still exit as soon as you send that signal because it's handled by golang and OS. In this command, we don't need any graceful shutdown behavior so I think it's ok to skip OS signal handling. Maybe I'm misunderstanding something?

Ahh I didn't know that there was default signal handling that gets turned off by signal.Notify (https://golang.org/pkg/os/signal/).

subcommand/consul-init/command.go

subcommand/consul-init/command_test.go

Co-authored-by: Luke Kysow <[email protected]>

ndhanushkodi

Just a clarification on the scope of this PR, the consul login logic and tests and command look great!!

connect-inject/container_init.go

ndhanushkodi · 2021-03-04T23:46:43Z

subcommand/common/common_test.go

+			Method: r.Method,
+			Path:   r.URL.Path,
+		})
+		b := "{\n  \"AccessorID\": \"926e2bd2-b344-d91b-0c83-ae89f372cd9b\",\n  \"SecretID\": \"b78d37c7-0ca7-5f4d-99ee-6d9975ce4586\",\n  \"Description\": \"token created via login\",\n  \"Roles\": [\n    {\n      \"ID\": \"3356c67c-5535-403a-ad79-c1d5f9df8fc7\",\n      \"Name\": \"demo\"\n    }\n  ],\n  \"ServiceIdentities\": [\n    {\n      \"ServiceName\": \"example\"\n    }\n  ],\n  \"Local\": true,\n  \"AuthMethod\": \"minikube\",\n  \"CreateTime\": \"2019-04-29T10:08:08.404370762-05:00\",\n  \"Hash\": \"nLimyD+7l6miiHEBmN/tvCelAmE/SbIXxcnTzG3pbGY=\",\n  \"CreateIndex\": 36,\n  \"ModifyIndex\": 36\n}"


This is a nifty way to stub out the server and assert on the api call! I love what you've done here.

ndhanushkodi · 2021-03-04T23:58:22Z

connect-inject/container_init.go

@@ -323,6 +323,21 @@ 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 = `
+{{- if .AuthMethod }}


Is this just the command without the loop that waits for the consul client to have a service instance registered with the local agent? (i.e is this only don't the consul login for now)?

oh I think you can ignore this, I think that is the case, and for right now the command is mostly doing the consul login.

Yeah it is only doing consul login right now. trying to strike a line between this PR and the next PR which will bring in non-ACL support, etc.

kschoche · 2021-03-05T00:00:59Z

connect-inject/cleanup_resource_ent_test.go

@@ -199,7 +199,7 @@ func TestDelete_ConsulNamespaces(t *testing.T) {
 			server, err := testutil.NewTestServerConfigT(t, nil)
 			defer server.Stop()
 			require.NoError(err)
-			server.WaitForLeader(t)
+			server.WaitForSerfCheck(t)


will rebase this at the end.

lkysow

I tested everything out and it's looking great. Just some small suggested changes remaining.

connect-inject/container_init.go

subcommand/flags/flag_map_value.go

subcommand/common/common.go

subcommand/common/common_test.go

lkysow · 2021-03-08T20:54:49Z

subcommand/connect-init/command_test.go

+			flags:  []string{"-meta", "pod=abcdefg"},
+			expErr: "-method must be set",
+		},
+	}


What about the other required flags?

I have them taking in defaults right now in the flag parsing, wasn't sure how to handle it, what do you think?

I'd say follow the other commands we have and how they deal with flag defaults. If there is no prior art then I'd say if a flag has a default then we don't need to check if it's nil and then we also wouldn't write a test for it.

Yeah that is what I ended up doing here. but let me look at some other tests and update the PR accordingly.

IMO, if they have defaults, then we don't need those validation errors since it's impossible for them to be empty.

Oh interesting. I would have said that if:

if c.flagBearerTokenFile == "" { c.UI.Error("-bearer-token-file must be set") return 1 } if c.flagTokenSinkFile == "" { c.UI.Error("-token-sink-file must be set") return 1 }

Will never run because those have defaults then we can just delete those lines and then no need to test.

subcommand/connect-init/command_test.go

subcommand/server-acl-init/command_test.go

connect-inject/container_init.go

ishustava

Looking pretty good! I really like the work you did here! I left some comments; they are mostly minor suggestions or questions. Otherwise, it looks good to me!

ishustava · 2021-03-08T19:36:53Z

subcommand/common/common_test.go

+	t.Parallel()
+	require := require.New(t)
+	counter := 0
+	consulServer, client, bearerTokenFile, tokenFile := startMockServer(t, "", &counter)


do we need the mock server for this test? I think it fails before it makes a call to consul

ishustava · 2021-03-08T19:38:21Z

subcommand/common/common_test.go

+		tokenFile,
+		testPodMeta,
+	)
+	require.Error(err, "unable to read bearerTokenFile")


this error should be no bearer token found in ...

subcommand/common/common_test.go

subcommand/connect-init/command.go

ishustava · 2021-03-08T22:11:49Z

subcommand/connect-init/command_test.go

+			flags:  []string{"-meta", "pod=abcdefg"},
+			expErr: "-method must be set",
+		},
+	}


IMO, if they have defaults, then we don't need those validation errors since it's impossible for them to be empty.

subcommand/connect-init/command.go

subcommand/flags/flag_map_value_test.go

subcommand/server-acl-init/command_test.go

ishustava

🎉

Left a couple of minor edits, but otherwise looks great!

subcommand/common/common_test.go

subcommand/connect-init/command.go

Co-authored-by: Iryna Shustava <[email protected]>

lkysow

🦊 great work. Note I didn't dockerize it myself on this last review.

lkysow · 2021-03-09T17:57:43Z

subcommand/common/common_test.go

+	// Validate that the token file was written to disk.
+	data, err := ioutil.ReadFile(tokenFile)
+	require.NoError(err)
+	require.Equal(string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586")


Suggested change

require.Equal(string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586")

require.Equal("b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", string(data))

Order of these arguments is exp, act so they should be swapped.

lkysow · 2021-03-09T17:58:26Z

subcommand/common/common_test.go

+// startMockServer starts an httptest server used to mock a Consul server's
+// /v1/acl/login endpoint. It also writes bearerTokenContents to a temp file.
+// apiCallCounter will be incremented on each call to /v1/acl/login.
+// It returns a consul client pointing at the server.
+func startMockServer(t *testing.T, apiCallCounter *int) *api.Client {


Suggested change

// startMockServer starts an httptest server used to mock a Consul server's

// /v1/acl/login endpoint. It also writes bearerTokenContents to a temp file.

// apiCallCounter will be incremented on each call to /v1/acl/login.

// It returns a consul client pointing at the server.

func startMockServer(t *testing.T, apiCallCounter *int) *api.Client {

// startMockServer starts an httptest server used to mock a Consul server's

// /v1/acl/login endpoint.

// apiCallCounter will be incremented on each call to /v1/acl/login.

// It returns a consul client pointing at the server.

func startMockServer(t *testing.T, apiCallCounter *int) *api.Client {

lkysow · 2021-03-09T18:00:30Z

subcommand/connect-init/command_test.go

+		TestRetry          bool
+		LoginAttemptsCount int
+		ExpCode            int
+		ExpErr             string


ExpErr isn't used anymore

* Add connect-init command to consul-k8s and build consul login api Co-authored-by: Luke Kysow <[email protected]> Co-authored-by: Iryna Shustava <[email protected]>

kschoche added the theme/tproxy Items related to transparent proxy label Feb 24, 2021

kschoche changed the title ~~Add consul login api~~ Add consul-init command and consul login api Feb 26, 2021

Add consul login api

4f1512f

Add consul-init command and plumb it through init container

kschoche force-pushed the tproxy-add-consul-login-api branch from 609e1f6 to 4f1512f Compare March 3, 2021 23:49

kschoche commented Mar 3, 2021

View reviewed changes

subcommand/common/common.go Outdated Show resolved Hide resolved

kschoche commented Mar 3, 2021

View reviewed changes

subcommand/common/common_test.go Outdated Show resolved Hide resolved

kschoche commented Mar 3, 2021

View reviewed changes

subcommand/consul-init/command.go Outdated Show resolved Hide resolved

kschoche requested review from a team, lkysow and ndhanushkodi and removed request for a team March 3, 2021 23:54

kschoche self-assigned this Mar 3, 2021

kschoche marked this pull request as ready for review March 3, 2021 23:56

kschoche added 4 commits March 4, 2021 08:10

Apply suggestions from code review

3aa2917

fixes

9a539f5

cleanup and test fixes

9afb242

oops

2fc0434

kschoche commented Mar 4, 2021

View reviewed changes

connect-inject/cleanup_resource_ent_test.go Outdated Show resolved Hide resolved

kschoche added 2 commits March 4, 2021 12:11

add signal handling test

2e13570

fix

979545f

kschoche added the area/connect Related to Connect service mesh, e.g. injection label Mar 4, 2021

lkysow reviewed Mar 4, 2021

View reviewed changes

subcommand/consul-init/command_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

c37d657

Co-authored-by: Luke Kysow <[email protected]>

ndhanushkodi approved these changes Mar 4, 2021

View reviewed changes

kschoche commented Mar 5, 2021

View reviewed changes

review comments

529347b

kschoche changed the title ~~Add consul-init command and consul login api~~ Add connect-init command and consul login api Mar 5, 2021

kschoche added 2 commits March 5, 2021 09:19

typo

65efef2

some review fixes, use backoff

b03657b

kschoche requested review from lkysow and ishustava March 8, 2021 17:27

lkysow reviewed Mar 8, 2021

View reviewed changes

kschoche added 3 commits March 8, 2021 15:35

quick fix

92a70f9

review fixes

589dd9c

add comments

f820237

ishustava reviewed Mar 8, 2021

View reviewed changes

kschoche added 2 commits March 8, 2021 17:04

changes per review comments

57ab4d8

remove unnecessary func

8506172

ishustava approved these changes Mar 8, 2021

View reviewed changes

subcommand/common/common_test.go Outdated Show resolved Hide resolved

subcommand/connect-init/command.go Outdated Show resolved Hide resolved

subcommand/connect-init/command.go Show resolved Hide resolved

kschoche and others added 2 commits March 9, 2021 08:12

Apply suggestions from code review

8c580d9

Co-authored-by: Iryna Shustava <[email protected]>

fix typo from review

13a68ce

lkysow approved these changes Mar 9, 2021

View reviewed changes

fixes

d99ac55

kschoche merged commit b456064 into feature-tproxy Mar 9, 2021

kschoche deleted the tproxy-add-consul-login-api branch March 9, 2021 18:21

	require.Equal(string(data), "b78d37c7-0ca7-5f4d-99ee-6d9975ce4586")
	require.Equal("b78d37c7-0ca7-5f4d-99ee-6d9975ce4586", string(data))

Add connect-init command and consul login api #446

Add connect-init command and consul login api #446

Conversation

kschoche commented Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschoche commented Feb 24, 2021 •

edited

Loading