Repository: kubernetes/ingress-nginx
Pull request #5269 (2020-03-19 17:04:33)

Ensure DeleteDeployment waits until there are no pods running
<!--- Provide a general summary of your changes in the Title above --->
<!--- Please don't @-mention people in PR or commit messages (do so in an additional comment). --->

## What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Which issue/s this PR fixes
<!--
(optional, in `fixes #<issue number>` format, will close that issue when PR gets merged):

fixes #
-->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I've read the [CONTRIBUTION](https://github.com/kubernetes/ingress-nginx/blob/master/CONTRIBUTING.md) guide
- [ ] I have added tests to cover my changes.
- [ ] All new and existing tests passed.



Patch:
diff --git a/cmd/nginx/flags.go b/cmd/nginx/flags.go
index 933678182..d2ead6b47 100644
--- a/cmd/nginx/flags.go
+++ b/cmd/nginx/flags.go
@@ -186,6 +186,7 @@ Takes the form "<host>:port". If not provided, no admission controller is starte
 
 	flags.StringVar(&nginx.MaxmindLicenseKey, "maxmind-license-key", "", `Maxmind license key to download GeoLite2 Databases.
 https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-geolite2-databases`)
+	flags.StringVar(&nginx.MaxmindEditionIDs, "maxmind-edition-ids", "GeoLite2-City,GeoLite2-ASN", `Maxmind edition ids to download GeoLite2 Databases.`)
 
 	flag.Set("logtostderr", "true")
 
@@ -310,12 +311,15 @@ https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-g
 		config.RootCAFile = *rootCAFile
 	}
 
