-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
All Hops Encrypted: TLS between activator and queue-Proxy #12815
Conversation
440ed18
to
4a79434
Compare
a52ab2a
to
87f3d49
Compare
252faa3
to
4cfda88
Compare
…S proxy (#2479) * Add `NewProxyAutoTLSTransport` and `DialTLSWithBackOff` to support TLS 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. * Fix lint * Fix review comments
4cfda88
to
c6a967d
Compare
986115d
to
685a942
Compare
Codecov Report
@@ Coverage Diff @@
## main #12815 +/- ##
==========================================
- Coverage 87.44% 87.01% -0.43%
==========================================
Files 196 197 +1
Lines 9782 14426 +4644
==========================================
+ Hits 8554 12553 +3999
- Misses 938 1578 +640
- Partials 290 295 +5
Continue to review full report at Codecov.
|
* Add certificates keys in config-network This patch adds the following certificate variables: - `activator-server-certs` - `queue-proxy-ca` - `queue-proxy-san` - `queue-proxy-server-certs` It is similar to #608. knative/serving#12815 and knative/serving#12770 verifeid the change. * Rename -server-certs with -cert-secret * Bump checksum * Fix lint
685a942
to
551f285
Compare
09e936c
to
3375537
Compare
I'll take a look at this tonight; I'm on my way to sitting break right now. |
3375537
to
51fb4f4
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nak3 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 |
@@ -123,7 +123,7 @@ func TestActivationHandler(t *testing.T) { | |||
|
|||
ctx, cancel, _ := rtesting.SetupFakeContextWithCancel(t) | |||
defer cancel() | |||
handler := New(ctx, test.throttler, rt, false /*usePassthroughLb*/, logging.FromContext(ctx)) | |||
handler := New(ctx, test.throttler, rt, false /*usePassthroughLb*/, logging.FromContext(ctx), false /* 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.
Could we add a test where this is true?
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 it is not possible to add the HTTPs proxy in this test code. TestNewHeaderPruningProxyHTTPS
covered the part instead.
@@ -69,7 +69,7 @@ func BenchmarkHandlerChain(b *testing.B) { | |||
}) | |||
|
|||
// Make sure to update this if the activator's main file changes. | |||
ah := New(ctx, fakeThrottler{}, rt, false, logger) | |||
ah := New(ctx, fakeThrottler{}, rt, false, logger, false /* 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.
what is the overhead if tls is true
, does it make sense to have run a benchmark with that too?
/hold for @evankanderson |
Afaik this is also protected from accidental misuse in this phase so let's get it in. |
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 have a slight concern that this PR will break namespaces which don't have the correct certificate and when the ConfigMap is set (mandatory volume mount in the new Deployment of a secret which doesn't exist, and activator only requesting https for those containers), but if the current e2e tests pass, I'm okay with this being a pre-alpha release.
CC @dprotaso just in case this ends up causing problems for the release in 2 business days.
cmd/activator/main.go
Outdated
// Enable TLS client when queue-proxy-ca is specified. | ||
// At this moment activator with TLS does not disable HTTP. | ||
// See also https://github.com/knative/serving/issues/12808. | ||
if networkConfig.QueueProxyCA != "" && networkConfig.QueueProxySAN != "" { |
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.
This requires a restart of activator to pick up the new network config.
For pre-alpha, this seems okay, but we should aim to make this configurable without a restart by beta.
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.
Note: if the secret name wasn't configurable, you could use a volume mount to achieve this addition to the system cert pool.
pool = x509.NewCertPool() | ||
} | ||
|
||
if ok := pool.AppendCertsFromPEM(caSecret.Data["ca.crt"]); !ok { |
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.
If the secret exists but doesn't have the right data, this will fail.
Do we need to undo the base64 encoding here before AppendCertsFromPEM
? (I don't recall what the client-go libraries assist with here.)
cmd/activator/main.go
Outdated
@@ -186,9 +213,12 @@ func main() { | |||
concurrencyReporter := activatorhandler.NewConcurrencyReporter(ctx, env.PodName, statCh) | |||
go concurrencyReporter.Run(ctx.Done()) | |||
|
|||
// Enable TLS against queue-proxy when the CA and SA are specified. | |||
tlsEnabled := networkConfig.QueueProxyCA != "" && networkConfig.QueueProxySAN != "" |
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.
This is repeating 160, it looks like. Can we set this there?
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.
(or make it part of the transport
)?
cmd/queue/main.go
Outdated
mainTLSServer, drain := buildServer(ctx, env, probe, stats, logger, concurrencyendpoint, true /* enable TLS */) | ||
tlsServers := map[string]*http.Server{ | ||
"tlsMain": mainTLSServer, | ||
"tlsAdmin": buildAdminServer(logger, drain), |
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.
It feels like moving the admin server to TLS is spread all over this function. What about moving this before the loop on 190 and explicitly moving the server from one map to the other. (It would add another tlsEnabled
condition to the function, but that feels simpler than the other logic.)
i.e. build a tlsServer
and httpServer
map, and then iterate through one or both after building the maps?
@@ -122,6 +139,11 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error) | |||
extraVolumes = append(extraVolumes, varTokenVolume) | |||
} | |||
|
|||
if cfg.Network.QueueProxyCertSecret != "" { |
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 this means changing the QueueProxtCertSecret
will cause all Knative Serving user deployments to get updated and rolled out, correct?
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.
Yes't that's correct.
9a229a1
to
1bf67e5
Compare
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.
/hold cancel
Still have a few pre-alpha and post -1.4 concerns, but I think this should be safe to go in tomorrow.
// See also https://github.com/knative/serving/issues/12808. | ||
var tlsServers map[string]*http.Server | ||
if tlsEnabled { | ||
mainTLSServer, drain := buildServer(ctx, env, probe, stats, logger, concurrencyendpoint, true /* enable 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.
It looks like the drain
from line 176 is lost?
Again, okay for pre-alpha, but it would be nice to unify the drains post 1.4
|
||
target := net.JoinHostPort("127.0.0.1", env.UserPort) | ||
|
||
httpProxy := pkghttp.NewHeaderPruningReverseProxy(target, pkghttp.NoHostOverride, activator.RevisionHeaders) | ||
httpProxy := pkghttp.NewHeaderPruningReverseProxy(target, pkghttp.NoHostOverride, activator.RevisionHeaders, false /* use HTTP */) | ||
httpProxy.Transport = buildTransport(env, logger) | ||
httpProxy.ErrorHandler = pkghandler.Error(logger) | ||
httpProxy.BufferPool = network.NewBufferPool() | ||
httpProxy.FlushInterval = network.FlushInterval | ||
|
||
breaker := buildBreaker(logger, env) |
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.
Also, we won't be counting concurrency properly across HTTP and HTTPS while the activator change rolls out (but again, a post-1.4 note, just worth adding a TODO here).
Thank you! Updated. |
/lgtm |
/retest
|
This patch fixes the following issues:
Fix #12502
Fix #12503
TODO:
NewTLSBackoffDialer
to pkg repo - AddNewProxyAutoTLSTransport
andDialTLSWithBackOff
to support TLS proxy pkg#2479