fix: modify cloned ingress instead of original ingress (#1282)

Author: akshaysngupta

pkg/controller/prune.go

@@ -46,8 +46,12 @@ func pruneProhibitedIngress(c *AppGwIngressController, appGw *n.ApplicationGatew
 	// Mutate the list of Ingresses by removing ones that AGIC should not be creating configuration.
 	for idx, ingress := range ingressList {
 		klog.V(5).Infof("Original Ingress[%d] Rules: %+v", idx, ingress.Spec.Rules)
-		ingressList[idx].Spec.Rules = brownfield.PruneIngressRules(ingress, cbCtx.ProhibitedTargets)
-		klog.V(5).Infof("Sanitized Ingress[%d] Rules: %+v", idx, ingress.Spec.Rules)
+
+		ingressClone := ingressList[idx].DeepCopy()
+		ingressClone.Spec.Rules = brownfield.PruneIngressRules(ingress, cbCtx.ProhibitedTargets)
+		ingressList[idx] = ingressClone
+
+		klog.V(5).Infof("Sanitized Ingress[%d] Rules: %+v", idx, ingressList[idx].Spec.Rules)
 	}
 
 	return ingressList

pkg/controller/prune_test.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/annotations"
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/appgw"
+	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/environment"
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests"
 	"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests/fixtures"
 )
@@ -202,4 +203,62 @@ var _ = Describe("prune function tests", func() {
 			Expect(prunedIngresses).To(ContainElement(ingressValid2))
 		})
 	})
+
+	Context("ensure pruneProhibitedIngress prunes ingress", func() {
+		env := environment.GetFakeEnv()
+		env.EnableBrownfieldDeployment = true
+		ingress := fixtures.GetIngressWithProhibitedTargetConflict()
+		cbCtx := &appgw.ConfigBuilderContext{
+			IngressList: []*networking.Ingress{
+				ingress,
+			},
+			ServiceList: []*v1.Service{
+				tests.NewServiceFixture(),
+			},
+			ProhibitedTargets:     fixtures.GetAzureIngressProhibitedTargets(),
+			DefaultAddressPoolID:  to.StringPtr("xx"),
+			DefaultHTTPSettingsID: to.StringPtr("yy"),
+			EnvVariables:          env,
+		}
+		appGw := fixtures.GetAppGateway()
+
+		validateOldIngress := func(oldIngress *networking.Ingress) {
+			// should have two rules
+			Expect(len(oldIngress.Spec.Rules)).To(Equal(2))
+
+			// should have rule 1 with OldHost as host and no paths
+			Expect(oldIngress.Spec.Rules[0].Host).To(Equal(tests.OtherHost))
+			Expect(len(oldIngress.Spec.Rules[0].HTTP.Paths)).To(Equal(0))
+
+			// should have rule 2 with Host as host and 2 path rules: /foo /fox
+			Expect(oldIngress.Spec.Rules[1].Host).To(Equal(tests.Host))
+			Expect(len(oldIngress.Spec.Rules[1].HTTP.Paths)).To(Equal(2))
+			Expect(oldIngress.Spec.Rules[1].HTTP.Paths[0].Path).To(Equal(fixtures.PathFoo))
+			Expect(oldIngress.Spec.Rules[1].HTTP.Paths[1].Path).To(Equal(fixtures.PathFox))
+		}
+
+		It("removes the ingress rules without modifying the original ingress", func() {
+			Expect(len(cbCtx.IngressList)).To(Equal(1))
+
+			// Get pointer to the old ingress object
+			oldIngress := cbCtx.IngressList[0]
+
+			// Validate that ingress follows the requirement
+			validateOldIngress(oldIngress)
+
+			// Prune: test.OtherHost and /fox are prohibited
+			_ = pruneProhibitedIngress(controller, &appGw, cbCtx, cbCtx.IngressList)
+
+			// Validate old ingress is the same as before
+			validateOldIngress(oldIngress)
+
+			// Validate new ingress
+			newIngress := cbCtx.IngressList[0]
+
+			Expect(len(newIngress.Spec.Rules)).To(Equal(1), "should have only 1 rule after pruning")
+			Expect(len(newIngress.Spec.Rules[0].HTTP.Paths)).To(Equal(1), "Rule should have only 1 path rule left")
+			Expect(oldIngress.Spec.Rules[1].Host).To(Equal(tests.Host), "Host for that path should be tests.Host")
+			Expect(oldIngress.Spec.Rules[1].HTTP.Paths[0].Path).To(Equal(fixtures.PathFoo), "Path should /foo; /fox should be removed")
+		})
+	})
 })

pkg/tests/fixtures/ingress.go

@@ -60,3 +60,52 @@ func GetIngress() *networking.Ingress {
 		},
 	}
 }
+
+// GetIngressWithProhibitedTargetConflict returns ingress with /foo and /fox as paths
+func GetIngressWithProhibitedTargetConflict() *networking.Ingress {
+	return &networking.Ingress{
+		Spec: networking.IngressSpec{
+			Rules: []networking.IngressRule{
+				{
+					// Rule with no Paths
+					Host: tests.OtherHost,
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{},
+					},
+				},
+				{
+					// Rule with Paths
+					Host: tests.Host,
+					IngressRuleValue: networking.IngressRuleValue{
+						HTTP: &networking.HTTPIngressRuleValue{
+							Paths: []networking.HTTPIngressPath{
+								{
+									Path: PathFoo,
+									Backend: networking.IngressBackend{
+										Service: &networking.IngressServiceBackend{
+											Name: tests.ServiceName,
+											Port: networking.ServiceBackendPort{
+												Number: 80,
+											},
+										},
+									},
+								},
+								{
+									Path: PathFox,
+									Backend: networking.IngressBackend{
+										Service: &networking.IngressServiceBackend{
+											Name: tests.ServiceName,
+											Port: networking.ServiceBackendPort{
+												Number: 443,
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+}