Add `NewProxyAutoTLSTransport` and `DialTLSWithBackOff` to support TLS proxy by nak3 · Pull Request #2479 · knative/pkg

nak3 · 2022-04-05T13:16:28Z

Part of: knative/serving#12503
PoC: knative/serving#12815

This patch NewProxyAutoTLSTransport which is `NewProxyAutoTransport + TLS config.
Current proxy does not support TLS but it needs for knative/serving#12503.

DialTLSWithBackOff is also DialWithBackOff + TLS config. It needs
newH2Transport which handles HTTP2 with TLS.

/cc @evankanderson @skonto @rhuss

…S proxy Part of: knative/serving#12503 PoC: knative/serving#12815 This patch `NewProxyAutoTLSTransport` which is `NewProxyAutoTransport + TLS config. Current proxy does not support TLS but it needs for knative/serving#12503. `DialTLSWithBackOff` is also `DialWithBackOff` + TLS config. It needs `newH2Transport` which handles HTTP2 with TLS.

codecov · 2022-04-05T13:23:52Z

Codecov Report

Merging #2479 (0ffe420) into main (c2f1f3e) will increase coverage by 0.39%.
The diff coverage is 40.54%.

❗ Current head 0ffe420 differs from pull request most recent head 253457e. Consider uploading reports for the commit 253457e to get more accurate results

@@            Coverage Diff             @@
##             main    #2479      +/-   ##
==========================================
+ Coverage   81.32%   81.71%   +0.39%     
==========================================
  Files         163      163              
  Lines        6499     9653    +3154     
==========================================
+ Hits         5285     7888    +2603     
- Misses        981     1529     +548     
- Partials      233      236       +3

Impacted Files	Coverage Δ
network/h2c.go	`16.66% <0.00%> (-8.34%)`	⬇️
network/transports.go	`71.95% <50.00%> (-14.10%)`	⬇️
webhook/certificates/resources/secret.go	`81.25% <0.00%> (-4.47%)`	⬇️
injection/informers.go	`64.70% <0.00%> (-4.05%)`	⬇️
metrics/workqueue.go	`80.82% <0.00%> (-2.90%)`	⬇️
apis/duck/v1alpha1/binding_types.go	`90.47% <0.00%> (-2.86%)`	⬇️
metrics/config.go	`75.55% <0.00%> (-2.80%)`	⬇️
webhook/certificates/resources/certs.go	`58.92% <0.00%> (-2.70%)`	⬇️
configmap/hash-gen/main.go	`62.96% <0.00%> (-2.50%)`	⬇️
apis/duck/v1/podspec_types.go	`93.10% <0.00%> (-2.14%)`	⬇️
... and 155 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f42bf4...253457e. Read the comment docs.

evankanderson

Do we want the ability to fall back to non-TLS transport during the transition if TLS is refused?

nak3 · 2022-04-05T14:44:48Z

I think we don't need it for now. AFAIK, the users, who requested this feature, want to use the strictly secure connection and they will complain about the fall-back feature if we added 😅

evankanderson · 2022-04-06T15:42:34Z

I think we need a transition plan for "current config" to "always secure". We can end up with any of the following transition scenarios:

Insecure activator -> secure queue-proxy
Secure activator -> insecure queue-proxy

I think we want to not serve errors during the update process, which means we either need a way to ensure we know the ordering, or have the ability to fall back to insecure during the transition and then "lock the gate" with a second config change

Full disclosure: I'd like to turn this on for every Knative user, all the time, in some future release.

nak3 · 2022-04-07T07:36:01Z

I see. In this case, knative/serving#12815 supports

Insecure activator -> secure queue-proxy

