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

* 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 <s.oneill@f5.com>

cmd/nginx-ingress/main.go

@@ -167,7 +167,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>`)
@@ -655,6 +655,7 @@ func main() {
 		IsPrometheusEnabled:          *enablePrometheusMetrics,
 		IsLatencyMetricsEnabled:      *enableLatencyMetrics,
 		IsTLSPassthroughEnabled:      *enableTLSPassthrough,
+		SnippetsEnabled:              *enableSnippets,
 	}
 
 	lbc := k8s.NewLoadBalancerController(lbcInput)

deployments/helm-chart/README.md

@@ -181,7 +181,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

deployments/helm-chart/values.yaml

@@ -168,7 +168,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.

docs-web/configuration/global-configuration/command-line-arguments.md

@@ -8,7 +8,7 @@ Below we describe the available command-line arguments:
 ```eval_rst
 .. option:: -enable-snippets
 
-	Enable custom NGINX configuration snippets in VirtualServer, VirtualServerRoute and TransportServer resources. (default false)
+	Enable custom NGINX configuration snippets in Ingress, VirtualServer, VirtualServerRoute and TransportServer resources. (default false)
 
 .. option:: -default-server-tls-secret <string>
 

docs-web/configuration/ingress-resources/advanced-configuration-with-snippets.md

@@ -68,6 +68,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
 

docs-web/installation/installation-with-helm.md

@@ -252,7 +252,7 @@ The following tables lists the configurable parameters of the NGINX Ingress cont
      - 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.
+     - 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.

docs-web/third-party-modules/opentracing.md

@@ -4,6 +4,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.** The default Ingress Controller images don’t include the OpenTracing module. To use OpenTracing, you need to build the image with that module. Follow the build instructions to build the image using `openshift-image` for NGINX or `openshift-image-plus` for NGINX Plus.
 By default, the Dockerfiles install Jaeger as a tracer. However, it is possible to replace Jaeger with other supported [tracers](https://github.com/opentracing-contrib/nginx-opentracing#building-from-source). For that, please modify the Dockerfile accordingly:

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 {

internal/k8s/configuration_test.go

@@ -35,6 +35,7 @@ func createTestConfiguration() *Configuration {
 		}),
 		validation.NewTransportServerValidator(isTLSPassthroughEnabled, snippetsEnabled, isPlus),
 		isTLSPassthroughEnabled,
+		snippetsEnabled,
 	)
 }
 

internal/k8s/controller.go

@@ -186,6 +186,7 @@ type NewLoadBalancerControllerInput struct {
 	IsPrometheusEnabled          bool
 	IsLatencyMetricsEnabled      bool
 	IsTLSPassthroughEnabled      bool
+	SnippetsEnabled              bool
 }
 
 // NewLoadBalancerController creates a controller
@@ -308,7 +309,8 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc
 		input.VirtualServerValidator,
 		input.GlobalConfigurationValidator,
 		input.TransportServerValidator,
-		input.IsTLSPassthroughEnabled)
+		input.IsTLSPassthroughEnabled,
+		input.SnippetsEnabled)
 
 	lbc.appProtectConfiguration = appprotect.NewConfiguration()
 

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

internal/k8s/validation_test.go

@@ -13,12 +13,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{
@@ -30,11 +27,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{
@@ -51,9 +45,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",
@@ -84,9 +75,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",
 			},
@@ -108,9 +96,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",
 			},
@@ -119,7 +104,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)
@@ -134,6 +119,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
 		isPlus                bool
 		appProtectEnabled     bool
 		internalRoutesEnabled bool
+		snippetsEnabled       bool
 		expectedErrors        []string
 		msg                   string
 	}{
@@ -507,6 +493,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",
 		},
@@ -518,9 +505,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{
@@ -530,6 +532,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",
 		},
@@ -541,9 +544,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{
@@ -1661,6 +1679,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 != "" {

tests/data/annotations/standard/annotations-ingress-snippets.yaml

@@ -0,0 +1,16 @@
+apiVersion: extensions/v1beta1
+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
+        backend:
+          serviceName: backend2-svc
+          servicePort: 80