Review feedback 1

Author: ciarams87

cmd/gateway/commands.go

@@ -172,10 +172,10 @@ func createStaticModeCommand() *cobra.Command {
 					LockName: leaderElectionLockName.String(),
 					Identity: podName,
 				},
-				Plus:                       plus,
-				TelemetryReportPeriod:      period,
-				Version:                    version,
-				EnableExperimentalFeatures: enableExperimental,
+				Plus:                  plus,
+				TelemetryReportPeriod: period,
+				Version:               version,
+				ExperimentalFeatures:  enableExperimental,
 			}
 
 			if err := static.StartManager(conf); err != nil {

examples/backend-tls/README.md

@@ -1,6 +1,8 @@
 # Backend TLS Policy Example
 
-In this example, we will create a Backend TLS Policy, attach it to our
+In this example, we will create a Backend TLS Policy, attach it to our application service, and then configure routing
+rules. The Backend TLS Policy will be picked up by NGF and the connection between NGF and the upstream server will use
+HTTPS.
 
 ## Running the Example
 
@@ -56,18 +58,9 @@ In this example, we will create a Backend TLS Policy, attach it to our
    kubectl apply -f policy.yaml
    ```
 
-## 3. Configure HTTPS Termination and Routing
+## 3. Configure HTTP Termination and Routing
 
-1. Create the Secret with a TLS certificate and key:
-
-   ```shell
-   kubectl apply -f app-secret.yaml
-   ```
-
-   The TLS certificate and key in this Secret are used to terminate the TLS connections for the secure-app application.
-   > **Important**: This certificate and key are for demo purposes only.
-
-2. Create the Gateway resource:
+1. Create the Gateway resource:
 
    ```shell
    kubectl apply -f gateway.yaml
@@ -77,19 +70,18 @@ In this example, we will create a Backend TLS Policy, attach it to our
     - `http` listener for HTTP traffic
     - `https` listener for HTTPS traffic. It terminates TLS connections using the `app-secret` we created in step 1.
 
-3. Create the HTTPRoute resources:
+2. Create the HTTPRoute resources:
 
    ```shell
    kubectl apply -f secure-app-routes.yaml
    ```
 
 ## 4. Test the Application
 
-To access the application, we will use `curl` to send requests to the `secure-app` Service over HTTPS. Since our
-certificate is self-signed, we will use curl's `--cacert` option to supply the certificate for verification.
+To access the application, we will use `curl` to send requests to the `secure-app` Service over HTTP.
 
 ```shell
-curl --resolve secure-app.example.com:$GW_HTTPS_PORT:$GW_IP https://secure-app.example.com:$GW_HTTPS_PORT/ --cacert ca.cert
+curl --resolve secure-app.example.com:$GW_PORT:$GW_IP http://secure-app.example.com:$GW_PORT/
 ```
 
 ```text

examples/backend-tls/app-secret.yaml

@@ -1,7 +1,7 @@
 kind: ConfigMap
 apiVersion: v1
 metadata:
-  name: backend-certs
+  name: backend-cert
 data:
   ca.crt: |
     -----BEGIN CERTIFICATE-----

examples/backend-tls/backend-certs-configmap.yaml

@@ -8,11 +8,3 @@ spec:
   - name: http
     port: 80
     protocol: HTTP
-  - name: https
-    port: 443
-    protocol: HTTPS
-    tls:
-      mode: Terminate
-      certificateRefs:
-      - kind: Secret
-        name: app-secret

examples/backend-tls/ca.cert

@@ -2,7 +2,6 @@ apiVersion: gateway.networking.k8s.io/v1alpha2
 kind: BackendTLSPolicy
 metadata:
   name: backend-tls
-  namespace: default
 spec:
   targetRef:
     group: ''
@@ -11,7 +10,7 @@ spec:
     namespace: default
   tls:
     caCertRefs:
-    - name: backend-certs
+    - name: backend-cert
       group: ''
       kind: ConfigMap
     hostname: secure-app.example.com

examples/backend-tls/gateway.yaml

@@ -1,28 +1,11 @@
 apiVersion: gateway.networking.k8s.io/v1
 kind: HTTPRoute
-metadata:
-  name: secure-app-tls-redirect
-spec:
-  parentRefs:
-  - name: gateway
-    sectionName: http
-  hostnames:
-  - "secure-app.example.com"
-  rules:
-  - filters:
-    - type: RequestRedirect
-      requestRedirect:
-        scheme: https
-        port: 443
----
-apiVersion: gateway.networking.k8s.io/v1
-kind: HTTPRoute
 metadata:
   name: secure-app
 spec:
   parentRefs:
   - name: gateway
-    sectionName: https
+    sectionName: http
   hostnames:
   - "secure-app.example.com"
   rules:

examples/backend-tls/policy.yaml

@@ -14,7 +14,7 @@ spec:
     spec:
       containers:
         - name: secure-app
-          image: nginxdemos/nginx-hello:plain-text
+          image: nginxdemos/nginx-unprivileged:plain-text
           ports:
             - containerPort: 8443
           volumeMounts:
@@ -59,8 +59,6 @@ data:
       ssl_certificate /etc/nginx/ssl/tls.crt;
       ssl_certificate_key /etc/nginx/ssl/tls.key;
 
-      proxy_ssl_server_name on;
-
       default_type text/plain;
 
       location / {

examples/backend-tls/secure-app-routes.yaml

@@ -38,8 +38,8 @@ type Config struct {
 	UpdateGatewayClassStatus bool
 	// Plus indicates whether NGINX Plus is being used.
 	Plus bool
-	// EnableExperimentalFeatures enables experimental features.
-	EnableExperimentalFeatures bool
+	// ExperimentalFeatures indicates if experimental features are enabled.
+	ExperimentalFeatures bool
 }
 
 // GatewayPodConfig contains information about this Pod.

examples/backend-tls/secure-app.yaml

@@ -203,7 +203,7 @@ func StartManager(cfg config.Config) error {
 	objects, objectLists := prepareFirstEventBatchPreparerArgs(
 		cfg.GatewayClassName,
 		cfg.GatewayNsName,
-		cfg.EnableExperimentalFeatures,
+		cfg.ExperimentalFeatures,
 	)
 	firstBatchPreparer := events.NewFirstEventBatchPreparerImpl(mgr.GetCache(), objects, objectLists)
 	eventLoop := events.NewEventLoop(
@@ -384,13 +384,13 @@ func registerControllers(
 		},
 	}
 
-	if cfg.EnableExperimentalFeatures {
+	if cfg.ExperimentalFeatures {
 		backendTLSObjs := []ctlrCfg{
 			{
 				objectType: &gatewayv1alpha2.BackendTLSPolicy{},
 			},
 			{
-				objectType: &apiv1.ConfigMap{},
+				objectType: &apiv1.ConfigMap{}, // TODO(ciarams87): Use only metadata predicate
 			},
 		}
 		controllerRegCfgs = append(controllerRegCfgs, backendTLSObjs...)

internal/mode/static/config/config.go

@@ -94,4 +94,6 @@ func TestGenerate(t *testing.T) {
 	g.Expect(configVersion).To(ContainSubstring(fmt.Sprintf("return 200 %d", conf.Version)))
 
 	g.Expect(files[3].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt"))
+	certBundle := string(files[3].Content)
+	g.Expect(certBundle).To(Equal("test-cert"))
 }

internal/mode/static/manager.go

@@ -90,7 +90,6 @@ type MapParameter struct {
 
 // ProxySSLVerify holds the proxied HTTPS server verification configuration.
 type ProxySSLVerify struct {
-	CertPath string
-	Hostname string
-	VerifyOn bool
+	TrustedCertificate string
+	Name               string
 }

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

@@ -3,6 +3,7 @@ package config
 import (
 	"encoding/json"
 	"fmt"
+	"os"
 	"strings"
 	gotemplate "text/template"
 
@@ -254,7 +255,7 @@ func updateLocationsForFilters(
 			}
 		}
 		buildLocations[i].ProxySetHeaders = proxySetHeaders
-		buildLocations[i].ProxySSLVerify = convertProxyTLSFromBackends(matchRule.BackendGroup.Backends)
+		buildLocations[i].ProxySSLVerify = createProxyTLSFromBackends(matchRule.BackendGroup.Backends)
 		proxyPass := createProxyPass(
 			matchRule.BackendGroup,
 			matchRule.Filters.RequestURLRewrite,
@@ -266,30 +267,54 @@ func updateLocationsForFilters(
 	return buildLocations
 }
 
-func convertProxyTLSFromBackends(backends []dataplane.Backend) *http.ProxySSLVerify {
+func createProxyTLSFromBackends(backends []dataplane.Backend) *http.ProxySSLVerify {
 	if len(backends) == 0 {
 		return nil
 	}
 	for _, b := range backends {
-		proxyVerify := convertBackendTLS(b.VerifyTLS)
+		proxyVerify := createProxySSLVerify(b.VerifyTLS)
 		if proxyVerify != nil {
 			// If any backend has a backend TLS policy defined, then we use that for the proxy SSL verification.
 			// If multiple backends in the group have a backend TLS policy defined, then we use the first one we find.
+			// TODO(ciarams87): Fix this
 			return proxyVerify
 		}
 	}
 	return nil
 }
 
-func convertBackendTLS(v *dataplane.VerifyTLS) *http.ProxySSLVerify {
+func createProxySSLVerify(v *dataplane.VerifyTLS) *http.ProxySSLVerify {
 	if v == nil || v.Hostname == "" {
 		return nil
 	}
+	var trustedCert string
+	if v.CertBundleID != "" {
+		trustedCert = generateCertBundleFileName(v.CertBundleID)
+	} else {
+		trustedCert = getRootCAPath()
+	}
 	return &http.ProxySSLVerify{
-		CertPath: generateCertBundleFileName(v.CertBundleID),
-		Hostname: v.Hostname,
-		VerifyOn: v.CertBundleID != "",
+		TrustedCertificate: trustedCert,
+		Name:               v.Hostname,
+	}
+}
+
+// TODO(ciarams87): Move this logic earlier
+func getRootCAPath() string {
+	certFiles := []string{
+		"/etc/ssl/cert.pem",                                 // Alpine Linux
+		"/etc/ssl/certs/ca-certificates.crt",                // Debian/Ubuntu/Gentoo etc.
+		"/etc/pki/tls/certs/ca-bundle.crt",                  // Fedora/RHEL 6
+		"/etc/ssl/ca-bundle.pem",                            // OpenSUSE
+		"/etc/pki/tls/cacert.pem",                           // OpenELEC
+		"/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem", // CentOS/RHEL 7
+	}
+	for _, certFile := range certFiles {
+		if _, err := os.Stat(certFile); err == nil {
+			return certFile
+		}
 	}
+	return ""
 }
 
 func createReturnValForRedirectFilter(filter *dataplane.HTTPRequestRedirectFilter, listenerPort int32) *http.Return {

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

@@ -53,14 +53,11 @@ server {
         proxy_http_version 1.1;
         proxy_pass {{ $l.ProxyPass }};
             {{- if $l.ProxySSLVerify }}
-        proxy_ssl_name {{ $l.ProxySSLVerify.Hostname }};
-                {{- if $l.ProxySSLVerify.VerifyOn }}
         proxy_ssl_verify on;
-        proxy_ssl_trusted_certificate {{ $l.ProxySSLVerify.CertPath }};
-                {{- end }}
+        proxy_ssl_name {{ $l.ProxySSLVerify.Hostname }};
+        proxy_ssl_trusted_certificate {{ $l.ProxySSLVerify.TrustedCertificate }};
             {{- end }}
         {{- end }}
-
     }
         {{ end }}
 }

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

@@ -621,19 +621,17 @@ func TestCreateServers(t *testing.T) {
 				ProxyPass:       "https://test_btp_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 				ProxySSLVerify: &http.ProxySSLVerify{
-					Hostname: "test-btp.example.com",
-					CertPath: "/etc/nginx/secrets/test-btp.crt",
-					VerifyOn: true,
+					Name:               "test-btp.example.com",
+					TrustedCertificate: "/etc/nginx/secrets/test-btp.crt",
 				},
 			},
 			{
 				Path:            "= /backend-tls-policy",
 				ProxyPass:       "https://test_btp_80$request_uri",
 				ProxySetHeaders: baseHeaders,
 				ProxySSLVerify: &http.ProxySSLVerify{
-					Hostname: "test-btp.example.com",
-					CertPath: "/etc/nginx/secrets/test-btp.crt",
-					VerifyOn: true,
+					Name:               "test-btp.example.com",
+					TrustedCertificate: "/etc/nginx/secrets/test-btp.crt",
 				},
 			},
 			{
@@ -1860,9 +1858,8 @@ func TestConvertBackendTLSFromGroup(t *testing.T) {
 				},
 			},
 			expected: &http.ProxySSLVerify{
-				CertPath: "/etc/nginx/secrets/default-my-cert.crt",
-				Hostname: "my-hostname",
-				VerifyOn: true,
+				TrustedCertificate: "/etc/nginx/secrets/default-my-cert.crt",
+				Name:               "my-hostname",
 			},
 		},
 		{
@@ -1899,16 +1896,15 @@ func TestConvertBackendTLSFromGroup(t *testing.T) {
 				},
 			},
 			expected: &http.ProxySSLVerify{
-				CertPath: "/etc/nginx/secrets/default-my-cert.crt",
-				Hostname: "my-hostname",
-				VerifyOn: true,
+				TrustedCertificate: "/etc/nginx/secrets/default-my-cert.crt",
+				Name:               "my-hostname",
 			},
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.msg, func(t *testing.T) {
-			result := convertProxyTLSFromBackends(tc.grp)
+			result := createProxyTLSFromBackends(tc.grp)
 			g.Expect(result).To(Equal(tc.expected))
 		})
 	}