Integrate consul-server-connection-manager library #449

sarahalsmiller · 2022-11-11T19:29:34Z

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

Tests added
CHANGELOG entry added

Run make changelog-entry for guidance in authoring a changelog entry, and
commit the resulting file, which should have a name matching your PR number.
Entries should use imperative present tense (e.g. Add support for...)

internal/commands/exec/exec.go

andrewstucki

making progress.

internal/consul/connection.go

internal/commands/exec/exec.go

andrewstucki

More comments, maybe we can pair tomorrow on this.

internal/commands/exec/command.go

internal/commands/server/server.go

…sul-server-connection-manager

andrewstucki · 2022-11-16T04:52:36Z

So I went ahead and validated this with the following agentless configurations that pretty much exercise the majority auth method + tls configuration for the discovery library:

internal server
TLS enabled + internal server
Managed ACLs + internal server
External server with static address
External server with connection string exec=echo 192.168.65.2

Despite the introduction of all the more flaky unit tests due to the upstream issues with the manager library use of the grpc balancer subpackage, I think we can merge with the inclusion of some changelog entries + a thorough review pass. We'll continue to vet this over the next few days with a variety of consul-k8s configurations. @sarahalsmiller since I'm out a good chunk of tomorrow morning, would you mind handling the changelog stuff?

mikemorris

Minor notes/questions but mostly LGTM!

internal/commands/exec/exec.go

mikemorris · 2022-11-16T16:11:47Z

internal/commands/exec/exec_test.go

 		AuthConfig: AuthConfig{
 			Method: "nonexistent",
 			Token:  "token",
 		},
 		isTest: true,
 	}))
-	require.Contains(t, buffer.String(), "error logging into consul")
+	require.Contains(t, buffer.String(), "no such file or directory")


This seems like a weird error, is it because we're expecting a file path for the bearer token or something? Should we add a test for if it's present but invalid, and/or is there an actual implementation issue here where setting the Token field in an AuthConfig isn't handled properly?

mikemorris · 2022-11-16T16:16:45Z

internal/commands/exec/exec_test.go

 }

 func TestRunExecLoginSuccessRegistrationFail(t *testing.T) {
-	t.Parallel()


I'm assuming this was needed because of the un-threadsafe behavior of how consul-server-connection-manager is registering the gRPC load balancer? Refs hashicorp/consul-server-connection-manager#25

mikemorris · 2022-11-16T16:29:23Z

internal/consul/connection.go

+	if !c.config.PlainText {
+		config.TLS = c.config.TLS
+	}


I was curious about the best way to check for whether TLS should be used for these gRPC connections - this approach seems reasonable, in
I took a different approach more similar to internal/commands/server/server.go and internal/commands/exec/exec.go though, checking

if consulAPITLSConfig.CAFile != "" || len(consulAPITLSConfig.CAPem) > 0

It looks like consul-dataplane is asking for an explicit -tls-disabled configuration https://github.com/hashicorp/consul-dataplane/#configuration-reference

Yeah, so Consul Dataplane uses that to see whether or not it should set a TLS parameter in the underlying library that we share usage of. We're doing the same with a PlainText option to the struct. For our codebase we have knowledge of the scheme we're trying to use to connect, so I just used that -- if we're over http we use plain text, if https use TLS.

mikemorris · 2022-11-16T16:43:18Z

internal/testing/e2e/consul.go

@@ -531,12 +534,12 @@ func CreateConsulACLPolicy(ctx context.Context, cfg *envconf.Config) (context.Co
 		return ctx, nil
 	}
 	env := consulEnvironment.(*consulTestEnvironment)
-	token, _, err := env.consulClient.ACL().Bootstrap()
+	token, _, err := env.consulClient.Internal().ACL().Bootstrap()


Could we add a ACL() method to the client wrapper since we plan to remove the Internal() catch-all soon?

mikemorris · 2022-11-16T16:44:27Z

internal/testing/e2e/gateway.go

 	// this should go away once we implement auth in the server bootup
-	consulClient.AddHeader("x-consul-token", ConsulInitialManagementToken(ctx))
+	consulClient.Internal().AddHeader("x-consul-token", ConsulInitialManagementToken(ctx))


Is this still needed at all now that we're using the consul-server-connection-manager integrated login flow?

Yes, this is the client that gets passed around with the root auth token in our tests so that we can actually check what's in Consul.

mikemorris · 2022-11-16T16:47:00Z

internal/commands/exec/exec_test.go

 	gwTesting "github.com/hashicorp/consul-api-gateway/internal/testing"
 )

