HTTPRoute: Add Reencrypt #81

Miciah · 2020-02-13T08:22:50Z

api/v1alpha1/httproute_types.go (HTTPRouteHost): Add TLS field.
(HTTPRouteTLS): New type. Allow specifying a destination CA certificate.
api/v1alpha1/generated.pb.go:
api/v1alpha1/generated.proto:
api/v1alpha1/zz_generated.deepcopy.go:
config/crd/bases/networking.x.k8s.io_httproutes.yaml:
docs/concepts/index.html:
docs/search/search_index.json: Regenerate.

Here is a draft of an approach for accommodating the re-encrypt use-case #124.

k8s-ci-robot · 2020-02-13T08:22:58Z

Hi @Miciah. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-02-13T08:23:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Miciah
To complete the pull request process, please assign bowei
You can assign the PR to them by writing /assign @bowei in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Miciah · 2020-02-13T08:25:40Z

api/v1alpha1/httproute_types.go

@@ -190,6 +195,19 @@ type HTTPRouteAction struct {
 	Extension *core.TypedLocalObjectReference `json:"extension"`
 }

+// HTTPReencrypt specifies parameters for re-encrypting the forwarded
+// connection.
+type HTTPReencrypt struct {


This type would be an appropriate place to add a field to configure a client certificate for mTLS in the future.

AppProtocol field is being added to k8s 1.18 and there might be a conflict for that here.
I think having this makes sense to me but we should have an explicit protocols field here to define what is the protocol to use when communicating with the upstream service. It can be added in the future. Just leaving a note here, not requesting any changes or advocating against this field.

hbagdi · 2020-02-13T22:39:58Z

api/v1alpha1/tcproute_types.go

+type TCPRouteHost struct {
+	// Hostname is a fully qualified domain name.  This must be a registered
+	// name as defined by RFC 3986.  Note that IP addresses cannot be used.
+	Hostname string `json:"hostname"`


There should be a destination for each host in this section, similar to HTTPRouteAction in the HTTPRoute resource. Am I missing something?

Yes, we will need to add targets as well. I wanted to focus this PR on the pieces that would enable the three use-cases that I listed (edge-termination, re-encrypt, and pass-through TLS), but I can fill out the TCPRoute target as part of this PR if that would make the idea clearer.

I'd recommend to have at least a mock struct in place with a TODO.

hbagdi · 2020-02-13T22:40:45Z

api/v1alpha1/tcproute_types.go

+// TCPRouteSpec defines the desired state of the TCPRoute.
+type TCPRouteSpec struct {
+	// Hosts is a list of Host definitions.
+	Hosts []TCPRouteHost `json:"hosts,omitempty"`


I think it would be better to call this Rules instead.
Also, s/TCPRouteHost/TCPRouteRule.

I wanted to keep the structure of TCPRoute similar to the structure of HTTPRoute where possible. Should we make the change you are suggesting to HTTPRoute as well?

HTTPRoute is up in the air right now because we are still working through the relationship of Gateway, HTTPRoute and host.

THe current api is:

hosts: - host: "foo.com" destinations # will be in future - host: "bar.com" SNIP

hosts seems not the best name here since it is not an array of hosts but something more.

hbagdi · 2020-02-13T22:46:09Z

docs-src/concepts.md

+`TCPRoute` does not dictate that the TLS connection encapsulate a
+specific protocol.  A common use-case for `TCPRoute` is to pass
+through connections to an HTTPS server, but in principle `TCPRoute`
+can be used to forward any protocol that is compatible with TLS.


There is another use-case that needs to be addressed with TCPRoute resource:

A user wants to route traffic based on TCP ports, meaning, don't even peek into the TCP stream. This is a subset of the use-case in #66.

We can merge this in and I can piggy back on top of these changes.
What I'm thinking is:

Define a port in addition to host for matching

Make the host optional. If it is not present, then the proxy performs port based routing, if the host is present, then the proxy performs SNI based routing.

In this use case, is it is one-to-one: one listener to one route? That could work. I am wary of implicit APIs, so maybe instead of empty host it could be an explicit * value, or something more explicit?

@hbagdi WDYT about filing a separate user story for that use case?

Opened up #96

hbagdi · 2020-02-13T22:47:59Z

api/v1alpha1/tcproute_types.go

+// TCPRouteHost is the configuration for a given host.
+type TCPRouteHost struct {
+	// Hostname is a fully qualified domain name.  This must be a registered
+	// name as defined by RFC 3986.  Note that IP addresses cannot be used.


Since Hostname is sort of overloaded in multiple layers of the OSI model, can we be explicit here that host in this case means the SNI in the TLS protocol?

Yeah, we can do that. I think it would be clearest to define range of acceptable values (as the godoc I wrote attempts to do), and then explain how the value is intended to be used (namely, to match the TLS SNI). Does that seem reasonable?

Sounds good to me. I'm not sure where it should be present, in godoc or somewhere else. At the moment, godoc seems like a good place.

bowei · 2020-02-13T23:26:43Z

/ok-to-test

bowei · 2020-02-13T23:38:24Z

api/v1alpha1/httproute_types.go

+	// Reencrypt specifies that the gateway controller should re-encrypt the
+	// forwarded connection.
+	// +optional
+	Reencrypt *HTTPReencrypt `json:"reencrypt"`


Can we use the Service app-protocol instead of putting it here?

@Miciah @bowei we need to make a decision on what persona is responsible for managing certificates. https://github.com/kubernetes-sigs/service-apis/pull/71/files#diff-e7d01144441bba257ce9bedd33a139c6R217 provides similar functionality but resides within Gateway. Is a Cluster Operator responsible for listener certs and a Dev responsible for backend certs?

@Miciah if the Dev persona is responsible for managing backend certs, then this PR is the preferred approach.

bowei · 2020-02-13T23:40:23Z

api/v1alpha1/tcproute_types.go

-type TcpRouteSpec struct {
-	// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
-	// Important: Run "make" to regenerate code after modifying this file
+// TCPRouteSpec defines the desired state of the TCPRoute.


Is this TCPRoute or TLSRoute?

TCP means plain TCP
TLSRoute seems to be more apt?

I don't have a preference on naming but we want to support both of these use-cases, TCP and TLS routing using the same CRD.
My leaning is slightly more towards TCPRoute.

Might be confusing for users, we will have to make sure it's unambiguous what happens, also can be kind of weird to mix TCP and TLS together (do users have this as a use case)? Also, TCP has no such thing as SNI, Host etc.

I'm guessing you are thinking of something like this?

gateway: port 80, protocol TCP port 443, protocol TLS routes: - myRoute myRoute: TCPRoute "foo.com" => svc1 * => svc2

Something more like:

TCP (port based routing):

- port: 4242 => svc1 - port: 2424 => svc2

This is the dumbest form of routing one can have essentially.
#96

TLS (SNI based routing):

- host/sni: foo.com => svc1 - host/sni: bar.com => svc2

This can optionally include port too. User story: I want TLS sessions for foo.com on port X to be routed to svc1 but foo.com on port Y should not be allowed.

bowei · 2020-02-13T23:40:59Z

docs-src/concepts.md


 #### `TCPRoute`

-TODO
+`TCPRoute` is a namespace-scoped resource defined by the application


See comment above (TCPRoute vs TLSRoute) naming

Ah, right, sorry, it is TCPRoute, but this paragraph suggests that it is necessarily TLS; I will update (and maybe try to incorporate the dedicated-port use-case described in #81 (comment)).

jpeach · 2020-02-14T04:19:29Z

I filed a user story specifically for TCP forwarding #94.

jpeach

General feedback ...

I think it would be better to address HTTPRoute and TCPRoute in separate PRs. In particular, because TCPRoute is just a placeholder right now, it seems like a more involved discussion,

For the "Reencrypt"; I'm not enamored of that terminology. TLS is a per-hop property and each hop can be configured independently, so re-encryption can be a misnomer if the front end wasn't encrypted in the first place.

danehans · 2020-02-14T18:44:45Z

I think it would be better to address HTTPRoute and TCPRoute in separate PRs. In particular, because TCPRoute is just a placeholder right now, it seems like a more involved discussion,

@Miciah I agree that TCPRoute should be a separate PR.

For the "Reencrypt"; I'm not enamored of that terminology. TLS is a per-hop property and each hop can be configured independently, so re-encryption can be a misnomer if the front end wasn't encrypted in the first place.

@Miciah Instead of Reencrypt, what if TLS was used to specify several different params (i.e. certs, tls version, etc.) for the gateway to apply when creating the backend TLS connection?

Miciah · 2020-02-14T21:11:11Z

@jpeach,

I think it would be better to address HTTPRoute and TCPRoute in separate PRs. In particular, because TCPRoute is just a placeholder right now, it seems like a more involved discussion,

I wanted to address them in the same PR in order to present a comprehensive, minimally redundant solution to cover the three related but disjoint use-cases. If it makes review easier, I can refocus this PR on HTTPRoute, open a separate PR for TCPRoute, and note in each PR's description that the two PRs are part of the same proposal.

For the "Reencrypt"; I'm not enamored of that terminology. TLS is a per-hop property and each hop can be configured independently, so re-encryption can be a misnomer if the front end wasn't encrypted in the first place.

Good point. We could rename the field to TLS... but it seems like it is less an action now, and it isn't a match or filter rule, so I'm not sure where it fits; should it go directly under HTTPRouteHost?

@danehans,

@Miciah Instead of Reencrypt, what if TLS was used to specify several different params (i.e. certs, tls version, etc.) for the gateway to apply when creating the backend TLS connection?

Agreed, TLS is a better field name. Thanks!

Miciah · 2020-02-14T22:03:58Z

Latest push deletes changes to TCPRoute, renames Reencrypt to TLS, and moves the TLS configuration from HTTPRouteAction to HTTPRouteHost.

Miciah · 2020-02-14T22:12:53Z

Rebased.

Miciah · 2020-02-14T22:15:58Z

I'll hold off on submitting a PR for TCPRoute while HTTPRouteHost is in flux (per #81 (comment)).

Miciah · 2020-02-19T20:11:52Z

Rebased.

Miciah · 2020-03-02T21:40:10Z

Rebased.

bowei · 2020-03-04T23:50:08Z

xref: #52

* api/v1alpha1/httproute_types.go (HTTPRouteHost): Add TLS field. (HTTPRouteTLS): New type. Allow specifying a destination CA certificate. * docs-src/concepts.md: Stub out description of HTTPRoute. * api/v1alpha1/generated.pb.go: * api/v1alpha1/generated.proto: * api/v1alpha1/zz_generated.deepcopy.go: * config/crd/bases/networking.x.k8s.io_httproutes.yaml: * docs/concepts/index.html: * docs/search/search_index.json: Regenerate.

Miciah · 2020-03-05T00:24:12Z

Rebase and regenerate deepcopy and protobuf.

k8s-ci-robot · 2020-03-18T18:38:20Z

@Miciah: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-04-23T23:30:02Z

@Miciah: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-service-apis-verify	`c8769b4`	link	`/test pull-service-apis-verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2020-07-22T23:51:07Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

robscott · 2020-08-13T22:26:23Z

Thanks for the work on this @Miciah! I think there's some overlap here with @hbagdi's more recent PR adding passthrough and terminate TLS modes. Because this PR has gotten stale I'm going to close this for now but don't hesitate to reopen this if we should still explore this further.

robscott · 2020-08-13T22:26:31Z

/close

k8s-ci-robot · 2020-08-13T22:26:39Z

@robscott: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Added frontend and middleware manifest files from Online Boutique v0.3.5

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 13, 2020

k8s-ci-robot requested review from bowei and thockin February 13, 2020 08:23

Miciah commented Feb 13, 2020

View reviewed changes

hbagdi reviewed Feb 13, 2020

View reviewed changes

hbagdi mentioned this pull request Feb 13, 2020

TLS Termination Policy #52

Closed

bowei reviewed Feb 13, 2020

View reviewed changes

jpeach reviewed Feb 14, 2020

View reviewed changes

danehans mentioned this pull request Feb 14, 2020

TLS: Align APIs with Personas #95

Closed

Miciah force-pushed the httproute-add-reencrypt-tcproute-add-hosts branch 3 times, most recently from abf7d2a to 1268bd1 Compare February 14, 2020 22:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020

Miciah force-pushed the httproute-add-reencrypt-tcproute-add-hosts branch from 1268bd1 to 418fb37 Compare February 14, 2020 22:17

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2020

Miciah force-pushed the httproute-add-reencrypt-tcproute-add-hosts branch from 418fb37 to 176ab95 Compare February 19, 2020 20:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2020

Miciah force-pushed the httproute-add-reencrypt-tcproute-add-hosts branch from 176ab95 to e6c37e6 Compare March 2, 2020 21:40

Miciah changed the title ~~HTTPRoute: Add Reencrypt; TCPRoute: Add Hosts~~ HTTPRoute: Add Reencrypt Mar 4, 2020

Miciah force-pushed the httproute-add-reencrypt-tcproute-add-hosts branch from e6c37e6 to c8769b4 Compare March 5, 2020 00:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2020

k8s-ci-robot closed this Aug 13, 2020

jaison-tiu pushed a commit to jaison-tiu/gateway-api that referenced this pull request Apr 14, 2022

Merge pull request kubernetes-sigs#81 from alizaidis/issue80

db51b3f

Added frontend and middleware manifest files from Online Boutique v0.3.5

HTTPRoute: Add Reencrypt #81

HTTPRoute: Add Reencrypt #81

Conversation

Miciah commented Feb 13, 2020 • edited Loading

k8s-ci-robot commented Feb 13, 2020

k8s-ci-robot commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Feb 14, 2020

jpeach left a comment

Choose a reason for hiding this comment

danehans commented Feb 14, 2020

Miciah commented Feb 14, 2020

Miciah commented Feb 14, 2020

Miciah commented Feb 14, 2020

Miciah commented Feb 14, 2020

Miciah commented Feb 19, 2020

Miciah commented Mar 2, 2020

bowei commented Mar 4, 2020

Miciah commented Mar 5, 2020

k8s-ci-robot commented Mar 18, 2020

k8s-ci-robot commented Apr 23, 2020

fejta-bot commented Jul 22, 2020

robscott commented Aug 13, 2020

robscott commented Aug 13, 2020

k8s-ci-robot commented Aug 13, 2020

Miciah commented Feb 13, 2020 •

edited

Loading

bowei Feb 14, 2020 •

edited

Loading