Skip to content

Fixes TLS Mode Defaulting #518

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2021
Merged

Fixes TLS Mode Defaulting #518

merged 1 commit into from
Jan 28, 2021

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Jan 6, 2021 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
Properly sets the "Terminate" default for gateway.spec.tls.mode.

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

Does this PR introduce a user-facing change?:
Yes, sets the default TLS mode to "Terminate".

@danehans danehans requested review from robscott and hbagdi January 6, 2021 18:27
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 6, 2021
@k8s-ci-robot k8s-ci-robot requested a review from thockin January 6, 2021 18:27
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2021
@robscott robscott mentioned this pull request Jan 6, 2021
Merged
@robscott
Copy link
Member

robscott commented Jan 6, 2021

Thanks! This LGTM other than all the boilerplate year changes. That should be resolved once #519 is in.

hbagdi
hbagdi approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

LGTM. This will need a rebase once #519 is merged.

@hbagdi
Copy link
Contributor

hbagdi commented Jan 6, 2021 •
edited
Loading

/hold

Just to make sure this doesn't get merged before #519.

@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 Jan 6, 2021
@hbagdi
Copy link
Contributor

hbagdi commented Jan 12, 2021 •
edited
Loading

@danehans Can you rebase to resolve the conflicts?
/hold cancel

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 12, 2021
@robscott
Copy link
Member

@danehans Thanks for making this PR! After a rebase this LGTM. One small nit - I think this is a user-facing change since we're adding a default value.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2021
@danehans
Copy link
Contributor Author

@robscott I updated the PR description to include user-facing changes.

hbagdi
hbagdi reviewed Jan 14, 2021
View reviewed changes
apis/v1alpha1/gateway_types.go Outdated
Mode TLSModeType `json:"mode,omitempty"`

// CertificateRef is the reference to Kubernetes object that
// contain a TLS certificate and private key.
// This certificate MUST be used for TLS handshakes for the domain
// this GatewayTLSConfig is associated with.
//
// This field is required when mode is set to "Terminate" and optional
// otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about lgtm this when I recalled why this was not the case earlier:
We wanted the user to explicitly set the condition to Terminate and make them supply the Certificate in that case.
In this case, if this default kicks in, and a CertificateRef is absent, then how do we handle this case?
We could add validation in the webhook for this case.

Do we still want this to default though?

Copy link
Member

Choose a reason for hiding this comment

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

That's a very good point - thanks for catching this! I think that approach makes sense, maybe instead of setting a default here we should clarify that both a CertificateRef and mode must be set as part of TLS config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hbagdi @robscott if ^ is the case, why not default Mode to "Passthrough" so CertificateRef can continue to be optional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because Passthrough is the rare case. It only makes sense with TCPRoute and TLSRoute (sni sniffing).

Copy link
Contributor Author

@danehans danehans Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If Mode is optional, then a user can supply a CertificateRef without setting Mode, resulting in an unusable TLS config. IMO, if "Terminate" is the common use-case, then it should be the default and the CertificateRef details documented (accomplished by this PR). @hbagdi @robscott @bowei thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The default of Terminate makes sense here. What should be the error condition that should be set when CertificateRef is not supplied?

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I'm understanding this correctly, this default will not come into play unless someone specifies TLS config. It will essentially allow a shortcut where most users would only need to specify CertificateRef here. Does that sound right? If so, I think this is a very reasonable default.

@hbagdi That's a great question. I think our existing InvalidCertificateRef condition would mostly apply here, even if the initial intent was slightly different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I'm understanding this correctly, this default will not come into play unless someone specifies TLS config. It will essentially allow a shortcut where most users would only need to specify CertificateRef here. Does that sound right?

That's correct. I pulled the patch down and tested it myself.
We actually missed specifying mode even in our example: https://github.com/kubernetes-sigs/service-apis/blob/27c4d2dcd3c70a65a1d512fc7a21d2ca4403d218/examples/tls-basic.yaml#L23-L29

@hbagdi That's a great question. I think our existing InvalidCertificateRef condition would mostly apply here, even if the initial intent was slightly different.

Sounds fair. If the overhead is not too much, I'd advocate for a new AbsentCertificateRef or a generic AbsentRef condition.

@hbagdi
Copy link
Contributor

hbagdi commented Jan 28, 2021

LGTM.
Leaving final approval to Rob.

@bowei
Copy link
Contributor

bowei commented Jan 28, 2021

/approve

@bowei
Copy link
Contributor

bowei commented Jan 28, 2021

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, danehans, hbagdi

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:
  • OWNERS [bowei,danehans,hbagdi]

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 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2021
@k8s-ci-robot k8s-ci-robot merged commit ec25823 into kubernetes-sigs:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbagdi hbagdi hbagdi approved these changes

@robscott robscott Awaiting requested review from robscott

@thockin thockin Awaiting requested review from thockin

Assignees

@bowei bowei

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/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The TLS Mode Field is not Being Defaulted
5 participants
@danehans @robscott @hbagdi @bowei @k8s-ci-robot