Setup managed proxy environment with API client env vars #4374

mkeeler · 2018-07-10T16:20:45Z

This adds:

Function on RuntimeConfig to generate an api.Config
Function on api.Config to generate the env vars for an API client
Added an APIConfig member to the proxy Manager
Proxy Manager will initialize a proxies environment by copying its environment (old behavior) and then appending all the env vars generated by the api.Config.GenerateEnv function

What I have tested thus far:

Consul with HTTP and HTTPS. To do this I created a proxy script and then configured my service to use it as a custom proxy command. It just saves off the env to a file so I can look at the vars afterwards.

#!/usr/bin/env bash
env > /Users/mkeeler/env.txt
exec /Users/mkeeler/Code/repos/consul/proxy-env-vars/bin/consul connect proxy "$@"

New Unit Tests for the api config's GenerateEnv function, the runtime config's APIConfig function and the proxy manager setting up the proxies env to have whatever vaules are in the ProxyEnv var.

…ged proxy

mitchellh

The abstract approach seems right, but I have some weird feelings about tightly coupling the agent/proxy package to a structure in the api package.

I tried to keep the "interface" (in the abstract sense, not Go sense) of the agent/proxy.Manager struct as loosely coupled as possible, to ease testing and refactoring in the future.

One idea perhaps is to just change the *api.Config directly to an Env []string field that would just be more generically "Append these enviornment variables to any launched proxies." This achieves the desired functionality without a tight coupling, and allows the consumer (agent) to potentially inject more or fewer env vars in the future as necessary.

mitchellh · 2018-07-10T21:54:53Z

api/api.go

@@ -405,6 +405,27 @@ func SetupTLSConfig(tlsConfig *TLSConfig) (*tls.Config, error) {
 	return tlsClientConfig, nil
 }

