Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Proxy Upstreams to Service Definition #4630

Closed
wants to merge 17 commits into from
Closed

Conversation

banks
Copy link
Member

@banks banks commented Sep 4, 2018

This PR adds Proxy Upstream definitions to be first-class within service registrations in two separate places.

The motivation is that as we integrate Envoy and other proxies, they all need ways to express and centrally manage this configuration (i.e. in catalog not just local agent state) so it needs to become a first class part of the service definitions.

The upstreams for managed proxies can now be specified directly in the connect.proxy.upstreams stanza rather than being further nested inside the opaque map connect.proxy.config as part of the application service registration.

In addition, "unmanaged" proxies can now register themselves with Kind = "connect-proxy" and with an explicit set of Upstreams as well as some other common proxy configuration.

The full details of this are documented in the doc changes inline here.

Deprecations

This PR deprecates two things:

  1. Specifying upstreams for managed proxies in the service definitions opaque config map
  2. Specifying an unmanaged proxy target in the ProxyDestination field

In both cases we gracefully accept the old value for now and translate it to the new setup. We should document these as deprecated though and remove them later after 1.3 release.

Testing

It turns out this apparently simple change was a huge pain given our current baggage around chancing config structs and supporting multiple formats and legacy key translations from snake to camel case etc.

I methodically considered and added tests for all of the following cases:

  • Agent Services endpoint response
  • Agent Service endpoint response
  • Agent Register endpoint
    • Unmanaged deprecated field
    • Unmanaged new fields
    • Managed deprecated upstreams
    • Managed new
  • Agent Proxy config endpoint response
  • Catalog Register
    • Unmanaged deprecated field
    • Unmanaged new fields
    • Managed deprecated upstreams
    • Managed new
  • Catalog Services endpoint response
  • Catalog Node endpoint response
  • Catalog Service endpoint response
  • API package methods for ALL of the above endpoints/cases
  • Config file parsing for service/services in JSON/HCL
  • Config file translations from deprecated to new fields

Even with all those tests, I still wasn't confident so I wrote an external sanity check script: https://github.com/banks/consul-upstream-config-e2e

This found a whole bunch of additional issues! For now I've left it out of our actual test suite because for each issue it did find I was able to add additional cases to the actual tests we have that covered them before fixing so I don't think it adds any benefit to include this script in the repo as it is. We will look at incorporating more cases like this though into our existing functional/e2e test suite though as I think this showed there is value for significant config changes (especially in service definitions which touch so many parts of the API and config code) to validate all the options.

Documentation

This is still TODO (!!). I will add it to this PR as a separate commit so it can be read in isolation more easily.

TODO

  • Update documentation for API and service definition

TODO:
 - docs for all endpoints to include cache support info
…roxy processes

Rationale: we have several other components of the agent that relate to Connect proxies for example the ProxyConfigManager component needed for Envoy work. Those things are pretty separate from the focus of this package so far which is only concerned with managing external proxy processes so it's nota good fit to put code for that in here, yet there is a naming clash if we have other packages related to proxy functionality that are not in the `agent/proxy` package.

Happy to bikeshed the name. I started by calling it `managedproxy` but `managedproxy.Manager` is especially unpleasant. `proxyprocess` seems good in that it's more specific about purpose but less clearly connected with the concept of "managed proxies". The names in use are cleaner though e.g. `proxyprocess.Manager`.

This rename was completed automatically using golang.org/x/tools/cmd/gomvpkg.

Depends on #4541
This includes:
 - Refactoring all internal structs used
 - Updated tests for both deprecated and new input for:
   - Agent Services endpoint response
   - Agent Service endpoint response
   - Agent Register endpoint
     - Unmanaged deprecated field
     - Unmanaged new fields
     - Managed deprecated upstreams
     - Managed new
   - Catalog Register
     - Unmanaged deprecated field
     - Unmanaged new fields
     - Managed deprecated upstreams
     - Managed new
   - Catalog Services endpoint response
   - Catalog Node endpoint response
   - Catalog Service endpoint response
 - Updated API tests for all of the above too (both deprecated and new forms of register)

TODO:
 - config package changes for on-disk service definitions
 - proxy config endpoint
 - built-in proxy support for new fields
…ele bugs found with ned to end test scripts...

TODO: tests still failing on one case that needs a fix. I think it's key translation for upstreams nested in Managed proxy struct.
@banks
Copy link
Member Author

banks commented Sep 5, 2018

TODO:

  • Responses still contain "ProxyDestination": "" for all service types but don't actually set it. Should add tests that assert we still set it at least for Kind = connect-proxy. We should probably also add omit-empty as it shows for typical services too currently.

 - omit some empty undocumented fields in API
 - Bring back ServiceProxyDestination in Catalog responses to not break backwards compat - this was removed assuming it was only used internally.
@banks
Copy link
Member Author

banks commented Sep 5, 2018

Documentation added along with fixes for some additional issues with API responses discovered while preparing the docs.

NOTES

@banks
Copy link
Member Author

banks commented Sep 5, 2018

Docs changes alone are: 26e1a6e and may be a good place to start. @pearkes any input on that welcome too. It at least explains (hopefully!) what the changes are and what the new UX will look like which might be good context for reviewing any of the code.

I know this is kinda big but I couldn't really see a way of breaking it up much more without being in an inconsistent and untestable state in the interim.

@banks banks closed this Sep 6, 2018
@banks banks changed the base branch from proxy-pkg-rename to f-envoy September 6, 2018 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant