-
Notifications
You must be signed in to change notification settings - Fork 460
feat: support infra deployment in the gateway namespace #5137
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
feat: support infra deployment in the gateway namespace #5137
Conversation
7bf3489
to
91fadc0
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5137 +/- ##
==========================================
- Coverage 65.41% 65.15% -0.26%
==========================================
Files 222 224 +2
Lines 35643 35857 +214
==========================================
+ Hits 23315 23364 +49
- Misses 10882 11048 +166
+ Partials 1446 1445 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
291e372
to
3d3932b
Compare
3d3932b
to
9cd1e1b
Compare
9cd1e1b
to
9ee9d48
Compare
ab8e446
to
7e27132
Compare
Hi @cnvergence Just checking in - any updates on this PR? Would love to see it land in 1.4.0. |
hey @zhaohuabing, getting back to this after Kubecon, I need to fix connection between control-plane and proxies, if everything will work again, I think it should be ready for 1.4.0 :) |
@cnvergence a quick way to solve this issue is to copy the This is not the most optimal solution, since all Gateway infras share the same client cert. As the next step, we can introduce logic in Envoy Gateway to automatically generate a unique cert secret per Gateway. This can be handled in a follow-up PR. Would love your input on this, @envoyproxy/gateway-maintainers |
-1 to this, app teams have will have access to this, and can take advantage |
@zhaohuabing
While we can introduce more options as the followup to this, we wanted to do it initially with the option 3. |
To move this forward, I agree that we can start without client cert and use the JWT token for client-side validation for the namespaced mode, but please keep mTLS for the current mode. We eventually will need mTLS for stronger security posture and compliance reasons. We may end up introducing a shim to exchange the JWT token for a client cert, and then establish the mTLS XDS connection for Envoy. This solution is similar to the approach taken by Istio's pilot agent. If @arkodg also feels this is the right approach, we can go ahead. |
b48eec8
to
b73753e
Compare
Yes, mTLS by default was always planned, the JWT method would only be used for the gatewayNamespacedMode. |
31850ce
to
d46e101
Compare
49fc667
to
f6e88f8
Compare
/retest |
} | ||
|
||
func validateKubeJWT(ctx context.Context, clientset *kubernetes.Clientset, token string) (bool, error) { | ||
tokenReview := &authenticationv1.TokenReview{ |
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.
are we validating that these token belongs to an envoy created by EG ?
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.
Just by the service account, I was thinking about it, we will need to have an exact name of the envoy pod
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.
Can we leverage the IR keys in the snapshot cache which map to the fleet deployment name and namespace?
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.
Tracked in this issue: #5863
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 added the basic check for deployment in snapshot cache, we will need to improve it and make it work with merged gateways mode
079ecae
to
38ec262
Compare
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
79afa42
to
91c297b
Compare
Signed-off-by: Karol Szwaj <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Thanks for the review, I will tackle open issues as the followups |
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.
LGTM. Thanks for the patience!
What type of PR is this?
feat: support infra deployment in the gateway namespace
What this PR does / why we need it:
Enable deploying Infra envoy proxies in the namespace of related Gateway resources.
GatewayNamespaceMode
deploy mode in the gateway config map.JWT token are validated using k8s TokenReview API .
Which issue(s) this PR fixes:
Fixes #2629
Release Notes: Yes