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

Adding GEP 2907: TLS Configuration Placement and Terminology #2910

Merged
merged 1 commit into from Apr 6, 2024

Conversation

robscott
Copy link
Member

What type of PR is this?
/kind gep

What this PR does / why we need it:
This adds a new memorandum GEP that attempts to chart a vision for where our TLS configuration will live and what it will be called.

Which issue(s) this PR fixes:
Fixes #2907

Does this PR introduce a user-facing change?:

NONE

/cc @arkodg @candita @youngnick

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 29, 2024
@robscott
Copy link
Member Author


| Proposed Placement | Name | Status |
|-|-|-|
| Gateway | `Gateway.BackendTLS.ClientCertificateRef` | Not Proposed |
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to Gateway.BackendTLS.ClientCertificateRef

@robscott robscott added this to the v1.1.0 milestone Apr 1, 2024
@youngnick
Copy link
Contributor

LGTM with one sadly blocking comment about explaining the naming things better.

Copy link
Contributor

@candita candita left a comment

Choose a reason for hiding this comment

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

Remembering the discussions around why the content of BackendTLSPolicy wasn't going to be added directly to Gateway, one reason was because BackendTLS should be accessible to the app developer role, and the burden of app or service-level certificate management shouldn't be placed on the cluster operator. How do we reconcile those discussions with this proposal? Add a new role, like application operator or similar?

If we kept both BackendTLSPolicy and Gateway.BackendTLS, then we'd need to define how and when one could override the other.

How about considering a new policy attachment called FrontendTLSPolicy, which would be available to cluster operators, but maybe not app developers. It would be similar to BackendTLSPolicy in many ways, but perhaps targetRefs could be listeners on the gateway? Would it work for @arkodg's use cases ?

geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Outdated Show resolved Hide resolved
geps/gep-2907/index.md Show resolved Hide resolved
* Define top level fields where TLS configuration can live.

## Non-Goals
* Add or change fields directly. (This may inspire changes in other GEPs
Copy link
Contributor

Choose a reason for hiding this comment

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

I see some proposed changes, so this is not really a non-goal.

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 guess what I'm trying to say here is that if we translate this into actual API changes, those will be tracked by the GEPs associated with those APIs. This GEP is a memorandum GEP and not meant to track or own any individual API changes, will try to clean this up.

Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding this GEP

geps/gep-2907/index.md Outdated Show resolved Hide resolved
@youngnick
Copy link
Contributor

/lgtm

subject to @candita and @Miciah being satisfied with language updates. This is a 1.1 release blocker, since it blocks moving forward with GEP-91, which is also a release blocker for 1.1.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024
@youngnick youngnick added the release-blocker MUST be completed to complete the milestone label Apr 4, 2024
|-|-|-|
| Gateway | `Gateway.Spec.BackendTLS.Validation` | Not Proposed |
| BackendTLSPolicy | `BackendTLSPolicy.Spec.Validation` | Experimental (Different Name) |

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we settle what to do hostnames/sans?

eg. in the client validation GEP we have

  // SubjectAltNames contains one or more alternate names to verify
  // the subject identity in the certificate presented by the client.
  //
  // Support: Core
  //
  // +optional
  // +kubebuilder:validation:MinItems=1
  SubjectAltNames []string `json:"subjectAltNames,omitempty"`

but in BackendTLSPolicy we have

// Hostname is used for two purposes in the connection between Gateways and
// backends:
//
// 1. Hostname MUST be used as the SNI to connect to the backend (RFC 6066).
// 2. Hostname MUST be used for authentication and MUST match the certificate
//    served by the matching backend.
//
// Support: Core
Hostname v1beta1.PreciseHostname `json:"hostname"`

It'd be great to one name these things the same and potentially settle on supporting alternate names in both frontend and backend use cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @arkodg

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I believe @arkodg has removed SANs from the client validation GEP. I think we definitely need to consider both if/when there's overlap again, I'm particularly interested in adding SANs to BackendTLSPolicy for example, but I think both are out of scope for v1.1 so we can follow up when they're proposed in the future.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024
@robscott
Copy link
Member Author

robscott commented Apr 4, 2024

Thanks everyone for the feedback! I think I've addressed everything now, so PTAL. I'm going to remove the hold so the next LGTM merges this. I think we've got pretty good consensus around this GEP at this point and I really want to unblock @arkodg's GEP that depends on this one.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024
@candita
Copy link
Contributor

candita commented Apr 4, 2024

Sorry, I feel like we are rushing this.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024
geps/gep-2907/index.md Outdated Show resolved Hide resolved
@robscott robscott force-pushed the tls-memorandum-gep branch 2 times, most recently from d90872e to 6de2c06 Compare April 4, 2024 22:16
geps/gep-2907/index.md Outdated Show resolved Hide resolved
## Proposed Segments
Note that this does not represent any form of commitment that any of these
fields or concepts will exist within Gateway API. If or when they do, we propose
the following naming structure:
Copy link
Contributor

@candita candita Apr 4, 2024

Choose a reason for hiding this comment

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

I'd like to summarize the proposed segments, only because when I look at them together they don't seem as unified as they could be:

Gateway.Listener.TLS.FrontendValidation
Gateway.Listener.TLS
Gateway.Spec.BackendTLS.ClientCertificateRef
Gateway.Spec.BackendTLS.Validation

To me the problem of dealing with the constraint of Listener is better solved by doing something like this:

Gateway.Listener.TLS.FrontendValidation
Gateway.Listener.TLS
Gateway.Transmitter.TLS.BackendValidation
Gateway.Transmitter.TLS

or even

Gateway.Listener.TLS.Validation
Gateway.Listener.TLS
Gateway.Transmitter.TLS.Validation
Gateway.Transmitter.TLS

We don't have to use the term Transmitter, just something other than BackendTLS, which clashes with BackendTLSPolicy. I feel a move toward this pattern presents an easier learning curve for people with memory challenges (like me).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good questions, I've added two separate but related sections to address this:

  • Why the inconsistency between Frontend and Backend TLS config on Gateways?
  • Why Not Listener level to match FrontendValidation?

I've also added a note that we should reconsider these answers before developing any kind of backend TLS config at a Gateway level. Hope that all works for you.

@candita
Copy link
Contributor

candita commented Apr 4, 2024

/cancel hold

@robscott
Copy link
Member Author

robscott commented Apr 6, 2024

Thanks for all the great feedback @candita, I think I've addressed everything now. Would appreciate an additional round of review and/or LGTM from anyone that has time so we can get this one in and unblock the dependent updates.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2024
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, robscott

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot merged commit f63954f into kubernetes-sigs:main Apr 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/gep PRs related to Gateway Enhancement Proposal(GEP) lgtm "Looks good to me", indicates that a PR is ready to be merged. release-blocker MUST be completed to complete the milestone release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEP: TLS Configuration Placement and Terminology
8 participants