Skip to content

fully support backend tls policy #11339

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

Merged
merged 11 commits into from
Jun 9, 2025
Merged

fully support backend tls policy #11339

merged 11 commits into from
Jun 9, 2025

Conversation

yuval-k
Copy link
Contributor

@yuval-k yuval-k commented Jun 5, 2025

Description

Finish the impl for backend tls policy

Change Type

/kind bug_fix

Changelog

NONE

No need for changelog as the original pr wasn't in a released version.

Additional Notes

still needs cleanup & tests

@github-actions github-actions bot added kind/fix Categorizes issue or PR as related to a bug. release-note-none labels Jun 5, 2025
yuval-k added 3 commits June 4, 2025 23:11
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
Signed-off-by: Yuval Kohavi <[email protected]>
@yuval-k yuval-k marked this pull request as ready for review June 5, 2025 12:55
@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 12:55
Copy link
Contributor

@Copilot Copilot AI left a 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 completes the implementation of the Backend TLS policy by wiring up CRD handling, translation logic, and tests.

  • Exposed BackendTLSPolicyGVR in the well-known API registry and included it in the translator’s test harness
  • Added translation code in the BackendTLSPolicy plugin to build Envoy TLS contexts (including both inline and SDS-backed CAs, plus SAN support)
  • Introduced new input/output fixtures (tls.yaml and tls-san.yaml) and expanded the gateway translator test suite

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/translator/test.go Include BackendTLSPolicyGVR in the CRD creation loop
internal/kgateway/wellknown/gwapi.go Define BackendTLSPolicyGVR for the new policy
internal/kgateway/translator/gateway/testutils/inputs/backendtlspolicy/*.yaml Add sample BackendTLSPolicy inputs (tls.yaml and tls-san.yaml)
internal/kgateway/translator/gateway/testutils/outputs/backendtlspolicy/*.yaml Add expected Envoy cluster/listener outputs for both scenarios
internal/kgateway/translator/gateway/gateway_translator_test.go Register translator entries for Backend TLS policy tests
internal/kgateway/extensions2/plugins/backendtlspolicy/ssl.go Update SSL helper to accept a validation context parameter
internal/kgateway/extensions2/plugins/backendtlspolicy/ssl_test.go Adjust tests for the updated ResolveUpstreamSslConfig signature
internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go Implement TLS policy translation (inline CA, SDS CA, SAN, error handling)
Comments suppressed due to low confidence (2)

internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go:187

  • The code uses fmt.Errorf but fmt is not imported, which will cause a compile error. Add import "fmt" to the top of the file.
perr := fmt.Errorf("%w: %v", ErrCreatingTLSConfig, err)

internal/kgateway/extensions2/plugins/backendtlspolicy/plugin.go:192

  • The default branch for invalid validation specs (ErrInvalidValidationSpec) isn't covered by any existing tests. Add a unit test to verify this error path when both WellKnownCACertificates and CACertificateRefs are absent or mutually exclusive.
return &policyIr, ErrInvalidValidationSpec

@puertomontt
Copy link
Contributor

is there a conformance test for this?

@yuval-k
Copy link
Contributor Author

yuval-k commented Jun 5, 2025

is there a conformance test for this?

good question. i just looked and i'm not seeing one

Copy link
Contributor

@lgadban lgadban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Approved with a very minor nit and some questions

hostname: example-san.com
- type: URI
uri: spifee://example-san.com

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: weird newline here and on line 85

Signed-off-by: Yuval Kohavi <[email protected]>
@yuval-k
Copy link
Contributor Author

yuval-k commented Jun 6, 2025

ok added e2e test. it passed locally

@lgadban lgadban added this pull request to the merge queue Jun 9, 2025
Merged via the queue into main with commit 31845ac Jun 9, 2025
20 checks passed
@lgadban lgadban deleted the yuval-k/backendtls branch June 9, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix Categorizes issue or PR as related to a bug. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants