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,
 	}
 }