Admission warning (#9975)

* Add warning feature in admission code

* Apply suggestions from code review

Co-authored-by: Josh Soref <[email protected]>

* Add deprecation and validation path notice

---------

Co-authored-by: Josh Soref <[email protected]>

Author: rikatz

internal/admission/controller/main.go

@@ -33,6 +33,7 @@ import (
 // contains invalid instructions
 type Checker interface {
 	CheckIngress(ing *networking.Ingress) error
+	CheckWarning(ing *networking.Ingress) ([]string, error)
 }
 
 // IngressAdmission implements the AdmissionController interface
@@ -85,6 +86,15 @@ func (ia *IngressAdmission) HandleAdmission(obj runtime.Object) (runtime.Object,
 		return review, nil
 	}
 
+	// Adds the warnings regardless of operation being allowed or not
+	warning, err := ia.Checker.CheckWarning(&ingress)
+	if err != nil {
+		klog.ErrorS(err, "failed to get ingress warnings")
+	}
+	if len(warning) > 0 {
+		status.Warnings = warning
+	}
+
 	if err := ia.Checker.CheckIngress(&ingress); err != nil {
 		klog.ErrorS(err, "invalid ingress configuration", "ingress", fmt.Sprintf("%v/%v", review.Request.Namespace, review.Request.Name))
 		status.Allowed = false

internal/admission/controller/main_test.go

@@ -38,6 +38,11 @@ func (ftc failTestChecker) CheckIngress(ing *networking.Ingress) error {
 	return nil
 }
 
+func (ftc failTestChecker) CheckWarning(ing *networking.Ingress) ([]string, error) {
+	ftc.t.Error("checker should not be called")
+	return nil, nil
+}
+
 type testChecker struct {
 	t   *testing.T
 	err error
@@ -50,6 +55,13 @@ func (tc testChecker) CheckIngress(ing *networking.Ingress) error {
 	return tc.err
 }
 
+func (tc testChecker) CheckWarning(ing *networking.Ingress) ([]string, error) {
+	if ing.ObjectMeta.Name != testIngressName {
+		tc.t.Errorf("CheckWarning should be called with %v ingress, but got %v", testIngressName, ing.ObjectMeta.Name)
+	}
+	return nil, tc.err
+}
+
 func TestHandleAdmission(t *testing.T) {
 	adm := &IngressAdmission{
 		Checker: failTestChecker{t: t},

internal/ingress/controller/controller.go

@@ -256,6 +256,54 @@ func (n *NGINXController) syncIngress(interface{}) error {
 	return nil
 }
 
+// GetWarnings returns a list of warnings an Ingress gets when being created.
+// The warnings are going to be used in an admission webhook, and they represent
+// a list of messages that users need to be aware (like deprecation notices)
+// when creating a new ingress object
+func (n *NGINXController) CheckWarning(ing *networking.Ingress) ([]string, error) {
+	warnings := make([]string, 0)
+
+	var deprecatedAnnotations = sets.NewString()
+	deprecatedAnnotations.Insert(
+		"enable-influxdb",
+		"influxdb-measurement",
+		"influxdb-port",
+		"influxdb-host",
+		"influxdb-server-name",
+		"secure-verify-ca-secret",
+		"fastcgi-params-configmap",
+		"fastcgi-index",
+	)
+
+	// Skip checks if the ingress is marked as deleted
+	if !ing.DeletionTimestamp.IsZero() {
+		return warnings, nil
+	}
+
+	anns := ing.GetAnnotations()
+	for k := range anns {
+		trimmedkey := strings.TrimPrefix(k, parser.AnnotationsPrefix+"/")
+		if deprecatedAnnotations.Has(trimmedkey) {
+			warnings = append(warnings, fmt.Sprintf("annotation %s is deprecated", k))
+		}
+	}
+
+	// Add each validation as a single warning
+	// rikatz: I know this is somehow a duplicated code from CheckIngress, but my goal was to deliver fast warning on this behavior. We
+	// can and should, tho, simplify this in the near future
+	if err := inspector.ValidatePathType(ing); err != nil {
+		if errs, is := err.(interface{ Unwrap() []error }); is {
+			for _, errW := range errs.Unwrap() {
+				warnings = append(warnings, errW.Error())
+			}
+		} else {
+			warnings = append(warnings, err.Error())
+		}
+	}
+
+	return warnings, nil
+}
+
 // CheckIngress returns an error in case the provided ingress, when added
 // to the current configuration, generates an invalid configuration
 func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {

internal/ingress/controller/controller_test.go

@@ -352,6 +352,113 @@ func TestCheckIngress(t *testing.T) {
 	})
 }
 
+func TestCheckWarning(t *testing.T) {
+
+	// Ensure no panic with wrong arguments
+	var nginx = &NGINXController{}
+
+	nginx.t = fakeTemplate{}
+	nginx.store = fakeIngressStore{
+		ingresses: []*ingress.Ingress{},
+	}
+
+	ing := &networking.Ingress{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "test-ingress-warning",
+			Namespace:   "user-namespace",
+			Annotations: map[string]string{},
+		},
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					Host: "example.com",
+				},
+			},
+		},
+	}
+	t.Run("when a deprecated annotation is used a warning should be returned", func(t *testing.T) {
+		ing.ObjectMeta.Annotations[parser.GetAnnotationWithPrefix("enable-influxdb")] = "true"
+		defer func() {
+			ing.ObjectMeta.Annotations = map[string]string{}
+		}()
+
+		warnings, err := nginx.CheckWarning(ing)
+		if err != nil {
+			t.Errorf("no error should be returned, but %s was returned", err)
+		}
+		if len(warnings) != 1 {
+			t.Errorf("expected 1 warning to occur but %d occured", len(warnings))
+		} else {
+			t.Logf("got warning %s correctly", warnings[0])
+		}
+	})
+
+	t.Run("When an invalid pathType is used, a warning should be returned", func(t *testing.T) {
+
+		rules := ing.Spec.DeepCopy().Rules
+		ing.Spec.Rules = []networking.IngressRule{
+			{
+				Host: "example.com",
+				IngressRuleValue: networking.IngressRuleValue{
+					HTTP: &networking.HTTPIngressRuleValue{
+						Paths: []networking.HTTPIngressPath{
+							{
+								Path:     "/xpto{$2}",
+								PathType: &pathTypePrefix,
+							},
+							{
+								Path:     "/ok",
+								PathType: &pathTypeExact,
+							},
+						},
+					},
+				},
+			},
+		}
+		defer func() {
+			ing.Spec.Rules = rules
+		}()
+
+		warnings, err := nginx.CheckWarning(ing)
+		if err != nil {
+			t.Errorf("no error should be returned, but %s was returned", err)
+		}
+		if len(warnings) != 1 {
+			t.Errorf("expected 1 warning to occur but %d occured", len(warnings))
+		} else {
+			t.Logf("got warnings %v correctly", warnings)
+		}
+
+		t.Run("adding invalid annotations increases the warning count", func(t *testing.T) {
+			ing.ObjectMeta.Annotations[parser.GetAnnotationWithPrefix("enable-influxdb")] = "true"
+			ing.ObjectMeta.Annotations[parser.GetAnnotationWithPrefix("secure-verify-ca-secret")] = "true"
+			ing.ObjectMeta.Annotations[parser.GetAnnotationWithPrefix("fastcgi-index")] = "blabla"
+			defer func() {
+				ing.ObjectMeta.Annotations = map[string]string{}
+			}()
+			warnings, err := nginx.CheckWarning(ing)
+			if err != nil {
+				t.Errorf("no error should be returned, but %s was returned", err)
+			}
+			if len(warnings) != 4 {
+				t.Errorf("expected 4 warning to occur but %d occured", len(warnings))
+			} else {
+				t.Logf("got warnings %v correctly", warnings)
+			}
+		})
+	})
+
+	t.Run("When the ingress is marked as deleted", func(t *testing.T) {
+		ing.DeletionTimestamp = &metav1.Time{
+			Time: time.Now(),
+		}
+
+		if warnings, err := nginx.CheckWarning(ing); err != nil || len(warnings) != 0 {
+			t.Errorf("when the ingress is marked as deleted, no warning should be returned")
+		}
+	})
+}
+
 func TestMergeAlternativeBackends(t *testing.T) {
 	testCases := map[string]struct {
 		ingress      *ingress.Ingress