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.

Sign up for GitHub

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

Jump to bottom

Add SidecarService Syntax sugar to Service Definition #4686

Merged
merged 9 commits into from
Sep 27, 2018
Merged

Add SidecarService Syntax sugar to Service Definition #4686

merged 9 commits into from
Sep 27, 2018

Conversation

banks
Copy link
Member

@banks banks commented Sep 17, 2018 •
edited
Loading

NOTE: this PR is against the f-envoy feature branch as it assumes some of the changes their to proxy definitions and will land along with that work.

SidecarService is a new configuration helper for service definitions for use with Connect.

It provided most of the automatic configuration that Managed Proxies benefit from, but without implying that we actually start and supervise the process for you. This design is detailed in the internal RFC.

This implementation includes:

  • Config and API changes to allow service(s).connect.sidecar_service nested service definitions
  • Tests for the default behaviour, and lifecycle management for both API and file-registrations (i.e. if you deregister a service that was registered with a SidecarService or remove the sidecar from a file-based config, the sidecar is automatically deregistered too.
  • API package updates to allow sidecar service registrations

Left for future PR

  • Documentation changes are intentionally left for later because they will need a large restructure as part of managed proxy changes proposed.
  • Proxy Config Endpoint/Built in proxy changes are left for a later PR just to keep the change set smaller.

TODO

As I wrote this PR description I realised that there is one case not tests (and I think not working):

  • Updating registration via API that removes a SidecarService should cause it to be deregistered.
  • Updating registration via API should also update sidecar registration (does already but not explicitly tested).

@banks banks requested review from mitchellh and a team September 17, 2018 14:11
@banks banks changed the base branch from master to f-envoy September 17, 2018 14:11
@banks
Copy link
Member Author

banks commented Sep 17, 2018

Updating registration via API that removes a SidecarService should cause it to be deregistered.

I wrote a test for this and then realised that we probably don't want that behaviour because:

  1. It would make it impossible to "read-modify-write" a registration via the API (e.g. to change it's Meta) because we never show the sidecar in the output of the catalog/agent service listing so any service that used sidecars would not be easily editable via API without copying the sidecar definition out of band.
  2. We have a precedent here already for checks - if you define a service with 2 checks in config file and then later re-register via API to update meta or token (say) but don't include the checks, we don't deregister the original checks.

I think this will need to be documented clearly and we can possibly re-visit but it seems better to not deregister in this case for now. you can always explicitly deregister the sidecar anyway and it's best to think of sidecars as registration syntax sugar rather than first class management of a separate nested resource anyway.

I added tests to cover updates generally and specifically for this decision anyway.

@banks banks mentioned this pull request Sep 18, 2018
Merged
mitchellh
mitchellh approved these changes Sep 19, 2018
View reviewed changes
agent/agent_test.go

services := a.State.Services()
if _, ok := services["rabbitmq"]; !ok {
t.Fatalf("missing service")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick but should these busing require helpers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah could do, it was copy-paste from other similar code I think.

@banks
Copy link
Member Author

banks commented Sep 21, 2018

There is one legit failing test here but it's fixed in 630c811 in the next PR in this series which will be merged at the same time.

@banks banks merged commit a1703d0 into f-envoy Sep 27, 2018
@banks banks deleted the sidecar-service branch September 27, 2018 14:01
banks added a commit that referenced this pull request Oct 4, 2018
@banks
Add SidecarService Syntax sugar to Service Definition (#4686)
1ebe6bd 
* Added new Config for SidecarService in ServiceDefinitions.

* WIP: all the code needed for SidecarService is written... none of it is tested other than config :). Need API updates too.

* Test coverage for the new sidecarServiceFromNodeService method.

* Test API registratrion with SidecarService

* Recursive Key Translation 🤦

* Add tests for nested sidecar defintion arrays to ensure they are translated correctly

* Use dedicated internal state rather than Service Meta for tracking sidecars for deregistration.

Add tests for deregistration.

* API struct for agent register. No other endpoint should be affected yet.

* Additional test cases to cover updates to API registrations
banks added a commit that referenced this pull request Oct 9, 2018
@banks
Add SidecarService Syntax sugar to Service Definition (#4686)
af7d081 
* Added new Config for SidecarService in ServiceDefinitions.

* WIP: all the code needed for SidecarService is written... none of it is tested other than config :). Need API updates too.

* Test coverage for the new sidecarServiceFromNodeService method.

* Test API registratrion with SidecarService

* Recursive Key Translation 🤦

* Add tests for nested sidecar defintion arrays to ensure they are translated correctly

* Use dedicated internal state rather than Service Meta for tracking sidecars for deregistration.

Add tests for deregistration.

* API struct for agent register. No other endpoint should be affected yet.

* Additional test cases to cover updates to API registrations
banks added a commit that referenced this pull request Oct 10, 2018
@banks
Add SidecarService Syntax sugar to Service Definition (#4686)
b8a20c0 
* Added new Config for SidecarService in ServiceDefinitions.

* WIP: all the code needed for SidecarService is written... none of it is tested other than config :). Need API updates too.

* Test coverage for the new sidecarServiceFromNodeService method.

* Test API registratrion with SidecarService

* Recursive Key Translation 🤦

* Add tests for nested sidecar defintion arrays to ensure they are translated correctly

* Use dedicated internal state rather than Service Meta for tracking sidecars for deregistration.

Add tests for deregistration.

* API struct for agent register. No other endpoint should be affected yet.

* Additional test cases to cover updates to API registrations
banks added a commit that referenced this pull request Oct 10, 2018
@banks
Add SidecarService Syntax sugar to Service Definition (#4686)
1e7eace 
* Added new Config for SidecarService in ServiceDefinitions.

* WIP: all the code needed for SidecarService is written... none of it is tested other than config :). Need API updates too.

* Test coverage for the new sidecarServiceFromNodeService method.

* Test API registratrion with SidecarService

* Recursive Key Translation 🤦

* Add tests for nested sidecar defintion arrays to ensure they are translated correctly

* Use dedicated internal state rather than Service Meta for tracking sidecars for deregistration.

Add tests for deregistration.

* API struct for agent register. No other endpoint should be affected yet.

* Additional test cases to cover updates to API registrations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanshasselberg hanshasselberg hanshasselberg left review comments

@kyhavlov kyhavlov kyhavlov approved these changes

@mitchellh mitchellh mitchellh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@banks @mitchellh @hanshasselberg @kyhavlov