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: introduce ExternalSNI field on service-defaults #6324

Merged
merged 4 commits into from
Aug 19, 2019

Conversation

rboyer
Copy link
Member

@rboyer rboyer commented Aug 14, 2019

Compiling with a service-defaults entry defining ExternalSNI will set an optional SNI field on each DiscoveryTarget to this value and flag the target as External=true. When SNI is set this value should be used for TLS connections to the instances of the target. If not set the default should be used.

Setting ExternalSNI will disable mesh gateway use for that target. It fundamentally alters how the compiler treats the service: Failover, Subsets, and Redirects cannot be configured on an external resolver.

This supersedes #6237. The main difference is this does not shift around all of the SNI generation, just the override aspect for now.

This version avoids some agent/proxycfg watch ordering complications that arise if the discovery chain compiler needs to accept a trust domain as an input argument.

@rboyer rboyer added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Aug 14, 2019
@rboyer rboyer added this to the 1.6.0-final milestone Aug 14, 2019
@rboyer rboyer requested review from lkysow and a team August 14, 2019 20:40
@rboyer rboyer self-assigned this Aug 14, 2019
@banks
Copy link
Member

banks commented Aug 16, 2019

(pre-diff-review response).

Overall it makes sense to constrain the change to what we need for external sources for now given the complication added by generating SNI inside the compiler.

I'm still not super clear on how requiring trust domain in the compiler alters the client watch code - the trust domain is always known on the servers so I was assuming it would just need to be passed in in the RPC handler that invokes compile. We should never need to override the trust domain from a client when invoking compile - it should always just be filled in by the current active TD on the server. Does that change anything?

Another option I'm open to if it simplifies things would be leaving the Compiler as you have it here but auto-populating any blank SNI fields in the result in the RPC handler before returning it. The RPC handler should always be able to load the current active trust domain from the state store without any complication. That said, I'm not sure that it's any simpler for the RPC handler to load that and do the default setting itself, vs, loading it and passing it into the compiler to use for default setting?

The eventual goal as mentioned in the original PR is that I'd like to lift the SNI logic out of the proxy-specific code so it doesn't have to be duplicated in future proxy integrations.

All that said, happy at a high level to land this as described and pick up one of the above options later if you think that's a quicker path to landing this for external overrides.

I'll go look at the code now ;)

Copy link
Member

@banks banks left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks R.B.

See what you think about my comments above. Does it change your assumptions about how hard it would be to apply defaults during Compile too or am I missing something? Either way happy to resolve that later but if we don't do it now, maybe we could make an issue to move SNI defualting to servers so we don't forget?

target.Service,
),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice, these help make the external thing less confusing when trying to configure features that won't work any more.

agent/structs/discovery_chain.go Outdated Show resolved Hide resolved
rboyer added 3 commits August 16, 2019 12:34
Compiling this will set an optional SNI field on each DiscoveryTarget.
When set this value should be used for TLS connections to the instances
of the target. If not set the default should be used.

Setting ExternalSNI will disable mesh gateway use for that target.
Co-Authored-By: Paul Banks <banks@banksco.de>
@rboyer rboyer merged commit ae79cda into release/1-6 Aug 19, 2019
@rboyer rboyer deleted the external-sni-config branch August 19, 2019 18:04
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants