-
Notifications
You must be signed in to change notification settings - Fork 596
[Feat] Translate BackendConfigPolicy TLS certs for AgentGateway #12633
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?
[Feat] Translate BackendConfigPolicy TLS certs for AgentGateway #12633
Conversation
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
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 translating BackendConfigPolicy in the agentgateway plugin by merging it with existing BackendTLSPolicy functionality. This allows both policies to be processed together into a single AgentGateway BackendTLS policy that handles both TLS origination and validation.
- Adds
BackendConfigPolicytranslation and processing in the agentgateway plugin - Implements merging logic for TLS and Config policies targeting the same backend
- Adds
WithSectionNamesupport toBackendConfigPolicyto matchBackendTLSPolicybehavior
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/agentgateway/plugins/collection.go |
Adds BackendConfigPolicy collection registration and synchronization |
pkg/agentgateway/plugins/backend_tls_plugin.go |
Implements BackendConfigPolicy translation and merging with BackendTLSPolicy |
pkg/agentgateway/plugins/backend_tls_plugin_test.go |
Comprehensive test coverage for policy translation and merging |
api/v1alpha1/backend_config_policy_types.go |
Updates BackendConfigPolicy to use target refs with section names |
install/helm/kgateway-crds/templates/gateway.kgateway.dev_backendconfigpolicies.yaml |
Adds sectionName field to CRD schema |
api/v1alpha1/zz_generated.deepcopy.go |
Updates generated code for new target reference types |
api/applyconfiguration/api/v1alpha1/backendconfigpolicyspec.go |
Updates apply configuration for new types |
internal/kgateway/extensions2/plugins/backendconfigpolicy/plugin.go |
Updates to use target refs with section names |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if bcfg.Spec.TLS.Files != nil { | ||
| // For file-based TLS, we need to read the files from the filesystem | ||
| if bcfg.Spec.TLS.Files.TLSCertificate != nil && *bcfg.Spec.TLS.Files.TLSCertificate != "" { | ||
| certData, err := os.ReadFile(*bcfg.Spec.TLS.Files.TLSCertificate) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reading TLS certificate file %s: %w", *bcfg.Spec.TLS.Files.TLSCertificate, err) | ||
| } | ||
| cert = wrapperspb.Bytes(certData) | ||
| } | ||
|
|
||
| if bcfg.Spec.TLS.Files.TLSKey != nil && *bcfg.Spec.TLS.Files.TLSKey != "" { | ||
| keyData, err := os.ReadFile(*bcfg.Spec.TLS.Files.TLSKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error reading TLS key file %s: %w", *bcfg.Spec.TLS.Files.TLSKey, err) | ||
| } | ||
| key = wrapperspb.Bytes(keyData) | ||
| } | ||
| } |
Copilot
AI
Oct 15, 2025
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.
Reading arbitrary files from the filesystem using os.ReadFile() without path validation poses a security risk. Consider implementing path validation or restricting file access to specific directories to prevent directory traversal attacks.
Signed-off-by: Fabian Gonzalez <[email protected]>
|
The code gen check fails, although I ran |
I think running make generated-codeshould cover at least some of the diff here so not sure why that's not working, but I think make go-generate-apisshould work here as well |
Signed-off-by: Fabian Gonzalez <[email protected]>
…anslation Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Signed-off-by: Fabian Gonzalez <[email protected]>
Question: When unsupported fields are set, do we still want to translate the valid portions? Or not? (PR does the former). If the former, would we want to mark the status as Accepted? Or not accepted? (PR does latter - confusing, but if marked as accepted, it may be easy to miss the message)
Resolved merge conflict + stats updates & ran locally. It worked, but I noticed the following behavior:
When I delete a BackendConfigPolicy resource, I get the following error logs. They are not breaking, as far as I could tell, but could be confusing. I'm not sure if a similar behavior occurs with other resources we run through the status syncer.
Description
This adds partial translation support for
BackendConfigPolicyin the agentgateway plugin.WithSectionNametoBackendConfigPolicyto fall in line with BackendTLSPolicy logic.BackendConfigPolicytranslation as its own plugin.insecure: trueto disable mtls. from local testing, when it was not set, i gotinvalid peer certificate: UnknownIssuer.Note: partial support because the
BackendConfigPolicyhas additional configuration whose translation isn't set here. We are only translating the TLS portion for now.Examples
fully valid
invalid (bad targetRef)
partially valid - single bad targetRef
partially valid - using parameter we currently do not translate
Limitations
TargetSelectors
TargetSelectorsupport. Currently, this is only translatingTargetRefs, this way there is a 1:1 match withBackendTLSPolicyselection. This allows us to easily merge the selections as-needed.todo: look if it'd be easy to set this up now that tls & config aren't being merged
Non-TLS Cert Fields
These changes are solely focused on adding support for TLS client certs through the agentgateway proxy. There are still a number of other fields part of the
BackendConfigPolicythat need translated for full BCP support.There is a status message for usage of unsupported fields.
Change Type
/kind new_feature
Changelog
Additional Notes
demo-bcp.mp4