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

design: add fallback certificate design doc #2428

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

stevesloka
Copy link
Member

Updates #1503

Signed-off-by: Steve Sloka slokas@vmware.com

@stevesloka stevesloka added this to the 1.4.0 milestone Apr 9, 2020
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #2428 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2428   +/-   ##
=======================================
  Coverage   76.72%   76.72%           
=======================================
  Files          68       68           
  Lines        5522     5522           
=======================================
  Hits         4237     4237           
  Misses       1188     1188           
  Partials       97       97           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b40d846...3c1d05c. Read the comment docs.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Overall, I think this is a good proposal. I have a couple of questions about the mechanics of the envoy filters, and where we put the Contour config. If you prefer, we can address these in PRs.

The doc uses both "fallback certificate" and "default certificate", so we should standardize on one (I think default is the most common term?)

design/fallbackcert.md Outdated Show resolved Hide resolved
design/fallbackcert.md Outdated Show resolved Hide resolved
design/fallbackcert.md Outdated Show resolved Hide resolved
design/fallbackcert.md Show resolved Hide resolved
design/fallbackcert.md Outdated Show resolved Hide resolved
Passthrough bool `json:"passthrough,omitempty"`

// +optional
FallbackCertificatedEnabled bool `json:"fallbackCertificateEnabled,omitempty""`
Copy link
Contributor

Choose a reason for hiding this comment

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

The only comparable option I could find in the API was EnableWebsockets, so maybe we should make this EnableFallbackCertificate or EnableDefaultCertificate for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that better, thanks for the suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this change, am I looking at the right commit?

design/fallbackcert.md Outdated Show resolved Hide resolved
@stevesloka
Copy link
Member Author

So I played around with this in Envoy for a bit. I'm not sure we can enable the fallback cert for specific vhosts. My filter match at the end looks for any that does not match previous filters (e.g. SNI hosts).

So in practice, if we enable this, it must be on for all vhosts unless I'm misreading the docs.

@jpeach
Copy link
Contributor

jpeach commented Apr 12, 2020

So I played around with this in Envoy for a bit. I'm not sure we can enable the fallback cert for specific vhosts. My filter match at the end looks for any that does not match previous filters (e.g. SNI hosts).

So in practice, if we enable this, it must be on for all vhosts unless I'm misreading the docs.

I think that what you can do is in this final filter chain, only add the vhosts that have opted in to accepting a default certificate, #2428 (comment)

@stevesloka
Copy link
Member Author

@jpeach I updated the few comments. Also, I think I figured out the routing pieces to only allow the fallback cert to be part of the specific routing chain.

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

The API looks good to me, just need to update the name of the fallback field.

Passthrough bool `json:"passthrough,omitempty"`

// +optional
FallbackCertificatedEnabled bool `json:"fallbackCertificateEnabled,omitempty""`
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this change, am I looking at the right commit?

@stevesloka
Copy link
Member Author

@jpeach I updated the design after some more work today. The big changes are the needs for having a new RDS config to point the fallback domains to: https://github.com/projectcontour/contour/blob/74217e0d1bf4f6833a67282e8157ea5bc08108e5/design/fallbackcert.md#envoy-api

@youngnick youngnick modified the milestones: 1.4.0, 1.5.0 Apr 24, 2020
Signed-off-by: Steve Sloka <slokas@vmware.com>
@stevesloka stevesloka merged commit 8a6dc0b into projectcontour:master Apr 27, 2020
@stevesloka stevesloka deleted the fallbackCertDesign branch April 27, 2020 16:01
@ravilr
Copy link

ravilr commented Apr 27, 2020

@stevesloka two questions:

  1. What about hosts and route rules configured through K8s Ingress resource? will this expose the RDS routes from ingress.spec.rules[].host configured through K8s Ingress resource under the fallback certificate TLS listener ? if not, can we enable/gate that also through a ingress annotation please?
  2. this feature could be used to also resolve Feature request: support external TLS with HTTP 301 upgrades #715 where the users are TLS terminating at an external LB and having Envoy behind that LB as a trusted proxy. Such deployments could now use the fallback certificate and configure the External LB->Envoy communication over TLS listener of Envoy.

@jpeach
Copy link
Contributor

jpeach commented Apr 28, 2020

@stevesloka two questions:

  1. What about hosts and route rules configured through K8s Ingress resource? will this expose the RDS routes from ingress.spec.rules[].host configured through K8s Ingress resource under the fallback certificate TLS listener ? if not, can we enable/gate that also through a ingress annotation please?

Supporting Ingress is tracked in #2453

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.

4 participants