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

Connect: support enabling proxies via service-defaults ConfigEntry #5856

Open
banks opened this issue May 16, 2019 · 0 comments
Open

Connect: support enabling proxies via service-defaults ConfigEntry #5856

banks opened this issue May 16, 2019 · 0 comments
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies type/enhancement Proposed improvement or new feature

Comments

@banks
Copy link
Member

banks commented May 16, 2019

As originally designed, we should allow a simple boolean in the service-defaults ConfigEntry for any service (inside a connect block for future expansion:

kind = "service-defaults"
name = "web"
protocol = "http"
connect {
  enable_sidecar_proxy = true
}

This was planned for 1.5.0 but ran into some issues in the way we resolve these in the client agent's ServiceManager.

Rough steps needed from memory are:

  • Enable resolving for all registrations again (provided config enables it) not just proxies
  • If the service has sidecars enabled centrally, then then create one and validate and register:

    consul/agent/agent.go

    Lines 3053 to 3072 in 45cdc80

    // Grab and validate sidecar if there is one too
    sidecar, sidecarChecks, sidecarToken, err := a.sidecarServiceFromNodeService(ns, service.Token)
    if err != nil {
    return fmt.Errorf("Failed to validate sidecar for service %q: %v", service.Name, err)
    }
    // Remove sidecar from NodeService now it's done it's job it's just a config
    // syntax sugar and shouldn't be persisted in local or server state.
    ns.Connect.SidecarService = nil
    if err := a.addServiceLocked(ns, chkTypes, false, service.Token, ConfigSourceLocal); err != nil {
    return fmt.Errorf("Failed to register service %q: %v", service.Name, err)
    }
    // If there is a sidecar service, register that too.
    if sidecar != nil {
    if err := a.addServiceLocked(sidecar, sidecarChecks, false, sidecarToken, ConfigSourceLocal); err != nil {
    return fmt.Errorf("Failed to register sidecar for service %q: %v", service.Name, err)
    }
    }

Notes:

  • we need to register the sidecar explicitly with a.AddService in the same way as above link - addServiceInternal won't do it for us and we need to do it in such a way that it goes back through the ServiceManager which will pick up it's config properly.
  • edge cases we need to account for but not totally sure how:
    1. the service definition already had sidecar_service = {}. This means that the config loader linked above already registered the sidecar for it and we must not override that as it may have had local customizations. I guess we could just not have a flag to AddService` that will only add it if there is no ID collision or something?
    • complication: think through the lifecycle here. For example, if the user has a custom proxy then we don't override it, but if they then remove their proxy on a service reload we should probably go back to setting up the default one - I think this would just work because the service reload would also re-register the service and so re-trigger the logic, but what above if they remove the current proxy via API? I don't think it's the end of the world if we don't handle that perfectly right away but worth considering.
    1. a sidecar was manually registered for the service already (not by sidecar service)
    • If the sidecar's ID is the same as the sidecar_service default on (e.g. web_sidecar_proxy) then this is effectively the same case as 1 above and the existing reg should override etc.
    • if the manual sidecar has a different ID then I guess we should still add the default one - it might be a separate sidecar is being run for reasons and the registration mostly doesn't hurt even if it's not actually consumed by a sidecar. the one gross thing is that -sidecar-for won't work as there will be too but we can just document "if you want to manually override a centrally configured proxy, ensure you use the same ID or you will end up with both registered" etc.
@banks banks added type/enhancement Proposed improvement or new feature theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies labels May 16, 2019
@s-mang s-mang assigned s-mang and unassigned s-mang Aug 5, 2019
@pearkes pearkes added this to the Upcoming milestone Aug 20, 2019
@jsosulska jsosulska removed this from the Upcoming milestone Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies type/enhancement Proposed improvement or new feature
Projects
None yet
Development

No branches or pull requests

4 participants