Add acls and tls to endpoints controller

[kschoche]

Changes proposed in this PR:

• Adds support for ACLs and TLS to endpoints controller
• Minor change to how we deal with consul clients in the endpoints controller+tests.

How I've tested this PR:
Unit tests for ACLS+TLS have been added which cover normal paths as well as the three places that where we request a new consulClient:

1. processUpstreams()
2. 2x in Reconcile() (deregisterServiceOnAllAgents() and regular Reconcile)

Manually run the connect-inject acceptance tests using hashicorp/consul-helm#886, with imageK8S: "kschoche/consul-k8s-dev" and uncommenting the TLS table tests.

How I expect reviewers to test this PR:
Run unit tests / manually deploy and run acceptance tests.

Note: I'm willing to table-fy and add ACL/TLS support to all of the endpoints-controller tests if we think this is really necessary.
I think the tests which use the consul client (TestReconcileDeleteEndpoint(), TestReconcileUpdateEndpoint()) where I have NOT added ACL/TLS tests are already covered by the the tests I added. I'm willing to make changes but for the sake of readability I didn't want to unnecessarily add complexity to the already long+awesome tests. Thoughts?

Checklist:

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

[ndhanushkodi]

Nice work Kyle!! I just had a few naming comments and small things. I think it makes sense that just TestReconcileUpdateEndpoint has the ACL/TLS tests since they cover the cases a client is created, I just added more to the comment on TestReconcileUpdateEndpoint to reflect that. I wish there was a way to have consistency with Create/Delete so you don't need to have context about why those tests are only for Update but I can't think of another way, so it makes sense to keep it the way it is. Maybe the next reviewer will think of something :)

[lkysow]

I like the idea of trying to get in here and test this. I thought it was a bit confusing as implemented because there's a number of things involved in getting clients:

1. EndpointsController.ConsulClient
2. EndpointsController.GetClient

We're only mocking out one of these.

I messed around with making ConsulClientCfg a field:

EndpointsController {
  ConsulClient *api.Client
  ConsulClientCfg *api.Config
}

And then having GetConsulClient (which should probably be renamed to something like RemoteConsulClient or ExternalConsulClient) use r.ConsulClientCfg when generating its client config:

// GetConsulClient returns an *api.Client that points at the consul agent local to the pod.
func (r *EndpointsController) GetConsulClient(scheme, port, ip string) (*api.Client, error) {
	newAddr := fmt.Sprintf("%s://%s:%s", scheme, ip, port)
	localConfig := r.ConsulClientCfg
	localConfig.Address = newAddr

	return consul.NewClient(localConfig)
}

Then in the tests you can pass in ConsulClientCfg: cfg which in most places is already generated. Thanks to you setting the IP of the pod to 127.0.0.1, this will then work.

I think this approach might be better because:

1. less confusion about clients and getting clients and what the difference between the two is
2. requires less work in tests since you don't need to generate a mock closure, just pass in the cfg you already needed to create the client
3. It's "less powerful" because it doesn't involve a closure and less powerful mocking is preferred due to simplicity

Not dead set on it though so interested in hearing why the current approach was taken.

[lkysow]

Suggested change

	ce, _ := api.MakeConfigEntry(api.ProxyDefaults, "pd")
	ce, _ := api.MakeConfigEntry(api.ProxyDefaults, "global")

I don't know how this worked because inside processUpstreams we look for a proxy defaults with name global. I think somewhere when you call ConfigEntries().Set it's ignoring the pd and using global but may as well set it to global here to avoid confusion.

[kschoche]

Good catch on this. Not sure how it worked either..

[kschoche]

@lkysow

Not dead set on it though so interested in hearing why the current approach was taken.

Current approach was really just taken because I couldnt think of what you suggested, I stared at the code too long after running into issues with GetConsulClient() overwriting the token!
I like this idea, and it's much cleaner, thanks for the suggestion!!

[lkysow]

looks good, just have important question about which cfg we use in command.go.

[lkysow]

I think we may need to use cfg here because there's a bunch of code above that is doing stuff to it after it gets it out of api.DefaultConfig():

	// create Consul API config object
	cfg := api.DefaultConfig()
	c.http.MergeOntoConfig(cfg)
	if cfg.TLSConfig.CAFile == "" && c.flagConsulCACert != "" {
		cfg.TLSConfig.CAFile = c.flagConsulCACert
	}
	consulURLRaw := cfg.Address
	// cfg.Address may or may not be prefixed with scheme.
	if !strings.Contains(cfg.Address, "://") {
		consulURLRaw = fmt.Sprintf("%s://%s", cfg.Scheme, cfg.Address)
	}
	consulURL, err := url.Parse(consulURLRaw)
	if err != nil {
		c.UI.Error(fmt.Sprintf("error parsing consul address %q: %s", consulURLRaw, err))
		return 1
	}

	// load CA file contents
	var consulCACert []byte
	if cfg.TLSConfig.CAFile != "" {
		var err error
		consulCACert, err = ioutil.ReadFile(cfg.TLSConfig.CAFile)
		if err != nil {
			c.UI.Error(fmt.Sprintf("error reading Consul's CA cert file %q: %s", cfg.TLSConfig.CAFile, err))
			return 1
		}
	}

I haven't tested this out in an acceptance test but I'm guessing we need to use that config?

[kschoche]

Ah good catch. At runtime it worked for me deployed on a cluster as-is, but we definitely can use cfg instead here.
I believe api.DefaultConfig() wouldn't have picked up flag overrides on command line, thx!

[lkysow]

Oops, I see you requested we run in acceptance test. I haven't done that so will do so as well.

[ndhanushkodi]

I love the new approach, and this looks great to me! I ran the TestConnectInject acceptance test as well with secure:true and they pass 🎉 awesome work!!

[ndhanushkodi]

I liked Luke's suggestion from here to name this remoteConsulClient or externalConsulClient

[kschoche]

remoteConsulClient looks great, switching to that, thanks!