CHERRY-PICK of #7665 - Remove snippet (#7666)

* Add option to force enabling snippet directives (#7665)

Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>

* Add missing key when cherry-picking

internal/ingress/controller/config/config.go

@@ -93,6 +93,10 @@ const (
 type Configuration struct {
 	defaults.Backend `json:",squash"`
 
+	// EnableSnippetDirectives enable users to add their own snippets via ingress annotation.
+	// If disabled, only snippets added via ConfigMap are added to ingress.
+	EnableSnippetDirectives bool `json:"enable-snippet-directives"`
+
 	// Sets the name of the configmap that contains the headers to pass to the client
 	AddHeaders string `json:"add-headers,omitempty"`
 
@@ -757,6 +761,7 @@ func NewDefault() Configuration {
 	defGlobalExternalAuth := GlobalExternalAuth{"", "", "", "", "", append(defResponseHeaders, ""), "", "", "", []string{}, map[string]string{}}
 
 	cfg := Configuration{
+		EnableSnippetDirectives:          true,
 		AllowBackendServerHeader:         false,
 		AccessLogPath:                    "/var/log/nginx/access.log",
 		AccessLogParams:                  "",

internal/ingress/controller/controller.go

@@ -235,27 +235,28 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
 		return fmt.Errorf("This deployment is trying to create a catch-all ingress while DisableCatchAll flag is set to true. Remove '.spec.backend' or set DisableCatchAll flag to false.")
 	}
 
-	if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
-		for key := range ing.ObjectMeta.GetAnnotations() {
+	cfg := n.store.GetBackendConfiguration()
+	cfg.Resolver = n.resolver
+
+	for key := range ing.ObjectMeta.GetAnnotations() {
+		if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
 			if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
 				return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix)
 			}
 		}
+
+		if !cfg.EnableSnippetDirectives && strings.HasSuffix(key, "-snippet") {
+			return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key)
+		}
+
+		if len(cfg.GlobalRateLimitMemcachedHost) == 0 && strings.HasPrefix(key, fmt.Sprintf("%s/%s", parser.AnnotationsPrefix, "global-rate-limit")) {
+			return fmt.Errorf("'global-rate-limit*' annotations require 'global-rate-limit-memcached-host' settings configured in the global configmap")
+		}
+
 	}
 
 	k8s.SetDefaultNGINXPathType(ing)
 
-	cfg := n.store.GetBackendConfiguration()
-	cfg.Resolver = n.resolver
-
-	if len(cfg.GlobalRateLimitMemcachedHost) == 0 {
-		for key := range ing.ObjectMeta.GetAnnotations() {
-			if strings.HasPrefix(key, fmt.Sprintf("%s/%s", parser.AnnotationsPrefix, "global-rate-limit")) {
-				return fmt.Errorf("'global-rate-limit*' annotations require 'global-rate-limit-memcached-host' settings configured in the global configmap")
-			}
-		}
-	}
-
 	allIngresses := n.store.ListIngresses()
 
 	filter := func(toCheck *ingress.Ingress) bool {
@@ -508,6 +509,30 @@ func (n *NGINXController) getConfiguration(ingresses []*ingress.Ingress) (sets.S
 	}
 }
 
+func dropSnippetDirectives(anns *annotations.Ingress, ingKey string) {
+	if anns != nil {
+		if anns.ConfigurationSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use configuration-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ConfigurationSnippet = ""
+		}
+		if anns.ServerSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use server-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ServerSnippet = ""
+		}
+
+		if anns.ModSecurity.Snippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use modsecurity-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ModSecurity.Snippet = ""
+		}
+
+		if anns.ExternalAuth.AuthSnippet != "" {
+			klog.V(3).Infof("Ingress %q tried to use auth-snippet and the annotation is disabled by the admin. Removing the annotation", ingKey)
+			anns.ExternalAuth.AuthSnippet = ""
+		}
+
+	}
+}
+
 // getBackendServers returns a list of Upstream and Server to be used by the
 // backend.  An upstream can be used in multiple servers if the namespace,
 // service name and port are the same.
