-
Notifications
You must be signed in to change notification settings - Fork 77
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
Align the encryption flags #858
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #858 +/- ##
==========================================
+ Coverage 82.26% 82.30% +0.03%
==========================================
Files 45 45
Lines 1720 1729 +9
==========================================
+ Hits 1415 1423 +8
+ Misses 264 263 -1
- Partials 41 43 +2
☔ View full report in Codecov by Sentry. |
About the expected CI errors: I see the following options
We'd need to do both approaches our configuration would be broken for a short time period. And we'd need to do the fixes in a short time period and before the release. Opinions? |
I think I could go for
I can get some quick PRs for serving and net-contour to use your new stuff ready. That way it can get into a passing state. And then we can move on to do the proper support of everything? |
7d165be
to
06ba58e
Compare
I would like to get approval from TOC just in case. Otherwise LGTM. |
@knative/technical-oversight-committee @dprotaso Could you please take a look: #858 (comment) |
31f2376
to
f87cb2a
Compare
TOC wouldn't make this call - just leads/approvers of the WG. It's an alpha feature so I don't think we should worry about the renaming. |
pkg/config/config.go
Outdated
ControlplaneTrustKey = "controlplane-trust" | ||
// KnativeInternalTLSKey is the name of the configuration whether | ||
// knative internal traffic is encrypted or not. | ||
KnativeInternalTLSKey = "knative-internal-tls" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Circling back on this name I wonder if we should call this
KnativeInternalTLSKey = "knative-internal-tls" | |
InternalComponentTLSKey = "internal-component-tls" |
Otherwise having knative
in the key name seems unnecessary since the operator knows we're configuring something about knative and internal
alone isn't specific enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I get the knative
part, but I'm not super convinced with the component
part (it seems a bit generic, which components?). Maybe something like this?
internal-communication-tls
system-internal-tls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like system-internal-tls
internal-communication-tls
still feels like it bleeds into cluster-local-domain-tls
or whatever we're calling that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool let's use system-internal-tls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, updated it to system-internal-tls
.
@KauzClay mind updating your two WIP PRs before we merge this here?
/lgtm @ReToCode will let you unhold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, ReToCode 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 |
/unhold @dprotaso can you override the failed checks? |
/override "Unit Test (knative/serving)" |
@dprotaso: Overrode contexts on behalf of dprotaso: Unit Test (knative-extensions/net-contour), Unit Test (knative/serving) In response to this:
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. |
/override "style / Golang / Lint" |
@dprotaso: Overrode contexts on behalf of dprotaso: style / Golang / Lint In response to this:
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. |
* use new internal TLS names * update deps
bumping knative.dev/net-contour d0422bc...bc7c9d1: > bc7c9d1 use new encryption flags from knative/networking#858 (# 958) Signed-off-by: Knative Automation <[email protected]>
bumping knative.dev/net-contour d0422bc...bc7c9d1: > bc7c9d1 use new encryption flags from knative/networking#858 (# 958) Signed-off-by: Knative Automation <[email protected]>
Changes
dataplane-trust
andcontrolplane-trust
I'd argue that its ok to drop the dataplane-& controlplane-trust flags, as they are clearly marked as ALPHA. IMHO, Introducing backward compatibility for those would make it really unread and unmaintainable. Also we have not implemented this feature and various values of those, so it's basically not possible to be reverse compatible.
/kind cleanup
Partially fixes knative/serving#14368
Release Note
Docs
PRs for docs will follow separately in knative/serving#14368