Review feedback

Author: ciarams87

cmd/gateway/commands.go

@@ -56,7 +56,7 @@ func createStaticModeCommand() *cobra.Command {
 		leaderElectionDisableFlag  = "leader-election-disable"
 		leaderElectionLockNameFlag = "leader-election-lock-name"
 		plusFlag                   = "nginx-plus"
-		experimentalEnableFlag     = "experimental-features-enable"
+		gwAPIExperimentalFlag      = "gateway-api-experimental-features"
 	)
 
 	// flag values
@@ -97,7 +97,7 @@ func createStaticModeCommand() *cobra.Command {
 
 		plus bool
 
-		enableExperimental bool
+		gwExperimentalFeatures bool
 	)
 
 	cmd := &cobra.Command{
@@ -175,7 +175,7 @@ func createStaticModeCommand() *cobra.Command {
 				Plus:                  plus,
 				TelemetryReportPeriod: period,
 				Version:               version,
-				ExperimentalFeatures:  enableExperimental,
+				ExperimentalFeatures:  gwExperimentalFeatures,
 			}
 
 			if err := static.StartManager(conf); err != nil {
@@ -290,8 +290,8 @@ func createStaticModeCommand() *cobra.Command {
 	)
 
 	cmd.Flags().BoolVar(
-		&enableExperimental,
-		experimentalEnableFlag,
+		&gwExperimentalFeatures,
+		gwAPIExperimentalFlag,
 		false,
 		"Enable the experimental features of Gateway API which are supported by NGINX Gateway Fabric. "+
 			"Requires the Gateway APIs installed from the experimental channel.",

deploy/helm-chart/templates/deployment.yaml

@@ -52,8 +52,8 @@ spec:
         {{- else }}
         - --leader-election-disable
         {{- end }}
-        {{- if .Values.nginxGateway.experimentalFeatures.enable }}
-        - --experimental-features-enable
+        {{- if .Values.nginxGateway.gwAPIExperimentalFeatures.enable }}
+        - --gateway-api-experimental-features
         {{- end }}
         env:
         - name: POD_IP

deploy/helm-chart/values.yaml

@@ -51,7 +51,7 @@ nginxGateway:
   ## extraVolumeMounts are the additional volume mounts for the nginx-gateway container.
   extraVolumeMounts: []
 
-  experimentalFeatures:
+  gwAPIExperimentalFeatures:
     ## Enable the experimental features of Gateway API which are supported by NGINX Gateway Fabric. Requires the Gateway
     ## APIs installed from the experimental channel.
     enable: false

docs/developer/quickstart.md

@@ -128,7 +128,7 @@ This will build the docker images `nginx-gateway-fabric:<your-user>` and `nginx-
    kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/standard-install.yaml
    ```
 
-   Alternatively, install Gateway API CRDs from the experimental channel:
+   If you're implementing experimental Gateway API features, install Gateway API CRDs from the experimental channel:
 
    ```shell
    kubectl apply -f https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.0.0/experimental-install.yaml

internal/framework/status/setters_test.go

@@ -865,6 +865,12 @@ func TestBtpStatusEqual(t *testing.T) {
 			},
 		}
 	}
+	prevMultiple := getPolicyStatus("ancestor1", "ns1", "ctlr1")
+	prevMultiple.Ancestors = append(prevMultiple.Ancestors, getPolicyStatus("ancestor2", "ns2", "ctlr2").Ancestors...)
+
+	currMultiple := getPolicyStatus("ancestor1", "ns1", "ctlr1")
+	currMultiple.Ancestors = append(currMultiple.Ancestors, getPolicyStatus("ancestor3", "ns3", "ctlr2").Ancestors...)
+
 	tests := []struct {
 		name           string
 		controllerName string
@@ -907,6 +913,13 @@ func TestBtpStatusEqual(t *testing.T) {
 			controllerName: "ctlr1",
 			expEqual:       false,
 		},
+		{
+			name:           "status not equal, different controller ancestor changed",
+			previous:       prevMultiple,
+			current:        currMultiple,
+			controllerName: "ctlr1",
+			expEqual:       false,
+		},
 	}
 
 	for _, test := range tests {

internal/framework/status/statuses.go

@@ -116,6 +116,7 @@ type GatewayClassStatus struct {
 	ObservedGeneration int64
 }
 
+// AncestorStatus holds status-related information related to how the BackendTLSPolicy binds to a specific ancestorRef.
 type AncestorStatus struct {
 	// GatewayNsName is the Namespaced name of the Gateway, which the ancestorRef references.
 	GatewayNsName types.NamespacedName

internal/mode/static/build_statuses.go

@@ -199,16 +199,7 @@ func buildBackendTLSPolicyStatuses(backendTLSPolicies map[types.NamespacedName]*
 
 	for nsname, backendTLSPolicy := range backendTLSPolicies {
 		if backendTLSPolicy.IsReferenced {
-			ignoreStatus := false
-			if !backendTLSPolicy.Valid {
-				for i := range backendTLSPolicy.Conditions {
-					if backendTLSPolicy.Conditions[i].Reason == string(staticConds.BackendTLSPolicyReasonIgnored) {
-						// We should not report the status of an ignored BackendTLSPolicy.
-						ignoreStatus = true
-					}
-				}
-			}
-			if !ignoreStatus {
+			if !backendTLSPolicy.Ignored {
 				statuses[nsname] = status.BackendTLSPolicyStatus{
 					AncestorStatuses: []status.AncestorStatus{
 						{

internal/mode/static/build_statuses_test.go

@@ -619,6 +619,7 @@ func TestBuildBackendTLSPolicyStatuses(t *testing.T) {
 	getBackendTLSPolicy := func(
 		name string,
 		valid bool,
+		ignored bool,
 		isReferenced bool,
 		conditions []conditions.Condition,
 	) *graph.BackendTLSPolicy {
@@ -631,15 +632,15 @@ func TestBuildBackendTLSPolicyStatuses(t *testing.T) {
 				},
 			},
 			Valid:        valid,
+			Ignored:      ignored,
 			IsReferenced: isReferenced,
 			Conditions:   conditions,
 			Gateway:      types.NamespacedName{Name: "gateway", Namespace: "test"},
 		}
 	}
 
-	attachedConds := []conditions.Condition{staticConds.NewBackendTLSPolicyAttached()}
+	attachedConds := []conditions.Condition{staticConds.NewBackendTLSPolicyAccepted()}
 	invalidConds := []conditions.Condition{staticConds.NewBackendTLSPolicyInvalid("invalid backendTLSPolicy")}
-	ignoredConds := []conditions.Condition{staticConds.NewBackendTLSPolicyIgnored("ignored backendTLSPolicy")}
 
 	tests := []struct {
 		backendTLSPolicies map[types.NamespacedName]*graph.BackendTLSPolicy
@@ -653,7 +654,7 @@ func TestBuildBackendTLSPolicyStatuses(t *testing.T) {
 		{
 			name: "valid backendTLSPolicy",
 			backendTLSPolicies: map[types.NamespacedName]*graph.BackendTLSPolicy{
-				{Namespace: "test", Name: "valid-bt"}: getBackendTLSPolicy("valid-bt", true, true, attachedConds),
+				{Namespace: "test", Name: "valid-bt"}: getBackendTLSPolicy("valid-bt", true, false, true, attachedConds),
 			},
 			expected: status.BackendTLSPolicyStatuses{
 				{Namespace: "test", Name: "valid-bt"}: {
@@ -669,7 +670,7 @@ func TestBuildBackendTLSPolicyStatuses(t *testing.T) {
 		{
 			name: "invalid backendTLSPolicy",
 			backendTLSPolicies: map[types.NamespacedName]*graph.BackendTLSPolicy{
-				{Namespace: "test", Name: "invalid-bt"}: getBackendTLSPolicy("invalid-bt", false, true, invalidConds),
+				{Namespace: "test", Name: "invalid-bt"}: getBackendTLSPolicy("invalid-bt", false, false, true, invalidConds),
 			},
 			expected: status.BackendTLSPolicyStatuses{
 				{Namespace: "test", Name: "invalid-bt"}: {
@@ -685,16 +686,16 @@ func TestBuildBackendTLSPolicyStatuses(t *testing.T) {
 		{
 			name: "ignored or not referenced backendTLSPolicies",
 			backendTLSPolicies: map[types.NamespacedName]*graph.BackendTLSPolicy{
-				{Namespace: "test", Name: "ignored-bt"}:     getBackendTLSPolicy("ignored-bt", false, true, ignoredConds),
-				{Namespace: "test", Name: "not-referenced"}: getBackendTLSPolicy("not-referenced", true, false, nil),
+				{Namespace: "test", Name: "ignored-bt"}:     getBackendTLSPolicy("ignored-bt", false, true, true, nil),
+				{Namespace: "test", Name: "not-referenced"}: getBackendTLSPolicy("not-referenced", true, false, false, nil),
 			},
 			expected: status.BackendTLSPolicyStatuses{},
 		},
 		{
 			name: "mix valid and ignored backendTLSPolicies",
 			backendTLSPolicies: map[types.NamespacedName]*graph.BackendTLSPolicy{
-				{Namespace: "test", Name: "ignored-bt"}: getBackendTLSPolicy("ignored-bt", false, true, ignoredConds),
-				{Namespace: "test", Name: "valid-bt"}:   getBackendTLSPolicy("valid-bt", true, true, attachedConds),
+				{Namespace: "test", Name: "ignored-bt"}: getBackendTLSPolicy("ignored-bt", false, true, true, nil),
+				{Namespace: "test", Name: "valid-bt"}:   getBackendTLSPolicy("valid-bt", true, false, true, attachedConds),
 			},
 			expected: status.BackendTLSPolicyStatuses{
 				{Namespace: "test", Name: "valid-bt"}: {

internal/mode/static/manager.go

@@ -388,9 +388,13 @@ func registerControllers(
 		backendTLSObjs := []ctlrCfg{
 			{
 				objectType: &gatewayv1alpha2.BackendTLSPolicy{},
+				options: []controller.Option{
+					controller.WithK8sPredicate(k8spredicate.GenerationChangedPredicate{}),
+				},
 			},
 			{
 				// FIXME(ciarams87): If possible, use only metadata predicate
+				// https://github.com/nginxinc/nginx-gateway-fabric/issues/1545
 				objectType: &apiv1.ConfigMap{},
 			},
 		}

internal/mode/static/nginx/config/generator.go

@@ -1,7 +1,6 @@
 package config
 
 import (
-	"encoding/base64"
 	"path/filepath"
 
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
@@ -96,15 +95,10 @@ func generatePEMFileName(id dataplane.SSLKeyPairID) string {
 }
 
 func generateCertBundle(id dataplane.CertBundleID, cert []byte) file.File {
-	data := make([]byte, base64.StdEncoding.DecodedLen(len(cert)))
-	_, err := base64.StdEncoding.Decode(data, cert)
-	if err != nil {
-		data = cert
-	}
 	return file.File{
-		Content: data,
+		Content: cert,
 		Path:    generateCertBundleFileName(id),
-		Type:    file.TypeSecret,
+		Type:    file.TypeRegular,
 	}
 }
 

internal/mode/static/nginx/config/servers.go

@@ -283,7 +283,7 @@ func createProxyTLSFromBackends(backends []dataplane.Backend) *http.ProxySSLVeri
 }
 
 func createProxySSLVerify(v *dataplane.VerifyTLS) *http.ProxySSLVerify {
-	if v == nil || v.Hostname == "" {
+	if v == nil {
 		return nil
 	}
 	var trustedCert string

internal/mode/static/nginx/config/servers_test.go

@@ -1869,10 +1869,7 @@ func TestConvertBackendTLSFromGroup(t *testing.T) {
 					UpstreamName: "my-upstream",
 					Valid:        true,
 					Weight:       1,
-					VerifyTLS: &dataplane.VerifyTLS{
-						CertBundleID: "",
-						Hostname:     "",
-					},
+					VerifyTLS:    nil,
 				},
 			},
 			expected: nil,
@@ -1900,6 +1897,24 @@ func TestConvertBackendTLSFromGroup(t *testing.T) {
 				Name:               "my-hostname",
 			},
 		},
+		{
+			msg: "tls enabled, system certs enabled",
+			grp: []dataplane.Backend{
+				{
+					UpstreamName: "my-upstream",
+					Valid:        true,
+					Weight:       1,
+					VerifyTLS: &dataplane.VerifyTLS{
+						Hostname:   "my-hostname",
+						RootCAPath: "/etc/ssl/certs/ca-certificates.crt",
+					},
+				},
+			},
+			expected: &http.ProxySSLVerify{
+				TrustedCertificate: "/etc/ssl/certs/ca-certificates.crt",
+				Name:               "my-hostname",
+			},
+		},
 	}
 
 	for _, tc := range tests {

internal/mode/static/state/conditions/conditions.go

@@ -5,6 +5,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	v1 "sigs.k8s.io/gateway-api/apis/v1"
+	"sigs.k8s.io/gateway-api/apis/v1alpha2"
 
 	ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
@@ -73,9 +74,6 @@ const (
 	// BackendTLSPolicyReasonAttached is the condition reason for the BackendTLSPolicy Attached condition.
 	BackendTLSPolicyReasonAttached BackendTLSPolicyConditionReason = "BackendTLSPolicyAttached"
 
-	// BackendTLSPolicyReasonIgnored is the condition reason for the BackendTLSPolicy being ignored by this Gateway.
-	BackendTLSPolicyReasonIgnored BackendTLSPolicyConditionReason = "BackendTLSPolicyIgnored"
-
 	// BackendTLSPolicyConditionValid is the condition type indicating whether the BackendTLS policy is valid.
 	BackendTLSPolicyConditionValid BackendTLSPolicyConditionType = "Valid"
 
@@ -570,34 +568,23 @@ func NewNginxGatewayInvalid(msg string) conditions.Condition {
 	}
 }
 
-// NewBackendTLSPolicyAttached returns a Condition that indicates that the BackendTLSPolicy config is valid and attached
-// to the Gateway.
-func NewBackendTLSPolicyAttached() conditions.Condition {
+// NewBackendTLSPolicyAccepted returns a Condition that indicates that the BackendTLSPolicy config is valid and accepted
+// by the Gateway.
+func NewBackendTLSPolicyAccepted() conditions.Condition {
 	return conditions.Condition{
-		Type:    string(BackendTLSPolicyConditionAttached),
+		Type:    string(v1alpha2.PolicyConditionAccepted),
 		Status:  metav1.ConditionTrue,
-		Reason:  string(BackendTLSPolicyReasonAttached),
-		Message: "BackendTLSPolicy is attached to the Gateway",
-	}
-}
-
-// NewBackendTLSPolicyIgnored returns a Condition that indicates that the BackendTLSPolicy config cannot be attached to
-// the Gateway and will be ignored.
-func NewBackendTLSPolicyIgnored(msg string) conditions.Condition {
-	return conditions.Condition{
-		Type:    string(BackendTLSPolicyConditionAttached),
-		Status:  metav1.ConditionFalse,
-		Reason:  string(BackendTLSPolicyReasonIgnored),
-		Message: msg,
+		Reason:  string(v1alpha2.PolicyReasonAccepted),
+		Message: "BackendTLSPolicy is accepted by the Gateway",
 	}
 }
 
 // NewBackendTLSPolicyInvalid returns a Condition that indicates that the BackendTLSPolicy config is invalid.
 func NewBackendTLSPolicyInvalid(msg string) conditions.Condition {
 	return conditions.Condition{
-		Type:    string(BackendTLSPolicyConditionValid),
+		Type:    string(v1alpha2.PolicyConditionAccepted),
 		Status:  metav1.ConditionFalse,
-		Reason:  string(BackendTLSPolicyReasonInvalid),
+		Reason:  string(v1alpha2.PolicyReasonInvalid),
 		Message: msg,
 	}
 }