Introduce virtual servers as a concrete type. #101

jpeach · 2020-02-16T21:26:20Z

This is an RFC sketch for discussion, not intended to be merged. Please do not merge.

Since the HTTPRoute terminates traffic (either HTTP or TLS), it behaves
more like an endpoint than list a route. Introducinng the explicit concept
of a virtual server improves the clarity of the object model by renaming
the existing Route objects using terminology that better describes what
they do.

Now that we have virtual server terminology, attach TLS information at
the server layer. This gives an explicit association between network
applications (as represented by a virtual server) and their TLS
configuration.

Listeners are still resources that are attached to a gateway, but they not
have a type, which can be "combined" or "dedicated". Dedicated listeners
host up to 1 virtual server, where combined listeners may host many
virtual servers. Combined listeners must have a discriminator in the
server protocol stack, which the controller implementation is required
to validate.

The TLS configuration is barely a sketch, but it consists of the
configuration for the comunicating party and a conviguration for
validating its peer. It's not yet clear to me if this structure will
suffice for both initiators and receivers.

Signed-off-by: James Peach jpeach@vmware.com

xref #72
xref #94
xref #49

Since the HTTPRoute terminates traffic (either HTTP or TLS), it behaves more like an endpoint than list a route. Introducinng the explicit concept of a virtual server improves the clarity of the object model by renaming the existing Route objects using terminology that better describes what they do. Now that we have virtual server terminology, attach TLS information at the server layer. This gives an explicit association between network applications (as represented by a virtual server) and their TLS configuration. Listeners are still resources that are attached to a gateway, but they not have a type, which can be "combined" or "dedicated". Dedicated listeners host up to 1 virtual server, where combined listeners may host many virtual servers. Combined listeners must have a discriminator in the server protocol stack, which the controller implementation is required to validate. The TLS configuration is barely a sketch, but it consists of the configuration for the comunicating party and a conviguration for validating its peer. It's not yet clear to me if this structure will suffice for both initiators and receivers. Signed-off-by: James Peach <jpeach@vmware.com>

k8s-ci-robot · 2020-02-16T21:26:28Z

Hi @jpeach. 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-16T21:26:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jpeach
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin 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

jpeach · 2020-02-16T21:46:01Z

/cc @youngnick @danehans

bowei · 2020-02-16T23:25:20Z

This is pretty complicated for the end user. Can you add a few sketches as to what this looks on a config level to your PR description.

api/v1alpha1/tcproute_types.go

bowei · 2020-02-16T23:28:08Z

api/v1alpha1/tls_types.go