and so we should ensure the ordering?
Opening both HTTP and HTTPS on queue-proxy is easy to support (actually knative/serving#12815 does) so I am prefer to it rather than adding the ability to fall back to insecure.

Just in case, I verified the scinario Insecure activator -> secure queue-proxy works fine as nak3/serving#41

nader-ziada · 2022-04-07T20:16:33Z

does this PR need to get in before we cut pkg - 1.4 on April 12th?

nak3 · 2022-04-07T23:50:28Z

Yes, I hope so...

ping @evankanderson @rhuss @skonto could you please review this?

evankanderson · 2022-04-08T12:42:38Z

I see. In this case, knative/serving#12815 supports

Insecure activator -> secure queue-proxy

and so we should ensure the ordering?
Opening both HTTP and HTTPS on queue-proxy is easy to support (actually knative/serving#12815 does) so I am prefer to it rather than adding the ability to fall back to insecure.

Just in case, I verified the scinario Insecure activator -> secure queue-proxy works fine as nak3/serving#41

If you want to enforce the ordering, there needs to be a way to enable the queue-proxy serving on both ports while the activator is still only expecting http traffic. What's the configuration that enables that?

(The same problem applies with ingress -> activator, BTW)

nak3 · 2022-04-08T13:02:30Z

I implemented current PRs in serving repos as queue-proxy serves on both ports when certs are deployed. So, actually there is no way to disable to serve HTTP port at the alpha stage. But I think it is fine because the HTTP port is not used when activator initiates the HTTPs request.

The ingress -> activator is same. Activator serves on both HTTP and HTTPS ports.

evankanderson

A few more comments, but most should be straightforward.

evankanderson · 2022-04-08T12:55:44Z

+	bo.Steps = 2
+
+	// Timeout. Use special testing IP address.
+	c, err = dialBackOffHelper(context.Background(), "tcp4", "198.18.0.254:8888", bo, tlsConf)


Could this use a localhost address rather than some Internet IP?

Alternately, use one of the documentation IP ranges so our tests aren't accidentally hitting a real IP.

(i.e. use 127.200.100.10:8888)

Another PR #2402 is changing it now (and it seems we need a little bit big change for this) so I leave it as it is.

My concern was that this is a real IP owned by Oracle, but I'm not going to block this PR on it.

nak3 · 2022-04-08T14:43:05Z

Thank you! Updated.

evankanderson

/lgtm
/approve

evankanderson · 2022-04-11T05:20:30Z

+	bo.Steps = 2
+
+	// Timeout. Use special testing IP address.
+	c, err = dialBackOffHelper(context.Background(), "tcp4", "198.18.0.254:8888", bo, tlsConf)


My concern was that this is a real IP owned by Oracle, but I'm not going to block this PR on it.

knative-prow · 2022-04-11T05:21:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, nak3

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~network/OWNERS~~ [evankanderson]

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

knative-prow Bot requested review from evankanderson, rhuss and skonto April 5, 2022 13:16

knative-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 5, 2022

nak3 mentioned this pull request Apr 5, 2022

All Hops Encrypted: TLS between activator and queue-Proxy knative/serving#12815

Merged

4 tasks

Fix lint

0ffe420

knative-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2022

evankanderson reviewed Apr 5, 2022

View reviewed changes

evankanderson reviewed Apr 8, 2022

View reviewed changes

Fix review comments

253457e

evankanderson reviewed Apr 11, 2022

View reviewed changes

knative-prow Bot assigned evankanderson Apr 11, 2022

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2022

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2022

knative-prow Bot merged commit ca82d2b into knative:main Apr 11, 2022

nak3 deleted the add-h2-transport branch April 11, 2022 08:50

nak3 mentioned this pull request Apr 11, 2022

All Hops Encrypted: Activator serving TLS knative/serving#12770

Merged

7 tasks

nak3 mentioned this pull request Sep 27, 2023

Use DialTLSContextFunc instead of tls.Config for NewProxyAutoTLSTransport #2842

Merged

Conversation

nak3 commented Apr 5, 2022

Uh oh!

codecov Bot commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

nak3 commented Apr 5, 2022

Uh oh!

evankanderson commented Apr 6, 2022

Uh oh!

nak3 commented Apr 7, 2022

Uh oh!

nader-ziada commented Apr 7, 2022

Uh oh!

nak3 commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson commented Apr 8, 2022

Uh oh!

nak3 commented Apr 8, 2022

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

evankanderson Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

evankanderson Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

nak3 Apr 8, 2022

Choose a reason for hiding this comment

Uh oh!

evankanderson Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nak3 commented Apr 8, 2022

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 5, 2022 •

edited

Loading

nak3 commented Apr 7, 2022 •

edited

Loading