-func TestRunExecLoginError(t *testing.T) {
-	t.Parallel()
+type testDPServer struct{}


Do we not actually need to mock the WatchServers method anywhere? Are we only exercising that in e2e tests?

No, we don't -- this is just a gRPC server that stubs out the endpoints. So the library actually hits this and issues real API calls against this. It's almost like using httptest.

internal/testing/e2e/consul.go

internal/commands/exec/command.go

andrewstucki

LGTM -- We'll want to hammer this some more with the helm changes to make sure we have all of our myriad of configurations fully vetted and documented.

Andrew Stucki and others added 7 commits November 8, 2022 16:17

Add consul client wrapper for exec-based management

405f128

Add missed

8d116c7

Add back in server entrypoint accidentally deleted

ef258f1

exec bootstraping conneciton manager code

ef7ccfb

Merge branch 'main' into sa-consul-server-connection-manager

4f607f8

fix mod file

7000c56

fix linting issue

04abb3e

sarahalsmiller added the pr/no-changelog Skip the CI check that requires a changelog entry label Nov 11, 2022

andrewstucki reviewed Nov 11, 2022

View reviewed changes

internal/commands/exec/exec.go Outdated Show resolved Hide resolved

sarahalsmiller and others added 6 commits November 12, 2022 12:21

checkpoint

f464ce4

checkpoint

f0c86c2

checkpoint

09d3dbd

checkpoint

cba44e9

linting issue

790137f

Merge branch 'main' into sa-consul-server-connection-manager

8cae195

andrewstucki suggested changes Nov 14, 2022

View reviewed changes

sarahalsmiller added 4 commits November 14, 2022 14:25

add watch call

c822ddf

fix typo

f26ecac

small fixes

11fbb84

checkpoint

85d5f3e

andrewstucki reviewed Nov 14, 2022

View reviewed changes

sarahalsmiller and others added 2 commits November 14, 2022 18:14

notes

119270f

Fix up lots of things

6defaa5

andrewstucki force-pushed the sa-consul-server-connection-manager branch from 8e02541 to 6defaa5 Compare November 15, 2022 21:08

andrewstucki marked this pull request as ready for review November 15, 2022 21:10

andrewstucki changed the title ~~WIP~~ Integrate consul-server-connection-manager library Nov 15, 2022

Andrew Stucki added 4 commits November 15, 2022 16:48

Merge branch 'main' of https://github.com/hashicorp/polar into sa-con…

9ce7be7

…sul-server-connection-manager

unquote environment var

e54156e

drop support for 1.11 and 1.12

1d0535a

update connection manager library

f66dca6

light fixes

b0966d6

mikemorris approved these changes Nov 16, 2022

View reviewed changes

mikemorris removed the pr/no-changelog Skip the CI check that requires a changelog entry label Nov 16, 2022

sarahalsmiller commented Nov 16, 2022

View reviewed changes

internal/testing/e2e/consul.go Outdated Show resolved Hide resolved

sarahalsmiller and others added 3 commits November 16, 2022 11:26

Update internal/testing/e2e/consul.go

4ec30a4

add ACL to interface

0342a03

add changelog entry

ad01725

andrewstucki suggested changes Nov 16, 2022

View reviewed changes

internal/commands/exec/command.go Outdated Show resolved Hide resolved

sarahalsmiller force-pushed the sa-consul-server-connection-manager branch from 0ea29e6 to ad01725 Compare November 16, 2022 18:50

revert changes/add commnet

57cb2ab

andrewstucki approved these changes Nov 16, 2022

View reviewed changes

sarahalsmiller merged commit faa8244 into main Nov 16, 2022

sarahalsmiller deleted the sa-consul-server-connection-manager branch November 16, 2022 19:45

mikemorris mentioned this pull request Nov 21, 2022

initial scaffolding for consul-server-connection-manager integration #407

Closed

2 tasks

nathancoleman mentioned this pull request Jan 30, 2023

deps: update consul-server-connection-manager #484

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate consul-server-connection-manager library #449

Integrate consul-server-connection-manager library #449

sarahalsmiller commented Nov 11, 2022

andrewstucki left a comment

andrewstucki left a comment

andrewstucki commented Nov 16, 2022

mikemorris left a comment

mikemorris Nov 16, 2022

mikemorris Nov 16, 2022

andrewstucki Nov 16, 2022

mikemorris Nov 16, 2022

andrewstucki Nov 16, 2022

mikemorris Nov 16, 2022

mikemorris Nov 16, 2022

andrewstucki Nov 16, 2022

mikemorris Nov 16, 2022

andrewstucki Nov 16, 2022

andrewstucki left a comment

Integrate consul-server-connection-manager library #449

Integrate consul-server-connection-manager library #449

Conversation

sarahalsmiller commented Nov 11, 2022

Changes proposed in this PR:

How I've tested this PR:

How I expect reviewers to test this PR:

Checklist:

andrewstucki left a comment

Choose a reason for hiding this comment

andrewstucki left a comment

Choose a reason for hiding this comment

andrewstucki commented Nov 16, 2022

mikemorris 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

Choose a reason for hiding this comment

andrewstucki left a comment

Choose a reason for hiding this comment