-	if nginx.MaxmindLicenseKey != "" {
+	if nginx.MaxmindLicenseKey != "" && nginx.MaxmindEditionIDs != "" {
+		if err := nginx.ValidateGeoLite2DBEditions(); err != nil {
+			return false, nil, err
+		}
 		klog.Info("downloading maxmind GeoIP2 databases...")
-		err := nginx.DownloadGeoLite2DB()
-		if err != nil {
+		if err := nginx.DownloadGeoLite2DB(); err != nil {
 			klog.Errorf("unexpected error downloading GeoIP2 database: %v", err)
 		}
+		config.MaxmindEditionFiles = nginx.MaxmindEditionFiles
 	}
 
 	return false, config, nil
diff --git a/docs/examples/customization/ssl-dh-param/README.md b/docs/examples/customization/ssl-dh-param/README.md
index 2a9b0f707..7b63081dd 100644
--- a/docs/examples/customization/ssl-dh-param/README.md
+++ b/docs/examples/customization/ssl-dh-param/README.md
@@ -27,7 +27,7 @@ $ kubectl create -f configmap.yaml
 ## Custom DH parameters secret
 
 ```console
-$> openssl dhparam 1024 2> /dev/null | base64
+$> openssl dhparam 4096 2> /dev/null | base64
 LS0tLS1CRUdJTiBESCBQQVJBTUVURVJ...
 ```
 
@@ -38,7 +38,7 @@ data:
   dhparam.pem: "LS0tLS1CRUdJTiBESCBQQVJBTUVURVJ..."
 kind: Secret
 metadata:
-  name: nginx-configuration
+  name: lb-dhparam
   namespace: ingress-nginx
   labels:
     app.kubernetes.io/name: ingress-nginx
diff --git a/internal/ingress/annotations/rewrite/main.go b/internal/ingress/annotations/rewrite/main.go
index 82d1ff282..9ac5c0b7e 100644
--- a/internal/ingress/annotations/rewrite/main.go
+++ b/internal/ingress/annotations/rewrite/main.go
@@ -17,9 +17,13 @@ limitations under the License.
 package rewrite
 
 import (
+	"net/url"
+
 	networking "k8s.io/api/networking/v1beta1"
+	"k8s.io/klog"
 
 	"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
+	"k8s.io/ingress-nginx/internal/ingress/errors"
 	"k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
@@ -90,8 +94,29 @@ func (a rewrite) Parse(ing *networking.Ingress) (interface{}, error) {
 		config.ForceSSLRedirect = a.r.GetDefaultBackend().ForceSSLRedirect
 	}
 
-	config.AppRoot, _ = parser.GetStringAnnotation("app-root", ing)
 	config.UseRegex, _ = parser.GetBoolAnnotation("use-regex", ing)
 
+	config.AppRoot, err = parser.GetStringAnnotation("app-root", ing)
+	if err != nil {
+		if !errors.IsMissingAnnotations(err) && !errors.IsInvalidContent(err) {
+			klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		}
+
+		return config, nil
+	}
+
+	u, err := url.ParseRequestURI(config.AppRoot)
+	if err != nil {
+		klog.Warningf("Annotation app-root contains an invalid value: %v", err)
+		config.AppRoot = ""
+		return config, nil
+	}
+
+	if u.IsAbs() {
+		klog.Warningf("Annotation app-root only allows absolute paths (%v)", config.AppRoot)
+		config.AppRoot = ""
+		return config, nil
+	}
+
 	return config, nil
 }
diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go
index 88d1a5c90..b9aa93a47 100644
--- a/internal/ingress/controller/config/config.go
+++ b/internal/ingress/controller/config/config.go
@@ -826,6 +826,7 @@ type TemplateConfig struct {
 	ListenPorts              *ListenPorts
 	PublishService           *apiv1.Service
 	EnableMetrics            bool
+	MaxmindEditionFiles      []string
 
 	PID        string
 	StatusPath string
diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go
index e943e5717..d7dd890d6 100644
--- a/internal/ingress/controller/controller.go
+++ b/internal/ingress/controller/controller.go
@@ -98,7 +98,8 @@ type Configuration struct {
 	ValidationWebhookCertPath string
 	ValidationWebhookKeyPath  string
 
-	GlobalExternalAuth *ngx_config.GlobalExternalAuth
+	GlobalExternalAuth  *ngx_config.GlobalExternalAuth
+	MaxmindEditionFiles []string
 }
 
 // GetPublishService returns the Service used to set the load-balancer status of Ingresses.
diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go
index 0cc1bbe34..cec908065 100644
--- a/internal/ingress/controller/nginx.go
+++ b/internal/ingress/controller/nginx.go
@@ -615,12 +615,12 @@ func (n NGINXController) generateTemplate(cfg ngx_config.Configuration, ingressC
 		ListenPorts:              n.cfg.ListenPorts,
 		PublishService:           n.GetPublishService(),
 		EnableMetrics:            n.cfg.EnableMetrics,
-
-		HealthzURI: nginx.HealthPath,
-		PID:        nginx.PID,
-		StatusPath: nginx.StatusPath,
-		StatusPort: nginx.StatusPort,
-		StreamPort: nginx.StreamPort,
+		MaxmindEditionFiles:      n.cfg.MaxmindEditionFiles,
+		HealthzURI:               nginx.HealthPath,
+		PID:                      nginx.PID,
+		StatusPath:               nginx.StatusPath,
+		StatusPort:               nginx.StatusPort,
+		StreamPort:               nginx.StreamPort,
 	}
 
 	tc.Cfg.Checksum = ingressCfg.ConfigurationChecksum
diff --git a/internal/nginx/maxmind.go b/internal/nginx/maxmind.go
index 1da8110a1..e6128eab5 100644
--- a/internal/nginx/maxmind.go
+++ b/internal/nginx/maxmind.go
@@ -30,12 +30,14 @@ import (
 // MaxmindLicenseKey maxmind license key to download databases
 var MaxmindLicenseKey = ""
 
-const (
-	geoIPPath = "/etc/nginx/geoip"
+// MaxmindEditionIDs maxmind editions (GeoLite2-City, GeoLite2-Country, GeoIP2-ISP, etc)
+var MaxmindEditionIDs = ""
 
-	geoLiteCityDB = "GeoLite2-City"
-	geoLiteASNDB  = "GeoLite2-ASN"
+// MaxmindEditionFiles maxmind databases on disk
+var MaxmindEditionFiles []string
 
+const (
+	geoIPPath   = "/etc/nginx/geoip"
 	dbExtension = ".mmdb"
 
 	maxmindURL = "https://download.maxmind.com/app/geoip_download?license_key=%v&edition_id=%v&suffix=tar.gz"
@@ -44,12 +46,10 @@ const (
 // GeoLite2DBExists checks if the required databases for
 // the GeoIP2 NGINX module are present in the filesystem
 func GeoLite2DBExists() bool {
-	if !fileExists(path.Join(geoIPPath, geoLiteASNDB+dbExtension)) {
-		return false
-	}
-
-	if !fileExists(path.Join(geoIPPath, geoLiteCityDB+dbExtension)) {
-		return false
+	for _, dbName := range strings.Split(MaxmindEditionIDs, ",") {
+		if !fileExists(path.Join(geoIPPath, dbName+dbExtension)) {
+			return false
+		}
 	}
 
 	return true
@@ -58,16 +58,13 @@ func GeoLite2DBExists() bool {
 // DownloadGeoLite2DB downloads the required databases by the
 // GeoIP2 NGINX module using a license key from MaxMind.
 func DownloadGeoLite2DB() error {
-	err := downloadDatabase(geoLiteCityDB)
-	if err != nil {
-		return err
-	}
-
-	err = downloadDatabase(geoLiteASNDB)
-	if err != nil {
-		return err
+	for _, dbName := range strings.Split(MaxmindEditionIDs, ",") {
+		err := downloadDatabase(dbName)
+		if err != nil {
+			return err
+		}
+		MaxmindEditionFiles = append(MaxmindEditionFiles, dbName+dbExtension)
 	}
-
 	return nil
 }
 
@@ -133,6 +130,29 @@ func downloadDatabase(dbName string) error {
 		fmt.Sprintf(maxmindURL, "XXXXXXX", dbName), mmdbFile)
 }
 
+// ValidateGeoLite2DBEditions check provided Maxmind database editions names
+func ValidateGeoLite2DBEditions() error {
+	allowedEditions := map[string]bool{
+		"GeoIP2-Anonymous-IP":    true,
+		"GeoIP2-Country":         true,
+		"GeoIP2-City":            true,
+		"GeoIP2-Connection-Type": true,
+		"GeoIP2-Domain":          true,
+		"GeoIP2-ISP":             true,
+		"GeoIP2-ASN":             true,
+		"GeoLite2-ASN":           true,
+		"GeoLite2-Country":       true,
+		"GeoLite2-City":          true,
+	}
+
+	for _, edition := range strings.Split(MaxmindEditionIDs, ",") {
+		if !allowedEditions[edition] {
+			return fmt.Errorf("unknown Maxmind GeoIP2 edition name: '%s'", edition)
+		}
+	}
+	return nil
+}
+
 func fileExists(filePath string) bool {
 	info, err := os.Stat(filePath)
 	if os.IsNotExist(err) {
diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
index c0b690dd2..52277075e 100755
--- a/rootfs/etc/nginx/template/nginx.tmpl
+++ b/rootfs/etc/nginx/template/nginx.tmpl
@@ -159,6 +159,8 @@ http {
     {{ if $cfg.UseGeoIP2 }}
     # https://github.com/leev/ngx_http_geoip2_module#example-usage
 
+    {{ range $index, $file := $all.MaxmindEditionFiles }}
+    {{ if eq $file "GeoLite2-City.mmdb" }}
     geoip2 /etc/nginx/geoip/GeoLite2-City.mmdb {
         $geoip2_city_country_code source=$remote_addr country iso_code;
         $geoip2_city_country_name source=$remote_addr country names en;
@@ -171,13 +173,52 @@ http {
         $geoip2_region_code source=$remote_addr subdivisions 0 iso_code;
         $geoip2_region_name source=$remote_addr subdivisions 0 names en;
     }
+    {{ end }}
+
+    {{ if eq $file "GeoIP2-City.mmdb" }}
+    geoip2 /etc/nginx/geoip/GeoIP2-City.mmdb {
+        $geoip2_city_country_code source=$remote_addr country iso_code;
+        $geoip2_city_country_name source=$remote_addr country names en;
+        $geoip2_city source=$remote_addr city names en;
+        $geoip2_postal_code source=$remote_addr postal code;
+        $geoip2_dma_code source=$remote_addr location metro_code;
+        $geoip2_latitude source=$remote_addr location latitude;
+        $geoip2_longitude source=$remote_addr location longitude;
+        $geoip2_time_zone source=$remote_addr location time_zone;
+        $geoip2_region_code source=$remote_addr subdivisions 0 iso_code;
+        $geoip2_region_name source=$remote_addr subdivisions 0 names en;
+    }
+    {{ end }}
 
+    {{ if eq $file "GeoLite2-ASN.mmdb" }}
     geoip2 /etc/nginx/geoip/GeoLite2-ASN.mmdb {
         $geoip2_asn source=$remote_addr autonomous_system_number;
         $geoip2_org source=$remote_addr autonomous_system_organization;
     }
     {{ end }}
 
+    {{ if eq $file "GeoIP2-ASN.mmdb" }}
+    geoip2 /etc/nginx/geoip/GeoIP2-ASN.mmdb {
+        $geoip2_asn source=$remote_addr autonomous_system_number;
+        $geoip2_org source=$remote_addr autonomous_system_organization;
+    }
+    {{ end }}
+
+    {{ if eq $file "GeoIP2-ISP.mmdb" }}
+    geoip2 /etc/nginx/geoip/GeoIP2-ISP.mmdb {
+        $geoip2_isp isp;
+        $geoip2_isp_org organization;
+    }
+    {{ end }}
+    {{ if eq $file "GeoIP2-Connection-Type.mmdb" }}
+    geoip2 /etc/nginx/geoip/GeoIP2-Connection-Type.mmdb {
+        $geoip2_connection_type connection_type;
+    }
+    {{ end }}
+    {{ end }}
+
+    {{ end }}
+
     aio                 threads;
     aio_write           on;
 
@@ -821,7 +862,7 @@ stream {
         proxy_ssl_verify_depth                  {{ $server.ProxySSL.VerifyDepth }};
         {{ if not (empty $server.ProxySSL.ProxySSLName) }}
         proxy_ssl_name                           {{ $server.ProxySSL.ProxySSLName }};
-        {{ end }}        
+        {{ end }}
         {{ end }}
 
         {{ if not (empty $server.ProxySSL.PemFileName) }}
@@ -855,9 +896,9 @@ stream {
         {{ $externalAuth = $all.Cfg.GlobalExternalAuth }}
         {{ end }}
 
-        {{ if not (empty $location.Rewrite.AppRoot)}}
+        {{ if not (empty $location.Rewrite.AppRoot) }}
         if ($uri = /) {
-            return 302 {{ $location.Rewrite.AppRoot }};
+            return 302 $scheme://$http_host{{ $location.Rewrite.AppRoot }};
         }
         {{ end }}
 


Test patch:
diff --git a/cmd/nginx/flags_test.go b/cmd/nginx/flags_test.go
index 6e826b932..dba2f43c0 100644
--- a/cmd/nginx/flags_test.go
+++ b/cmd/nginx/flags_test.go
@@ -75,3 +75,16 @@ func TestFlagConflict(t *testing.T) {
 		t.Fatalf("Expected an error parsing flags but none returned")
 	}
 }
+
+func TestMaxmindEdition(t *testing.T) {
+	resetForTesting(func() { t.Fatal("Parsing failed") })
+
+	oldArgs := os.Args
+	defer func() { os.Args = oldArgs }()
+	os.Args = []string{"cmd", "--publish-service", "namespace/test", "--http-port", "0", "--https-port", "0", "--maxmind-license-key", "0000000", "--maxmind-edition-ids", "GeoLite2-City, TestCheck"}
+
+	_, _, err := parseFlags()
+	if err == nil {
+		t.Fatalf("Expected an error parsing flags but none returned")
+	}
+}
diff --git a/internal/ingress/annotations/rewrite/main_test.go b/internal/ingress/annotations/rewrite/main_test.go
index 03afaa3c0..beece494a 100644
--- a/internal/ingress/annotations/rewrite/main_test.go
+++ b/internal/ingress/annotations/rewrite/main_test.go
@@ -41,8 +41,9 @@ func buildIngress() *networking.Ingress {
 
 	return &networking.Ingress{
 		ObjectMeta: meta_v1.ObjectMeta{
-			Name:      "foo",
-			Namespace: api.NamespaceDefault,
+			Name:        "foo",
+			Namespace:   api.NamespaceDefault,
+			Annotations: map[string]string{},
 		},
 		Spec: networking.IngressSpec{
 			Backend: &networking.IngressBackend{
@@ -163,19 +164,42 @@ func TestForceSSLRedirect(t *testing.T) {
 	}
 }
 func TestAppRoot(t *testing.T) {
-	ing := buildIngress()
-
-	data := map[string]string{}
-	data[parser.GetAnnotationWithPrefix("app-root")] = "/app1"
-	ing.SetAnnotations(data)
-
-	i, _ := NewParser(mockBackend{redirect: true}).Parse(ing)
-	redirect, ok := i.(*Config)
-	if !ok {
-		t.Errorf("expected a App Context")
-	}
-	if redirect.AppRoot != "/app1" {
-		t.Errorf("Unexpected value got in AppRoot")
+	ap := NewParser(mockBackend{redirect: true})
+
+	testCases := []struct {
+		title       string
+		path        string
+		expected    string
+		errExpected bool
+	}{
+		{"Empty path should return an error", "", "", true},
+		{"Relative paths are not allowed", "demo", "", true},
+		{"Path / should pass", "/", "/", false},
+		{"Path /demo should pass", "/demo", "/demo", false},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.title, func(t *testing.T) {
+			ing := buildIngress()
+			ing.Annotations[parser.GetAnnotationWithPrefix("app-root")] = testCase.path
+			i, err := ap.Parse(ing)
+			if err != nil {
+				if testCase.errExpected {
+					return
+				}
+
+				t.Fatalf("%v: unexpected error obtaining running address/es: %v", testCase.title, err)
+			}
+
+			rewrite, ok := i.(*Config)
+			if !ok {
+				t.Fatalf("expected a rewrite Config")
+			}
+
+			if testCase.expected != rewrite.AppRoot {
+				t.Fatalf("%v: expected AppRoot with value %v but was returned: %v", testCase.title, testCase.expected, rewrite.AppRoot)
+			}
+		})
 	}
 }
 
diff --git a/test/e2e/annotations/approot.go b/test/e2e/annotations/approot.go
index a43d2e3ba..a73c60253 100644
--- a/test/e2e/annotations/approot.go
+++ b/test/e2e/annotations/approot.go
@@ -45,7 +45,7 @@ var _ = framework.DescribeAnnotation("app-root", func() {
 		f.WaitForNginxServer(host,
 			func(server string) bool {
 				return strings.Contains(server, `if ($uri = /) {`) &&
-					strings.Contains(server, `return 302 /foo;`)
+					strings.Contains(server, `return 302 $scheme://$http_host/foo;`)
 			})
 
 		f.HTTPTestClient().
diff --git a/test/e2e/framework/deployment.go b/test/e2e/framework/deployment.go
index 751147891..35a060cd0 100644
--- a/test/e2e/framework/deployment.go
+++ b/test/e2e/framework/deployment.go
@@ -368,9 +368,14 @@ func (f *Framework) NewDeployment(name, image string, port int32, replicas int32
 func (f *Framework) DeleteDeployment(name string) error {
 	d, err := f.KubeClientSet.AppsV1().Deployments(f.Namespace).Get(name, metav1.GetOptions{})
 	assert.Nil(ginkgo.GinkgoT(), err, "getting deployment")
-	err = f.KubeClientSet.AppsV1().Deployments(f.Namespace).Delete(name, &metav1.DeleteOptions{})
+
+	grace := int64(0)
+	err = f.KubeClientSet.AppsV1().Deployments(f.Namespace).Delete(name, &metav1.DeleteOptions{
+		GracePeriodSeconds: &grace,
+	})
 	assert.Nil(ginkgo.GinkgoT(), err, "deleting deployment")
-	return WaitForPodsDeleted(f.KubeClientSet, time.Second*60, f.Namespace, metav1.ListOptions{
+
+	return WaitForPodsDeleted(f.KubeClientSet, 2*time.Minute, f.Namespace, metav1.ListOptions{
 		LabelSelector: labelSelectorToString(d.Spec.Selector.MatchLabels),
 	})
 }