fix: clean up path rules added by agic when using prohibited target (#…

…1610) (#1613)

* clean up path rules added by AGIC

* fix e2e

* remove crd install

* fix test

* update timelimit

helm/ingress-azure/crds/azureapplicationgatewayrewrite.yaml

@@ -3,7 +3,6 @@ kind: CustomResourceDefinition
 metadata:
   name: azureapplicationgatewayrewrites.appgw.ingress.azure.io
   annotations:
-    "helm.sh/hook": crd-install
     "api-approved.kubernetes.io": "https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1272"
 spec:
   group: appgw.ingress.azure.io

helm/ingress-azure/crds/azureingressprohibitedtarget.yaml

@@ -3,7 +3,6 @@ kind: CustomResourceDefinition
 metadata:
   name: azureingressprohibitedtargets.appgw.ingress.k8s.io
   annotations:
-    "helm.sh/hook": crd-install
     "api-approved.kubernetes.io": "https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1272"
 spec:
   group: appgw.ingress.k8s.io

helm/ingress-azure/templates/NOTES.txt

@@ -33,7 +33,7 @@ Configuration Details:
     - Verbosity level: {{ .Values.verbosityLevel }}
 {{- if .Values.appgw }}
 {{- if .Values.appgw.shared }}
-    - Multi-cluster / Shared App Gateway is enabled; Use "kubectl get AzureIngressProhibitedTargets" to view and modify config
+    - Shared App Gateway is enabled; Use "kubectl get AzureIngressProhibitedTargets" to view and modify config
 {{- end }}
 {{- end }}
 {{- if .Values.armAuth }}

helm/ingress-azure/templates/crds.yaml

@@ -1,9 +1,5 @@
 {{- if .Values.appgw -}}
 {{- if .Values.appgw.shared -}}
-{{- range $path, $bytes := .Files.Glob "crds/*.yaml" }}
-  {{ $.Files.Get $path }}
----
-{{- end }}
 {{- $watchNamespace := .Values.kubernetes.watchNamespace -}}
 {{- if not .Values.appgw.prohibitedTargets }}
 apiVersion: appgw.ingress.k8s.io/v1

pkg/appgw/cleanup.go

@@ -6,11 +6,29 @@
 package appgw
 
 import (
+	"fmt"
 	"strings"
 
 	n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-03-01/network"
 )
 
+// CleanUpPathRulesAddedByAGIC removes path rules that are created by AGIC
+func (c *appGwConfigBuilder) CleanUpPathRulesAddedByAGIC() {
+	pathRuleNamePrefix := fmt.Sprintf("%s%s-", agPrefix, prefixPathRule)
+
+	// Remove path rules that are created by AGIC
+	for _, pathMap := range *c.appGw.URLPathMaps {
+		var pathRulesAddedManually []n.ApplicationGatewayPathRule
+		for _, pathRule := range *pathMap.PathRules {
+			if !strings.HasPrefix(*pathRule.Name, pathRuleNamePrefix) {
+				pathRulesAddedManually = append(pathRulesAddedManually, pathRule)
+			}
+		}
+
+		pathMap.PathRules = &pathRulesAddedManually
+	}
+}
+
 // CleanUpUnusedDefaults removes the default backend and default http settings if they are not used by any ingress
 func (c *appGwConfigBuilder) CleanUpUnusedDefaults() {
 	if !c.isPoolUsed(DefaultBackendAddressPoolName) {

pkg/appgw/cleanup_test.go

@@ -0,0 +1,53 @@
+// -------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+// --------------------------------------------------------------------------------------------
+
+package appgw
+
+import (
+	n "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-03-01/network"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Cleanup", func() {
+	Context("CleanUpPathRules", func() {
+		var c *appGwConfigBuilder
+		agicAddedPathRule := generatePathRuleName("test", "test", 0, 0)
+		userAddedPathRule := "user-added-path-rule"
+
+		BeforeEach(func() {
+			c = &appGwConfigBuilder{
+				appGw: n.ApplicationGateway{
+					ApplicationGatewayPropertiesFormat: &n.ApplicationGatewayPropertiesFormat{
+						URLPathMaps: &[]n.ApplicationGatewayURLPathMap{
+							{
+								ApplicationGatewayURLPathMapPropertiesFormat: &n.ApplicationGatewayURLPathMapPropertiesFormat{
+									PathRules: &[]n.ApplicationGatewayPathRule{
+										{
+											Name: &agicAddedPathRule,
+										},
+										{
+											Name: &userAddedPathRule,
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			}
+		})
+
+		It("should remove path rules that are created by AGIC", func() {
+			c.CleanUpPathRulesAddedByAGIC()
+
+			Expect(*c.appGw.URLPathMaps).To(HaveLen(1))
+
+			pathRule := *(*c.appGw.URLPathMaps)[0].PathRules
+			Expect(pathRule).To(HaveLen(1))
+			Expect(*pathRule[0].Name).To(Equal(userAddedPathRule))
+		})
+	})
+})

pkg/appgw/requestroutingrules.go

@@ -30,6 +30,8 @@ func (c *appGwConfigBuilder) RequestRoutingRules(cbCtx *ConfigBuilderContext) er
 	requestRoutingRules, pathMaps := c.getRules(cbCtx)
 
 	if cbCtx.EnvVariables.EnableBrownfieldDeployment {
+		c.CleanUpPathRulesAddedByAGIC()
+
 		rCtx := brownfield.NewExistingResources(c.appGw, cbCtx.ProhibitedTargets, nil)
 		{
 			// PathMaps we obtained from App Gateway - we segment them into ones AGIC is and is not allowed to change.

scripts/e2e/cmd/runner/environment.go

@@ -23,6 +23,9 @@ const (
 	// AppGwNameVarName is the name of the applicationGatewayName
 	AppGwNameVarName = "applicationGatewayName"
 
+	// PublicIPAddressNameVarName is the name of the publicIPAddressName
+	PublicIPAddressNameVarName = "publicIPAddressName"
+
 	// KubeConfigVarName is the name of the KUBECONFIG
 	KubeConfigVarName = "KUBECONFIG"
 
@@ -41,6 +44,7 @@ type EnvVariables struct {
 	SubscriptionID        string
 	ResourceGroupName     string
 	AppGwName             string
+	PublicIPAddressName   string
 	SubResourceNamePrefix string
 	KubeConfigFilePath    string
 	ObjectID              string
@@ -53,6 +57,7 @@ func GetEnv() *EnvVariables {
 		SubscriptionID:        os.Getenv(SubscriptionIDVarName),
 		ResourceGroupName:     os.Getenv(ResourceGroupNameVarName),
 		AppGwName:             os.Getenv(AppGwNameVarName),
+		PublicIPAddressName:   os.Getenv(PublicIPAddressNameVarName),
 		SubResourceNamePrefix: os.Getenv(SubResourceNamePrefixVarName),
 		KubeConfigFilePath:    GetEnvironmentVariable(KubeConfigVarName, "~/.kube/config", nil),
 		ObjectID:              os.Getenv(ObjectIDVarName),

scripts/e2e/cmd/runner/helper.go

@@ -121,6 +121,35 @@ func getApplicationGatewaysClient() (*n.ApplicationGatewaysClient, error) {
 	return &client, nil
 }
 
+func getPublicIPAddressesClient() (*n.PublicIPAddressesClient, error) {
+	env := GetEnv()
+
+	settings, err := auth.GetSettingsFromEnvironment()
+	if err != nil {
+		return nil, err
+	}
+
+	client := n.NewPublicIPAddressesClientWithBaseURI(settings.Environment.ResourceManagerEndpoint, GetEnv().SubscriptionID)
+	var authorizer autorest.Authorizer
+	if env.AzureAuthLocation != "" {
+		// https://docs.microsoft.com/en-us/azure/developer/go/azure-sdk-authorization#use-file-based-authentication
+		authorizer, err = auth.NewAuthorizerFromFile(n.DefaultBaseURI)
+	} else {
+		authorizer, err = settings.GetAuthorizer()
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	client.Authorizer = authorizer
+	err = client.AddToUserAgent(UserAgent)
+	if err != nil {
+		return nil, err
+	}
+
+	return &client, nil
+}
+
 func getRoleAssignmentsClient() (*a.RoleAssignmentsClient, error) {
 	env := GetEnv()
 
@@ -842,6 +871,29 @@ func getGateway() (*n.ApplicationGateway, error) {
 	return &gateway, nil
 }
 
+func getPublicIPAddress() (*n.PublicIPAddress, error) {
+	env := GetEnv()
+
+	klog.Info("preparing public ip client")
+	client, err := getPublicIPAddressesClient()
+	if err != nil {
+		return nil, err
+	}
+
+	publicIP, err := client.Get(
+		context.TODO(),
+		env.ResourceGroupName,
+		env.PublicIPAddressName,
+		"",
+	)
+
+	if err != nil {
+		return nil, err
+	}
+
+	return &publicIP, nil
+}
+
 func supportsNetworkingV1IngressPackage(client clientset.Interface) bool {
 	version119, _ := version.ParseGeneric("v1.19.0")
 

scripts/e2e/cmd/runner/networking-v1-lfu_one_namespace_one_ingress_test.go

@@ -11,10 +11,8 @@ package runner
 import (
 	"context"
 	"fmt"
-	"strings"
 	"time"
 
-	"github.com/Azure/go-autorest/autorest/to"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
@@ -43,33 +41,8 @@ var _ = Describe("networking-v1-LFU", func() {
 			cleanUp(clientset)
 		})
 
-		It("[prohibited-target-test] prohibited service should be available to be accessed", func() {
-			// get ip address for 1 ingress
-			klog.Info("Getting public IP from blacklisted Ingress...")
-			publicIP, _ := getPublicIP(clientset, "test-brownfield-ns")
-			Expect(publicIP).ToNot(Equal(""))
-
-			//prohibited service will be kept by agic
-			url_blacklist := fmt.Sprintf("http://%s/blacklist", publicIP)
-			_, err = makeGetRequest(url_blacklist, "brownfield-blacklist-ns.host", 200, true)
-			Expect(err).To(BeNil())
-
-			//delete namespaces for blacklist testing
-			deleteOptions := metav1.DeleteOptions{
-				GracePeriodSeconds: to.Int64Ptr(0),
-			}
-
-			klog.Info("Delete namespaces test-brownfield-ns after blacklist testing...")
-			err = clientset.CoreV1().Namespaces().Delete(context.TODO(), "test-brownfield-ns", deleteOptions)
-			Expect(err).To(BeNil())
-		})
-
-		It("[sub-resource-prefix] should be use the sub-resource-prefix to prefix sub-resources", func() {
-			env := GetEnv()
-			klog.Infof("'subResourceNamePrefix': %s", env.SubResourceNamePrefix)
-			Expect(env.SubResourceNamePrefix).ToNot(Equal(""), "Please make sure that environment variable 'subResourceNamePrefix' is set")
-
-			namespaceName := "e2e-sub-resource-prefix"
+		It("[prohibited-target-test] prohibited target should be available to be accessed", func() {
+			namespaceName := "e2e-prohibited-target"
 			ns := &v1.Namespace{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: namespaceName,
@@ -79,26 +52,38 @@ var _ = Describe("networking-v1-LFU", func() {
 			_, err = clientset.CoreV1().Namespaces().Create(context.TODO(), ns, metav1.CreateOptions{})
 			Expect(err).To(BeNil())
 
-			SSLE2ERedirectYamlPath := "testdata/networking-v1/one-namespace-one-ingress/ssl-e2e-redirect/app.yaml"
-			klog.Info("Applying yaml: ", SSLE2ERedirectYamlPath)
-			err = applyYaml(clientset, crdClient, namespaceName, SSLE2ERedirectYamlPath)
+			appYamlPath := "testdata/networking-v1/one-namespace-one-ingress/prohibited-target/app.yaml"
+			klog.Info("Applying yaml: ", appYamlPath)
+			err = applyYaml(clientset, crdClient, namespaceName, appYamlPath)
 			Expect(err).To(BeNil())
 			time.Sleep(30 * time.Second)
 
-			gateway, err := getGateway()
+			// get ip address for 1 ingress
+			klog.Info("Getting public IP of the app gateway")
+			ip, err := getPublicIPAddress()
+			Expect(err).To(BeNil())
+
+			publicIP := *ip.IPAddress
+			klog.Infof("Public IP: %s", publicIP)
+
+			protectedPath := fmt.Sprintf("http://%s/landing/", publicIP)
+			_, err = makeGetRequest(protectedPath, "www.microsoft.com", 302, true)
 			Expect(err).To(BeNil())
 
-			prefixUsed := false
-			for _, listener := range *gateway.HTTPListeners {
-				klog.Infof("checking listener %s for %s", *listener.Name, env.SubResourceNamePrefix)
-				if strings.HasPrefix(*listener.Name, env.SubResourceNamePrefix) {
-					klog.Infof("found %s that uses the prefix", *listener.Name)
-					prefixUsed = true
-					break
-				}
-			}
+			ingressPath := fmt.Sprintf("http://%s/aspnet", publicIP)
+			_, err = makeGetRequest(ingressPath, "www.microsoft.com", 200, true)
+			Expect(err).To(BeNil())
 
-			Expect(prefixUsed).To(BeTrue(), "%s wasn't used for naming the sub-resource of app gateway. Currently, this check looks at HTTP listener only", env.SubResourceNamePrefix)
+			klog.Info("Deleting yaml: ", appYamlPath)
+			err = deleteYaml(clientset, crdClient, namespaceName, appYamlPath)
+			Expect(err).To(BeNil())
+			time.Sleep(30 * time.Second)
+
+			_, err = makeGetRequest(protectedPath, "www.microsoft.com", 302, true)
+			Expect(err).To(BeNil())
+
+			_, err = makeGetRequest(ingressPath, "www.microsoft.com", 502, true)
+			Expect(err).To(BeNil())
 		})
 
 		AfterEach(func() {

scripts/e2e/cmd/runner/networking-v1-mfu_one_namespace_many_ingresses_test.go

@@ -267,7 +267,7 @@ var _ = Describe("networking-v1-MFU", func() {
 				}
 
 				return len(exampleComListeners) == 2
-			}, 60*time.Second, 5*time.Second).Should(BeTrue())
+			}, 10*time.Minute, 5*time.Second).Should(BeTrue())
 
 			// Check that both listeners have the same frontend port
 			klog.Info("Checking that both listeners have the same frontend port...")

scripts/e2e/cmd/runner/testdata/networking-v1/one-namespace-one-ingress/prohibited-target/app.yaml

@@ -0,0 +1,60 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: aspnet
+spec:
+  selector:
+    matchLabels:
+      app: aspnet
+  replicas: 1
+  template:
+    metadata:
+      labels:
+        app: aspnet
+    spec:
+      containers:
+        - name: aspnet
+          imagePullPolicy: IfNotPresent
+          image: mcr.microsoft.com/dotnet/samples:aspnetapp
+          ports:
+            - containerPort: 8080
+          resources:
+            requests:
+              cpu: 10m
+              memory: 10Mi
+            limits:
+              cpu: 100m
+              memory: 100Mi
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: aspnet
+spec:
+  selector:
+    app: aspnet
+  ports:
+  - protocol: TCP
+    port: 80
+    targetPort: 8080
+---
+apiVersion: networking.k8s.io/v1
+kind: Ingress
+metadata:
+  name: aspnet
+  annotations:
+    appgw.ingress.kubernetes.io/backend-path-prefix: "/"
+
+spec:
+  ingressClassName: "azure-application-gateway"
+  rules:
+    - host: www.microsoft.com
+      http:
+        paths:
+        - path: /aspnet
+          backend:
+            service:
+              name: aspnet
+              port:
+                number: 80
+          pathType: Prefix