Merge master into release/1.5 (#1349)

* fixed subscriptionId description (#1345) * k8s: Add retry logic to get server version (#1344) * add retry logic to get server version * add output of server version Co-authored-by: Akshay Gupta <[email protected]> * networking/v1: add support for ingress path type (#1346) * add support for ingress path type * add test for / * fix: don't initialize ingress class when k8s doesn't support v1/ingress (#1347) * fix: panic when ingress class is not supported on k8s * add test * docs: add changelog 1.5 (#1348) * add changelog 1.5 * add retry fix * fix reference Co-authored-by: Carlos Mendible <[email protected]> Co-authored-by: Shuanglu <[email protected]>
Azure · Feb 18, 2022 · 0a4f032 · 0a4f032
1 parent cba7004
commit 0a4f032
Show file tree

Hide file tree

Showing 12 changed files with 666 additions and 20 deletions.
diff --git a/CHANGELOG/CHANGELOG-1.5.md b/CHANGELOG/CHANGELOG-1.5.md
@@ -0,0 +1,69 @@
+- [How to try](#how-to-try)
+- [v1.5.1](#v151)
+  - [Features](#features)
+  - [Fixes](#fixes)
+- [v1.5.0](#v150)
+  - [Features](#features-1)
+  - [Fixes](#fixes-1)
+- [v1.5.0-rc1](#v150-rc1)
+  - [Features](#features-2)
+  - [Fixes](#fixes-2)
+
+# v1.5.1
+
+## Features
+*  [[#1122](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1346) Add support for ingress.rules.path.pathType property
+
+## Fixes
+* [#1347](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1347) fix(v1/ingress): fix panic due to ingress class when k8s <= 1.19
+* [#1344](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1344) fix(v1/ingress): retry getting server version to get past transient issues
+
+# v1.5.0
+
+## Features
+* [#1329](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1329) Support for ingress class resource in v1/ingress
+* [#1280](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1280) Add cookie-based-affinity-distinct-name annotation
+* [#1287](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1287) Add rewrite rule set annotation to reference an existing rewrite rule on Application Gateway
+
+## Fixes
+* [#1322](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1322) fix(port): port reference by name in ingress
+
+# v1.5.0-rc1
+
+## Features
+* [#1197](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1197) Support for v1/ingress (maintaining compatibility with v1beta1/ingress for clsuters <= 1.22)
+* [#1324](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1324) Support for multi-arch images: amd64/arm64
+* [#1169](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1169) Resource quota exposed through helm chart
+> Note:
+>  * Support for ingress class resource will added in a future release
+>  * Support for ingress.rules.path.pathType property will added in a future release
+
+## Fixes
+* [#1282](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1282) fix(ingress): Modify cloned ingress instead of original ingress
+* [#1271](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1271) fix(private clouds): Use correct endpoints for AAD and Azure in private clouds
+* [#1273](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1273) fix(config): panic when processing duplicate paths in urlpathmap
+* [#1220](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1220) fix(crd): Upgrade prohibited target CRD api-version and add tests
+* [#1278](https://github.com/Azure/application-gateway-kubernetes-ingress/pull/1278) fix(prohibited target): incorrect merge when rules being merged reference the same path map
+
+## How to try:
+```bash
+# Add helm repo / update AGIC repo
+helm repo add application-gateway-kubernetes-ingress https://appgwingress.blob.core.windows.net/ingress-azure-helm-package/
+helm repo update
+
+# Install
+helm install \
+  <release-name> \
+  -f helm-config.yaml \
+  application-gateway-kubernetes-ingress/ingress-azure \
+
+# or 
+
+# Upgrade
+# https://github.com/Azure/application-gateway-kubernetes-ingress/blob/master/docs/how-tos/helm-upgrade.md
+# --reuse-values   when upgrading, reuse the last release's values and merge in any overrides from the command line via --set and -f. If '--reset-values' is specified, this is ignored
+helm upgrade \
+  <release-name> \
+  application-gateway-kubernetes-ingress/ingress-azure \
+  --reuse-values
+```
diff --git a/docs/developers/build.md b/docs/developers/build.md
@@ -33,7 +33,7 @@ The file will contain a JSON blob with the following shape:
 {
   "clientId": "...",
   "clientSecret": "...",
-  "subscriptionId": "<your-azure-resource-group>",
+  "subscriptionId": "<your-azure-subscription-id>",
   "tenantId": "...",
   "activeDirectoryEndpointUrl": "https://login.microsoftonline.com",
   "resourceManagerEndpointUrl": "https://management.azure.com/",

diff --git a/pkg/appgw/configbuilder.go b/pkg/appgw/configbuilder.go
@@ -217,8 +217,9 @@ func generateListenerID(ingress *networking.Ingress, rule *networking.IngressRul
 	if overridePort != nil {
 		if *overridePort > 0 && *overridePort < 65000 {
 			frontendPort = *overridePort
+			klog.V(5).Infof("Using custom port specified in the override annotation: %d", *overridePort)
 		} else {
-			klog.V(5).Infof("Invalid custom port configuration (%d). Setting listener port to default : %d", *overridePort, frontendPort)
+			klog.V(5).Infof("Derived listener port from ingress: %d", frontendPort)
 		}
 
 	}

diff --git a/pkg/appgw/ingress_rules.go b/pkg/appgw/ingress_rules.go
@@ -52,7 +52,7 @@ func (c *appGwConfigBuilder) applyToListener(rule *networking.IngressRule) bool
 	for pathIdx := range rule.HTTP.Paths {
 		path := &rule.HTTP.Paths[pathIdx]
 		// if there is path that is /, /* , empty string, then apply the waf policy to the listener.
-		if len(path.Path) == 0 || path.Path == "/" || path.Path == "/*" {
+		if isPathCatchAll(path.Path, path.PathType) {
 			return true
 		}
 	}

diff --git a/pkg/appgw/requestroutingrules.go b/pkg/appgw/requestroutingrules.go
@@ -269,7 +269,7 @@ func (c *appGwConfigBuilder) getDefaultFromRule(cbCtx *ConfigBuilderContext, lis
 	defBackend := ingress.Spec.DefaultBackend
 	for pathIdx := range rule.HTTP.Paths {
 		path := &rule.HTTP.Paths[pathIdx]
-		if path.Path == "" || path.Path == "/*" || path.Path == "/" {
+		if isPathCatchAll(path.Path, path.PathType) {
 			defBackend = &path.Backend
 			defPath = path
 			defRule = rule
@@ -303,7 +303,7 @@ func (c *appGwConfigBuilder) getPathRules(cbCtx *ConfigBuilderContext, listenerI
 	pathRules := make([]n.ApplicationGatewayPathRule, 0)
 	for pathIdx := range rule.HTTP.Paths {
 		path := &rule.HTTP.Paths[pathIdx]
-		if len(path.Path) == 0 || path.Path == "/*" || path.Path == "/" {
+		if isPathCatchAll(path.Path, path.PathType) {
 			continue
 		}
 
@@ -314,7 +314,7 @@ func (c *appGwConfigBuilder) getPathRules(cbCtx *ConfigBuilderContext, listenerI
 			Name: to.StringPtr(pathRuleName),
 			ID:   to.StringPtr(c.appGwIdentifier.pathRuleID(pathMapName, pathRuleName)),
 			ApplicationGatewayPathRulePropertiesFormat: &n.ApplicationGatewayPathRulePropertiesFormat{
-				Paths: &[]string{path.Path},
+				Paths: &[]string{preparePathFromPathType(path.Path, path.PathType)},
 			},
 		}
 
@@ -439,3 +439,27 @@ func printPathRule(pathRule n.ApplicationGatewayPathRule) string {
 
 	return s
 }
+
+// preparePathFromPathType uses pathType property to modify ingress.Path to append/remove "*"
+// if pathType == Prefix, "*" is appended if not provided by the user
+// if pathType == Exact, "*" is trimmed
+func preparePathFromPathType(path string, pathType *networking.PathType) string {
+	if pathType != nil {
+		if *pathType == networking.PathTypeExact {
+			return strings.TrimSuffix(path, "*")
+		}
+
+		if *pathType == networking.PathTypePrefix && !strings.HasSuffix(path, "*") {
+			return path + "*"
+		}
+	}
+	return path
+}
+
+// Application Gateway doesn't allow exact path for "/"
+// Code="ApplicationGatewayPathRulePathShouldHaveNonEmptyMatchValue" Message="Path / should have a nonempty match value followed by '/'  in PathRule xxxx
+// So, Path "/" with pathType:Exact cannot be added into the path rule. Thus, we don't support it.
+// "/" for any path type will be treated as a prefix match.
+func isPathCatchAll(path string, pathType *networking.PathType) bool {
+	return len(path) == 0 || path == "/*" || path == "/"
+}
diff --git a/pkg/appgw/requestroutingrules_test.go b/pkg/appgw/requestroutingrules_test.go
@@ -937,4 +937,121 @@ var _ = Describe("Test routing rules generations", func() {
 		})
 
 	})
+
+	Context("test pathType in ingress", func() {
+		configBuilder := newConfigBuilderFixture(nil)
+		endpoint := tests.NewEndpointsFixture()
+		service := tests.NewServiceFixture(*tests.NewServicePortsFixture()...)
+		ingress := tests.NewIngressTestWithVariousPathTypeFixture(tests.Namespace, "ingress-with-path-type")
+
+		_ = configBuilder.k8sContext.Caches.Endpoints.Add(endpoint)
+		_ = configBuilder.k8sContext.Caches.Service.Add(service)
+		_ = configBuilder.k8sContext.Caches.Ingress.Add(ingress)
+
+		cbCtx := &ConfigBuilderContext{
+			IngressList:           []*networking.Ingress{&ingress},
+			ServiceList:           []*v1.Service{service},
+			DefaultAddressPoolID:  to.StringPtr("xx"),
+			DefaultHTTPSettingsID: to.StringPtr("yy"),
+		}
+
+		_, urlPathMaps := configBuilder.getRules(cbCtx)
+		urlPathMap := urlPathMaps[0]
+		pathRules := *urlPathMaps[0].PathRules
+
+		It("should have 8 paths", func() {
+			Expect(len(pathRules)).To(Equal(8))
+		})
+
+		It("should add * to paths with pathType:prefix", func() {
+			paths := *(pathRules[0].Paths)
+			Expect(paths[0]).To(Equal("/prefix0*"))
+
+			paths = *(pathRules[1].Paths)
+			Expect(paths[0]).To(Equal("/prefix1*"))
+		})
+
+		It("should trim * from paths with pathType:exact", func() {
+			paths := *(pathRules[2].Paths)
+			Expect(paths[0]).To(Equal("/exact2"))
+
+			paths = *(pathRules[3].Paths)
+			Expect(paths[0]).To(Equal("/exact3"))
+		})
+
+		It("should not modify paths with pathType:implementationSpecific", func() {
+			paths := *(pathRules[4].Paths)
+			Expect(paths[0]).To(Equal("/ims4*"))
+
+			paths = *(pathRules[5].Paths)
+			Expect(paths[0]).To(Equal("/ims5"))
+		})
+
+		It("should not modify paths with pathType:nil", func() {
+			paths := *(pathRules[6].Paths)
+			Expect(paths[0]).To(Equal("/nil6*"))
+
+			paths = *(pathRules[7].Paths)
+			Expect(paths[0]).To(Equal("/nil7"))
+		})
+
+		It("should have default matching /*", func() {
+			Expect(*urlPathMap.DefaultBackendAddressPool.ID).To(HaveSuffix("pool---namespace-----service-name---80-bp-9876"))
+			Expect(*urlPathMap.DefaultBackendHTTPSettings.ID).To(HaveSuffix("bp---namespace-----service-name---80-9876-ingress-with-path-type"))
+		})
+	})
+
+	Context("test preparePathFromPathType", func() {
+		It("should append * when pathType is Prefix", func() {
+			pathType := networking.PathTypePrefix
+
+			Expect(preparePathFromPathType("/path", &pathType)).To(Equal("/path*"))
+			Expect(preparePathFromPathType("/path*", &pathType)).To(Equal("/path*"))
+		})
+
+		It("should trim when pathType is Exact", func() {
+			pathType := networking.PathTypeExact
+
+			Expect(preparePathFromPathType("/path", &pathType)).To(Equal("/path"))
+			Expect(preparePathFromPathType("/path*", &pathType)).To(Equal("/path"))
+		})
+
+		It("should not modify when pathType is ImplementationSpecific", func() {
+			pathType := networking.PathTypeImplementationSpecific
+
+			Expect(preparePathFromPathType("/path", &pathType)).To(Equal("/path"))
+			Expect(preparePathFromPathType("/path*", &pathType)).To(Equal("/path*"))
+		})
+
+		It("should not modify when pathType is nil", func() {
+			Expect(preparePathFromPathType("/path", nil)).To(Equal("/path"))
+			Expect(preparePathFromPathType("/path*", nil)).To(Equal("/path*"))
+		})
+	})
+
+	Context("test isPathCatchAll", func() {
+		// Application Gateway doesn't allow exact path for "/"
+		It("should be false if / and pathType:exact", func() {
+			pathTypeExact := networking.PathTypeExact
+			Expect(isPathCatchAll("/", &pathTypeExact)).To(BeTrue())
+			Expect(isPathCatchAll("/*", &pathTypeExact)).To(BeTrue())
+		})
+
+		It("should be true if / and pathType:nil", func() {
+			Expect(isPathCatchAll("/", nil)).To(BeTrue())
+			Expect(isPathCatchAll("/*", nil)).To(BeTrue())
+		})
+
+		It("should be true if / and pathType:prefix", func() {
+			pathTypePrefix := networking.PathTypePrefix
+			Expect(isPathCatchAll("/", &pathTypePrefix)).To(BeTrue())
+			Expect(isPathCatchAll("/*", &pathTypePrefix)).To(BeTrue())
+		})
+
+		It("should be true if / and pathType:implementationSpecific", func() {
+			pathTypeIMS := networking.PathTypeImplementationSpecific
+			Expect(isPathCatchAll("/", &pathTypeIMS)).To(BeTrue())
+			Expect(isPathCatchAll("/*", &pathTypeIMS)).To(BeTrue())
+		})
+	})
 })
diff --git a/pkg/k8scontext/context.go b/pkg/k8scontext/context.go
@@ -76,15 +76,13 @@ func NewContext(kubeClient kubernetes.Interface, crdClient versioned.Interface,
 
 	if IsNetworkingV1PackageSupported {
 		informerCollection.Ingress = informerFactory.Networking().V1().Ingresses().Informer()
-		informerCollection.IngressClass = informerFactory.Networking().V1().IngressClasses().Informer()
 	} else {
 		informerCollection.Ingress = informerFactory.Extensions().V1beta1().Ingresses().Informer()
 	}
 
 	cacheCollection := CacheCollection{
 		Endpoints:                          informerCollection.Endpoints.GetStore(),
 		Ingress:                            informerCollection.Ingress.GetStore(),
-		IngressClass:                       informerCollection.IngressClass.GetStore(),
 		Pods:                               informerCollection.Pods.GetStore(),
 		Secret:                             informerCollection.Secret.GetStore(),
 		Service:                            informerCollection.Service.GetStore(),
@@ -146,7 +144,6 @@ func NewContext(kubeClient kubernetes.Interface, crdClient versioned.Interface,
 	// Register event handlers.
 	informerCollection.Endpoints.AddEventHandler(resourceHandler)
 	informerCollection.Ingress.AddEventHandler(ingressResourceHandler)
-	informerCollection.IngressClass.AddEventHandler(resourceHandler)
 	informerCollection.Pods.AddEventHandler(resourceHandler)
 	informerCollection.Secret.AddEventHandler(secretResourceHandler)
 	informerCollection.Service.AddEventHandler(resourceHandler)
@@ -156,6 +153,12 @@ func NewContext(kubeClient kubernetes.Interface, crdClient versioned.Interface,
 	informerCollection.MultiClusterService.AddEventHandler(resourceHandler)
 	informerCollection.MultiClusterIngress.AddEventHandler(resourceHandler)
 
+	if IsNetworkingV1PackageSupported {
+		informerCollection.IngressClass = informerFactory.Networking().V1().IngressClasses().Informer()
+		informerCollection.IngressClass.AddEventHandler(resourceHandler)
+		cacheCollection.IngressClass = informerCollection.IngressClass.GetStore()
+	}
+
 	return context
 }
 
@@ -188,13 +191,16 @@ func (c *Context) Run(stopChannel chan struct{}, omitCRDs bool, envVariables env
 		c.informers.Service,
 		c.informers.Secret,
 		c.informers.Ingress,
-		c.informers.IngressClass,
 
 		//TODO: enabled by ccp feature flag
 		// c.informers.AzureApplicationGatewayBackendPool,
 		// c.informers.AzureApplicationGatewayInstanceUpdateStatus,
 	}
 
+	if IsNetworkingV1PackageSupported {
+		sharedInformers = append(sharedInformers, c.informers.IngressClass)
+	}
+
 	// For AGIC to watch for these CRDs the EnableBrownfieldDeploymentVarName env variable must be set to true
 	if envVariables.EnableBrownfieldDeployment {
 		sharedInformers = append(sharedInformers, c.informers.AzureIngressProhibitedTarget)
@@ -870,6 +876,10 @@ func (c *Context) isServiceReferencedByAnyIngress(service *v1.Service) bool {
 
 // getIngressClassResource gets ingress class object with specified name
 func (c *Context) getIngressClassResource(ingressClassName string) *networking.IngressClass {
+	if c.Caches.IngressClass == nil {
+		return nil
+	}
+
 	ingressClassInterface, exist, err := c.Caches.IngressClass.GetByKey(ingressClassName)
 	if err != nil {
 		return nil