adding support for secrets for backendtlspolicy #3084

porthorian · 2025-02-03T00:39:47Z

Proposed changes

The proposed changes here adds the support for Secrets to be used according to the BackednTLSPolicy Custom Resource. This helps further the implementation of nginx-gateway-fabric to support the Gateway API more.

Problem

Currently BackendTLSPolicy only supports readying in tls certifications and ca certifications via a config map. This does not work when you are using cert-manager for instance since it puts this information into a kubernetes secret rather then a config map.

Solution

The Solution here is to hook into the existing Configuration structure that builds the dataplane nginx configuration that is served in the pod. Allowing for secrets and configmaps to be read into this array of CertBundles.

Testing

Did some end to end testing here and built 2 packages that can be used with this code.

https://github.com/porthorian/nginx-gateway-fabric/pkgs/container/nginx-gateway-fabric%2Fnginx
https://github.com/porthorian/nginx-gateway-fabric/pkgs/container/nginx-gateway-fabric

These were then deployed and made a BackendTLSPolicy with the secret that was created with cert-manager.

apiVersion: gateway.networking.k8s.io/v1alpha3
kind: BackendTLSPolicy
metadata:
  name: tls-upstream-unifi
spec:
  targetRefs:
    - kind: Service
      name: unifi
      group: ""
  validation:
    caCertificateRefs:
      - name: unifi-signed-cert
        group: ''
        kind: Secret
    hostname: {your-domain-name}

—

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: self-signed-svc-cert
spec:
  dnsNames:
    - {your-domain-name}
  secretName: unifi-signed-cert
  commonName: unifi
  issuerRef:
    name: self-signed-ca-issuer
    kind: ClusterIssuer
    group: cert-manager.io
  keystores:
    jks:
      alias: unifi
      create: true
      passwordSecretRef:
        name: unifi-keystore
        key: password

Reviewer Focus

Since this is a working draft here - places that should be focused on is making sure I am inline with the current standards and everyone is statisified with how this was structurally laid out before moving onto polishing it up and writing some additional tests.

Issues

Closes #2629

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

* Adds support for secrets to be used in BackendTLSPolicy Gateway API for tls certificates and ca certificates.

sjberman

Overall I think the approach looks good here! Haven't yet tested it myself, but will get around to that. Thanks for this.

sjberman · 2025-02-03T16:38:54Z

internal/mode/static/state/dataplane/configuration.go

+		CertBundles: buildCertBundles(
+			buildRefCertificateBundles(g.ReferencedSecrets, g.ReferencedCaCertConfigMaps),
+			backendGroups),


Suggested change

CertBundles: buildCertBundles(

buildRefCertificateBundles(g.ReferencedSecrets, g.ReferencedCaCertConfigMaps),

backendGroups),

CertBundles: buildCertBundles(

buildRefCertificateBundles(g.ReferencedSecrets, g.ReferencedCaCertConfigMaps),

backendGroups,

),

nit: style

sjberman · 2025-02-03T16:39:29Z

internal/mode/static/state/dataplane/configuration.go

+func buildRefCertificateBundles(
+	secrets map[types.NamespacedName]*graph.Secret,
+	configMaps map[types.NamespacedName]*graph.CaCertConfigMap) []graph.CertificateBundle {


Suggested change

func buildRefCertificateBundles(

secrets map[types.NamespacedName]*graph.Secret,

configMaps map[types.NamespacedName]*graph.CaCertConfigMap) []graph.CertificateBundle {

func buildRefCertificateBundles(

secrets map[types.NamespacedName]*graph.Secret,

configMaps map[types.NamespacedName]*graph.CaCertConfigMap,

) []graph.CertificateBundle {

nit: style

sjberman · 2025-02-03T16:43:07Z

internal/mode/static/state/graph/backend_tls_policy.go

-		path := field.NewPath("tls.cacertrefs[0]")
-		return field.Invalid(path, btp.Spec.Validation.CACertificateRefs[0], err.Error())
+
+	if selectedCertRef.Kind == "ConfigMap" {


Let's do a switch statement on the Kinds

sjberman · 2025-02-03T16:43:40Z

internal/mode/static/state/graph/backend_tls_policy.go

+			return field.Invalid(path, selectedCertRef, err.Error())
+		}
+	} else {
+		return fmt.Errorf("`%s` invalid certificate reference supported", selectedCertRef.Kind)


Suggested change

return fmt.Errorf("`%s` invalid certificate reference supported", selectedCertRef.Kind)

return fmt.Errorf("invalid certificate reference %q", selectedCertRef.Kind)

sjberman · 2025-02-03T16:50:38Z

internal/mode/static/state/graph/certificate_bundle.go

+	}
+	block, _ := pem.Decode(data)
+	if block == nil {
+		return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block", CAKey)


Suggested change

return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block", CAKey)

return fmt.Errorf("the data field %q must hold a valid CERTIFICATE PEM block", CAKey)

%q will wrap the string in quotes so it's easier to spot in the message.

(I know this was just copied here, but figured we might as well fix it)

sjberman · 2025-02-03T16:50:59Z

internal/mode/static/state/graph/certificate_bundle.go

+		return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block", CAKey)
+	}
+	if block.Type != "CERTIFICATE" {
+		return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block, but got '%s'", CAKey, block.Type)


Suggested change

return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block, but got '%s'", CAKey, block.Type)

return fmt.Errorf("the data field %q must hold a valid CERTIFICATE PEM block, but got %q", CAKey, block.Type)

porthorian added 2 commits February 2, 2025 19:22

add support for secrets for backendtlspolicy

1170634

merge main

e0c4365

porthorian requested a review from a team as a code owner February 3, 2025 00:39

porthorian mentioned this pull request Feb 3, 2025

adding support for secrets for backendtlspolicy #2976

Closed

6 tasks

sjberman reviewed Feb 3, 2025

View reviewed changes

sindhushiv added this to the v2.0.0 milestone Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding support for secrets for backendtlspolicy #3084

adding support for secrets for backendtlspolicy #3084

porthorian commented Feb 3, 2025 •

edited

Loading

sjberman left a comment

sjberman Feb 3, 2025

sjberman Feb 3, 2025

sjberman Feb 3, 2025

sjberman Feb 3, 2025

sjberman Feb 3, 2025

sjberman Feb 3, 2025

	return fmt.Errorf("`%s` invalid certificate reference supported", selectedCertRef.Kind)
	return fmt.Errorf("invalid certificate reference %q", selectedCertRef.Kind)

	return fmt.Errorf("the data field %s must hold a valid CERTIFICATE PEM block", CAKey)
	return fmt.Errorf("the data field %q must hold a valid CERTIFICATE PEM block", CAKey)

adding support for secrets for backendtlspolicy #3084

Are you sure you want to change the base?

adding support for secrets for backendtlspolicy #3084

Conversation

porthorian commented Feb 3, 2025 • edited Loading

Proposed changes

Problem

Solution

Testing

Reviewer Focus

Issues

Checklist

Release notes

sjberman left a comment

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

sjberman Feb 3, 2025

Choose a reason for hiding this comment

porthorian commented Feb 3, 2025 •

edited

Loading