-
Notifications
You must be signed in to change notification settings - Fork 138
Add support for backend tls config for Gateways #3900
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3900 +/- ##
==========================================
- Coverage 86.85% 86.80% -0.06%
==========================================
Files 128 128
Lines 16503 16569 +66
Branches 62 62
==========================================
+ Hits 14334 14382 +48
- Misses 1992 2009 +17
- Partials 177 178 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6e0570b
to
81d973e
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.
Pull Request Overview
This PR adds support for backend TLS configuration on Gateways to enable secure communication between the gateway and backend pods using client certificates. The implementation includes validation logic, configuration building, and nginx template updates to support TLS handshake with client certificate authentication.
- Adds experimental backend TLS support via
Gateway.Spec.BackendTLS.ClientCertificateRef
- Implements validation and reference grant checking for cross-namespace secret references
- Updates NGINX configuration to include gateway client certificates in the proxy configuration
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
internal/controller/state/graph/gateway.go | Core gateway validation logic for backend TLS configuration |
internal/controller/state/graph/gateway_test.go | Comprehensive test coverage for backend TLS validation scenarios |
internal/controller/state/dataplane/configuration.go | SSL key pair building and base HTTP config updates |
internal/controller/nginx/config/base_http_config_template.go | NGINX template for gateway client certificates |
internal/controller/state/conditions/conditions.go | New condition types for secret reference validation |
internal/controller/state/graph/graph.go | Graph building updates to pass experimental features flag |
Comments suppressed due to low confidence (1)
internal/controller/state/dataplane/configuration.go:1
- Inconsistent formatting with extra comma and closing parenthesis on separate lines. Should follow the existing pattern of having the closing parenthesis on the same line as the last parameter.
package dataplane
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
func NewGatewaySecretRefNotPermitted(msg string) Condition { | ||
return Condition{ | ||
Type: string(GatewayReasonResolvedRefs), | ||
Status: metav1.ConditionFalse, | ||
Reason: string(GatewayReasonSecretRefNotPermitted), | ||
Message: msg, | ||
} | ||
} | ||
|
||
// NewGatewaySecretRefInvalid returns Condition that indicates that the Gateway references a TLS secret that is invalid. | ||
func NewGatewaySecretRefInvalid(msg string) Condition { | ||
return Condition{ | ||
Type: string(GatewayReasonResolvedRefs), | ||
Status: metav1.ConditionFalse, | ||
Reason: string(GatewayReasonSecretRefInvalid), | ||
Message: msg, | ||
} | ||
} | ||
|
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.
currently working on the bug related to marking HTTPRoute invalid leading to issues and I am wondering if this is still valid? Should we mark the gateway invalid? its not syntactically wrong for it to be marked false. We just won't reference the secret in the our configuration so I would like to change this to Accepted: true
but i'd like to get second opinions.
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.
The Gateway API community may be able to clarify for sure, but I'm fairly confident this should be Accepted: false
. If a user wants a secure connection, we definitely shouldn't ignore that if the Secret is invalid. That would defeat the purpose of a secure connection if we just default to plaintext and insecure if their certs aren't able to be used.
|
||
// GatewayReasonSecretRefNotPermitted is used with the "GatewayResolvedRefs" condition when the | ||
// secretRef resource is not permitted by any ReferenceGrant. | ||
GatewayReasonSecretRefNotPermitted v1.GatewayConditionReason = "SecretRefNotPermitted" |
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 these available or will these be available to import from the gateway api?
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.
No, these weren't available on gateway api only reason related to parametersRef so didn't want to use those to avoid confusion
name: "gatewayclass belongs to a different controller", | ||
store: createStateWithGatewayClass(differentControllerGC), | ||
expected: &Graph{}, | ||
experimental: 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.
Why set this to true here? Is it really changing anything?
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.
Did this for code coverage. Does not change anything but that's also a path to check
assertBuildConfiguration(g, result, test.expConf) | ||
}) | ||
} | ||
} |
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.
Thanks for cleaning this up a bit. But holy cow, nearly 5500 lines. This is getting unwieldy.
af9ad2e
to
8168ac4
Compare
name: "gatewayclass belongs to a different controller", | ||
store: createStateWithGatewayClass(differentControllerGC), | ||
expected: &Graph{}, | ||
experimentalEnabled: 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.
It would be nice if we could actually verify that this does something, instead of purely trying to get code coverage
Proposed changes
Write a clear and concise description that helps reviewers understand the purpose and impact of your changes. Use the
following format:
Problem: Users want to be able to specify their gateway's identity when communicating to the backend pods.
Solution: Add a field to provide a secret name that stores that gateways cert and key to be used when doing TLS handshake with backend pods
NOTE: I refactored this test because it was not easy to debug in such large tests as part of this PR.
Ran unit tests multiple times to avoid data race situations.
Testing: Unit tests added as needed , No IPv6 testing done for this (not related with ports or in container network)
Manual tests
Tested with Securing backend traffic by additionally requiring client certificates from backend and ensuring they are signed by the right CN to verify identity
my secure-app config
NGF config
Curl and logs to verify client -- I signed CA with gateway CN so that's what we should see get logged in the backend
Please focus on (optional): If you any specific areas where you would like reviewers to focus their attention or provide
specific feedback, add them here.
Closes #3153
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.