@@ -522,6 +547,10 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		for _, rule := range ing.Spec.Rules {
 			host := rule.Host
 			if host == "" {
@@ -789,6 +818,11 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
 
 	for _, ing := range data {
 		anns := ing.ParsedAnnotations
+		ingKey := k8s.MetaNamespaceKey(ing)
+
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
 
 		var defBackend string
 		if ing.Spec.Backend != nil {
@@ -1069,6 +1103,10 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		// default upstream name
 		un := du.Name
 
@@ -1145,6 +1183,10 @@ func (n *NGINXController) createServers(data []*ingress.Ingress,
 		ingKey := k8s.MetaNamespaceKey(ing)
 		anns := ing.ParsedAnnotations
 
+		if !n.store.GetBackendConfiguration().EnableSnippetDirectives {
+			dropSnippetDirectives(anns, ingKey)
+		}
+
 		if anns.Canary.Enabled {
 			klog.V(2).Infof("Ingress %v is marked as Canary, ignoring", ingKey)
 			continue

internal/ingress/controller/controller_test.go

@@ -42,6 +42,7 @@ import (
 	"k8s.io/ingress-nginx/internal/ingress"
 	"k8s.io/ingress-nginx/internal/ingress/annotations"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/canary"
+	"k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/proxyssl"
 	"k8s.io/ingress-nginx/internal/ingress/annotations/sessionaffinity"
@@ -56,11 +57,12 @@ import (
 )
 
 type fakeIngressStore struct {
-	ingresses []*ingress.Ingress
+	ingresses     []*ingress.Ingress
+	configuration ngx_config.Configuration
 }
 
-func (fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
-	return ngx_config.Configuration{}
+func (fis fakeIngressStore) GetBackendConfiguration() ngx_config.Configuration {
+	return fis.configuration
 }
 
 func (fakeIngressStore) GetConfigMap(key string) (*corev1.ConfigMap, error) {
@@ -246,6 +248,9 @@ func TestCheckIngress(t *testing.T) {
 		})
 
 		t.Run("When the default annotation prefix is used despite an override", func(t *testing.T) {
+			defer func() {
+				parser.AnnotationsPrefix = "nginx.ingress.kubernetes.io"
+			}()
 			parser.AnnotationsPrefix = "ingress.kubernetes.io"
 			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/backend-protocol"] = "GRPC"
 			nginx.command = testNginxTestCommand{
@@ -257,6 +262,23 @@ func TestCheckIngress(t *testing.T) {
 			}
 		})
 
+		t.Run("When snippets are disabled and user tries to use snippet annotation", func(t *testing.T) {
+			nginx.store = fakeIngressStore{
+				ingresses: []*ingress.Ingress{},
+				configuration: ngx_config.Configuration{
+					EnableSnippetDirectives: false,
+				},
+			}
+			nginx.command = testNginxTestCommand{
+				t:   t,
+				err: nil,
+			}
+			ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/server-snippet"] = "bla"
+			if err := nginx.CheckIngress(ing); err == nil {
+				t.Errorf("with a snippet annotation, ingresses using the default should be rejected")
+			}
+		})
+
 		t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) {
 			backendBefore := ing.Spec.Backend
 			disableCatchAllBefore := nginx.cfg.DisableCatchAll
@@ -284,6 +306,9 @@ func TestCheckIngress(t *testing.T) {
 		})
 
 		t.Run("When the ingress is in a different namespace than the watched one", func(t *testing.T) {
+			defer func() {
+				nginx.cfg.Namespace = "test-namespace"
+			}()
 			nginx.command = testNginxTestCommand{
 				t:   t,
 				err: fmt.Errorf("test error"),
@@ -2075,6 +2100,83 @@ func TestGetBackendServers(t *testing.T) {
 				}
 			},
 		},
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "not-allowed-snippet",
+							Namespace: "default",
+							Annotations: map[string]string{
+								"nginx.ingress.kubernetes.io/server-snippet":         "bla",
+								"nginx.ingress.kubernetes.io/configuration-snippet":  "blo",
+								"nginx.ingress.kubernetes.io/whitelist-source-range": "10.0.0.0/24",
+							},
+						},
+						Spec: networking.IngressSpec{
+							Rules: []networking.IngressRule{
+								{
+									Host: "example.com",
+									IngressRuleValue: networking.IngressRuleValue{
+										HTTP: &networking.HTTPIngressRuleValue{
+											Paths: []networking.HTTPIngressPath{
+												{
+													Path:     "/path1",
+													PathType: &pathTypePrefix,
+													Backend: networking.IngressBackend{
+														ServiceName: "path1-svc",
+														ServicePort: intstr.IntOrString{
+															Type:   intstr.Int,
+															IntVal: 80,
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{
+						Whitelist:            ipwhitelist.SourceRange{CIDR: []string{"10.0.0.0/24"}},
+						ServerSnippet:        "bla",
+						ConfigurationSnippet: "blo",
+					},
+				},
+			},
+			Validate: func(ingresses []*ingress.Ingress, upstreams []*ingress.Backend, servers []*ingress.Server) {
+				if len(servers) != 2 {
+					t.Errorf("servers count should be 2, got %d", len(servers))
+					return
+				}
+				s := servers[1]
+
+				if s.ServerSnippet != "" {
+					t.Errorf("server snippet should be empty, got '%s'", s.ServerSnippet)
+				}
+
+				if s.Locations[0].ConfigurationSnippet != "" {
+					t.Errorf("config snippet should be empty, got '%s'", s.Locations[0].ConfigurationSnippet)
+				}
+
+				if len(s.Locations[0].Whitelist.CIDR) != 1 || s.Locations[0].Whitelist.CIDR[0] != "10.0.0.0/24" {
+					t.Errorf("allow list was incorrectly dropped, len should be 1 and contain 10.0.0.0/24")
+				}
+
+			},
+			SetConfigMap: func(ns string) *v1.ConfigMap {
+				return &v1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:     "config",
+						SelfLink: fmt.Sprintf("/api/v1/namespaces/%s/configmaps/config", ns),
+					},
+					Data: map[string]string{
+						"enable-snippet-directives": "false",
+					},
+				}
+			},
+		},
 	}
 
 	for _, testCase := range testCases {