+func (c *Config) GenerateEnv() []string {
+	env := make([]string, 10)


Nitpick but seems a little over-optimized. I dunno why we would but if we had to reorder stuff below or anything its a little bit much. Almost equally performant would be to just make with a cap of 10 make([]string, 0, 10) and use append below which will never realloc and is pretty optimized by the Go compiler.

mitchellh · 2018-07-10T21:56:55Z

agent/config/runtime.go

+		cfg.Scheme = "http"
+	} else if len(unixAddrs) > 0 {
+		cfg.Address = "unix://" + unixAddrs[0]
+		cfg.Scheme = "http"


Is the scheme here right?

The client is still talking HTTP just over a unix socket instead of a TCP connection. So it depends on how this param is used in the api code - @mkeeler maybe add a comment about why this is http and not unix since it's not obvious if this is right.

I think this will actually get ignored because the address has a scheme in it but in that case I'd suggest we leave it blank.

You are right this will be ignored currently.

While there isn't much point as all the data is flowing through your kernel, you could do https over the unix socket. Having the Scheme set to http for both seemed logical for what protocol we intend to talk over the l4 transport.

banks

@mitchellh I have to take the blame for the api.Config thing.

My gut feeling was that I wanted to encapsulate config in a proper struct - I initially suggested a new struct in config package for it until Matt pointed out it would just be a duplicate of api.Config.

I have only fuzzy reasons for preferring that - having a map or slice of env vars plumbed through several layers feels like it will end up being hacked to support transporting stuff we don't actually need to put in ENV but do need some other place like in a JSON payload sent to configure something.

Keeping a struct felt like a cleaner semantic thing to plumb around between components than a bag of stuff for that reason. "Here is all you need to configure API clients" vs. "Here is some stuff for the ENV".

It's potentially over-engineering though - we don't have any need for these except for setting ENV vars for now so we could defer the abstraction for now.

I'm happy to leave it as a ProxyEnv []string or something for now but I'd still marginally prefer a struct - one in config if we thing the api import is unclean although that introduces more duplication.

banks · 2018-07-11T11:24:59Z

agent/config/runtime.go

+		cfg.Scheme = "http"
+	} else if len(unixAddrs) > 0 {
+		cfg.Address = "unix://" + unixAddrs[0]
+		cfg.Scheme = "http"


The client is still talking HTTP just over a unix socket instead of a TCP connection. So it depends on how this param is used in the api code - @mkeeler maybe add a comment about why this is http and not unix since it's not obvious if this is right.

I think this will actually get ignored because the address has a scheme in it but in that case I'd suggest we leave it blank.

mkeeler · 2018-07-11T13:17:05Z

It would be just as easy to generate the []string of env vars one level up in the agent code. I have no issues with not propagating the api Config struct into the proxy.

banks · 2018-07-11T13:33:00Z

agent/proxy/manager.go

@@ -434,7 +437,7 @@ func (m *Manager) newProxy(mp *local.ManagedProxy) (Proxy, error) {
 		}

 		// Pass in the environmental variables for the proxy process
-		cmd.Env = os.Environ()
+		cmd.Env = append(os.Environ(), m.ProxyEnv...)


Should we de-dupe this? I'm not sure what the behaviour is if any of the CONSUL_* vars are already set in agent's ENV across different platforms.

I feel like we should allow the agent's own ENV vars to take precedence. In general if they are set they are likely set to same thing anyway but it provides a nice way to be able to override the choice of which protocol/listener to pass which we could document. The edge case of some legacy garbage value in ENV being used seems like a reasonable trade.

As for de-dupe it should be unnecessary. With the current code the ProxyEnv vars will be used. However we could just do it like this append(m.ProxyEnv, os.Environ()...) and then the agents env will be used.

Another note is that regardless of the env we set they can still use a custom proxy command like env CONSUL_HTTP_ADDR=.... <path to consul> connect proxy to override anything we set.

Proxy now doesn’t need to know anything about the api as we pass env vars to it instead of the api config.

mitchellh · 2018-07-11T14:09:38Z

@banks What @mkeeler suggested (and ended up doing I see now) is exactly what I was thinking: you just generate the flat slice one level up.

I think the argument for "over-engineering" can fall on either side, but I think lowering cross-package requirements unless they make a lot of sense or are obviously needed is generally the right design decision. Beyond that, *api.Config felt too big of a scope. It almost feels like what you want here is an interface of struct fields (not possible in Go) so its limited just to access params, but I still think more generally modifying env vars is the correct abstraction at the proxy manager level. Its easy to have this discussion though. 😄I could type a long multi-para "why" but won't, unless you want me too, haha.

mitchellh · 2018-07-11T14:13:12Z

Added* to the above: I should note that I think using *api.Config everywhere else and adding the function to it to generate the right env vars is totally right otherwise, the approach in this PR is great.

banks

Agreed this looks good.

If Env contains duplicate environment keys, only the last
value in the slice for each duplicate key is used.

So yeah dupes are not a big deal. The ability to do global override seems nicish with env merged the other way but you're right that it can be done in the proxy command anyway so I think this is good.

Add runtime -> api.Config tests

…ddrs

…rifyOutgoing

Pass around an API Config object and convert to env vars for the mana…

3b6eef8

…ged proxy

mkeeler requested a review from banks July 10, 2018 16:20

mitchellh reviewed Jul 10, 2018

View reviewed changes

banks reviewed Jul 11, 2018

View reviewed changes

mkeeler added this to the 1.2.1 milestone Jul 11, 2018

banks requested changes Jul 11, 2018

View reviewed changes

mkeeler force-pushed the feature/proxy-env-vars branch from c15c558 to 85aa770 Compare July 11, 2018 13:33

PR Updates

c54b43b

Proxy now doesn’t need to know anything about the api as we pass env vars to it instead of the api config.

mkeeler force-pushed the feature/proxy-env-vars branch from 85aa770 to c54b43b Compare July 11, 2018 13:45

banks approved these changes Jul 11, 2018

View reviewed changes

mkeeler added 4 commits July 11, 2018 16:50

Update proxy manager test - test passing ProxyEnv vars

c8df4b8

Look specifically for tcp instead of unix

700a275

Add runtime -> api.Config tests

Use type switch instead of .Network for more reliably detecting UnixA…

22e4058

…ddrs

Add some tests for GenerateEnv

7dfd2ab

mkeeler changed the title ~~WIP: Setup managed proxy environment with API client env vars~~ Setup managed proxy environment with API client env vars Jul 12, 2018

Set api.Config’s InsecureSkipVerify to the value of !RuntimeConfig.Ve…

0f56ed2

…rifyOutgoing

mkeeler merged commit 7572ca0 into master Jul 12, 2018

mkeeler deleted the feature/proxy-env-vars branch July 12, 2018 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup managed proxy environment with API client env vars #4374

Setup managed proxy environment with API client env vars #4374

mkeeler commented Jul 10, 2018 •

edited

Loading

mitchellh left a comment

mitchellh Jul 10, 2018

mitchellh Jul 10, 2018

banks Jul 11, 2018

mkeeler Jul 11, 2018

banks left a comment

banks Jul 11, 2018

mkeeler commented Jul 11, 2018

banks Jul 11, 2018

mkeeler Jul 11, 2018

mkeeler Jul 11, 2018

mitchellh commented Jul 11, 2018

mitchellh commented Jul 11, 2018

banks left a comment

Setup managed proxy environment with API client env vars #4374

Setup managed proxy environment with API client env vars #4374

Conversation

mkeeler commented Jul 10, 2018 • edited Loading

mitchellh 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

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchellh commented Jul 11, 2018

mitchellh commented Jul 11, 2018

banks left a comment

Choose a reason for hiding this comment

mkeeler commented Jul 10, 2018 •

edited

Loading