From b3206c53272546651bd3931d1082acba539c8138 Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Fri, 1 Nov 2019 15:37:05 +0000 Subject: [PATCH 1/3] Add health status uri cli arg --- cmd/nginx-ingress/main.go | 6 +++++- .../helm-chart/templates/controller-daemonset.yaml | 1 + .../helm-chart/templates/controller-deployment.yaml | 1 + deployments/helm-chart/values.yaml | 5 ++++- docs/cli-arguments.md | 8 +++++--- internal/configs/config_params.go | 1 + internal/configs/configmaps.go | 1 + internal/configs/configurator_test.go | 1 + internal/configs/version1/config.go | 1 + internal/configs/version1/nginx-plus.tmpl | 2 +- internal/configs/version1/nginx.tmpl | 2 +- 11 files changed, 22 insertions(+), 7 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index a2b31467e1..9c3c3507bb 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -40,9 +40,12 @@ var ( gitCommit string healthStatus = flag.Bool("health-status", false, - `Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. + `Add a location based on the value of health-status-uri to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller`) + healthStatusURI = flag.String("health-status-uri", "/nginx-health", + `Sets the location of health status uri in the default server. Requires -health-status`) + proxyURL = flag.String("proxy", "", `Use a proxy server to connect to Kubernetes API started by "kubectl proxy" command. For testing purposes only. The Ingress controller does not start NGINX and does not write any generated NGINX configuration files to disk`) @@ -320,6 +323,7 @@ func main() { staticCfgParams := &configs.StaticConfigParams{ HealthStatus: *healthStatus, + HealthStatusURI: *healthStatusURI, NginxStatus: *nginxStatus, NginxStatusAllowCIDRs: allowedCIDRs, NginxStatusPort: *nginxStatusPort, diff --git a/deployments/helm-chart/templates/controller-daemonset.yaml b/deployments/helm-chart/templates/controller-daemonset.yaml index e9f1b8e999..944ae4af85 100644 --- a/deployments/helm-chart/templates/controller-daemonset.yaml +++ b/deployments/helm-chart/templates/controller-daemonset.yaml @@ -91,6 +91,7 @@ spec: - -watch-namespace={{ .Values.controller.watchNamespace }} {{- end }} - -health-status={{ .Values.controller.healthStatus }} + - -health-status-uri={{ .Values.controller.healthStatusURI }} - -nginx-debug={{ .Values.controller.nginxDebug }} - -v={{ .Values.controller.logLevel }} - -nginx-status={{ .Values.controller.nginxStatus.enable }} diff --git a/deployments/helm-chart/templates/controller-deployment.yaml b/deployments/helm-chart/templates/controller-deployment.yaml index d5ec53be2d..9fcf4beb78 100644 --- a/deployments/helm-chart/templates/controller-deployment.yaml +++ b/deployments/helm-chart/templates/controller-deployment.yaml @@ -89,6 +89,7 @@ spec: - -watch-namespace={{ .Values.controller.watchNamespace }} {{- end }} - -health-status={{ .Values.controller.healthStatus }} + - -health-status-uri={{ .Values.controller.healthStatusURI }} - -nginx-debug={{ .Values.controller.nginxDebug }} - -v={{ .Values.controller.logLevel }} - -nginx-status={{ .Values.controller.nginxStatus.enable }} diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml index 20e8c645e2..2adbe39044 100644 --- a/deployments/helm-chart/values.yaml +++ b/deployments/helm-chart/values.yaml @@ -106,10 +106,13 @@ controller: ## Enable the custom resources. enableCustomResources: false - ## Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. + ## Add a location based on the value of health-status-uri to the default server. The location responds with the 200 status code for any request. ## Useful for external health-checking of the Ingress controller. healthStatus: false + ## Set the name of the location for healthStatus. Requires -health-status. + healthStatusURI: "/nginx-health" + nginxStatus: ## Enable the NGINX stub_status, or the NGINX Plus API. enable: true diff --git a/docs/cli-arguments.md b/docs/cli-arguments.md index 2339ae2e55..a62168f035 100644 --- a/docs/cli-arguments.md +++ b/docs/cli-arguments.md @@ -11,7 +11,7 @@ Usage of ./nginx-ingress: the file "/etc/nginx/secrets/default" does not exist, the Ingress controller will fail to start -wildcard-tls-secret string A Secret with a TLS certificate and key for TLS termination of every Ingress host for which TLS termination is enabled but the Secret is not specified. - Format: /. If the argument is not set, for such Ingress hosts NGINX will break any attempt to establish a TLS connection. + Format: /. If the argument is not set, for such Ingress hosts NGINX will break any attempt to establish a TLS connection. If the argument is set, but the Ingress controller is not able to fetch the Secret from Kubernetes API, the Ingress controller will fail to start. -enable-custom-resources Enable custom resources @@ -21,8 +21,10 @@ Usage of ./nginx-ingress: Specifies the name of the service with the type LoadBalancer through which the Ingress controller pods are exposed externally. The external address of the service is used when reporting the status of Ingress resources. Requires -report-ingress-status. -health-status - Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. - Useful for external health-checking of the Ingress controller + Add a location based on the value of health-status-uri to the default server. The location responds with the 200 status code for any request. + Useful for external health-checking of the Ingress controller + -health-status-uri string + Sets the location of health status uri in the default server. Requires -health-status (default "/nginx-health") -ingress-class string A class of the Ingress controller. The Ingress controller only processes Ingress resources that belong to its class - i.e. have the annotation "kubernetes.io/ingress.class" equal to the class. Additionally, diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index 84d81e2eea..16917ffb6e 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -86,6 +86,7 @@ type ConfigParams struct { // StaticConfigParams holds immutable NGINX configuration parameters that affect the main NGINX config. type StaticConfigParams struct { HealthStatus bool + HealthStatusURI string NginxStatus bool NginxStatusAllowCIDRs []string NginxStatusPort int diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index 920a78ef1f..b58171af75 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -430,6 +430,7 @@ func ParseConfigMap(cfgm *v1.ConfigMap, nginxPlus bool) *ConfigParams { func GenerateNginxMainConfig(staticCfgParams *StaticConfigParams, config *ConfigParams) *version1.MainConfig { nginxCfg := &version1.MainConfig{ HealthStatus: staticCfgParams.HealthStatus, + HealthStatusURI: staticCfgParams.HealthStatusURI, NginxStatus: staticCfgParams.NginxStatus, NginxStatusAllowCIDRs: staticCfgParams.NginxStatusAllowCIDRs, NginxStatusPort: staticCfgParams.NginxStatusPort, diff --git a/internal/configs/configurator_test.go b/internal/configs/configurator_test.go index d28ecb7702..2826cd809c 100644 --- a/internal/configs/configurator_test.go +++ b/internal/configs/configurator_test.go @@ -13,6 +13,7 @@ import ( func createTestStaticConfigParams() *StaticConfigParams { return &StaticConfigParams{ HealthStatus: true, + HealthStatusURI: "/nginx-health", NginxStatus: true, NginxStatusAllowCIDRs: []string{"127.0.0.1"}, NginxStatusPort: 8080, diff --git a/internal/configs/version1/config.go b/internal/configs/version1/config.go index 94306bde39..48ef3cdb6e 100644 --- a/internal/configs/version1/config.go +++ b/internal/configs/version1/config.go @@ -131,6 +131,7 @@ type MainConfig struct { ErrorLogLevel string StreamLogFormat string HealthStatus bool + HealthStatusURI string NginxStatus bool NginxStatusAllowCIDRs []string NginxStatusPort int diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index 3b6014107a..a89916cdc7 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -106,7 +106,7 @@ http { {{end}} {{if .HealthStatus}} - location /nginx-health { + location {{.HealthStatusURI}} { default_type text/plain; return 200 "healthy\n"; } diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index b9f8759cca..b51e69573b 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -100,7 +100,7 @@ http { {{end}} {{if .HealthStatus}} - location /nginx-health { + location {{.HealthStatusURI}} { default_type text/plain; return 200 "healthy\n"; } From a414d535cd451a513562cae05259b35d40abe314 Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Mon, 4 Nov 2019 12:11:47 +0000 Subject: [PATCH 2/3] Update docs and Add location validation --- cmd/nginx-ingress/main.go | 24 +++++++++++++++++++++++- deployments/helm-chart/values.yaml | 2 +- docs/cli-arguments.md | 2 +- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 9c3c3507bb..ac1b69e957 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "os/signal" + "regexp" "strings" "syscall" "time" @@ -44,7 +45,7 @@ var ( Useful for external health-checking of the Ingress controller`) healthStatusURI = flag.String("health-status-uri", "/nginx-health", - `Sets the location of health status uri in the default server. Requires -health-status`) + `Sets the URI of health status location in the default server. Requires -health-status`) proxyURL = flag.String("proxy", "", `Use a proxy server to connect to Kubernetes API started by "kubectl proxy" command. For testing purposes only. @@ -140,6 +141,11 @@ func main() { os.Exit(0) } + healthStatusURIValidationError := validateLocation(*healthStatusURI) + if healthStatusURIValidationError != nil { + glog.Fatalf("Invalid value for health-status-uri: %v", healthStatusURIValidationError) + } + statusLockNameValidationError := validateResourceName(*leaderElectionLockName) if statusLockNameValidationError != nil { glog.Fatalf("Invalid value for leader-election-lock-name: %v", statusLockNameValidationError) @@ -519,3 +525,19 @@ func getAndValidateSecret(kubeClient *kubernetes.Clientset, secretNsName string) } return secret, nil } + +const locationFmt = `/[^\s{};]*` +const locationErrMsg = "must start with / and must not include any whitespace character, `{`, `}` or `;`" + +var locationRegexp = regexp.MustCompile("^" + locationFmt + "$") + +func validateLocation(location string) error { + if location == "" { + return fmt.Errorf("invalid location: an empty string is an invalid location") + } + if !locationRegexp.MatchString(location) { + msg := validation.RegexError(locationErrMsg, locationFmt, "/", "/path", "/path/subpath-123") + return fmt.Errorf("invalid location format: %v", msg) + } + return nil +} diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml index 2adbe39044..87bd663cfb 100644 --- a/deployments/helm-chart/values.yaml +++ b/deployments/helm-chart/values.yaml @@ -110,7 +110,7 @@ controller: ## Useful for external health-checking of the Ingress controller. healthStatus: false - ## Set the name of the location for healthStatus. Requires -health-status. + ## Sets the URI of health status location in the default server. Requires -health-status (default "/nginx-health"). healthStatusURI: "/nginx-health" nginxStatus: diff --git a/docs/cli-arguments.md b/docs/cli-arguments.md index a62168f035..00c3c3abb7 100644 --- a/docs/cli-arguments.md +++ b/docs/cli-arguments.md @@ -24,7 +24,7 @@ Usage of ./nginx-ingress: Add a location based on the value of health-status-uri to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller -health-status-uri string - Sets the location of health status uri in the default server. Requires -health-status (default "/nginx-health") + Sets the URI of health status location in the default server. Requires -health-status (default "/nginx-health") -ingress-class string A class of the Ingress controller. The Ingress controller only processes Ingress resources that belong to its class - i.e. have the annotation "kubernetes.io/ingress.class" equal to the class. Additionally, From dd3eeb1afaf00f8e430303cd2f90b37b0b211c9a Mon Sep 17 00:00:00 2001 From: Dean Coakley Date: Tue, 5 Nov 2019 09:52:26 +0000 Subject: [PATCH 3/3] Fix loc validation. Add unit test. Add docs --- cmd/nginx-ingress/main.go | 6 +++--- cmd/nginx-ingress/main_test.go | 26 ++++++++++++++++++++++++++ deployments/helm-chart/README.md | 1 + deployments/helm-chart/values.yaml | 2 +- 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index ac1b69e957..97638c10d4 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -532,11 +532,11 @@ const locationErrMsg = "must start with / and must not include any whitespace ch var locationRegexp = regexp.MustCompile("^" + locationFmt + "$") func validateLocation(location string) error { - if location == "" { - return fmt.Errorf("invalid location: an empty string is an invalid location") + if location == "" || location == "/" { + return fmt.Errorf("invalid location format: '%v' is an invalid location", location) } if !locationRegexp.MatchString(location) { - msg := validation.RegexError(locationErrMsg, locationFmt, "/", "/path", "/path/subpath-123") + msg := validation.RegexError(locationErrMsg, locationFmt, "/path", "/path/subpath-123") return fmt.Errorf("invalid location format: %v", msg) } return nil diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index bd1933d7da..a93abcad87 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -98,3 +98,29 @@ func TestValidateCIDRorIP(t *testing.T) { } } } + +func TestValidateLocation(t *testing.T) { + badLocations := []string{ + "", + "/", + " /test", + "/bad;", + } + for _, badLocation := range badLocations { + err := validateLocation(badLocation) + if err == nil { + t.Errorf("validateLocation(%v) returned no error when it should have returned an error", badLocation) + } + } + + goodLocations := []string{ + "/test", + "/test/subtest", + } + for _, goodLocation := range goodLocations { + err := validateLocation(goodLocation) + if err != nil { + t.Errorf("validateLocation(%v) returned an error when it should have returned no error: %v", goodLocation, err) + } + } +} diff --git a/deployments/helm-chart/README.md b/deployments/helm-chart/README.md index c36edf7eb2..79b405a165 100644 --- a/deployments/helm-chart/README.md +++ b/deployments/helm-chart/README.md @@ -86,6 +86,7 @@ Parameter | Description | Default `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | "" `controller.enableCustomResources` | Enable the custom resources. | false `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false +`controller.healthStatusURI` | Sets the URI of health status location in the default server. Requires `contoller.healthStatus`. | "/nginx-health" `controller.nginxStatus.enable` | Enable the NGINX stub_status, or the NGINX Plus API. | true `controller.nginxStatus.port` | Set the port where the NGINX stub_status or the NGINX Plus API is exposed. | 8080 `controller.nginxStatus.allowCidrs` | Whitelist IPv4 IP/CIDR blocks to allow access to NGINX stub_status or the NGINX Plus API. Separate multiple IP/CIDR by commas. | 127.0.0.1 diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml index 87bd663cfb..68006080e2 100644 --- a/deployments/helm-chart/values.yaml +++ b/deployments/helm-chart/values.yaml @@ -110,7 +110,7 @@ controller: ## Useful for external health-checking of the Ingress controller. healthStatus: false - ## Sets the URI of health status location in the default server. Requires -health-status (default "/nginx-health"). + ## Sets the URI of health status location in the default server. Requires contoller.healthStatus. healthStatusURI: "/nginx-health" nginxStatus: