Force activator always in path when activator CA is specified#12790
Conversation
dc4de02 to
1d5f022
Compare
As described in the feature doc of knative#11906, activator needs to serve TLS traffic for the traffic from ingress. So, this patch force SKS proxy mode when `activator-ca` in `config-network` is specified.
1d5f022 to
15845a5
Compare
Codecov Report
@@ Coverage Diff @@
## main #12790 +/- ##
==========================================
+ Coverage 87.51% 87.54% +0.02%
==========================================
Files 196 196
Lines 9782 9787 +5
==========================================
+ Hits 8561 8568 +7
+ Misses 934 932 -2
Partials 287 287
Continue to review full report at Codecov.
|
|
Do we need to force the activator to always be in the path? I thought the ingress => queue proxy would also be encrypted too |
|
Yes, it should be in the future. But for the initial Alpha release, we will force the activator to always be in the path.
|
|
I'm not sure if Dave is suggesting that we might want to extend the comment around line 135 in |
evankanderson
left a comment
There was a problem hiding this comment.
/lgtm
/hold
(for the typo and possibly other comment changes)
|
Thank you! I opened #12797 and extended the comment with the link. |
psschwei
left a comment
There was a problem hiding this comment.
Just thinking out loud here, but do we want to potentially include some kind of validation check for both this setting and a custom TBC value (since by keeping the activator in the path, we're more or less setting TBC = -1 and any other value would essentially get overridden)? It could be overkill, but might also be nice to catch any conflicts at the validation stage.
|
I'm sure the validation would be useful, but the "activator always in the path" solution is temporary state. So I am inclined to consider about the validation later if "activator always in the path" solution cannot be dropped soon. In addition, I am lack of knowledge about the auto scale... So I'm worried that I can include the validation without any side effect. |
|
@evankanderson @skonto @rhuss bump |
evankanderson
left a comment
There was a problem hiding this comment.
This seems safe on its own; the worst case is that you have less-good scaling behavior by setting a key in config-network, but you don't end up being broken altogether by it.
One concern about the structure of the code in kpa.go, but otherwise LGTM.
/approve
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
As described in the feature doc of #11906,
activator needs to serve TLS traffic for the traffic from ingress.
So, this patch force SKS proxy mode when
activator-cainconfig-networkis specified.Part of #11906