+// - gcp: https://cloud.google.com/load-balancing/docs/use-ssl-policies#creating_an_ssl_policy_with_a_custom_profile
+// - aws: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/create-https-listener.html#describe-ssl-policies
+// - azure: https://docs.microsoft.com/en-us/azure/app-service/configure-ssl-bindings#enforce-tls-1112
+type TLSConfiguration struct {


As I mentioned in on a related issue, for the intial API, most these things will be placed in implementation-specific extensions.

We unfortunately need to consider a subset for the core and extended APIs.

This doesn't change the items that can be configured much. The main thing here is that I break out an explicit (but currently un-specced) object for TLS peer validation. I think that's a useful approach that will help us manage the different concerns around TLS config.

For usability I would like to see a more simplified names, i.e. TLSConfig or TLSOptions.

bowei · 2020-02-16T23:28:45Z

api/v1alpha1/gateway_types.go

+const ListenerProtocolTCP ListenerProtocolType = "TCP"
+const ListenerProtocolUDP ListenerProtocolType = "UDP"
+
+type Listenertype string


Typo here...

Signed-off-by: James Peach <jpeach@vmware.com>

bowei · 2020-02-16T23:43:12Z

For my own understanding:

Push TLS configuration down to the Route level (What you've renamed "xxxServer").
Added a flag for validating listener <=> server hostnames is singular.

How do we control who specifies a cert if we don't want the app dev to be able to select their own TLS configuration?
This loses some of the split of responsibility that we've had in the other separation.

jpeach · 2020-02-17T00:41:04Z

How do we control who specifies a cert if we don't want the app dev to be able to select their own TLS configuration?

Do we need to capture a user story for this? This PR explores how to associate a TLS config with an endpoint (virtual host), but you can imagine there's more than one way to do that. We could have another object reference to the secrets in a TLS config, or we could reference TLS configurations by name.

This loses some of the split of responsibility that we've had in the other separation.

The other separation suffers from the problem that the API doesn't easily express how virtual hosts are associated with their TLS configuration. This association is a pretty fundamental property of an application, so I think that exposing it in the API is important.

We can still allow the responsibility for TLS (keys in particular) to be split, while preserving this association.

danehans · 2020-02-17T17:37:36Z

/hold since the intention of this PR is for discussion only.

danehans · 2020-02-17T17:41:47Z

api/v1alpha1/gateway_types.go

+
+type Listenertype string
+
+// DedicatedListenerType describes a listener with exactly on attached server.


danehans · 2020-02-17T17:42:30Z

api/v1alpha1/gateway_types.go

+// All traffic received by this listener is terminated by this server.
+const DedicatedListenerType ListenerType = "Dedicated"
+
+// CombinedListenerType describes a listener what may have multiple attached


s/what/that/

danehans · 2020-02-17T17:44:14Z

api/v1alpha1/gateway_types.go

+// CombinedListenerType describes a listener what may have multiple attached
+// servers. The protocol recognized by this listener must have a discriminator
+// that can be used to select the appropriate server for the incoming request.
+// In the case of a streaming data over TLS, the ALPN protocol name can sepect


sepect... do you mean inspect?

I think I meant to say "the ALPN protocol name can be used to select".

danehans · 2020-02-17T17:47:56Z

api/v1alpha1/gateway_types.go

+// that can be used to select the appropriate server for the incoming request.
+// In the case of a streaming data over TLS, the ALPN protocol name can sepect
+// between different StreamServer specification. In the case of HTTP traffic,
+// the HTTP Host (i.e. H2 authority) provides the discriminator.


Is "HTTP Host" the Host header? If so, please add the word header.

Is SNI server_name considered a discriminator for TLS?

Yep, this is the Host header (aka HTTP/2 authority header).

Right, SNI would be the TLS discriminator.

danehans · 2020-02-17T17:52:59Z

api/v1alpha1/httproute_types.go

+	// The ALPNProtocols field in this TLSAcceptor must contain only valid
+	// HTTP protocol identifiers, i.e. "http/0.9", "http/1.0", "http/1.1",
+	// "h2". Implementations may accept only a subset of these values if
+	// the underlying proxy implementation does not implement the


Remove "proxy".

danehans · 2020-02-17T18:11:14Z

api/v1alpha1/tls_types.go

+// TLSInitiator describes the TLS configuration for a party that initiates TLS
+// sessions.
+//
+// TODO(jpeach): consider merging TLSAcceptor and TLSInitiator if it turns out


I think removing TLSInitiator and TLSAcceptor and embedding TLSConfiguration in types will reduce complexity for users. If we find that an initiator and acceptor require different properties then let's introduce the additional complexity.

Yeh, I ended up thinking something similar. I think that the separation between configuration and peer validation makes sense. I'm not sold that initiator vs. acceptor is needed :)

danehans · 2020-02-17T18:18:11Z

How do we control who specifies a cert if we don't want the app dev to be able to select their own TLS configuration?
This loses some of the split of responsibility that we've had in the other separation.

@bowei @jpeach IMO we need to get #95 resolved to move forward with this or any of the other PR's that address TLS.

/cc @Miciah

k8s-ci-robot · 2020-02-17T18:18:12Z

@danehans: GitHub didn't allow me to request PR reviews from the following users: Miciah.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

How do we control who specifies a cert if we don't want the app dev to be able to select their own TLS configuration?
This loses some of the split of responsibility that we've had in the other separation.

@bowei @jpeach IMO we need to get #95 resolved to move forward with this or any of the other PR's that address TLS.

/cc @Miciah

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.

youngnick · 2020-02-17T23:57:11Z

Yeah, I agree that #95 is a good place to coalesce around until we get the underlying API type definitions sorted out.

k8s-ci-robot · 2020-02-27T20:01:19Z

@jpeach: 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.

bowei · 2020-03-04T23:21:37Z

/ok-to-test

k8s-ci-robot · 2020-03-04T23:23:18Z

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

Test name	Commit	Details	Rerun command
pull-service-apis-golint	`83bcedd`	link	`/test pull-service-apis-golint`
pull-service-apis-gofmt	`83bcedd`	link	`/test pull-service-apis-gofmt`
pull-service-apis-docs	`83bcedd`	link	`/test pull-service-apis-docs`
pull-service-apis-build	`83bcedd`	link	`/test pull-service-apis-build`
pull-service-apis-boilerplate	`83bcedd`	link	`/test pull-service-apis-boilerplate`

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.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 16, 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 16, 2020

k8s-ci-robot requested review from bowei and thockin February 16, 2020 21:26

k8s-ci-robot requested review from danehans and youngnick February 16, 2020 21:46

bowei reviewed Feb 16, 2020

View reviewed changes

api/v1alpha1/tcproute_types.go Show resolved Hide resolved

bowei reviewed Feb 16, 2020

View reviewed changes

Fix StreamServer rename errors.

83bcedd

Signed-off-by: James Peach <jpeach@vmware.com>

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2020

danehans reviewed Feb 17, 2020

View reviewed changes

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

This was referenced Feb 28, 2020

Refactor httproute type to virtualserver #119

Closed

Refactors APIs #120

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2020

jpeach closed this Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce virtual servers as a concrete type. #101

Introduce virtual servers as a concrete type. #101

jpeach commented Feb 16, 2020

k8s-ci-robot commented Feb 16, 2020

k8s-ci-robot commented Feb 16, 2020

jpeach commented Feb 16, 2020

bowei commented Feb 16, 2020

bowei Feb 16, 2020

jpeach Feb 17, 2020

danehans Feb 17, 2020

bowei Feb 16, 2020

jpeach Feb 17, 2020

bowei commented Feb 16, 2020 •

edited

Loading

jpeach commented Feb 17, 2020

danehans commented Feb 17, 2020

danehans Feb 17, 2020

danehans Feb 17, 2020

danehans Feb 17, 2020

jpeach Feb 17, 2020

danehans Feb 17, 2020

danehans Feb 17, 2020

jpeach Feb 17, 2020

danehans Feb 17, 2020

danehans Feb 17, 2020

jpeach Feb 17, 2020

danehans commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

youngnick commented Feb 17, 2020

k8s-ci-robot commented Feb 27, 2020

bowei commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020


		type Listenertype string

		// DedicatedListenerType describes a listener with exactly on attached server.

Introduce virtual servers as a concrete type. #101

Introduce virtual servers as a concrete type. #101

Conversation

jpeach commented Feb 16, 2020

k8s-ci-robot commented Feb 16, 2020

k8s-ci-robot commented Feb 16, 2020

jpeach commented Feb 16, 2020

bowei commented Feb 16, 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

bowei commented Feb 16, 2020 • edited Loading

jpeach commented Feb 17, 2020

danehans commented Feb 17, 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

danehans commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

youngnick commented Feb 17, 2020

k8s-ci-robot commented Feb 27, 2020

bowei commented Mar 4, 2020

k8s-ci-robot commented Mar 4, 2020

bowei commented Feb 16, 2020 •

edited

Loading