Apply -enable-snippets cli arg to Ingresses (#2124)

pleshakov · soneillf5 · web-flow · commit f4709eb057ff · 2021-10-28T11:46:36.000+01:00
* Apply -enable-snippets cli arg to Ingresses

* Update docs

* add snippet flag python tests

* removing snippets check as we rely on validation

* removing used param

Co-authored-by: Sean O'Neill &lt;s.oneill@f5.com&gt;
diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go
@@ -154,7 +154,7 @@ var (
 		"Enable preview policies")
 
 	enableSnippets = flag.Bool("enable-snippets", false,
-		"Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources.")
+		"Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources.")
 
 	globalConfiguration = flag.String("global-configuration", "",
 		`The namespace/name of the GlobalConfiguration resource for global configuration of the Ingress Controller. Requires -enable-custom-resources. Format: <namespace>/<name>`)
@@ -638,6 +638,7 @@ func main() {
 		IsPrometheusEnabled:          *enablePrometheusMetrics,
 		IsLatencyMetricsEnabled:      *enableLatencyMetrics,
 		IsTLSPassthroughEnabled:      *enableTLSPassthrough,
+		SnippetsEnabled:              *enableSnippets,
 	}
 
 	lbc := k8s.NewLoadBalancerController(lbcInput)
diff --git a/deployments/helm-chart/README.md b/deployments/helm-chart/README.md
@@ -180,7 +180,7 @@ Parameter | Description | Default
 `controller.enableTLSPassthrough` | Enable TLS Passthrough on port 443. Requires `controller.enableCustomResources`. | false
 `controller.globalConfiguration.create` | Creates the GlobalConfiguration custom resource. Requires `controller.enableCustomResources`. | false
 `controller.globalConfiguration.spec` | The spec of the GlobalConfiguration for defining the global configuration parameters of the Ingress Controller. | {}
-`controller.enableSnippets` | Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources. | false
+`controller.enableSnippets` | Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources. | false
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
 `controller.healthStatusURI` | Sets the URI of health status location in the default server. Requires `controller.healthStatus`. | "/nginx-health"
 `controller.nginxStatus.enable` | Enable the NGINX stub_status, or the NGINX Plus API. | true
diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml
@@ -155,7 +155,7 @@ controller:
       #   port: 5353
       #   protocol: TCP
 
-  ## Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources.
+  ## Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources.
   enableSnippets: false
 
   ## Add a location based on the value of health-status-uri to the default server. The location responds with the 200 status code for any request.
diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md
@@ -18,7 +18,7 @@ Below we describe the available command-line arguments:
 
 ### -enable-snippets
 
-Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources.
+Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources.
 
 Default `false`.  
 &nbsp;
diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-snippets.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-snippets.md
@@ -81,6 +81,7 @@ server {
 ## Summary of Snippets
 
 See the [snippets annotations](/nginx-ingress-controller/configuration/ingress-resources/advanced-configuration-with-annotations/#snippets-and-custom-templates) documentation for more information.
+However, because of the disadvantages described below, snippets are disabled by default. To use snippets, set the [`enable-snippets`](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments#cmdoption-enable-snippets) command-line argument.
 
 ## Disadvantages of Using Snippets
 
diff --git a/docs/content/configuration/security.md b/docs/content/configuration/security.md
@@ -41,6 +41,7 @@ We recommend the following for the most secure configuration:
    we recommend [configuring HTTPS](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments/#cmdoption-prometheus-tls-secret) for Prometheus.
 
 ### Snippets
-[Snippets](/nginx-ingress-controller/configuration/ingress-resources/advanced-configuration-with-snippets/)
-Snippets will be disabled by default in the future. 
-Be sure to understand the implications of configurations you provide through the Snippets capability.
+
+Snippets allow you to insert raw NGINX config into different contexts of NGINX configuration and are supported for [Ingress](/nginx-ingress-controller/configuration/ingress-resources/advanced-configuration-with-snippets/), [VirtualServer/VirtualServerRoute](/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/#using-snippets), and [TransportServer](/nginx-ingress-controller/configuration/transportserver-resource/#using-snippets) resources. Additionally, the [ConfigMap](/nginx-ingress-controller/configuration/global-configuration/configmap-resource#snippets-and-custom-templates) resource configures snippets globally.
+
+Snippets are disabled by default. To use snippets, set the [`enable-snippets`](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments#cmdoption-enable-snippets) command-line argument. Note that for the ConfigMap resource, snippets are always enabled.
diff --git a/docs/content/installation/installation-with-helm.md b/docs/content/installation/installation-with-helm.md
@@ -181,7 +181,7 @@ The following tables lists the configurable parameters of the NGINX Ingress cont
 |``controller.enableTLSPassthrough`` | Enable TLS Passthrough on port 443. Requires ``controller.enableCustomResources``. | false | 
 |``controller.globalConfiguration.create`` | Creates the GlobalConfiguration custom resource. Requires ``controller.enableCustomResources``. | false | 
 |``controller.globalConfiguration.spec`` | The spec of the GlobalConfiguration for defining the global configuration parameters of the Ingress Controller. | {} | 
-|``controller.enableSnippets`` | Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources. | false | 
+|``controller.enableSnippets`` | Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources. | false | 
 |``controller.healthStatus`` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false | 
 |``controller.healthStatusURI`` | Sets the URI of health status location in the default server. Requires ``controller.healthStatus``. | "/nginx-health" | 
 |``controller.nginxStatus.enable`` | Enable the NGINX stub_status, or the NGINX Plus API. | true | 
diff --git a/docs/content/third-party-modules/opentracing.md b/docs/content/third-party-modules/opentracing.md
@@ -13,6 +13,8 @@ The Ingress Controller supports [OpenTracing](https://opentracing.io/) with the
 
 This document explains how to use OpenTracing with the Ingress Controller.
 
+**Note**: The examples below use the snippets annotations, which are disabled by default. To use snippets, set the [`enable-snippets`](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments#cmdoption-enable-snippets) command-line argument.
+
 ## Prerequisites
 1. **Use the Ingress Controller image with OpenTracing.** You can find the images with NGINX or NGINX Plus with OpenTracing listed [here](/nginx-ingress-controller/technical-specifications/#supported-docker-images). Alternatively, you can follow the build instructions to build the image using `debian-image-opentracing` for NGINX or `debian-image-opentracing-plus` for NGINX Plus.
 [Jaeger](https://github.com/jaegertracing/jaeger-client-cpp), [Zipkin](https://github.com/rnburn/zipkin-cpp-opentracing) and [Datadog](https://github.com/DataDog/dd-opentracing-cpp/) tracers are installed by default.
diff --git a/internal/k8s/configuration.go b/internal/k8s/configuration.go
@@ -348,6 +348,7 @@ type Configuration struct {
 	appProtectEnabled       bool
 	internalRoutesEnabled   bool
 	isTLSPassthroughEnabled bool
+	snippetsEnabled         bool
 
 	lock sync.RWMutex
 }
@@ -362,6 +363,7 @@ func NewConfiguration(
 	globalConfigurationValidator *validation.GlobalConfigurationValidator,
 	transportServerValidator *validation.TransportServerValidator,
 	isTLSPassthroughEnabled bool,
+	snippetsEnabled bool,
 ) *Configuration {
 	return &Configuration{
 		hosts:                        make(map[string]Resource),
@@ -385,6 +387,7 @@ func NewConfiguration(
 		appProtectEnabled:            appProtectEnabled,
 		internalRoutesEnabled:        internalRoutesEnabled,
 		isTLSPassthroughEnabled:      isTLSPassthroughEnabled,
+		snippetsEnabled:              snippetsEnabled,
 	}
 }
 
@@ -399,7 +402,7 @@ func (c *Configuration) AddOrUpdateIngress(ing *networking.Ingress) ([]ResourceC
 	if !c.hasCorrectIngressClass(ing) {
 		delete(c.ingresses, key)
 	} else {
-		validationError = validateIngress(ing, c.isPlus, c.appProtectEnabled, c.internalRoutesEnabled).ToAggregate()
+		validationError = validateIngress(ing, c.isPlus, c.appProtectEnabled, c.internalRoutesEnabled, c.snippetsEnabled).ToAggregate()
 		if validationError != nil {
 			delete(c.ingresses, key)
 		} else {
diff --git a/internal/k8s/configuration_test.go b/internal/k8s/configuration_test.go
@@ -34,6 +34,7 @@ func createTestConfiguration() *Configuration {
 		}),
 		validation.NewTransportServerValidator(isTLSPassthroughEnabled, snippetsEnabled, isPlus),
 		isTLSPassthroughEnabled,
+		snippetsEnabled,
 	)
 }
 
diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go
@@ -185,6 +185,7 @@ type NewLoadBalancerControllerInput struct {
 	IsPrometheusEnabled          bool
 	IsLatencyMetricsEnabled      bool
 	IsTLSPassthroughEnabled      bool
+	SnippetsEnabled              bool
 }
 
 // NewLoadBalancerController creates a controller
@@ -307,7 +308,8 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc
 		input.VirtualServerValidator,
 		input.GlobalConfigurationValidator,
 		input.TransportServerValidator,
-		input.IsTLSPassthroughEnabled)
+		input.IsTLSPassthroughEnabled,
+		input.SnippetsEnabled)
 
 	lbc.appProtectConfiguration = appprotect.NewConfiguration()
 
diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go
@@ -68,6 +68,7 @@ type annotationValidationContext struct {
 	appProtectEnabled     bool
 	internalRoutesEnabled bool
 	fieldPath             *field.Path
+	snippetsEnabled       bool
 }
 
 type (
@@ -114,8 +115,12 @@ var (
 			validateRequiredAnnotation,
 			validateServerTokensAnnotation,
 		},
-		serverSnippetsAnnotation:   {},
-		locationSnippetsAnnotation: {},
+		serverSnippetsAnnotation: {
+			validateSnippetsAnnotation,
+		},
+		locationSnippetsAnnotation: {
+			validateSnippetsAnnotation,
+		},
 		proxyConnectTimeoutAnnotation: {
 			validateRequiredAnnotation,
 			validateTimeAnnotation,
@@ -273,6 +278,7 @@ func validateIngress(
 	isPlus bool,
 	appProtectEnabled bool,
 	internalRoutesEnabled bool,
+	snippetsEnabled bool,
 ) field.ErrorList {
 	allErrs := field.ErrorList{}
 	allErrs = append(allErrs, validateIngressAnnotations(
@@ -282,6 +288,7 @@ func validateIngress(
 		appProtectEnabled,
 		internalRoutesEnabled,
 		field.NewPath("annotations"),
+		snippetsEnabled,
 	)...)
 
 	allErrs = append(allErrs, validateIngressSpec(&ing.Spec, field.NewPath("spec"))...)
@@ -302,6 +309,7 @@ func validateIngressAnnotations(
 	appProtectEnabled bool,
 	internalRoutesEnabled bool,
 	fieldPath *field.Path,
+	snippetsEnabled bool,
 ) field.ErrorList {
 	allErrs := field.ErrorList{}
 
@@ -316,6 +324,7 @@ func validateIngressAnnotations(
 				appProtectEnabled:     appProtectEnabled,
 				internalRoutesEnabled: internalRoutesEnabled,
 				fieldPath:             fieldPath.Child(name),
+				snippetsEnabled:       snippetsEnabled,
 			}
 			allErrs = append(allErrs, validateIngressAnnotation(context)...)
 		}
@@ -524,6 +533,15 @@ func validateRewriteListAnnotation(context *annotationValidationContext) field.E
 	return allErrs
 }
 
+func validateSnippetsAnnotation(context *annotationValidationContext) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if !context.snippetsEnabled {
+		return append(allErrs, field.Forbidden(context.fieldPath, "snippet specified but snippets feature is not enabled"))
+	}
+	return allErrs
+}
+
 func validateIsBool(v string) error {
 	_, err := configs.ParseBool(v)
 	return err
diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go
@@ -14,12 +14,9 @@ import (
 
 func TestValidateIngress(t *testing.T) {
 	tests := []struct {
-		ing                   *networking.Ingress
-		isPlus                bool
-		appProtectEnabled     bool
-		internalRoutesEnabled bool
-		expectedErrors        []string
-		msg                   string
+		ing            *networking.Ingress
+		expectedErrors []string
+		msg            string
 	}{
 		{
 			ing: &networking.Ingress{
@@ -31,11 +28,8 @@ func TestValidateIngress(t *testing.T) {
 					},
 				},
 			},
-			isPlus:                false,
-			appProtectEnabled:     false,
-			internalRoutesEnabled: false,
-			expectedErrors:        nil,
-			msg:                   "valid input",
+			expectedErrors: nil,
+			msg:            "valid input",
 		},
 		{
 			ing: &networking.Ingress{
@@ -52,9 +46,6 @@ func TestValidateIngress(t *testing.T) {
 					},
 				},
 			},
-			isPlus:                false,
-			appProtectEnabled:     false,
-			internalRoutesEnabled: false,
 			expectedErrors: []string{
 				`annotations.nginx.org/mergeable-ingress-type: Invalid value: "invalid": must be one of: 'master' or 'minion'`,
 				"spec.rules[0].host: Required value",
@@ -85,9 +76,6 @@ func TestValidateIngress(t *testing.T) {
 					},
 				},
 			},
-			isPlus:                false,
-			appProtectEnabled:     false,
-			internalRoutesEnabled: false,
 			expectedErrors: []string{
 				"spec.rules[0].http.paths: Too many: 1: must have at most 0 items",
 			},
@@ -109,9 +97,6 @@ func TestValidateIngress(t *testing.T) {
 					},
 				},
 			},
-			isPlus:                false,
-			appProtectEnabled:     false,
-			internalRoutesEnabled: false,
 			expectedErrors: []string{
 				"spec.rules[0].http.paths: Required value: must include at least one path",
 			},
@@ -120,7 +105,7 @@ func TestValidateIngress(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		allErrs := validateIngress(test.ing, test.isPlus, test.appProtectEnabled, test.internalRoutesEnabled)
+		allErrs := validateIngress(test.ing, false, false, false, false)
 		assertion := assertErrors("validateIngress()", test.msg, allErrs, test.expectedErrors)
 		if assertion != "" {
 			t.Error(assertion)
@@ -135,6 +120,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 		isPlus                bool
 		appProtectEnabled     bool
 		internalRoutesEnabled bool
+		snippetsEnabled       bool
 		expectedErrors        []string
 		msg                   string
 	}{
@@ -508,6 +494,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 			isPlus:                false,
 			appProtectEnabled:     false,
 			internalRoutesEnabled: false,
+			snippetsEnabled:       true,
 			expectedErrors:        nil,
 			msg:                   "valid nginx.org/server-snippets annotation, single-value",
 		},
@@ -519,9 +506,24 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 			isPlus:                false,
 			appProtectEnabled:     false,
 			internalRoutesEnabled: false,
+			snippetsEnabled:       true,
 			expectedErrors:        nil,
 			msg:                   "valid nginx.org/server-snippets annotation, multi-value",
 		},
+		{
+			annotations: map[string]string{
+				"nginx.org/server-snippets": "snippet-1",
+			},
+			specServices:          map[string]bool{},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			internalRoutesEnabled: false,
+			snippetsEnabled:       false,
+			expectedErrors: []string{
+				`annotations.nginx.org/server-snippets: Forbidden: snippet specified but snippets feature is not enabled`,
+			},
+			msg: "invalid nginx.org/server-snippets annotation when snippets are disabled",
+		},
 
 		{
 			annotations: map[string]string{
@@ -531,6 +533,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 			isPlus:                false,
 			appProtectEnabled:     false,
 			internalRoutesEnabled: false,
+			snippetsEnabled:       true,
 			expectedErrors:        nil,
 			msg:                   "valid nginx.org/location-snippets annotation, single-value",
 		},
@@ -542,9 +545,24 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 			isPlus:                false,
 			appProtectEnabled:     false,
 			internalRoutesEnabled: false,
+			snippetsEnabled:       true,
 			expectedErrors:        nil,
 			msg:                   "valid nginx.org/location-snippets annotation, multi-value",
 		},
+		{
+			annotations: map[string]string{
+				"nginx.org/location-snippets": "snippet-1",
+			},
+			specServices:          map[string]bool{},
+			isPlus:                false,
+			appProtectEnabled:     false,
+			internalRoutesEnabled: false,
+			snippetsEnabled:       false,
+			expectedErrors: []string{
+				`annotations.nginx.org/location-snippets: Forbidden: snippet specified but snippets feature is not enabled`,
+			},
+			msg: "invalid nginx.org/location-snippets annotation when snippets are disabled",
+		},
 
 		{
 			annotations: map[string]string{
@@ -1662,6 +1680,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 				test.appProtectEnabled,
 				test.internalRoutesEnabled,
 				field.NewPath("annotations"),
+				test.snippetsEnabled,
 			)
 			assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors)
 			if assertion != "" {
diff --git a/tests/data/annotations/standard/annotations-ingress-snippets.yaml b/tests/data/annotations/standard/annotations-ingress-snippets.yaml
@@ -0,0 +1,20 @@
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  annotations:
+    kubernetes.io/ingress.class: "nginx"
+    nginx.org/server-snippets: "tcp_nodelay on;"
+  name: annotations-ingress
+spec:
+  rules:
+  - host: annotations.example.com
+    http:
+      paths:
+      - path: /backend2
+        pathType: Prefix
+        backend:
+          service:
+            name: backend2-svc
+            port:
+              number: 80
+
diff --git a/tests/suite/test_snippet_flag.py b/tests/suite/test_snippet_flag.py

Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,7 @@ func createTestConfiguration() *Configuration {`
`34`	`34`	`}),`
`35`	`35`	`validation.NewTransportServerValidator(isTLSPassthroughEnabled, snippetsEnabled, isPlus),`
`36`	`36`	`isTLSPassthroughEnabled,`
	`37`	`+ snippetsEnabled,`
`37`	`38`	`)`
`38`	`39`	`}`
`39`	`40`