From c2ed0e6eb215374da10a482c74d2fbd0b0d7e84f Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Sun, 6 Nov 2022 23:04:45 +0000 Subject: [PATCH 01/66] poc for healthcheck inside NIC --- cmd/nginx-ingress/main.go | 11 ++++++++ internal/configs/configurator.go | 37 ++++++++++++++++++++++++++ internal/configs/virtualserver.go | 14 +++++----- internal/configs/virtualserver_test.go | 12 ++++----- 4 files changed, 61 insertions(+), 13 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 3c2bd743d6..0908dd7a96 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -16,6 +16,7 @@ import ( "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" "github.com/nginxinc/kubernetes-ingress/internal/configs/version2" + "github.com/nginxinc/kubernetes-ingress/internal/healthcheck" "github.com/nginxinc/kubernetes-ingress/internal/k8s" "github.com/nginxinc/kubernetes-ingress/internal/k8s/secrets" "github.com/nginxinc/kubernetes-ingress/internal/metrics" @@ -122,6 +123,8 @@ func main() { transportServerValidator := cr_validation.NewTransportServerValidator(*enableTLSPassthrough, *enableSnippets, *nginxPlus) virtualServerValidator := cr_validation.NewVirtualServerValidator(cr_validation.IsPlus(*nginxPlus), cr_validation.IsDosEnabled(*appProtectDos), cr_validation.IsCertManagerEnabled(*enableCertManager), cr_validation.IsExternalDNSEnabled(*enableExternalDNS)) + createHealthEndpoint(plusClient, cnf) + lbcInput := k8s.NewLoadBalancerControllerInput{ KubeClient: kubeClient, ConfClient: confClient, @@ -656,6 +659,14 @@ func createPlusAndLatencyCollectors( return plusCollector, syslogListener, lc } +func createHealthEndpoint( + plusClient *client.NginxClient, + cnf *configs.Configurator, +) { + glog.Infof("Starting Health Endpoint") + go healthcheck.RunHealthCheck(plusClient, cnf) +} + func processGlobalConfiguration() { if *globalConfiguration != "" { _, _, err := k8s.ParseNamespaceName(*globalConfiguration) diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 237f199004..aeb05921f1 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -265,6 +265,43 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error) return warnings, nil } +func (cnf *Configurator) GetVirtualServer(hostname string) *conf_v1.VirtualServer { + for _, vsEx := range cnf.virtualServers { + if vsEx.VirtualServer.Spec.Host == hostname { + return vsEx.VirtualServer + } + } + return nil +} + +func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) []string { + + glog.Infof("Get upstreamName for vs: %s", vs.Spec.Host) + + var upstreamNames = make([]string, 0) + + virtualServerUpstreamNamer := NewUpstreamNamerForVirtualServer(vs) + + for _, u := range vs.Spec.Upstreams { + upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(u.Name) + glog.Infof("upstream: %s, upstreamName: %s", u.Name, upstreamName) + upstreamNames = append(upstreamNames, upstreamName) + } + + return upstreamNames +} + +func (cnf *Configurator) GetUpstreamsforHost(hostname string) []string { + + glog.Infof("Get upstream for host: %s", hostname) + var vs = cnf.GetVirtualServer(hostname) + + if vs != nil { + return cnf.GetUpstreamsforVirtualServer(vs) + } + return nil +} + func (cnf *Configurator) addOrUpdateIngress(ingEx *IngressEx) (Warnings, error) { apResources := cnf.updateApResources(ingEx) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 8390f54162..b693ac065f 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -132,14 +132,14 @@ type upstreamNamer struct { namespace string } -func newUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *upstreamNamer { +func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf("vs_%s_%s", virtualServer.Namespace, virtualServer.Name), namespace: virtualServer.Namespace, } } -func newUpstreamNamerForVirtualServerRoute( +func NewUpstreamNamerForVirtualServerRoute( virtualServer *conf_v1.VirtualServer, virtualServerRoute *conf_v1.VirtualServerRoute, ) *upstreamNamer { @@ -334,7 +334,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( // necessary for generateLocation to know what Upstream each Location references crUpstreams := make(map[string]conf_v1.Upstream) - virtualServerUpstreamNamer := newUpstreamNamerForVirtualServer(vsEx.VirtualServer) + virtualServerUpstreamNamer := NewUpstreamNamerForVirtualServer(vsEx.VirtualServer) var upstreams []version2.Upstream var statusMatches []version2.StatusMatch var healthChecks []version2.HealthCheck @@ -373,7 +373,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } // generate upstreams for each VirtualServerRoute for _, vsr := range vsEx.VirtualServerRoutes { - upstreamNamer := newUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) + upstreamNamer := NewUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) for _, u := range vsr.Spec.Upstreams { if (sslConfig == nil || !vsc.cfgParams.HTTP2) && isGRPC(u.Type) { vsc.addWarningf(vsr, "gRPC cannot be configured for upstream %s. gRPC requires enabled HTTP/2 and TLS termination", u.Name) @@ -523,7 +523,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( // generate config for subroutes of each VirtualServerRoute for _, vsr := range vsEx.VirtualServerRoutes { isVSR := true - upstreamNamer := newUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) + upstreamNamer := NewUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr) for _, r := range vsr.Spec.Subroutes { errorPages := errorPageDetails{ pages: r.ErrorPages, @@ -2252,7 +2252,7 @@ func createUpstreamsForPlus( var upstreams []version2.Upstream isPlus := true - upstreamNamer := newUpstreamNamerForVirtualServer(virtualServerEx.VirtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(virtualServerEx.VirtualServer) vsc := newVirtualServerConfigurator(baseCfgParams, isPlus, false, staticParams, false) for _, u := range virtualServerEx.VirtualServer.Spec.Upstreams { @@ -2273,7 +2273,7 @@ func createUpstreamsForPlus( } for _, vsr := range virtualServerEx.VirtualServerRoutes { - upstreamNamer = newUpstreamNamerForVirtualServerRoute(virtualServerEx.VirtualServer, vsr) + upstreamNamer = NewUpstreamNamerForVirtualServerRoute(virtualServerEx.VirtualServer, vsr) for _, u := range vsr.Spec.Upstreams { isExternalNameSvc := virtualServerEx.ExternalNameSvcs[GenerateExternalNameSvcKey(vsr.Namespace, u.Service)] if isExternalNameSvc { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 413417e3f2..4d193abd75 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -96,7 +96,7 @@ func TestUpstreamNamerForVirtualServer(t *testing.T) { Namespace: "default", }, } - upstreamNamer := newUpstreamNamerForVirtualServer(&virtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(&virtualServer) upstream := "test" expected := "vs_default_cafe_test" @@ -121,7 +121,7 @@ func TestUpstreamNamerForVirtualServerRoute(t *testing.T) { Namespace: "default", }, } - upstreamNamer := newUpstreamNamerForVirtualServerRoute(&virtualServer, &virtualServerRoute) + upstreamNamer := NewUpstreamNamerForVirtualServerRoute(&virtualServer, &virtualServerRoute) upstream := "test" expected := "vs_default_cafe_vsr_default_coffee_test" @@ -5675,7 +5675,7 @@ func TestGenerateSplits(t *testing.T) { Namespace: "default", }, } - upstreamNamer := newUpstreamNamerForVirtualServer(&virtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(&virtualServer) variableNamer := newVariableNamer(&virtualServer) scIndex := 1 cfgParams := ConfigParams{} @@ -5886,7 +5886,7 @@ func TestGenerateDefaultSplitsConfig(t *testing.T) { Namespace: "default", }, } - upstreamNamer := newUpstreamNamerForVirtualServer(&virtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(&virtualServer) variableNamer := newVariableNamer(&virtualServer) index := 1 @@ -6072,7 +6072,7 @@ func TestGenerateMatchesConfig(t *testing.T) { }, }, } - upstreamNamer := newUpstreamNamerForVirtualServer(&virtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(&virtualServer) variableNamer := newVariableNamer(&virtualServer) index := 1 scIndex := 2 @@ -6454,7 +6454,7 @@ func TestGenerateMatchesConfigWithMultipleSplits(t *testing.T) { Namespace: "default", }, } - upstreamNamer := newUpstreamNamerForVirtualServer(&virtualServer) + upstreamNamer := NewUpstreamNamerForVirtualServer(&virtualServer) variableNamer := newVariableNamer(&virtualServer) index := 1 scIndex := 2 From b759a840886d7b8ee924612532d120e22b7b3d1b Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Wed, 9 Nov 2022 12:29:33 +0000 Subject: [PATCH 02/66] add listener.go --- internal/healthcheck/listener.go | 63 ++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 internal/healthcheck/listener.go diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go new file mode 100644 index 0000000000..a63edbbeb2 --- /dev/null +++ b/internal/healthcheck/listener.go @@ -0,0 +1,63 @@ +package healthcheck + +import ( + "fmt" + "github.com/nginxinc/kubernetes-ingress/internal/configs" + "github.com/nginxinc/nginx-plus-go-client/client" + "k8s.io/utils/strings/slices" + "path" + + "net/http" + "strconv" + "strings" + + "github.com/golang/glog" +) + +const healthEndpoint = "/healthcheck" + +func RunHealthCheck(plusClient *client.NginxClient, + cnf *configs.Configurator) { + glog.Infof("Running Health Endpoint at port 9000") + runServer(strconv.Itoa(9000), plusClient, cnf) +} + +func runServer(port string, plusClient *client.NginxClient, cnf *configs.Configurator) { + http.HandleFunc("/healthcheck/", func(w http.ResponseWriter, r *http.Request) { + var hostname = path.Base(r.URL.Path) + glog.Infof("Path: %s", hostname) + + var upstreamNames = cnf.GetUpstreamsforHost(hostname) + glog.Infof("Upstream Names: %s", upstreamNames) + + upstreams, err := plusClient.GetUpstreams() + var states []string + + for name, u := range *upstreams { + if slices.Contains(upstreamNames, name) { + for _, p := range u.Peers { + glog.Infof("Peer ID: %s, Name: %s, State: %s ", p.ID, p.Name, p.State) + states = append(states, "IP: "+p.Name+", State: "+p.State) + } + } + } + + w.WriteHeader(http.StatusCreated) + + resp := ` + NGINX Ingress Controller + +

NGINX Ingress Controller

Hostname: ` + hostname + + `

upstreams: ` + strings.Join(upstreamNames[:], ", ") + + `

states: ` + strings.Join(states[:], ", ") + + `

` + _, err = w.Write([]byte(resp)) + if err != nil { + glog.Warningf("Error while sending a response for the '/' path: %v", err) + } + }) + address := fmt.Sprintf(":%v", port) + glog.Infof("Starting Healthcheck listener on: %v%v", address, healthEndpoint) + glog.Fatal("Error in Healthcheck listener server: ", http.ListenAndServe(address, nil)) + +} From ea63d10d4e0dc4bede12a9f5f0ba6635b73238fd Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 9 Nov 2022 14:13:48 +0000 Subject: [PATCH 03/66] Add changes discussed in the meeting --- cmd/nginx-ingress/main.go | 5 +---- internal/configs/configurator.go | 4 ++-- internal/healthcheck/listener.go | 11 ++++++++--- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 0908dd7a96..8b1425d470 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -659,10 +659,7 @@ func createPlusAndLatencyCollectors( return plusCollector, syslogListener, lc } -func createHealthEndpoint( - plusClient *client.NginxClient, - cnf *configs.Configurator, -) { +func createHealthEndpoint(plusClient *client.NginxClient, cnf *configs.Configurator) { glog.Infof("Starting Health Endpoint") go healthcheck.RunHealthCheck(plusClient, cnf) } diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index aeb05921f1..76bb9a2421 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -278,7 +278,7 @@ func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) glog.Infof("Get upstreamName for vs: %s", vs.Spec.Host) - var upstreamNames = make([]string, 0) + upstreamNames := make([]string, 0, len(vs.Spec.Upstreams)) virtualServerUpstreamNamer := NewUpstreamNamerForVirtualServer(vs) @@ -294,7 +294,7 @@ func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) func (cnf *Configurator) GetUpstreamsforHost(hostname string) []string { glog.Infof("Get upstream for host: %s", hostname) - var vs = cnf.GetVirtualServer(hostname) + vs := cnf.GetVirtualServer(hostname) if vs != nil { return cnf.GetUpstreamsforVirtualServer(vs) diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go index a63edbbeb2..0113b418bd 100644 --- a/internal/healthcheck/listener.go +++ b/internal/healthcheck/listener.go @@ -2,10 +2,11 @@ package healthcheck import ( "fmt" + "path" + "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" "k8s.io/utils/strings/slices" - "path" "net/http" "strconv" @@ -24,13 +25,17 @@ func RunHealthCheck(plusClient *client.NginxClient, func runServer(port string, plusClient *client.NginxClient, cnf *configs.Configurator) { http.HandleFunc("/healthcheck/", func(w http.ResponseWriter, r *http.Request) { - var hostname = path.Base(r.URL.Path) + hostname := path.Base(r.URL.Path) glog.Infof("Path: %s", hostname) - var upstreamNames = cnf.GetUpstreamsforHost(hostname) + upstreamNames := cnf.GetUpstreamsforHost(hostname) glog.Infof("Upstream Names: %s", upstreamNames) upstreams, err := plusClient.GetUpstreams() + if err != nil { + // handle error + http.Error(w, err.Error(), http.StatusInternalServerError) + } var states []string for name, u := range *upstreams { From 3405d0afb1a3d4052a71a986d9b00d6b028aa291 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 9 Nov 2022 15:19:49 +0000 Subject: [PATCH 04/66] Add initial implementation of the server --- internal/healthcheck/http.go | 38 +++++++++ internal/healthcheck/http_test.go | 125 ++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+) create mode 100644 internal/healthcheck/http.go create mode 100644 internal/healthcheck/http_test.go diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go new file mode 100644 index 0000000000..e919e03507 --- /dev/null +++ b/internal/healthcheck/http.go @@ -0,0 +1,38 @@ +package healthcheck + +import ( + "crypto/tls" + "net/http" + "time" +) + +// ListenAndServeTLS is a drop-in replacement for a default +// ListenAndServeTLS func from the http std library. The func +// takes not paths to a cert and a key but slices of bytes representing +// content of the files used by the original http.ListenAndServeTLS function. +func ListenAndServeTLS(addr string, cert, key []byte, handler http.Handler) error { + tlsCert, err := LoadX509KeyPair(cert, key) + if err != nil { + return err + } + + server := &http.Server{ + Addr: addr, + Handler: handler, + TLSConfig: &tls.Config{ + Certificates: []tls.Certificate{tlsCert}, + }, + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + + return server.ListenAndServeTLS("", "") +} + +// LoadX509KeyPair is a drop-in replacement for the LoadX509KeyPair +// from the http package from the std library. It takes not files +// containing a cert and a key but a slice of bytes representing +// the content of the corresponding files. +func LoadX509KeyPair(cert, key []byte) (tls.Certificate, error) { + return tls.X509KeyPair(cert, key) +} diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/http_test.go new file mode 100644 index 0000000000..36ddc75a27 --- /dev/null +++ b/internal/healthcheck/http_test.go @@ -0,0 +1,125 @@ +package healthcheck_test + +import ( + "testing" + + "github.com/nginxinc/kubernetes-ingress/internal/healthcheck" +) + +func TestLoadsKeyPairOnValidInut(t *testing.T) { + t.Parallel() + _, err := healthcheck.LoadX509KeyPair(validCert, validKey) + if err != nil { + t.Fatal(err) + } +} + +func TestDoesNotLoadCertsOnInvalidInput(t *testing.T) { + t.Parallel() + _, err := healthcheck.LoadX509KeyPair(invalidCert, invalidKey) + if err == nil { + t.Fatal("want err on invalid cert/key, got nil") + } +} + +var ( + validCert = []byte(`-----BEGIN CERTIFICATE----- +MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJJ +RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD +DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu +aW8wHhcNMjIxMTA1MTQzNDE1WhcNMzIxMTAyMTQzNDE1WjBuMQswCQYDVQQGEwJJ +RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD +DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu +aW8wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDVz06I4rTqOSI4bnEJ +GVy17QytuCiCZm0iPmjw2EJrc21FTk23zscHOMUAOc2HodeUmYyBjo+ZnPl2Tk9i +dyWU3wou3ZIQQaOi87meJ/evltUHiC49olNsYe9U8bB31/6URFKaMH7rD7zfAXpS +DbWdd84g7hfZIMQSLRPdBz958lkVPaSfPua58LkKZgmkThvh5Ah0HNKPn0z9idTQ +5oftFlPYTHvXvFYPoVNOjYfVbqxnmDJrbuqy0tkVjFoYHrT4aNkFIS/CgFjpYwb4 +j8yuprFNCAGjS7hDUDQaeHNKqTWvk+QT28pLNXc1BfA88DTMb0G6glZi67sDeN9H +q4K7AgMBAAEwDQYJKoZIhvcNAQELBQADggEBACAuyHRQodEaql4QXb5mGFSuQuAv +QxHdSSkdelDFf8s3ThBWgahuw9Lwz7FwXuFSh8tirK/3fb+OFwWB/xQdHjL6hl13 +ccxLNY/ydrKeHraLCWLu5TZ5BIvAHfFTpf5sbQrBkf4G4+Y3rX55asBTrzO9sPTT +bOjDHn+Eaa6QdEoyOvpRY+zGB1++XJqn/xFYjVrNg6Neh8/cfILSV46HNaqp3FSr +0NOWiGxG3Qk3UGoVQifhFNO5SsoYNfnDnwWx2cW4KTklxak0wt3KaloYbQUv2GKw +MsyNKpmyUpwnLnY0glII4IvP59oAEZR/wEI0bpM5ddWZuDu4Ie5WNJ+TG3g= +-----END CERTIFICATE----- +`) + + validKey = []byte(`-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEA1c9OiOK06jkiOG5xCRlcte0MrbgogmZtIj5o8NhCa3NtRU5N +t87HBzjFADnNh6HXlJmMgY6PmZz5dk5PYncllN8KLt2SEEGjovO5nif3r5bVB4gu +PaJTbGHvVPGwd9f+lERSmjB+6w+83wF6Ug21nXfOIO4X2SDEEi0T3Qc/efJZFT2k +nz7mufC5CmYJpE4b4eQIdBzSj59M/YnU0OaH7RZT2Ex717xWD6FTTo2H1W6sZ5gy +a27qstLZFYxaGB60+GjZBSEvwoBY6WMG+I/MrqaxTQgBo0u4Q1A0GnhzSqk1r5Pk +E9vKSzV3NQXwPPA0zG9BuoJWYuu7A3jfR6uCuwIDAQABAoIBADbyIZKYADo5GIw8 +BZx7AhJWqu1x6Ccqv10PgNR0Hw2SCkDHUL2tzAQVGLtoH2N9ufMcSrl4s3qclpdK +pKf/So8pimpk0oaO98iGrerxBnv/XRukaY25S4sM1/6SZfFGdswPitLJJ7SsxLLi +pFa14zhmc3iO9137R6gMIZCprixeHSiUrxe8f0L7m5KkyuyUN3+ItIT70+btTKlO +bkEAjKvahIWqbq5QDmsn+v0g2QEu5Y7QCxRuXIgPEj2/CDt6c1jodN/uuDIWBuSb +V2oeI9q79J5ccEC+Z5UlRx1+VBiaepRi1k9Dy6h3mZSCrUBeAreW1hriuVAKiP3I +o/G2wsECgYEA8jcEY4XVoL/vtrDzdW53usjR0v6R6RWRb+1/9Zm8XoZwu7onza3p +6P//qxp56eOKfDldpofYAbxov+kBzaLSQclGHWGZ5ZUEu8eJpl6TcJsZLOd85Hrk +AA4kVfoeQ99UTbMfghtZkdFydmkL/5ADFBa4y9Ds5hIOYQ5BLNadvasCgYEA4fpv ++sb43mFxTI1tJNVkAznW5bGbrfEVQcy3SAN4325hffiGE8emB9ckUJqHGvMzN6nX +iN9H+frzOqHWECThN0Zs2hgHNjkP3FMYCCUqJI1mT45qZNqZ3/pdQ5cqKL9ZW2cL +6sUheSIB9hBXquQk7RDjwvNj8bfFiEhtsSEHnzECgYBb7yv4RnUmVZO76QAPY4WI +XO7fQgbJzIjuTdwSsW6BBlBFwMuY0tkEuh4lqJ/7eYU3z2JPciI3znaH2P35OkLJ ++4ZkYoZSULSCPaNuhVk7FXOByr9pzYc6yiNaitvv8RWDhGiCLrVZloD2lrqaHuQ8 +PL+ZhMxWKyZQCmQMi81FjwKBgGHNTeGvc85rRenn27DxWhO7WLKYp9QkXxrXSwuz +1QB+eVtX0E+HPOhvyJvKBWc4kpYov8vRNwmN/u8FU+wwyfhuVnYdqCFjmOW2YNRF +oXOobvtHm+yCX858QRkbt3djOX1Bn/q/zrjqawbgE9E2ZHTltm2NgVgAPVG6Zx8e +OHpBAoGAfSDXJUddJOO41Myc/tmuTQcIpfVl/7xEahB6awjj2YUZVgV6366JZvvT +EAvtOoAzeD++O2fxQCXbxnlAXgn8hAgyOnW45Pmi6MuCr3uUJTxfNi8VEnH3RO7m +XVv4gl18+nrhTQT7M7iyLi2maa9FCjMwgLMjhYQr4Gs1kPKyhXA= +-----END RSA PRIVATE KEY----- +`) + + invalidCert = []byte(`-----BEGIN CERTIFICATE----- +MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJA +RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD +DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu +aW8wHhcNMjIxMTA1MTQzNDE1WhcNMzIxMTAyMTQzNDE1WjBuMQswCQYDVQQGEwJJ +RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD +DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu +aW8wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDVz06I4rTqOSI4bnEJ +GVy17QytuCiCZm0iPmjw2EJrc21FTk23zscHOMUAOc2HodeUmYyBjo+ZnPl2Tk9i +dyWU3wou3ZIQQaOi87meJ/evltUHiC49olNsYe9U8bB31/6URFKaMH7rD7zfAXpS +DbWdd84g7hfZIMQSLRPdBz958lkVPaSfPua58LkKZgmkThvh5Ah0HNKPn0z9idTQ +5oftFlPYTHvXvFYPoVNOjYfVbqxnmDJrbuqy0tkVjFoYHrT4aNkFIS/CgFjpYwb4 +j8yuprFNCAGjS7hDUDQaeHNKqTWvk+QT28pLNXc1BfA88DTMb0G6glZi67sDeN9H +q4K7AgMBAAEwDQYJKoZIhvcNAQELBQADggEBACAuyHRQodEaql4QXb5mGFSuQuAv +QxHdSSkdelDFf8s3ThBWgahuw9Lwz7FwXuFSh8tirK/3fb+OFwWB/xQdHjL6hl13 +ccxLNY/ydrKeHraLCWLu5TZ5BIvAHfFTpf5sbQrBkf4G4+Y3rX55asBTrzO9sPTT +bOjDHn+Eaa6QdEoyOvpRY+zGB1++XJqn/xFYjVrNg6Neh8/cfILSV46HNaqp3FSr +0NOWiGxG3Qk3UGoVQifhFNO5SsoYNfnDnwWx2cW4KTklxak0wt3KaloYbQUv2GKw +MsyNKpmyUpwnLnY0glII4IvP59oAEZR/wEI0bpM5ddWZuDu4Ie5WNJ+TG3g= +-----END CERTIFICATE-----`) + + invalidKey = []byte(`-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEA1c9OiOK06jkiOG5xCRlcte0MrbgogmZtIj5o8NhCa3NtRU5N +t87HBzjFADnNh6HXlJmMgY6PmZz5dk5PYncllN8KLt2SEEGjovO5nif3r5bVB4gu +PaJTbGHvVPGwd9f+lERSmjB+6w+83wF6Ug21nXfOIO4X2SDEEi0T3Qc/efJZFT2k +nz7mufC5CmYJpE4b4eQIdBzSj59M/YnU0OaH7RZT2Ex717xWD6FTTo2H1W6sZ5gy +a27qstLZFYxaGB60+GjZBSEvwoBY6WMG+I/MrqaxTQgBo0u4Q1A0GnhzSqk1r5Pk +E9vKSzV3NQXwPPA0zG9BuoJWYuu7A3jfR6uCuwIDAQABAoIBADbyIZKYADo5GIw8 +BZx7AhJWqu1x6Ccqv10PgNR0Hw2SCkDHUL2tzAQVGLtoH2N9ufMcSrl4s3qclpdK +pKf/So8pimpk0oaO98iGrerxBnv/XRukaY25S4sM1/6SZfFGdswPitLJJ7SsxLLi +pFa14zhmc3iO9137R6gMIZCprixeHSiUrxe8f0L7m5KkyuyUN3+ItIT70+btTKlO +bkEAjKvahIWqbq5QDmsn+v0g2QEu5Y7QCxRuXIgPEj2/CDt6c1jodN/uuDIWBuSb +V2oeI9q79J5ccEC+Z5UlRx1+VBiaepRi1k9Dy6h3mZSCrUBeAreW1hriuVAKiP3I +o/G2wsECgYEA8jcEY4XVoL/vtrDzdW53usjR0v6R6RWRb+1/9Zm8XoZwu7onza3p +6P//qxp56eOKfDldpofYAbxov+kBzaLSQclGHWGZ5ZUEu8eJpl6TcJsZLOd85Hrk +AA4kVfoeQ99UTbMfghtZkdFydmkL/5ADFBa4y9Ds5hIOYQ5BLNadvasCgYEA4fpv ++sb43mFxTI1tJNVkAznW5bGbrfEVQcy3SAN4325hffiGE8emB9ckUJqHGvMzN6nX +iN9H+frzOqHWECThN0Zs2hgHNjkP3FMYCCUqJI1mT45qZNqZ3/pdQ5cqKL9ZW2cL +6sUheSIB9hBXquQk7RDjwvNj8bfFiEhtsSEHnzECgYBb7yv4RnUmVZO76QAPY4WI +XO7fQgbJzIjuTdwSsW6BBlBFwMuY0tkEuh4lqJ/7eYU3z2JPciI3znaH2P35OkLJ ++4ZkYoZSULSCPaNuhVk7FXOByr9pzYc6yiNaitvv8RWDhGiCLrVZloD2lrqaHuQ8 +PL+ZhMxWKyZQCmQMi81FjwKBgGHNTeGvc85rRenn27DxWhO7WLKYp9QkXxrXSwuz +1QB+eVtX0E+HPOhvyJvKBWc4kpYov8vRNwmN/u8FU+wwyfhuVnYdqCFjmOW2YNRF +oXOobvtHm+yCX858QRkbt3djOX1Bn/q/zrjqawbgE9E2ZHTltm2NgVgAPVG6Zx8e +OHpBAoGAfSDXJUddJOO41Myc/tmuTQcIpfVl/7xEahB6awjj2YUZVgV6366JZvvT +EAvtOoAzeD++O2fxQCXbxnlAXgn8hAgyOnW45Pmi6MuCr3uUJTxfNi8VEnH3RO7m +XVv4gl18+nrhTQT7M7iyLi2maa9FCjMwgLMjhYQr4Gs1kPKyhXA= +-----END RSA PRIVATE KEY-----`) +) From e0bebd07a06591fbe42a894ec9bc82be4ee80c3d Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 9 Nov 2022 15:24:56 +0000 Subject: [PATCH 05/66] Minor syntax update --- internal/healthcheck/listener.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go index 0113b418bd..3915f0b8ff 100644 --- a/internal/healthcheck/listener.go +++ b/internal/healthcheck/listener.go @@ -17,8 +17,7 @@ import ( const healthEndpoint = "/healthcheck" -func RunHealthCheck(plusClient *client.NginxClient, - cnf *configs.Configurator) { +func RunHealthCheck(plusClient *client.NginxClient, cnf *configs.Configurator) { glog.Infof("Running Health Endpoint at port 9000") runServer(strconv.Itoa(9000), plusClient, cnf) } From 852e577898f4bbaa0d859d82cd93427de521cc80 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 10 Nov 2022 12:19:31 +0000 Subject: [PATCH 06/66] Add a web server and a router --- go.mod | 1 + go.sum | 2 + internal/healthcheck/handlers.go | 101 +++++++++++++++++++++++++++++++ internal/healthcheck/http.go | 30 +++++++++ 4 files changed, 134 insertions(+) create mode 100644 internal/healthcheck/handlers.go diff --git a/go.mod b/go.mod index 790761ac2b..8cc12678da 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/aws/aws-sdk-go-v2/config v1.18.0 github.com/aws/aws-sdk-go-v2/service/marketplacemetering v1.13.22 github.com/cert-manager/cert-manager v1.10.0 + github.com/go-chi/chi v1.5.4 github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang/glog v1.0.0 github.com/google/go-cmp v0.5.9 diff --git a/go.sum b/go.sum index d75eb9a6e3..a5a03f9527 100644 --- a/go.sum +++ b/go.sum @@ -112,6 +112,8 @@ github.com/fsnotify/fsnotify v1.5.4 h1:jRbGcIw6P2Meqdwuo0H1p6JVLbL5DHKAKlYndzMwV github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF0+Y1A= github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= +github.com/go-chi/chi v1.5.4 h1:QHdzF2szwjqVV4wmByUnTcsbIg7UGaQ0tPF2t5GcAIs= +github.com/go-chi/chi v1.5.4/go.mod h1:uaf8YgoFazUOkPBG7fxPftUylNumIev9awIWOENIuEg= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= diff --git a/internal/healthcheck/handlers.go b/internal/healthcheck/handlers.go new file mode 100644 index 0000000000..05bd17ebc6 --- /dev/null +++ b/internal/healthcheck/handlers.go @@ -0,0 +1,101 @@ +package healthcheck + +import ( + "encoding/json" + "net/http" + "strings" + + "github.com/go-chi/chi" + "github.com/golang/glog" + "github.com/nginxinc/kubernetes-ingress/internal/configs" + "github.com/nginxinc/nginx-plus-go-client/client" + "k8s.io/utils/strings/slices" +) + +type HealthHandler struct { + client *client.NginxClient + cnf *configs.Configurator +} + +// Info returns basic information about healthcheck service. +func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { + // for now it is a placeholder for the response body + // we would return to a caller on GET request to '/' + info := struct { + Info string + Version string + Usage string + }{ + Info: "extended healthcheck endpoint", + Version: "0.1", + Usage: "/{hostname}", + } + data, err := json.Marshal(info) + if err != nil { + glog.Error("error marshalling result", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusOK) + if _, err = w.Write(data); err != nil { + glog.Error("error writing result", err) + } +} + +// Retrieve finds health stats for a host identified by an hostname in the request URL. +func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { + hostname := chi.URLParam(r, "hostname") + + upstreams, err := h.client.GetUpstreams() + if err != nil { + glog.Errorf("error retriving upstreams for host: %s", hostname) + w.WriteHeader(http.StatusInternalServerError) + return + } + upstreamNames := h.cnf.GetUpstreamsforHost(hostname) + + stats := countStats(upstreams, upstreamNames) + + data, err := json.Marshal(stats) + if err != nil { + glog.Error("error marshalling result", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", "application/json; charset=utf-8") + // NOTE: + // Here we need logic to setup correct header depending on the stats result! + w.WriteHeader(http.StatusOK) + if _, err = w.Write(data); err != nil { + glog.Error("error writing result", err) + } +} + +type hostStats struct { + Total int // Total number of configured servers (peers) + Up int // The number of servers (peers) with 'up' status + Unhealthy int // The number of servers (peers) with 'down' status +} + +func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { + total, up := 0, 0 + + for name, u := range *upstreams { + if slices.Contains(upstreamNames, name) { + for _, p := range u.Peers { + total++ + if strings.ToLower(p.State) == "up" { + up++ + } + } + } + } + unhealthy := total - up + return hostStats{ + Total: total, + Up: up, + Unhealthy: unhealthy, + } +} diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index e919e03507..21020f1867 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -2,8 +2,13 @@ package healthcheck import ( "crypto/tls" + "fmt" "net/http" "time" + + "github.com/go-chi/chi" + "github.com/nginxinc/kubernetes-ingress/internal/configs" + "github.com/nginxinc/nginx-plus-go-client/client" ) // ListenAndServeTLS is a drop-in replacement for a default @@ -36,3 +41,28 @@ func ListenAndServeTLS(addr string, cert, key []byte, handler http.Handler) erro func LoadX509KeyPair(cert, key []byte) (tls.Certificate, error) { return tls.X509KeyPair(cert, key) } + +// API constructs an http.Handler with all healtcheck routes. +func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { + health := HealthHandler{ + client: client, + cnf: cnf, + } + mux := chi.NewRouter() + mux.MethodFunc(http.MethodGet, "/", health.Info) + mux.MethodFunc(http.MethodGet, "/healthcheck/{hostname}", health.Retrieve) + return mux +} + +// RunHealtcheckServer takes configs and starts healtcheck service. +func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator) { + healthServer := http.Server{ + Addr: fmt.Sprintf(":%s", port), + Handler: API(nc, cnf), + + // For now hardcoded! + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + healthServer.ListenAndServe() +} From 482a6739764e834efce642d0aa4bc1b6212717d4 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 10 Nov 2022 13:26:42 +0000 Subject: [PATCH 07/66] Refactor, handle err, simplify --- internal/healthcheck/handlers.go | 101 ------------------- internal/healthcheck/http.go | 163 ++++++++++++++++++++++++++----- 2 files changed, 138 insertions(+), 126 deletions(-) delete mode 100644 internal/healthcheck/handlers.go diff --git a/internal/healthcheck/handlers.go b/internal/healthcheck/handlers.go deleted file mode 100644 index 05bd17ebc6..0000000000 --- a/internal/healthcheck/handlers.go +++ /dev/null @@ -1,101 +0,0 @@ -package healthcheck - -import ( - "encoding/json" - "net/http" - "strings" - - "github.com/go-chi/chi" - "github.com/golang/glog" - "github.com/nginxinc/kubernetes-ingress/internal/configs" - "github.com/nginxinc/nginx-plus-go-client/client" - "k8s.io/utils/strings/slices" -) - -type HealthHandler struct { - client *client.NginxClient - cnf *configs.Configurator -} - -// Info returns basic information about healthcheck service. -func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { - // for now it is a placeholder for the response body - // we would return to a caller on GET request to '/' - info := struct { - Info string - Version string - Usage string - }{ - Info: "extended healthcheck endpoint", - Version: "0.1", - Usage: "/{hostname}", - } - data, err := json.Marshal(info) - if err != nil { - glog.Error("error marshalling result", err) - w.WriteHeader(http.StatusInternalServerError) - return - } - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(http.StatusOK) - if _, err = w.Write(data); err != nil { - glog.Error("error writing result", err) - } -} - -// Retrieve finds health stats for a host identified by an hostname in the request URL. -func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { - hostname := chi.URLParam(r, "hostname") - - upstreams, err := h.client.GetUpstreams() - if err != nil { - glog.Errorf("error retriving upstreams for host: %s", hostname) - w.WriteHeader(http.StatusInternalServerError) - return - } - upstreamNames := h.cnf.GetUpstreamsforHost(hostname) - - stats := countStats(upstreams, upstreamNames) - - data, err := json.Marshal(stats) - if err != nil { - glog.Error("error marshalling result", err) - w.WriteHeader(http.StatusInternalServerError) - return - } - - w.Header().Set("Content-Type", "application/json; charset=utf-8") - // NOTE: - // Here we need logic to setup correct header depending on the stats result! - w.WriteHeader(http.StatusOK) - if _, err = w.Write(data); err != nil { - glog.Error("error writing result", err) - } -} - -type hostStats struct { - Total int // Total number of configured servers (peers) - Up int // The number of servers (peers) with 'up' status - Unhealthy int // The number of servers (peers) with 'down' status -} - -func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { - total, up := 0, 0 - - for name, u := range *upstreams { - if slices.Contains(upstreamNames, name) { - for _, p := range u.Peers { - total++ - if strings.ToLower(p.State) == "up" { - up++ - } - } - } - } - unhealthy := total - up - return hostStats{ - Total: total, - Up: up, - Unhealthy: unhealthy, - } -} diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 21020f1867..abe6db5ff7 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -2,15 +2,153 @@ package healthcheck import ( "crypto/tls" + "encoding/json" "fmt" "net/http" + "strings" "time" "github.com/go-chi/chi" + "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" + "k8s.io/utils/strings/slices" ) +// RunHealtcheckServer takes configs and starts healtcheck service. +func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator) error { + healthServer := http.Server{ + Addr: fmt.Sprintf(":%s", port), + Handler: API(nc, cnf), + + // For now hardcoded! + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + if err := healthServer.ListenAndServe(); err != nil { + glog.Error("error starting healtcheck server", err) + return fmt.Errorf("starting healtcheck server: %w", err) + } + return nil +} + +// API constructs an http.Handler with all healtcheck routes. +func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { + health := HealthHandler{ + client: client, + cnf: cnf, + } + mux := chi.NewRouter() + mux.MethodFunc(http.MethodGet, "/", health.Info) + mux.MethodFunc(http.MethodGet, "/healthcheck/{hostname}", health.Retrieve) + return mux +} + +// =================================================================================== +// Handlers +// =================================================================================== + +type HealthHandler struct { + client *client.NginxClient + cnf *configs.Configurator +} + +// Info returns basic information about healthcheck service. +func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { + // for now it is a placeholder for the response body + // we would return to a caller on GET request to '/' + info := struct { + Info string + Version string + Usage string + }{ + Info: "extended healthcheck endpoint", + Version: "0.1", + Usage: "/{hostname}", + } + data, err := json.Marshal(info) + if err != nil { + glog.Error("error marshalling result", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusOK) + if _, err = w.Write(data); err != nil { + glog.Error("error writing result", err) + } +} + +// Retrieve finds health stats for a host identified by an hostname in the request URL. +func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { + hostname := chi.URLParam(r, "hostname") + + upstreams, err := h.client.GetUpstreams() + if err != nil { + glog.Errorf("error retriving upstreams for host: %s", hostname) + w.WriteHeader(http.StatusInternalServerError) + return + } + upstreamNames := h.cnf.GetUpstreamsforHost(hostname) + + stats := countStats(upstreams, upstreamNames) + + data, err := json.Marshal(stats) + if err != nil { + glog.Error("error marshalling result", err) + w.WriteHeader(http.StatusInternalServerError) + return + } + + w.Header().Set("Content-Type", "application/json; charset=utf-8") + // NOTE: + // Here we need logic to setup correct header depending on the stats result! + // current one is initial implementation. + // Do ww have to calculate ratio of total/down to return a different status? + // TBD! + switch stats.Up { + case 0: + w.WriteHeader(http.StatusServiceUnavailable) + default: + w.WriteHeader(http.StatusOK) + } + if _, err = w.Write(data); err != nil { + glog.Error("error writing result", err) + } +} + +func (h *HealthHandler) Status(w http.ResponseWriter, r *http.Request) { + // possible implement website that currently is server from listener.go ? + w.WriteHeader(http.StatusNotImplemented) +} + +type hostStats struct { + Total int // Total number of configured servers (peers) + Up int // The number of servers (peers) with 'up' status + Unhealthy int // The number of servers (peers) with 'down' status +} + +func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { + total, up := 0, 0 + + for name, u := range *upstreams { + if slices.Contains(upstreamNames, name) { + for _, p := range u.Peers { + total++ + if strings.ToLower(p.State) == "up" { + up++ + } + } + } + } + unhealthy := total - up + return hostStats{ + Total: total, + Up: up, + Unhealthy: unhealthy, + } +} + // ListenAndServeTLS is a drop-in replacement for a default // ListenAndServeTLS func from the http std library. The func // takes not paths to a cert and a key but slices of bytes representing @@ -41,28 +179,3 @@ func ListenAndServeTLS(addr string, cert, key []byte, handler http.Handler) erro func LoadX509KeyPair(cert, key []byte) (tls.Certificate, error) { return tls.X509KeyPair(cert, key) } - -// API constructs an http.Handler with all healtcheck routes. -func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { - health := HealthHandler{ - client: client, - cnf: cnf, - } - mux := chi.NewRouter() - mux.MethodFunc(http.MethodGet, "/", health.Info) - mux.MethodFunc(http.MethodGet, "/healthcheck/{hostname}", health.Retrieve) - return mux -} - -// RunHealtcheckServer takes configs and starts healtcheck service. -func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator) { - healthServer := http.Server{ - Addr: fmt.Sprintf(":%s", port), - Handler: API(nc, cnf), - - // For now hardcoded! - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, - } - healthServer.ListenAndServe() -} From b7257d14af87822c4798553fbbbe6d4bc6eecb40 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 10 Nov 2022 13:33:38 +0000 Subject: [PATCH 08/66] Updated docs --- internal/healthcheck/http.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index abe6db5ff7..e984ee310a 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -118,7 +118,7 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { } func (h *HealthHandler) Status(w http.ResponseWriter, r *http.Request) { - // possible implement website that currently is server from listener.go ? + // possible implement website that currently is served from listener.go ? w.WriteHeader(http.StatusNotImplemented) } @@ -128,6 +128,7 @@ type hostStats struct { Unhealthy int // The number of servers (peers) with 'down' status } +// countStats calculates and returns statistics. func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { total, up := 0, 0 From dc5050c317e580faaac3eef2145c83749e3871a1 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 10 Nov 2022 14:39:27 +0000 Subject: [PATCH 09/66] Update data type expected by log message --- internal/healthcheck/listener.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go index 3915f0b8ff..a467917269 100644 --- a/internal/healthcheck/listener.go +++ b/internal/healthcheck/listener.go @@ -40,7 +40,7 @@ func runServer(port string, plusClient *client.NginxClient, cnf *configs.Configu for name, u := range *upstreams { if slices.Contains(upstreamNames, name) { for _, p := range u.Peers { - glog.Infof("Peer ID: %s, Name: %s, State: %s ", p.ID, p.Name, p.State) + glog.Infof("Peer ID: %v, Name: %v, State: %v", p.ID, p.Name, p.State) states = append(states, "IP: "+p.Name+", State: "+p.State) } } From 0877b5ae8103541ac05bb7d2c1adf1f29767e5eb Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 11 Nov 2022 12:14:38 +0000 Subject: [PATCH 10/66] Add TLS support for the healthcheck server --- internal/healthcheck/http.go | 84 +++++++++++++++---------------- internal/healthcheck/http_test.go | 22 -------- 2 files changed, 41 insertions(+), 65 deletions(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index e984ee310a..7599927e03 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -3,11 +3,14 @@ package healthcheck import ( "crypto/tls" "encoding/json" + "errors" "fmt" "net/http" "strings" "time" + v1 "k8s.io/api/core/v1" + "github.com/go-chi/chi" "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/configs" @@ -16,22 +19,54 @@ import ( ) // RunHealtcheckServer takes configs and starts healtcheck service. -func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator) error { +func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) error { + if secret == nil { + healthServer := http.Server{ + Addr: fmt.Sprintf(":%s", port), + Handler: API(nc, cnf), + ReadTimeout: 10 * time.Second, + WriteTimeout: 10 * time.Second, + } + if err := healthServer.ListenAndServe(); err != nil { + return fmt.Errorf("starting healtcheck server: %w", err) + } + } + + tlsCert, err := makeCert(secret) + if err != nil { + return fmt.Errorf("creating tls cert %w", err) + } + healthServer := http.Server{ Addr: fmt.Sprintf(":%s", port), Handler: API(nc, cnf), - - // For now hardcoded! + TLSConfig: &tls.Config{ + Certificates: []tls.Certificate{tlsCert}, + }, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, } - if err := healthServer.ListenAndServe(); err != nil { - glog.Error("error starting healtcheck server", err) - return fmt.Errorf("starting healtcheck server: %w", err) + if err := healthServer.ListenAndServeTLS("", ""); err != nil { + return fmt.Errorf("starting healtcheck tls server: %w", err) } return nil } +// makeCert takes k8s Secret and returns tls Certificate for the server. +// It errors if either cert, or key are not present in the Secret. +func makeCert(s *v1.Secret) (tls.Certificate, error) { + cert, ok := s.Data[v1.TLSCertKey] + if !ok { + return tls.Certificate{}, errors.New("missing tls cert") + } + key, ok := s.Data[v1.TLSPrivateKeyKey] + if !ok { + return tls.Certificate{}, errors.New("missing tls key") + + } + return tls.X509KeyPair(cert, key) +} + // API constructs an http.Handler with all healtcheck routes. func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { health := HealthHandler{ @@ -92,7 +127,6 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { upstreamNames := h.cnf.GetUpstreamsforHost(hostname) stats := countStats(upstreams, upstreamNames) - data, err := json.Marshal(stats) if err != nil { glog.Error("error marshalling result", err) @@ -101,11 +135,6 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { } w.Header().Set("Content-Type", "application/json; charset=utf-8") - // NOTE: - // Here we need logic to setup correct header depending on the stats result! - // current one is initial implementation. - // Do ww have to calculate ratio of total/down to return a different status? - // TBD! switch stats.Up { case 0: w.WriteHeader(http.StatusServiceUnavailable) @@ -149,34 +178,3 @@ func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { Unhealthy: unhealthy, } } - -// ListenAndServeTLS is a drop-in replacement for a default -// ListenAndServeTLS func from the http std library. The func -// takes not paths to a cert and a key but slices of bytes representing -// content of the files used by the original http.ListenAndServeTLS function. -func ListenAndServeTLS(addr string, cert, key []byte, handler http.Handler) error { - tlsCert, err := LoadX509KeyPair(cert, key) - if err != nil { - return err - } - - server := &http.Server{ - Addr: addr, - Handler: handler, - TLSConfig: &tls.Config{ - Certificates: []tls.Certificate{tlsCert}, - }, - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, - } - - return server.ListenAndServeTLS("", "") -} - -// LoadX509KeyPair is a drop-in replacement for the LoadX509KeyPair -// from the http package from the std library. It takes not files -// containing a cert and a key but a slice of bytes representing -// the content of the corresponding files. -func LoadX509KeyPair(cert, key []byte) (tls.Certificate, error) { - return tls.X509KeyPair(cert, key) -} diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/http_test.go index 36ddc75a27..bd82f08586 100644 --- a/internal/healthcheck/http_test.go +++ b/internal/healthcheck/http_test.go @@ -1,27 +1,5 @@ package healthcheck_test -import ( - "testing" - - "github.com/nginxinc/kubernetes-ingress/internal/healthcheck" -) - -func TestLoadsKeyPairOnValidInut(t *testing.T) { - t.Parallel() - _, err := healthcheck.LoadX509KeyPair(validCert, validKey) - if err != nil { - t.Fatal(err) - } -} - -func TestDoesNotLoadCertsOnInvalidInput(t *testing.T) { - t.Parallel() - _, err := healthcheck.LoadX509KeyPair(invalidCert, invalidKey) - if err == nil { - t.Fatal("want err on invalid cert/key, got nil") - } -} - var ( validCert = []byte(`-----BEGIN CERTIFICATE----- MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJJ From b2f1b91c64b58d6f0e0ba890ef453d6d92b4cb77 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 11 Nov 2022 12:20:41 +0000 Subject: [PATCH 11/66] Fix typos --- internal/healthcheck/http.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 7599927e03..f4f98965cc 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -28,7 +28,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi WriteTimeout: 10 * time.Second, } if err := healthServer.ListenAndServe(); err != nil { - return fmt.Errorf("starting healtcheck server: %w", err) + return fmt.Errorf("starting healthcheck server: %w", err) } } @@ -47,7 +47,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi WriteTimeout: 10 * time.Second, } if err := healthServer.ListenAndServeTLS("", ""); err != nil { - return fmt.Errorf("starting healtcheck tls server: %w", err) + return fmt.Errorf("starting healthcheck tls server: %w", err) } return nil } From f08179c4dfbca41ebae6c9f023e05ea841b29a12 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 11 Nov 2022 12:30:57 +0000 Subject: [PATCH 12/66] Add placeholders for server tests --- internal/healthcheck/http_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/http_test.go index bd82f08586..1d1ae007d4 100644 --- a/internal/healthcheck/http_test.go +++ b/internal/healthcheck/http_test.go @@ -1,5 +1,23 @@ package healthcheck_test +import "testing" + +func TestHealthCheckServer_RespondsWithStatisticsOnValidHostname(t *testing.T) { + t.Parallel() + t.FailNow() + +} + +func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnValidHostname(t *testing.T) { + t.Parallel() + t.FailNow() +} + +func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnInvalidHostname(t *testing.T) { + t.Parallel() + t.FailNow() +} + var ( validCert = []byte(`-----BEGIN CERTIFICATE----- MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJJ From 2c0fc9a7228b540893e7bcdf5b776443b3d7b1d6 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 11 Nov 2022 16:25:55 +0000 Subject: [PATCH 13/66] Check if hostname exist and handle http 404 --- internal/healthcheck/http.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index f4f98965cc..fd69b6781d 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -118,13 +118,19 @@ func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { hostname := chi.URLParam(r, "hostname") + upstreamNames := h.cnf.GetUpstreamsforHost(hostname) + if len(upstreamNames) == 0 { + glog.Errorf("no upstreams for hostname %s or hostname does not exist", hostname) + w.WriteHeader(http.StatusNotFound) + return + } + upstreams, err := h.client.GetUpstreams() if err != nil { glog.Errorf("error retriving upstreams for host: %s", hostname) w.WriteHeader(http.StatusInternalServerError) return } - upstreamNames := h.cnf.GetUpstreamsforHost(hostname) stats := countStats(upstreams, upstreamNames) data, err := json.Marshal(stats) From fa830da98e53182194d748568a7f4da1b45b6077 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 11 Nov 2022 16:56:34 +0000 Subject: [PATCH 14/66] Update url path to include 'probe' --- internal/healthcheck/http.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index fd69b6781d..6aa17a6175 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -75,7 +75,7 @@ func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { } mux := chi.NewRouter() mux.MethodFunc(http.MethodGet, "/", health.Info) - mux.MethodFunc(http.MethodGet, "/healthcheck/{hostname}", health.Retrieve) + mux.MethodFunc(http.MethodGet, "/probe/{hostname}", health.Retrieve) return mux } @@ -99,7 +99,7 @@ func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { }{ Info: "extended healthcheck endpoint", Version: "0.1", - Usage: "/{hostname}", + Usage: "/probe/{hostname}", } data, err := json.Marshal(info) if err != nil { From ae819447b49a6cd7a7b017ac2c67b8b7bea6e862 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Sat, 12 Nov 2022 00:01:47 +0000 Subject: [PATCH 15/66] Refactor handler, add tests --- go.mod | 1 + go.sum | 2 + internal/healthcheck/http.go | 102 +++++++++-------- internal/healthcheck/http_test.go | 183 +++++++++++++++++++++++++++++- 4 files changed, 234 insertions(+), 54 deletions(-) diff --git a/go.mod b/go.mod index 8cc12678da..92a8273bb6 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/marketplacemetering v1.13.22 github.com/cert-manager/cert-manager v1.10.0 github.com/go-chi/chi v1.5.4 + github.com/go-chi/httprate v0.7.0 github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang/glog v1.0.0 github.com/google/go-cmp v0.5.9 diff --git a/go.sum b/go.sum index a5a03f9527..4be2fca18d 100644 --- a/go.sum +++ b/go.sum @@ -114,6 +114,8 @@ github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= github.com/go-chi/chi v1.5.4 h1:QHdzF2szwjqVV4wmByUnTcsbIg7UGaQ0tPF2t5GcAIs= github.com/go-chi/chi v1.5.4/go.mod h1:uaf8YgoFazUOkPBG7fxPftUylNumIev9awIWOENIuEg= +github.com/go-chi/httprate v0.7.0 h1:8W0dF7Xa2Duz2p8ncGaehIphrxQGNlOtoGY0+NRRfjQ= +github.com/go-chi/httprate v0.7.0/go.mod h1:6GOYBSwnpra4CQfAKXu8sQZg+nZ0M1g9QnyFvxrAB8A= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 6aa17a6175..304f54c565 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -12,18 +12,25 @@ import ( v1 "k8s.io/api/core/v1" "github.com/go-chi/chi" + "github.com/go-chi/httprate" "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" "k8s.io/utils/strings/slices" ) -// RunHealtcheckServer takes configs and starts healtcheck service. +// RunHealtcheckServer takes config params, creates the health server and starts it. +// It errors if the server can't be started or provided secret is not valid +// (tls certificate cannot be created) and the health service with TLS support can't start. func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) error { + + getUpstreamsForHost := UpstreamsForHost(cnf) + getUpstreamsFromNginx := NginxUpstreams(nc) + if secret == nil { healthServer := http.Server{ Addr: fmt.Sprintf(":%s", port), - Handler: API(nc, cnf), + Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, } @@ -39,7 +46,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi healthServer := http.Server{ Addr: fmt.Sprintf(":%s", port), - Handler: API(nc, cnf), + Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), TLSConfig: &tls.Config{ Certificates: []tls.Certificate{tlsCert}, }, @@ -67,65 +74,63 @@ func makeCert(s *v1.Secret) (tls.Certificate, error) { return tls.X509KeyPair(cert, key) } +// =================================================================================== +// Handlers +// =================================================================================== + // API constructs an http.Handler with all healtcheck routes. -func API(client *client.NginxClient, cnf *configs.Configurator) http.Handler { +func API(upstreamsFromHost func(string) []string, upstreamsFromNginx func() (*client.Upstreams, error)) http.Handler { health := HealthHandler{ - client: client, - cnf: cnf, + UpstreamsForHost: upstreamsFromHost, + NginxUpstreams: upstreamsFromNginx, } mux := chi.NewRouter() - mux.MethodFunc(http.MethodGet, "/", health.Info) + mux.Use(httprate.Limit(10, 1*time.Second, httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "rate limit exceeded", http.StatusTooManyRequests) + })), + ) mux.MethodFunc(http.MethodGet, "/probe/{hostname}", health.Retrieve) return mux } -// =================================================================================== -// Handlers -// =================================================================================== - -type HealthHandler struct { - client *client.NginxClient - cnf *configs.Configurator +// UpstreamsForHost takes configurator and returns a func +// that is reposnsible for retriving upstreams for the given hostname. +func UpstreamsForHost(cnf *configs.Configurator) func(hostname string) []string { + return func(hostname string) []string { + return cnf.GetUpstreamsforHost(hostname) + } } -// Info returns basic information about healthcheck service. -func (h *HealthHandler) Info(w http.ResponseWriter, r *http.Request) { - // for now it is a placeholder for the response body - // we would return to a caller on GET request to '/' - info := struct { - Info string - Version string - Usage string - }{ - Info: "extended healthcheck endpoint", - Version: "0.1", - Usage: "/probe/{hostname}", - } - data, err := json.Marshal(info) - if err != nil { - glog.Error("error marshalling result", err) - w.WriteHeader(http.StatusInternalServerError) - return - } - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(http.StatusOK) - if _, err = w.Write(data); err != nil { - glog.Error("error writing result", err) +// NginxUpstreams takes an instance of NGNX client and returns a func +// that returns all upstreams. +func NginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { + return func() (*client.Upstreams, error) { + upstreams, err := nc.GetUpstreams() + if err != nil { + return nil, err + } + return upstreams, nil } } -// Retrieve finds health stats for a host identified by an hostname in the request URL. +// HealthHandler holds dependency used in its method(s). +type HealthHandler struct { + UpstreamsForHost func(host string) []string + NginxUpstreams func() (*client.Upstreams, error) +} + +// Retrieve finds health stats for the host identified by a hostname in the request URL. func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { hostname := chi.URLParam(r, "hostname") - upstreamNames := h.cnf.GetUpstreamsforHost(hostname) + upstreamNames := h.UpstreamsForHost(hostname) if len(upstreamNames) == 0 { glog.Errorf("no upstreams for hostname %s or hostname does not exist", hostname) w.WriteHeader(http.StatusNotFound) return } - upstreams, err := h.client.GetUpstreams() + upstreams, err := h.NginxUpstreams() if err != nil { glog.Errorf("error retriving upstreams for host: %s", hostname) w.WriteHeader(http.StatusInternalServerError) @@ -139,7 +144,6 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) return } - w.Header().Set("Content-Type", "application/json; charset=utf-8") switch stats.Up { case 0: @@ -149,24 +153,22 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { } if _, err = w.Write(data); err != nil { glog.Error("error writing result", err) + http.Error(w, "internal error", http.StatusInternalServerError) } } -func (h *HealthHandler) Status(w http.ResponseWriter, r *http.Request) { - // possible implement website that currently is served from listener.go ? - w.WriteHeader(http.StatusNotImplemented) -} - -type hostStats struct { +// HostStats holds information about total, up and +// unhealthy number of 'peers' associated with the +// given host. +type HostStats struct { Total int // Total number of configured servers (peers) Up int // The number of servers (peers) with 'up' status Unhealthy int // The number of servers (peers) with 'down' status } // countStats calculates and returns statistics. -func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { +func countStats(upstreams *client.Upstreams, upstreamNames []string) HostStats { total, up := 0, 0 - for name, u := range *upstreams { if slices.Contains(upstreamNames, name) { for _, p := range u.Peers { @@ -178,7 +180,7 @@ func countStats(upstreams *client.Upstreams, upstreamNames []string) hostStats { } } unhealthy := total - up - return hostStats{ + return HostStats{ Total: total, Up: up, Unhealthy: unhealthy, diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/http_test.go index 1d1ae007d4..0a9bb7e2b3 100644 --- a/internal/healthcheck/http_test.go +++ b/internal/healthcheck/http_test.go @@ -1,24 +1,199 @@ package healthcheck_test -import "testing" +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nginxinc/kubernetes-ingress/internal/healthcheck" + "github.com/nginxinc/nginx-plus-go-client/client" +) + +func TestHealthCheckServer_ReturnsValidStatsForAllPeersUpOnValidHostname(t *testing.T) { + t.Parallel() + + req, err := http.NewRequest(http.MethodGet, "/probe/bar.tea.com", nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getNginxUpstreams) + + h.ServeHTTP(resp, req) + + want := healthcheck.HostStats{ + Total: 3, + Up: 3, + Unhealthy: 0, + } + + var got healthcheck.HostStats + if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) + } +} + +func TestHealthCheckServer_ReturnsValidStatsForAllPeersDownOnValidHostname(t *testing.T) { + t.Parallel() + + req, err := http.NewRequest(http.MethodGet, "/probe/bar.tea.com", nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + fn := func(h string) []string { + u, ok := upstreams[h] + if !ok { + return []string{} + } + return u + } + + ngu := func() (*client.Upstreams, error) { + return &allPeersUnhealthy, nil + } + + h := healthcheck.API(fn, ngu) + + h.ServeHTTP(resp, req) + + want := healthcheck.HostStats{ + Total: 3, + Up: 0, + Unhealthy: 3, + } + + if !cmp.Equal(http.StatusServiceUnavailable, resp.Code) { + t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) + } + + var gotStats healthcheck.HostStats + if err := json.Unmarshal(resp.Body.Bytes(), &gotStats); err != nil { + t.Fatal(err) + } + + if !cmp.Equal(want, gotStats) { + t.Error(cmp.Diff(want, gotStats)) + } +} func TestHealthCheckServer_RespondsWithStatisticsOnValidHostname(t *testing.T) { t.Parallel() - t.FailNow() + //t.FailNow() } func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnValidHostname(t *testing.T) { t.Parallel() - t.FailNow() + //t.FailNow() } func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnInvalidHostname(t *testing.T) { t.Parallel() - t.FailNow() + //t.FailNow() +} + +func getNginxUpstreams() (*client.Upstreams, error) { + ups := client.Upstreams{ + "upstream1": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + "upstream2": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + "upstream3": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + } + return &ups, nil +} + +func getUpstreamsForHost(host string) []string { + upstreams := map[string][]string{ + "foo.tea.com": {"upstream1", "upstream2"}, + "bar.tea.com": {"upstream1"}, + } + u, ok := upstreams[host] + if !ok { + return []string{} + } + return u } var ( + upstreams = map[string][]string{ + "foo.tea.com": {"upstream1", "upstream2"}, + "bar.tea.com": {"upstream1"}, + } + + allPeersUp = client.Upstreams{ + "upstream1": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + "upstream2": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + "upstream3": client.Upstream{ + Peers: []client.Peer{ + {State: "Up"}, + {State: "Up"}, + {State: "Up"}, + }, + }, + } + + allPeersUnhealthy = client.Upstreams{ + "upstream1": client.Upstream{ + Peers: []client.Peer{ + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, + }, + }, + "upstream2": client.Upstream{ + Peers: []client.Peer{ + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, + }, + }, + "upstream3": client.Upstream{ + Peers: []client.Peer{ + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, + }, + }, + } + validCert = []byte(`-----BEGIN CERTIFICATE----- MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJJ RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD From 8f56d3f1b3359f36eca72f276252c894e826dc98 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Mon, 14 Nov 2022 12:31:35 +0000 Subject: [PATCH 16/66] create flags for health probe --- cmd/nginx-ingress/flags.go | 19 ++++++++++ cmd/nginx-ingress/main.go | 23 +++++++++--- internal/healthcheck/listener.go | 62 +++----------------------------- 3 files changed, 42 insertions(+), 62 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index c4bf503e76..50acbc780a 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -135,6 +135,15 @@ var ( prometheusMetricsListenPort = flag.Int("prometheus-metrics-listen-port", 9113, "Set the port where the Prometheus metrics are exposed. [1024 - 65535]") + enableHealthProbe = flag.Bool("enable-health-probe", false, + `Enable health probe for external load balancers. Requires -nginx-plus`) + + healthProbeTLSSecretName = flag.String("health-probe-tls-secret", "", + `A Secret with a TLS certificate and key for TLS termination of the health probe.`) + + healthProbeListenPort = flag.Int("health-probe-listen-port", 9000, + "Enable health probe and set the port where the health probe endpoint are exposed. Requires -nginx-plus. [1024 - 65535]") + enableCustomResources = flag.Bool("enable-custom-resources", true, "Enable custom resources") @@ -256,6 +265,11 @@ func parseFlags() { *enableLatencyMetrics = false } + if *enableHealthProbe && !*nginxPlus { + glog.Warning("enable-health-probe flag support is for NGINX Plus, health probe endpoint will not be exposed") + *enableHealthProbe = false + } + if *enableCertManager && !*enableCustomResources { glog.Fatal("enable-cert-manager flag requires -enable-custom-resources") } @@ -332,6 +346,11 @@ func validationChecks() { glog.Fatalf("Invalid value for ready-status-port: %v", readyStatusPortValidationError) } + healthProbePortValidationError := validatePort(*healthProbeListenPort) + if healthProbePortValidationError != nil { + glog.Fatalf("Invalid value for health-probe-listen-port: %v", metricsPortValidationError) + } + var err error allowedCIDRs, err = parseNginxStatusAllowCIDRs(*nginxStatusAllowCIDRs) if err != nil { diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 8b1425d470..2dbd6e755e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -123,7 +123,7 @@ func main() { transportServerValidator := cr_validation.NewTransportServerValidator(*enableTLSPassthrough, *enableSnippets, *nginxPlus) virtualServerValidator := cr_validation.NewVirtualServerValidator(cr_validation.IsPlus(*nginxPlus), cr_validation.IsDosEnabled(*appProtectDos), cr_validation.IsCertManagerEnabled(*enableCertManager), cr_validation.IsExternalDNSEnabled(*enableExternalDNS)) - createHealthEndpoint(plusClient, cnf) + createHealthProbeEndpoint(kubeClient, plusClient, cnf) lbcInput := k8s.NewLoadBalancerControllerInput{ KubeClient: kubeClient, @@ -431,6 +431,10 @@ func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationVali forbiddenListenerPorts[*prometheusMetricsListenPort] = true } + if *enableHealthProbe { + forbiddenListenerPorts[*healthProbeListenPort] = true + } + return cr_validation.NewGlobalConfigurationValidator(forbiddenListenerPorts) } @@ -659,9 +663,20 @@ func createPlusAndLatencyCollectors( return plusCollector, syslogListener, lc } -func createHealthEndpoint(plusClient *client.NginxClient, cnf *configs.Configurator) { - glog.Infof("Starting Health Endpoint") - go healthcheck.RunHealthCheck(plusClient, cnf) +func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) { + var healthProbeSecret *api_v1.Secret + var err error + + if *healthProbeTLSSecretName != "" { + healthProbeSecret, err = getAndValidateSecret(kubeClient, *healthProbeTLSSecretName) + if err != nil { + glog.Fatalf("Error trying to get the health probe TLS secret %v: %v", *healthProbeTLSSecretName, err) + } + } + + if *enableHealthProbe { + go healthcheck.RunHealthCheck(*healthProbeListenPort, plusClient, cnf, healthProbeSecret) + } } func processGlobalConfiguration() { diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go index a467917269..ec7373e0e8 100644 --- a/internal/healthcheck/listener.go +++ b/internal/healthcheck/listener.go @@ -1,67 +1,13 @@ package healthcheck import ( - "fmt" - "path" + apiv1 "k8s.io/api/core/v1" + "strconv" "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" - "k8s.io/utils/strings/slices" - - "net/http" - "strconv" - "strings" - - "github.com/golang/glog" ) -const healthEndpoint = "/healthcheck" - -func RunHealthCheck(plusClient *client.NginxClient, cnf *configs.Configurator) { - glog.Infof("Running Health Endpoint at port 9000") - runServer(strconv.Itoa(9000), plusClient, cnf) -} - -func runServer(port string, plusClient *client.NginxClient, cnf *configs.Configurator) { - http.HandleFunc("/healthcheck/", func(w http.ResponseWriter, r *http.Request) { - hostname := path.Base(r.URL.Path) - glog.Infof("Path: %s", hostname) - - upstreamNames := cnf.GetUpstreamsforHost(hostname) - glog.Infof("Upstream Names: %s", upstreamNames) - - upstreams, err := plusClient.GetUpstreams() - if err != nil { - // handle error - http.Error(w, err.Error(), http.StatusInternalServerError) - } - var states []string - - for name, u := range *upstreams { - if slices.Contains(upstreamNames, name) { - for _, p := range u.Peers { - glog.Infof("Peer ID: %v, Name: %v, State: %v", p.ID, p.Name, p.State) - states = append(states, "IP: "+p.Name+", State: "+p.State) - } - } - } - - w.WriteHeader(http.StatusCreated) - - resp := ` - NGINX Ingress Controller - -

NGINX Ingress Controller

Hostname: ` + hostname + - `

upstreams: ` + strings.Join(upstreamNames[:], ", ") + - `

states: ` + strings.Join(states[:], ", ") + - `

` - _, err = w.Write([]byte(resp)) - if err != nil { - glog.Warningf("Error while sending a response for the '/' path: %v", err) - } - }) - address := fmt.Sprintf(":%v", port) - glog.Infof("Starting Healthcheck listener on: %v%v", address, healthEndpoint) - glog.Fatal("Error in Healthcheck listener server: ", http.ListenAndServe(address, nil)) - +func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *apiv1.Secret) { + RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) } From 65eff0b44d8a7928725ae60781a67ca0c2517fe3 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 14 Nov 2022 18:47:44 +0000 Subject: [PATCH 17/66] Handle errors from starting servers --- internal/configs/configurator.go | 6 +- internal/configs/virtualserver.go | 2 + internal/healthcheck/doc.go | 2 + internal/healthcheck/http.go | 34 ++-- internal/healthcheck/http_test.go | 309 ++++++++++++++---------------- internal/healthcheck/listener.go | 13 +- 6 files changed, 176 insertions(+), 190 deletions(-) create mode 100644 internal/healthcheck/doc.go diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 76bb9a2421..ffc2210552 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -265,6 +265,7 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error) return warnings, nil } +// GetVirtualServer takes a hostname and returns a VS for the given hostname. func (cnf *Configurator) GetVirtualServer(hostname string) *conf_v1.VirtualServer { for _, vsEx := range cnf.virtualServers { if vsEx.VirtualServer.Spec.Host == hostname { @@ -274,8 +275,8 @@ func (cnf *Configurator) GetVirtualServer(hostname string) *conf_v1.VirtualServe return nil } +// GetUpstreamsforVirtualServer takes VS and returns a slice of upstreams. func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) []string { - glog.Infof("Get upstreamName for vs: %s", vs.Spec.Host) upstreamNames := make([]string, 0, len(vs.Spec.Upstreams)) @@ -291,8 +292,9 @@ func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) return upstreamNames } +// GetUpstreamsforHost takes a hostname and returns a slice of upstreams +// for the given hostname. func (cnf *Configurator) GetUpstreamsforHost(hostname string) []string { - glog.Infof("Get upstream for host: %s", hostname) vs := cnf.GetVirtualServer(hostname) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index b693ac065f..021f99621b 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -132,6 +132,7 @@ type upstreamNamer struct { namespace string } +// NewUpstreamNamerForVirtualServer creates a new namer. func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf("vs_%s_%s", virtualServer.Namespace, virtualServer.Name), @@ -139,6 +140,7 @@ func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *ups } } +// NewUpstreamNamerForVirtualServerRoute creates a new namer. func NewUpstreamNamerForVirtualServerRoute( virtualServer *conf_v1.VirtualServer, virtualServerRoute *conf_v1.VirtualServerRoute, diff --git a/internal/healthcheck/doc.go b/internal/healthcheck/doc.go new file mode 100644 index 0000000000..bef2038916 --- /dev/null +++ b/internal/healthcheck/doc.go @@ -0,0 +1,2 @@ +// Package healthcheck provides a deep healtcheck service. +package healthcheck diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 304f54c565..88dde28133 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -23,7 +23,6 @@ import ( // It errors if the server can't be started or provided secret is not valid // (tls certificate cannot be created) and the health service with TLS support can't start. func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) error { - getUpstreamsForHost := UpstreamsForHost(cnf) getUpstreamsFromNginx := NginxUpstreams(nc) @@ -49,6 +48,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), TLSConfig: &tls.Config{ Certificates: []tls.Certificate{tlsCert}, + MinVersion: tls.VersionTLS12, }, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, @@ -69,19 +69,14 @@ func makeCert(s *v1.Secret) (tls.Certificate, error) { key, ok := s.Data[v1.TLSPrivateKeyKey] if !ok { return tls.Certificate{}, errors.New("missing tls key") - } return tls.X509KeyPair(cert, key) } -// =================================================================================== -// Handlers -// =================================================================================== - // API constructs an http.Handler with all healtcheck routes. -func API(upstreamsFromHost func(string) []string, upstreamsFromNginx func() (*client.Upstreams, error)) http.Handler { +func API(upstreamsForHost func(string) []string, upstreamsFromNginx func() (*client.Upstreams, error)) http.Handler { health := HealthHandler{ - UpstreamsForHost: upstreamsFromHost, + UpstreamsForHost: upstreamsForHost, NginxUpstreams: upstreamsFromNginx, } mux := chi.NewRouter() @@ -94,15 +89,15 @@ func API(upstreamsFromHost func(string) []string, upstreamsFromNginx func() (*cl } // UpstreamsForHost takes configurator and returns a func -// that is reposnsible for retriving upstreams for the given hostname. +// that is reposnsible for retrieving upstreams for the given hostname. func UpstreamsForHost(cnf *configs.Configurator) func(hostname string) []string { return func(hostname string) []string { return cnf.GetUpstreamsforHost(hostname) } } -// NginxUpstreams takes an instance of NGNX client and returns a func -// that returns all upstreams. +// NginxUpstreams takes an instance of NGNX client and returns +// a func that returns all upstreams. func NginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { return func() (*client.Upstreams, error) { upstreams, err := nc.GetUpstreams() @@ -113,7 +108,7 @@ func NginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { } } -// HealthHandler holds dependency used in its method(s). +// HealthHandler holds dependency for its method(s). type HealthHandler struct { UpstreamsForHost func(host string) []string NginxUpstreams func() (*client.Upstreams, error) @@ -132,7 +127,7 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { upstreams, err := h.NginxUpstreams() if err != nil { - glog.Errorf("error retriving upstreams for host: %s", hostname) + glog.Errorf("error retrieving upstreams for host: %s", hostname) w.WriteHeader(http.StatusInternalServerError) return } @@ -140,7 +135,7 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { stats := countStats(upstreams, upstreamNames) data, err := json.Marshal(stats) if err != nil { - glog.Error("error marshalling result", err) + glog.Error("error marshaling result", err) w.WriteHeader(http.StatusInternalServerError) return } @@ -161,9 +156,9 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { // unhealthy number of 'peers' associated with the // given host. type HostStats struct { - Total int // Total number of configured servers (peers) - Up int // The number of servers (peers) with 'up' status - Unhealthy int // The number of servers (peers) with 'down' status + Total int + Up int + Unhealthy int } // countStats calculates and returns statistics. @@ -173,9 +168,10 @@ func countStats(upstreams *client.Upstreams, upstreamNames []string) HostStats { if slices.Contains(upstreamNames, name) { for _, p := range u.Peers { total++ - if strings.ToLower(p.State) == "up" { - up++ + if strings.ToLower(p.State) != "up" { + continue } + up++ } } } diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/http_test.go index 0a9bb7e2b3..ae72535730 100644 --- a/internal/healthcheck/http_test.go +++ b/internal/healthcheck/http_test.go @@ -1,7 +1,9 @@ package healthcheck_test import ( + "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -11,97 +13,154 @@ import ( "github.com/nginxinc/nginx-plus-go-client/client" ) -func TestHealthCheckServer_ReturnsValidStatsForAllPeersUpOnValidHostname(t *testing.T) { +func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersUpOnValidHostname(t *testing.T) { t.Parallel() - req, err := http.NewRequest(http.MethodGet, "/probe/bar.tea.com", nil) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) if err != nil { t.Fatal(err) } resp := httptest.NewRecorder() - h := healthcheck.API(getUpstreamsForHost, getNginxUpstreams) - + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) h.ServeHTTP(resp, req) + if !cmp.Equal(http.StatusOK, resp.Code) { + t.Error(cmp.Diff(http.StatusOK, resp.Code)) + } + want := healthcheck.HostStats{ Total: 3, Up: 3, Unhealthy: 0, } - var got healthcheck.HostStats if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { t.Fatal(err) } - if !cmp.Equal(want, got) { t.Error(cmp.Diff(want, got)) } } -func TestHealthCheckServer_ReturnsValidStatsForAllPeersDownOnValidHostname(t *testing.T) { +func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersDownOnValidHostname(t *testing.T) { t.Parallel() - req, err := http.NewRequest(http.MethodGet, "/probe/bar.tea.com", nil) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) if err != nil { t.Fatal(err) } resp := httptest.NewRecorder() - fn := func(h string) []string { - u, ok := upstreams[h] - if !ok { - return []string{} - } - return u - } + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUnhealthy) + h.ServeHTTP(resp, req) - ngu := func() (*client.Upstreams, error) { - return &allPeersUnhealthy, nil + if !cmp.Equal(http.StatusServiceUnavailable, resp.Code) { + t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) } - h := healthcheck.API(fn, ngu) - - h.ServeHTTP(resp, req) - want := healthcheck.HostStats{ Total: 3, Up: 0, Unhealthy: 3, } - if !cmp.Equal(http.StatusServiceUnavailable, resp.Code) { - t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) + var got healthcheck.HostStats + if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) } +} + +func TestHealthCheckServer_ReturnsValidStatsAndCorrectHTTPStatusCodeForPartOfPeersDownOnValidHostname(t *testing.T) { + t.Parallel() - var gotStats healthcheck.HostStats - if err := json.Unmarshal(resp.Body.Bytes(), &gotStats); err != nil { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) + if err != nil { t.Fatal(err) } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXPartiallyUp) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusOK, resp.Code) { + t.Error(cmp.Diff(http.StatusOK, resp.Code)) + } + + want := healthcheck.HostStats{ + Total: 3, + Up: 1, + Unhealthy: 2, + } - if !cmp.Equal(want, gotStats) { - t.Error(cmp.Diff(want, gotStats)) + var got healthcheck.HostStats + if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) } } -func TestHealthCheckServer_RespondsWithStatisticsOnValidHostname(t *testing.T) { +func TestHealthCheckServer_RespondsWithHTTPErrCodeOnNotExistingHostname(t *testing.T) { t.Parallel() - //t.FailNow() + // 'foo.mocha.org' represents not existing hostname + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/foo.mocha.org", nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXNotExistingHost) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusNotFound, resp.Code) { + t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) + } } -func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnValidHostname(t *testing.T) { +func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnErrorFromNGINXAPI(t *testing.T) { t.Parallel() - //t.FailNow() + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/foo.tea.com", nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXErrorFromAPI) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusInternalServerError, resp.Code) { + t.Error(cmp.Diff(http.StatusInternalServerError, resp.Code)) + } } -func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnInvalidHostname(t *testing.T) { - t.Parallel() - //t.FailNow() +// getUpstreamsForHost is a helper func faking response from IC. +func getUpstreamsForHost(host string) []string { + upstreams := map[string][]string{ + "foo.tea.com": {"upstream1", "upstream2"}, + "bar.tea.com": {"upstream1"}, + } + u, ok := upstreams[host] + if !ok { + return []string{} + } + return u } -func getNginxUpstreams() (*client.Upstreams, error) { +// getUpstreamsFromNGINXAllUP is a helper func used +// for faking response data from NGINX API. It responds +// with all upstreams and 'peers' in 'Up' state. +// +// Upstreams retrieved using NGINX API client: +// foo.tea.com -> upstream1, upstream2 +// bar.tea.com -> upstream2 +func getUpstreamsFromNGINXAllUp() (*client.Upstreams, error) { ups := client.Upstreams{ "upstream1": client.Upstream{ Peers: []client.Peer{ @@ -128,169 +187,85 @@ func getNginxUpstreams() (*client.Upstreams, error) { return &ups, nil } -func getUpstreamsForHost(host string) []string { - upstreams := map[string][]string{ - "foo.tea.com": {"upstream1", "upstream2"}, - "bar.tea.com": {"upstream1"}, - } - u, ok := upstreams[host] - if !ok { - return []string{} - } - return u -} - -var ( - upstreams = map[string][]string{ - "foo.tea.com": {"upstream1", "upstream2"}, - "bar.tea.com": {"upstream1"}, - } - - allPeersUp = client.Upstreams{ +// getUpstreamsFromNGINXAllUnhealthy is a helper func used +// for faking response data from NGINX API. It responds +// with all upstreams and 'peers' in 'Down' (Unhealthy) state. +// +// Upstreams retrieved using NGINX API client: +// foo.tea.com -> upstream1, upstream2 +// bar.tea.com -> upstream2 +func getUpstreamsFromNGINXAllUnhealthy() (*client.Upstreams, error) { + ups := client.Upstreams{ "upstream1": client.Upstream{ Peers: []client.Peer{ - {State: "Up"}, - {State: "Up"}, - {State: "Up"}, + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, }, }, "upstream2": client.Upstream{ Peers: []client.Peer{ - {State: "Up"}, - {State: "Up"}, - {State: "Up"}, + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, }, }, "upstream3": client.Upstream{ Peers: []client.Peer{ - {State: "Up"}, - {State: "Up"}, - {State: "Up"}, + {State: "Down"}, + {State: "Down"}, + {State: "Down"}, }, }, } + return &ups, nil +} - allPeersUnhealthy = client.Upstreams{ +// getUpstreamsFromNGINXPartiallyUp is a helper func used +// for faking response data from NGINX API. It responds +// with some upstreams and 'peers' in 'Down' (Unhealthy) state, +// and some upstreams and 'peers' in 'Up' state. +// +// Upstreams retrieved using NGINX API client +// foo.tea.com -> upstream1, upstream2 +// bar.tea.com -> upstream2 +func getUpstreamsFromNGINXPartiallyUp() (*client.Upstreams, error) { + ups := client.Upstreams{ "upstream1": client.Upstream{ Peers: []client.Peer{ {State: "Down"}, {State: "Down"}, - {State: "Down"}, + {State: "Up"}, }, }, "upstream2": client.Upstream{ Peers: []client.Peer{ {State: "Down"}, {State: "Down"}, - {State: "Down"}, + {State: "Up"}, }, }, "upstream3": client.Upstream{ Peers: []client.Peer{ {State: "Down"}, - {State: "Down"}, + {State: "Up"}, {State: "Down"}, }, }, } + return &ups, nil +} - validCert = []byte(`-----BEGIN CERTIFICATE----- -MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJJ -RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD -DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu -aW8wHhcNMjIxMTA1MTQzNDE1WhcNMzIxMTAyMTQzNDE1WjBuMQswCQYDVQQGEwJJ -RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD -DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu -aW8wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDVz06I4rTqOSI4bnEJ -GVy17QytuCiCZm0iPmjw2EJrc21FTk23zscHOMUAOc2HodeUmYyBjo+ZnPl2Tk9i -dyWU3wou3ZIQQaOi87meJ/evltUHiC49olNsYe9U8bB31/6URFKaMH7rD7zfAXpS -DbWdd84g7hfZIMQSLRPdBz958lkVPaSfPua58LkKZgmkThvh5Ah0HNKPn0z9idTQ -5oftFlPYTHvXvFYPoVNOjYfVbqxnmDJrbuqy0tkVjFoYHrT4aNkFIS/CgFjpYwb4 -j8yuprFNCAGjS7hDUDQaeHNKqTWvk+QT28pLNXc1BfA88DTMb0G6glZi67sDeN9H -q4K7AgMBAAEwDQYJKoZIhvcNAQELBQADggEBACAuyHRQodEaql4QXb5mGFSuQuAv -QxHdSSkdelDFf8s3ThBWgahuw9Lwz7FwXuFSh8tirK/3fb+OFwWB/xQdHjL6hl13 -ccxLNY/ydrKeHraLCWLu5TZ5BIvAHfFTpf5sbQrBkf4G4+Y3rX55asBTrzO9sPTT -bOjDHn+Eaa6QdEoyOvpRY+zGB1++XJqn/xFYjVrNg6Neh8/cfILSV46HNaqp3FSr -0NOWiGxG3Qk3UGoVQifhFNO5SsoYNfnDnwWx2cW4KTklxak0wt3KaloYbQUv2GKw -MsyNKpmyUpwnLnY0glII4IvP59oAEZR/wEI0bpM5ddWZuDu4Ie5WNJ+TG3g= ------END CERTIFICATE----- -`) - - validKey = []byte(`-----BEGIN RSA PRIVATE KEY----- -MIIEogIBAAKCAQEA1c9OiOK06jkiOG5xCRlcte0MrbgogmZtIj5o8NhCa3NtRU5N -t87HBzjFADnNh6HXlJmMgY6PmZz5dk5PYncllN8KLt2SEEGjovO5nif3r5bVB4gu -PaJTbGHvVPGwd9f+lERSmjB+6w+83wF6Ug21nXfOIO4X2SDEEi0T3Qc/efJZFT2k -nz7mufC5CmYJpE4b4eQIdBzSj59M/YnU0OaH7RZT2Ex717xWD6FTTo2H1W6sZ5gy -a27qstLZFYxaGB60+GjZBSEvwoBY6WMG+I/MrqaxTQgBo0u4Q1A0GnhzSqk1r5Pk -E9vKSzV3NQXwPPA0zG9BuoJWYuu7A3jfR6uCuwIDAQABAoIBADbyIZKYADo5GIw8 -BZx7AhJWqu1x6Ccqv10PgNR0Hw2SCkDHUL2tzAQVGLtoH2N9ufMcSrl4s3qclpdK -pKf/So8pimpk0oaO98iGrerxBnv/XRukaY25S4sM1/6SZfFGdswPitLJJ7SsxLLi -pFa14zhmc3iO9137R6gMIZCprixeHSiUrxe8f0L7m5KkyuyUN3+ItIT70+btTKlO -bkEAjKvahIWqbq5QDmsn+v0g2QEu5Y7QCxRuXIgPEj2/CDt6c1jodN/uuDIWBuSb -V2oeI9q79J5ccEC+Z5UlRx1+VBiaepRi1k9Dy6h3mZSCrUBeAreW1hriuVAKiP3I -o/G2wsECgYEA8jcEY4XVoL/vtrDzdW53usjR0v6R6RWRb+1/9Zm8XoZwu7onza3p -6P//qxp56eOKfDldpofYAbxov+kBzaLSQclGHWGZ5ZUEu8eJpl6TcJsZLOd85Hrk -AA4kVfoeQ99UTbMfghtZkdFydmkL/5ADFBa4y9Ds5hIOYQ5BLNadvasCgYEA4fpv -+sb43mFxTI1tJNVkAznW5bGbrfEVQcy3SAN4325hffiGE8emB9ckUJqHGvMzN6nX -iN9H+frzOqHWECThN0Zs2hgHNjkP3FMYCCUqJI1mT45qZNqZ3/pdQ5cqKL9ZW2cL -6sUheSIB9hBXquQk7RDjwvNj8bfFiEhtsSEHnzECgYBb7yv4RnUmVZO76QAPY4WI -XO7fQgbJzIjuTdwSsW6BBlBFwMuY0tkEuh4lqJ/7eYU3z2JPciI3znaH2P35OkLJ -+4ZkYoZSULSCPaNuhVk7FXOByr9pzYc6yiNaitvv8RWDhGiCLrVZloD2lrqaHuQ8 -PL+ZhMxWKyZQCmQMi81FjwKBgGHNTeGvc85rRenn27DxWhO7WLKYp9QkXxrXSwuz -1QB+eVtX0E+HPOhvyJvKBWc4kpYov8vRNwmN/u8FU+wwyfhuVnYdqCFjmOW2YNRF -oXOobvtHm+yCX858QRkbt3djOX1Bn/q/zrjqawbgE9E2ZHTltm2NgVgAPVG6Zx8e -OHpBAoGAfSDXJUddJOO41Myc/tmuTQcIpfVl/7xEahB6awjj2YUZVgV6366JZvvT -EAvtOoAzeD++O2fxQCXbxnlAXgn8hAgyOnW45Pmi6MuCr3uUJTxfNi8VEnH3RO7m -XVv4gl18+nrhTQT7M7iyLi2maa9FCjMwgLMjhYQr4Gs1kPKyhXA= ------END RSA PRIVATE KEY----- -`) - - invalidCert = []byte(`-----BEGIN CERTIFICATE----- -MIIDWDCCAkACCQDrQWfdxr0rezANBgkqhkiG9w0BAQsFADBuMQswCQYDVQQGEwJA -RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD -DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu -aW8wHhcNMjIxMTA1MTQzNDE1WhcNMzIxMTAyMTQzNDE1WjBuMQswCQYDVQQGEwJJ -RTEPMA0GA1UEBwwGRHVibGluMRMwEQYDVQQKDApTZXZlbkJ5dGVzMRYwFAYDVQQD -DA1zZXZlbmJ5dGVzLmlvMSEwHwYJKoZIhvcNAQkBFhJpbmZvQHNldmVuYnl0ZXMu -aW8wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDVz06I4rTqOSI4bnEJ -GVy17QytuCiCZm0iPmjw2EJrc21FTk23zscHOMUAOc2HodeUmYyBjo+ZnPl2Tk9i -dyWU3wou3ZIQQaOi87meJ/evltUHiC49olNsYe9U8bB31/6URFKaMH7rD7zfAXpS -DbWdd84g7hfZIMQSLRPdBz958lkVPaSfPua58LkKZgmkThvh5Ah0HNKPn0z9idTQ -5oftFlPYTHvXvFYPoVNOjYfVbqxnmDJrbuqy0tkVjFoYHrT4aNkFIS/CgFjpYwb4 -j8yuprFNCAGjS7hDUDQaeHNKqTWvk+QT28pLNXc1BfA88DTMb0G6glZi67sDeN9H -q4K7AgMBAAEwDQYJKoZIhvcNAQELBQADggEBACAuyHRQodEaql4QXb5mGFSuQuAv -QxHdSSkdelDFf8s3ThBWgahuw9Lwz7FwXuFSh8tirK/3fb+OFwWB/xQdHjL6hl13 -ccxLNY/ydrKeHraLCWLu5TZ5BIvAHfFTpf5sbQrBkf4G4+Y3rX55asBTrzO9sPTT -bOjDHn+Eaa6QdEoyOvpRY+zGB1++XJqn/xFYjVrNg6Neh8/cfILSV46HNaqp3FSr -0NOWiGxG3Qk3UGoVQifhFNO5SsoYNfnDnwWx2cW4KTklxak0wt3KaloYbQUv2GKw -MsyNKpmyUpwnLnY0glII4IvP59oAEZR/wEI0bpM5ddWZuDu4Ie5WNJ+TG3g= ------END CERTIFICATE-----`) - - invalidKey = []byte(`-----BEGIN RSA PRIVATE KEY----- -MIIEogIBAAKCAQEA1c9OiOK06jkiOG5xCRlcte0MrbgogmZtIj5o8NhCa3NtRU5N -t87HBzjFADnNh6HXlJmMgY6PmZz5dk5PYncllN8KLt2SEEGjovO5nif3r5bVB4gu -PaJTbGHvVPGwd9f+lERSmjB+6w+83wF6Ug21nXfOIO4X2SDEEi0T3Qc/efJZFT2k -nz7mufC5CmYJpE4b4eQIdBzSj59M/YnU0OaH7RZT2Ex717xWD6FTTo2H1W6sZ5gy -a27qstLZFYxaGB60+GjZBSEvwoBY6WMG+I/MrqaxTQgBo0u4Q1A0GnhzSqk1r5Pk -E9vKSzV3NQXwPPA0zG9BuoJWYuu7A3jfR6uCuwIDAQABAoIBADbyIZKYADo5GIw8 -BZx7AhJWqu1x6Ccqv10PgNR0Hw2SCkDHUL2tzAQVGLtoH2N9ufMcSrl4s3qclpdK -pKf/So8pimpk0oaO98iGrerxBnv/XRukaY25S4sM1/6SZfFGdswPitLJJ7SsxLLi -pFa14zhmc3iO9137R6gMIZCprixeHSiUrxe8f0L7m5KkyuyUN3+ItIT70+btTKlO -bkEAjKvahIWqbq5QDmsn+v0g2QEu5Y7QCxRuXIgPEj2/CDt6c1jodN/uuDIWBuSb -V2oeI9q79J5ccEC+Z5UlRx1+VBiaepRi1k9Dy6h3mZSCrUBeAreW1hriuVAKiP3I -o/G2wsECgYEA8jcEY4XVoL/vtrDzdW53usjR0v6R6RWRb+1/9Zm8XoZwu7onza3p -6P//qxp56eOKfDldpofYAbxov+kBzaLSQclGHWGZ5ZUEu8eJpl6TcJsZLOd85Hrk -AA4kVfoeQ99UTbMfghtZkdFydmkL/5ADFBa4y9Ds5hIOYQ5BLNadvasCgYEA4fpv -+sb43mFxTI1tJNVkAznW5bGbrfEVQcy3SAN4325hffiGE8emB9ckUJqHGvMzN6nX -iN9H+frzOqHWECThN0Zs2hgHNjkP3FMYCCUqJI1mT45qZNqZ3/pdQ5cqKL9ZW2cL -6sUheSIB9hBXquQk7RDjwvNj8bfFiEhtsSEHnzECgYBb7yv4RnUmVZO76QAPY4WI -XO7fQgbJzIjuTdwSsW6BBlBFwMuY0tkEuh4lqJ/7eYU3z2JPciI3znaH2P35OkLJ -+4ZkYoZSULSCPaNuhVk7FXOByr9pzYc6yiNaitvv8RWDhGiCLrVZloD2lrqaHuQ8 -PL+ZhMxWKyZQCmQMi81FjwKBgGHNTeGvc85rRenn27DxWhO7WLKYp9QkXxrXSwuz -1QB+eVtX0E+HPOhvyJvKBWc4kpYov8vRNwmN/u8FU+wwyfhuVnYdqCFjmOW2YNRF -oXOobvtHm+yCX858QRkbt3djOX1Bn/q/zrjqawbgE9E2ZHTltm2NgVgAPVG6Zx8e -OHpBAoGAfSDXJUddJOO41Myc/tmuTQcIpfVl/7xEahB6awjj2YUZVgV6366JZvvT -EAvtOoAzeD++O2fxQCXbxnlAXgn8hAgyOnW45Pmi6MuCr3uUJTxfNi8VEnH3RO7m -XVv4gl18+nrhTQT7M7iyLi2maa9FCjMwgLMjhYQr4Gs1kPKyhXA= ------END RSA PRIVATE KEY-----`) -) +// getUpstreamsFromNGINXNotExistingHost is a helper func used +// for faking response data from NGINX API. It responds +// with empty upstreams on a request for not existing host. +func getUpstreamsFromNGINXNotExistingHost() (*client.Upstreams, error) { + ups := client.Upstreams{} + return &ups, nil +} + +// getUpstreamsFromNGINXErrorFromAPI is a helper func used +// for faking err response from NGINX API client. +func getUpstreamsFromNGINXErrorFromAPI() (*client.Upstreams, error) { + return nil, errors.New("nginx api error") +} diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go index ec7373e0e8..702205588c 100644 --- a/internal/healthcheck/listener.go +++ b/internal/healthcheck/listener.go @@ -1,13 +1,22 @@ package healthcheck import ( - apiv1 "k8s.io/api/core/v1" "strconv" + apiv1 "k8s.io/api/core/v1" + + "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" ) +// RunHealthCheck starts the deep healthcheck service. +// +// If the server encounters an error and it can't start +// it exits with code 255 (glog.Fatal). func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *apiv1.Secret) { - RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) + err := RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) + if err != nil { + glog.Fatal(err) + } } From 19384514d170d9ec79fed036590f6b7fe7c03714 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 14 Nov 2022 22:25:05 +0000 Subject: [PATCH 18/66] Support min TLS v1.0 --- internal/healthcheck/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 88dde28133..11a693330a 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -48,7 +48,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), TLSConfig: &tls.Config{ Certificates: []tls.Certificate{tlsCert}, - MinVersion: tls.VersionTLS12, + MinVersion: tls.VersionTLS10, }, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, From b9e2d5f358ce2412d7ce713deb248038edd300fa Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 15 Nov 2022 10:21:43 +0000 Subject: [PATCH 19/66] Update min TLS version to 1.2 --- internal/healthcheck/http.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/healthcheck/http.go b/internal/healthcheck/http.go index 11a693330a..88dde28133 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/http.go @@ -48,7 +48,7 @@ func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Confi Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), TLSConfig: &tls.Config{ Certificates: []tls.Certificate{tlsCert}, - MinVersion: tls.VersionTLS10, + MinVersion: tls.VersionTLS12, }, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, From 3ee548020c61f2626afabcfe0ca8ce49d2a5dbd9 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 15 Nov 2022 10:48:53 +0000 Subject: [PATCH 20/66] Simplify package structure --- .../healthcheck/{http.go => healthcheck.go} | 25 ++++++++++++++----- .../{http_test.go => healthcheck_test.go} | 0 internal/healthcheck/listener.go | 22 ---------------- 3 files changed, 19 insertions(+), 28 deletions(-) rename internal/healthcheck/{http.go => healthcheck.go} (86%) rename internal/healthcheck/{http_test.go => healthcheck_test.go} (100%) delete mode 100644 internal/healthcheck/listener.go diff --git a/internal/healthcheck/http.go b/internal/healthcheck/healthcheck.go similarity index 86% rename from internal/healthcheck/http.go rename to internal/healthcheck/healthcheck.go index 88dde28133..303ca1f450 100644 --- a/internal/healthcheck/http.go +++ b/internal/healthcheck/healthcheck.go @@ -6,9 +6,11 @@ import ( "errors" "fmt" "net/http" + "strconv" "strings" "time" + apiv1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" "github.com/go-chi/chi" @@ -19,12 +21,23 @@ import ( "k8s.io/utils/strings/slices" ) +// RunHealthCheck starts the deep healthcheck service. +// +// If the server encounters an error and it can't start +// it exits with code 255 (glog.Fatal). +func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *apiv1.Secret) { + err := RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) + if err != nil { + glog.Fatal(err) + } +} + // RunHealtcheckServer takes config params, creates the health server and starts it. // It errors if the server can't be started or provided secret is not valid // (tls certificate cannot be created) and the health service with TLS support can't start. func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) error { - getUpstreamsForHost := UpstreamsForHost(cnf) - getUpstreamsFromNginx := NginxUpstreams(nc) + getUpstreamsForHost := upstreamsForHost(cnf) + getUpstreamsFromNginx := nginxUpstreams(nc) if secret == nil { healthServer := http.Server{ @@ -88,17 +101,17 @@ func API(upstreamsForHost func(string) []string, upstreamsFromNginx func() (*cli return mux } -// UpstreamsForHost takes configurator and returns a func +// upstreamsForHost takes configurator and returns a func // that is reposnsible for retrieving upstreams for the given hostname. -func UpstreamsForHost(cnf *configs.Configurator) func(hostname string) []string { +func upstreamsForHost(cnf *configs.Configurator) func(hostname string) []string { return func(hostname string) []string { return cnf.GetUpstreamsforHost(hostname) } } -// NginxUpstreams takes an instance of NGNX client and returns +// nginxUpstreams takes an instance of NGNX client and returns // a func that returns all upstreams. -func NginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { +func nginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { return func() (*client.Upstreams, error) { upstreams, err := nc.GetUpstreams() if err != nil { diff --git a/internal/healthcheck/http_test.go b/internal/healthcheck/healthcheck_test.go similarity index 100% rename from internal/healthcheck/http_test.go rename to internal/healthcheck/healthcheck_test.go diff --git a/internal/healthcheck/listener.go b/internal/healthcheck/listener.go deleted file mode 100644 index 702205588c..0000000000 --- a/internal/healthcheck/listener.go +++ /dev/null @@ -1,22 +0,0 @@ -package healthcheck - -import ( - "strconv" - - apiv1 "k8s.io/api/core/v1" - - "github.com/golang/glog" - "github.com/nginxinc/kubernetes-ingress/internal/configs" - "github.com/nginxinc/nginx-plus-go-client/client" -) - -// RunHealthCheck starts the deep healthcheck service. -// -// If the server encounters an error and it can't start -// it exits with code 255 (glog.Fatal). -func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *apiv1.Secret) { - err := RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) - if err != nil { - glog.Fatal(err) - } -} From dcebe1cef601ed1672775ec9b126115e349f09bd Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 15 Nov 2022 11:05:10 +0000 Subject: [PATCH 21/66] Remove double import --- internal/healthcheck/healthcheck.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 303ca1f450..dba6bc5af1 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -10,7 +10,6 @@ import ( "strings" "time" - apiv1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" "github.com/go-chi/chi" @@ -25,7 +24,7 @@ import ( // // If the server encounters an error and it can't start // it exits with code 255 (glog.Fatal). -func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *apiv1.Secret) { +func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *v1.Secret) { err := RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) if err != nil { glog.Fatal(err) From 6ba4114be87362972567b051939dbfbcf70083c2 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 15 Nov 2022 11:19:34 +0000 Subject: [PATCH 22/66] Round flow to the left --- internal/healthcheck/healthcheck.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index dba6bc5af1..375cf7e001 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -177,16 +177,18 @@ type HostStats struct { func countStats(upstreams *client.Upstreams, upstreamNames []string) HostStats { total, up := 0, 0 for name, u := range *upstreams { - if slices.Contains(upstreamNames, name) { - for _, p := range u.Peers { - total++ - if strings.ToLower(p.State) != "up" { - continue - } - up++ + if !slices.Contains(upstreamNames, name) { + continue + } + for _, p := range u.Peers { + total++ + if strings.ToLower(p.State) != "up" { + continue } + up++ } } + unhealthy := total - up return HostStats{ Total: total, From d4fa8fb1570434e794f3ba9fd34d7e4d38734a92 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 16 Nov 2022 15:19:18 +0000 Subject: [PATCH 23/66] Sanitize hostname param --- internal/healthcheck/healthcheck.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 375cf7e001..349f41058d 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -128,7 +128,7 @@ type HealthHandler struct { // Retrieve finds health stats for the host identified by a hostname in the request URL. func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { - hostname := chi.URLParam(r, "hostname") + hostname := strings.TrimSpace(chi.URLParam(r, "hostname")) upstreamNames := h.UpstreamsForHost(hostname) if len(upstreamNames) == 0 { From 9c01b845a68835e75f56e47f15efd0733c97c7b4 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 16 Nov 2022 16:29:24 +0000 Subject: [PATCH 24/66] Use different var name for sanitized input --- internal/healthcheck/healthcheck.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 349f41058d..562086bfe9 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -92,7 +92,7 @@ func API(upstreamsForHost func(string) []string, upstreamsFromNginx func() (*cli NginxUpstreams: upstreamsFromNginx, } mux := chi.NewRouter() - mux.Use(httprate.Limit(10, 1*time.Second, httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { + mux.Use(httprate.Limit(1000, 1*time.Second, httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { http.Error(w, "rate limit exceeded", http.StatusTooManyRequests) })), ) @@ -128,18 +128,19 @@ type HealthHandler struct { // Retrieve finds health stats for the host identified by a hostname in the request URL. func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { - hostname := strings.TrimSpace(chi.URLParam(r, "hostname")) + hostname := chi.URLParam(r, "hostname") + sanitizedHostname := strings.TrimSpace(hostname) - upstreamNames := h.UpstreamsForHost(hostname) + upstreamNames := h.UpstreamsForHost(sanitizedHostname) if len(upstreamNames) == 0 { - glog.Errorf("no upstreams for hostname %s or hostname does not exist", hostname) + glog.Errorf("no upstreams for hostname %s or hostname does not exist", sanitizedHostname) w.WriteHeader(http.StatusNotFound) return } upstreams, err := h.NginxUpstreams() if err != nil { - glog.Errorf("error retrieving upstreams for host: %s", hostname) + glog.Errorf("error retrieving upstreams for host: %s", sanitizedHostname) w.WriteHeader(http.StatusInternalServerError) return } From b4270c04ef3a29fb7898aabed9ced732f7f4b170 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 17 Nov 2022 12:55:58 +0000 Subject: [PATCH 25/66] Sanitize params --- internal/configs/virtualserver.go | 9 +++++---- internal/healthcheck/healthcheck.go | 13 ++++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 021f99621b..a565a70043 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -133,6 +133,8 @@ type upstreamNamer struct { } // NewUpstreamNamerForVirtualServer creates a new namer. +// +//nolint:golint func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf("vs_%s_%s", virtualServer.Namespace, virtualServer.Name), @@ -141,10 +143,9 @@ func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *ups } // NewUpstreamNamerForVirtualServerRoute creates a new namer. -func NewUpstreamNamerForVirtualServerRoute( - virtualServer *conf_v1.VirtualServer, - virtualServerRoute *conf_v1.VirtualServerRoute, -) *upstreamNamer { +// +//nolint:golint +func NewUpstreamNamerForVirtualServerRoute(virtualServer *conf_v1.VirtualServer, virtualServerRoute *conf_v1.VirtualServerRoute) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf( "vs_%s_%s_vsr_%s_%s", diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 562086bfe9..2bed34a60a 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -129,18 +129,18 @@ type HealthHandler struct { // Retrieve finds health stats for the host identified by a hostname in the request URL. func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { hostname := chi.URLParam(r, "hostname") - sanitizedHostname := strings.TrimSpace(hostname) + sanitizedHostname := sanitize(hostname) upstreamNames := h.UpstreamsForHost(sanitizedHostname) if len(upstreamNames) == 0 { - glog.Errorf("no upstreams for hostname %s or hostname does not exist", sanitizedHostname) + glog.Errorf("no upstreams for requested hostname %s or hostname does not exist", sanitizedHostname) w.WriteHeader(http.StatusNotFound) return } upstreams, err := h.NginxUpstreams() if err != nil { - glog.Errorf("error retrieving upstreams for host: %s", sanitizedHostname) + glog.Errorf("error retrieving upstreams for requested hostname: %s", sanitizedHostname) w.WriteHeader(http.StatusInternalServerError) return } @@ -165,6 +165,13 @@ func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { } } +func sanitize(s string) string { + hostname := strings.TrimSpace(s) + hostname = strings.ReplaceAll(hostname, "\n", "") + hostname = strings.ReplaceAll(hostname, "\r", "") + return hostname +} + // HostStats holds information about total, up and // unhealthy number of 'peers' associated with the // given host. From 4d8e4557b9d3ad7ffec765b6e0d39440857ab03b Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 17 Nov 2022 16:59:01 +0000 Subject: [PATCH 26/66] Add a middleware for hostname length validation --- internal/healthcheck/healthcheck.go | 36 +++++++++---- internal/healthcheck/healthcheck_test.go | 68 ++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 11 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 2bed34a60a..3a4272ffa7 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -1,6 +1,7 @@ package healthcheck import ( + "bytes" "crypto/tls" "encoding/json" "errors" @@ -13,7 +14,6 @@ import ( v1 "k8s.io/api/core/v1" "github.com/go-chi/chi" - "github.com/go-chi/httprate" "github.com/golang/glog" "github.com/nginxinc/kubernetes-ingress/internal/configs" "github.com/nginxinc/nginx-plus-go-client/client" @@ -92,11 +92,9 @@ func API(upstreamsForHost func(string) []string, upstreamsFromNginx func() (*cli NginxUpstreams: upstreamsFromNginx, } mux := chi.NewRouter() - mux.Use(httprate.Limit(1000, 1*time.Second, httprate.WithLimitHandler(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "rate limit exceeded", http.StatusTooManyRequests) - })), - ) - mux.MethodFunc(http.MethodGet, "/probe/{hostname}", health.Retrieve) + mux.Route("/probe", func(r chi.Router) { + r.With(hostnameLengthVerifier).Get("/{hostname}", health.Retrieve) + }) return mux } @@ -129,18 +127,18 @@ type HealthHandler struct { // Retrieve finds health stats for the host identified by a hostname in the request URL. func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { hostname := chi.URLParam(r, "hostname") - sanitizedHostname := sanitize(hostname) + host := sanitize(hostname) - upstreamNames := h.UpstreamsForHost(sanitizedHostname) + upstreamNames := h.UpstreamsForHost(host) if len(upstreamNames) == 0 { - glog.Errorf("no upstreams for requested hostname %s or hostname does not exist", sanitizedHostname) + glog.Errorf("no upstreams for requested hostname %s or hostname does not exist", host) w.WriteHeader(http.StatusNotFound) return } upstreams, err := h.NginxUpstreams() if err != nil { - glog.Errorf("error retrieving upstreams for requested hostname: %s", sanitizedHostname) + glog.Errorf("error retrieving upstreams for requested hostname: %s", host) w.WriteHeader(http.StatusInternalServerError) return } @@ -172,6 +170,22 @@ func sanitize(s string) string { return hostname } +// hostnameLengthVerifier is a middleware that checks max +// length of the hostname param passed in the probe url. +// +// ref: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DomainNameFormat.html +func hostnameLengthVerifier(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + name := chi.URLParam(r, "hostname") + length := bytes.NewReader([]byte(name)).Len() + if length > 255 { + w.WriteHeader(http.StatusRequestURITooLong) + return + } + h.ServeHTTP(w, r) + }) +} + // HostStats holds information about total, up and // unhealthy number of 'peers' associated with the // given host. @@ -181,7 +195,7 @@ type HostStats struct { Unhealthy int } -// countStats calculates and returns statistics. +// countStats calculates and returns statistics for a host. func countStats(upstreams *client.Upstreams, upstreamNames []string) HostStats { total, up := 0, 0 for name, u := range *upstreams { diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go index ae72535730..49cc9d819d 100644 --- a/internal/healthcheck/healthcheck_test.go +++ b/internal/healthcheck/healthcheck_test.go @@ -4,6 +4,8 @@ import ( "context" "encoding/json" "errors" + "fmt" + "math/rand" "net/http" "net/http/httptest" "testing" @@ -13,6 +15,72 @@ import ( "github.com/nginxinc/nginx-plus-go-client/client" ) +func TestHealthCheckServer_ReturnsValidHTTPErrorOnMissingHostname(t *testing.T) { + t.Parallel() + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/", nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusNotFound, resp.Code) { + t.Error(cmp.Diff(http.StatusNotFound, resp.Code)) + } +} + +func generateStringOfLength(n int) string { + const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" + b := make([]byte, n) + for i := range b { + b[i] = letterBytes[rand.Intn(len(letterBytes))] + } + return string(b) +} + +func TestHealthCheckServer_ReturnsValidHTTPErrorOnTooLongHostname(t *testing.T) { + t.Parallel() + + hostname := generateStringOfLength(256) + url := fmt.Sprintf("/probe/%s", hostname) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusRequestURITooLong, resp.Code) { + t.Error(cmp.Diff(http.StatusRequestURITooLong, resp.Code)) + } +} + +func TestHealthCheckServer_ReturnsValidHTTPCodeOnValidHostnameLength(t *testing.T) { + t.Parallel() + + hostname := generateStringOfLength(254) + url := fmt.Sprintf("/probe/%s", hostname) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) + if err != nil { + t.Fatal(err) + } + resp := httptest.NewRecorder() + + h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) + h.ServeHTTP(resp, req) + + if !cmp.Equal(http.StatusNotFound, resp.Code) { + t.Error(cmp.Diff(http.StatusNotFound, resp.Code)) + } +} + func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersUpOnValidHostname(t *testing.T) { t.Parallel() From 04a6b61aef0845c7373d61ba43f51ff06c775229 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 17 Nov 2022 17:01:46 +0000 Subject: [PATCH 27/66] Remove unused dependency --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index 92a8273bb6..8cc12678da 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/marketplacemetering v1.13.22 github.com/cert-manager/cert-manager v1.10.0 github.com/go-chi/chi v1.5.4 - github.com/go-chi/httprate v0.7.0 github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang/glog v1.0.0 github.com/google/go-cmp v0.5.9 diff --git a/go.sum b/go.sum index 4be2fca18d..a5a03f9527 100644 --- a/go.sum +++ b/go.sum @@ -114,8 +114,6 @@ github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= github.com/go-chi/chi v1.5.4 h1:QHdzF2szwjqVV4wmByUnTcsbIg7UGaQ0tPF2t5GcAIs= github.com/go-chi/chi v1.5.4/go.mod h1:uaf8YgoFazUOkPBG7fxPftUylNumIev9awIWOENIuEg= -github.com/go-chi/httprate v0.7.0 h1:8W0dF7Xa2Duz2p8ncGaehIphrxQGNlOtoGY0+NRRfjQ= -github.com/go-chi/httprate v0.7.0/go.mod h1:6GOYBSwnpra4CQfAKXu8sQZg+nZ0M1g9QnyFvxrAB8A= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= From ae9c6d61b6f75230a0c0f716639cedf694306f65 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 17 Nov 2022 17:21:08 +0000 Subject: [PATCH 28/66] Fix linter issues --- internal/configs/virtualserver.go | 4 ++-- internal/healthcheck/healthcheck_test.go | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index a565a70043..da05e3b787 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -134,7 +134,7 @@ type upstreamNamer struct { // NewUpstreamNamerForVirtualServer creates a new namer. // -//nolint:golint +//nolint:revive func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf("vs_%s_%s", virtualServer.Namespace, virtualServer.Name), @@ -144,7 +144,7 @@ func NewUpstreamNamerForVirtualServer(virtualServer *conf_v1.VirtualServer) *ups // NewUpstreamNamerForVirtualServerRoute creates a new namer. // -//nolint:golint +//nolint:revive func NewUpstreamNamerForVirtualServerRoute(virtualServer *conf_v1.VirtualServer, virtualServerRoute *conf_v1.VirtualServerRoute) *upstreamNamer { return &upstreamNamer{ prefix: fmt.Sprintf( diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go index 49cc9d819d..4ddc6cb3fe 100644 --- a/internal/healthcheck/healthcheck_test.go +++ b/internal/healthcheck/healthcheck_test.go @@ -32,11 +32,12 @@ func TestHealthCheckServer_ReturnsValidHTTPErrorOnMissingHostname(t *testing.T) } } +//nolint:gosec func generateStringOfLength(n int) string { const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" b := make([]byte, n) for i := range b { - b[i] = letterBytes[rand.Intn(len(letterBytes))] + b[i] = letterBytes[rand.Intn(len(letterBytes))] //golint:gosec } return string(b) } From a7ea1870e78d2a7632dfc14defff1f48796c3824 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 21 Nov 2022 10:54:28 +0000 Subject: [PATCH 29/66] Simplify healthcheck server and tests --- internal/healthcheck/doc.go | 2 - internal/healthcheck/healthcheck.go | 152 ++++++++----------- internal/healthcheck/healthcheck_test.go | 182 ++++++++++++++--------- 3 files changed, 175 insertions(+), 161 deletions(-) delete mode 100644 internal/healthcheck/doc.go diff --git a/internal/healthcheck/doc.go b/internal/healthcheck/doc.go deleted file mode 100644 index bef2038916..0000000000 --- a/internal/healthcheck/doc.go +++ /dev/null @@ -1,2 +0,0 @@ -// Package healthcheck provides a deep healtcheck service. -package healthcheck diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 3a4272ffa7..120a1bd601 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -1,7 +1,9 @@ +// Package healthcheck provides primitives for running deep healtcheck service. package healthcheck import ( "bytes" + "context" "crypto/tls" "encoding/json" "errors" @@ -21,122 +23,82 @@ import ( ) // RunHealthCheck starts the deep healthcheck service. -// -// If the server encounters an error and it can't start -// it exits with code 255 (glog.Fatal). func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *v1.Secret) { - err := RunHealtcheckServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) + hs, err := NewHealthServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) if err != nil { glog.Fatal(err) } + glog.Fatal(hs.ListenAndServe()) +} + +// HealthServer holds data required for running +// the healthcheck server. +type HealthServer struct { + Server *http.Server + URL string + UpstreamsForHost func(host string) []string + NginxUpstreams func() (*client.Upstreams, error) } -// RunHealtcheckServer takes config params, creates the health server and starts it. -// It errors if the server can't be started or provided secret is not valid -// (tls certificate cannot be created) and the health service with TLS support can't start. -func RunHealtcheckServer(port string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) error { - getUpstreamsForHost := upstreamsForHost(cnf) - getUpstreamsFromNginx := nginxUpstreams(nc) - - if secret == nil { - healthServer := http.Server{ - Addr: fmt.Sprintf(":%s", port), - Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), +// NewHealthServer creates Health Server. If secret is provided, +// the server is configured with TLS Config. +func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) (*HealthServer, error) { + hs := HealthServer{ + Server: &http.Server{ + Addr: addr, ReadTimeout: 10 * time.Second, WriteTimeout: 10 * time.Second, - } - if err := healthServer.ListenAndServe(); err != nil { - return fmt.Errorf("starting healthcheck server: %w", err) - } - } - - tlsCert, err := makeCert(secret) - if err != nil { - return fmt.Errorf("creating tls cert %w", err) + }, + URL: fmt.Sprintf("http://%s/", addr), + UpstreamsForHost: cnf.GetUpstreamsforHost, + NginxUpstreams: nc.GetUpstreams, } - healthServer := http.Server{ - Addr: fmt.Sprintf(":%s", port), - Handler: API(getUpstreamsForHost, getUpstreamsFromNginx), - TLSConfig: &tls.Config{ + if secret != nil { + tlsCert, err := makeCert(secret) + if err != nil { + return nil, fmt.Errorf("creating tls sert: %w", err) + } + hs.Server.TLSConfig = &tls.Config{ Certificates: []tls.Certificate{tlsCert}, MinVersion: tls.VersionTLS12, - }, - ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, - } - if err := healthServer.ListenAndServeTLS("", ""); err != nil { - return fmt.Errorf("starting healthcheck tls server: %w", err) - } - return nil -} - -// makeCert takes k8s Secret and returns tls Certificate for the server. -// It errors if either cert, or key are not present in the Secret. -func makeCert(s *v1.Secret) (tls.Certificate, error) { - cert, ok := s.Data[v1.TLSCertKey] - if !ok { - return tls.Certificate{}, errors.New("missing tls cert") - } - key, ok := s.Data[v1.TLSPrivateKeyKey] - if !ok { - return tls.Certificate{}, errors.New("missing tls key") + } + hs.URL = fmt.Sprintf("https://%s/", addr) } - return tls.X509KeyPair(cert, key) + return &hs, nil } -// API constructs an http.Handler with all healtcheck routes. -func API(upstreamsForHost func(string) []string, upstreamsFromNginx func() (*client.Upstreams, error)) http.Handler { - health := HealthHandler{ - UpstreamsForHost: upstreamsForHost, - NginxUpstreams: upstreamsFromNginx, - } +// ListenAndServe starts healthcheck server. +func (hs *HealthServer) ListenAndServe() error { mux := chi.NewRouter() mux.Route("/probe", func(r chi.Router) { - r.With(hostnameLengthVerifier).Get("/{hostname}", health.Retrieve) + r.With(hostnameLengthVerifier).Get("/{hostname}", hs.Retrieve) }) - return mux -} - -// upstreamsForHost takes configurator and returns a func -// that is reposnsible for retrieving upstreams for the given hostname. -func upstreamsForHost(cnf *configs.Configurator) func(hostname string) []string { - return func(hostname string) []string { - return cnf.GetUpstreamsforHost(hostname) + hs.Server.Handler = mux + if hs.Server.TLSConfig != nil { + return hs.Server.ListenAndServeTLS("", "") } + return hs.Server.ListenAndServe() } -// nginxUpstreams takes an instance of NGNX client and returns -// a func that returns all upstreams. -func nginxUpstreams(nc *client.NginxClient) func() (*client.Upstreams, error) { - return func() (*client.Upstreams, error) { - upstreams, err := nc.GetUpstreams() - if err != nil { - return nil, err - } - return upstreams, nil - } -} - -// HealthHandler holds dependency for its method(s). -type HealthHandler struct { - UpstreamsForHost func(host string) []string - NginxUpstreams func() (*client.Upstreams, error) +// Shutdown shuts down healthcheck server. +func (hs *HealthServer) Shutdown(ctx context.Context) error { + return hs.Server.Shutdown(ctx) } // Retrieve finds health stats for the host identified by a hostname in the request URL. -func (h *HealthHandler) Retrieve(w http.ResponseWriter, r *http.Request) { +func (hs *HealthServer) Retrieve(w http.ResponseWriter, r *http.Request) { hostname := chi.URLParam(r, "hostname") host := sanitize(hostname) - upstreamNames := h.UpstreamsForHost(host) + upstreamNames := hs.UpstreamsForHost(host) if len(upstreamNames) == 0 { glog.Errorf("no upstreams for requested hostname %s or hostname does not exist", host) w.WriteHeader(http.StatusNotFound) return } - upstreams, err := h.NginxUpstreams() + upstreams, err := hs.NginxUpstreams() if err != nil { glog.Errorf("error retrieving upstreams for requested hostname: %s", host) w.WriteHeader(http.StatusInternalServerError) @@ -170,15 +132,33 @@ func sanitize(s string) string { return hostname } -// hostnameLengthVerifier is a middleware that checks max -// length of the hostname param passed in the probe url. +// makeCert takes k8s Secret and returns tls Certificate for the server. +// It errors if either cert, or key are not present in the Secret. +func makeCert(s *v1.Secret) (tls.Certificate, error) { + cert, ok := s.Data[v1.TLSCertKey] + if !ok { + return tls.Certificate{}, errors.New("missing tls cert") + } + key, ok := s.Data[v1.TLSPrivateKeyKey] + if !ok { + return tls.Certificate{}, errors.New("missing tls key") + } + return tls.X509KeyPair(cert, key) +} + +// MaxHostnameLength is the max number of bytes +// that the healthcheck service accepts for hostname. // // ref: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DomainNameFormat.html +const MaxHostnameLength = 255 + +// hostnameLengthVerifier is a middleware that checks max +// length of the hostname param passed in the probe url. func hostnameLengthVerifier(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { name := chi.URLParam(r, "hostname") length := bytes.NewReader([]byte(name)).Len() - if length > 255 { + if length > MaxHostnameLength { w.WriteHeader(http.StatusRequestURITooLong) return } diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go index 4ddc6cb3fe..a3e1e8e81f 100644 --- a/internal/healthcheck/healthcheck_test.go +++ b/internal/healthcheck/healthcheck_test.go @@ -4,10 +4,10 @@ import ( "context" "encoding/json" "errors" - "fmt" + "log" "math/rand" + "net" "net/http" - "net/http/httptest" "testing" "github.com/google/go-cmp/cmp" @@ -15,20 +15,54 @@ import ( "github.com/nginxinc/nginx-plus-go-client/client" ) -func TestHealthCheckServer_ReturnsValidHTTPErrorOnMissingHostname(t *testing.T) { - t.Parallel() +// newTestHealthServer is a helper func responsible for creating, +// starting and shutting down healthcheck server for each test. +func newTestHealthServer(t *testing.T) *healthcheck.HealthServer { + t.Helper() + + l, err := net.Listen("tcp", ":0") //nolint:gosec + if err != nil { + t.Fatal(err) + } + defer l.Close() //nolint:errcheck - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/", nil) + addr := l.Addr().String() + hs, err := healthcheck.NewHealthServer(addr, nil, nil, nil) if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) - h.ServeHTTP(resp, req) + go func() { + err := hs.ListenAndServe() + if !errors.Is(err, http.ErrServerClosed) { + log.Fatal(err) + } + }() + + t.Cleanup(func() { + err := hs.Shutdown(context.Background()) + if err != nil { + t.Fatal(err) + } + }) + return hs +} + +func TestHealthCheckServer_Returns404OnMissingHostname(t *testing.T) { + t.Parallel() - if !cmp.Equal(http.StatusNotFound, resp.Code) { - t.Error(cmp.Diff(http.StatusNotFound, resp.Code)) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXAllUp + + resp, err := http.Get(hs.URL + "probe/") //nolint:noctx + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() //nolint:errcheck + + if resp.StatusCode != http.StatusNotFound { + t.Error(resp.StatusCode) } } @@ -37,65 +71,64 @@ func generateStringOfLength(n int) string { const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" b := make([]byte, n) for i := range b { - b[i] = letterBytes[rand.Intn(len(letterBytes))] //golint:gosec + b[i] = letterBytes[rand.Intn(len(letterBytes))] } return string(b) } -func TestHealthCheckServer_ReturnsValidHTTPErrorOnTooLongHostname(t *testing.T) { +func TestHealthCheckServer_Returns414OnTooLongHostname(t *testing.T) { t.Parallel() - hostname := generateStringOfLength(256) - url := fmt.Sprintf("/probe/%s", hostname) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXAllUp - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) + hostname := generateStringOfLength(256) + resp, err := http.Get(hs.URL + "probe/" + hostname) //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() + defer resp.Body.Close() //nolint:errcheck - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) - h.ServeHTTP(resp, req) - - if !cmp.Equal(http.StatusRequestURITooLong, resp.Code) { - t.Error(cmp.Diff(http.StatusRequestURITooLong, resp.Code)) + if resp.StatusCode != http.StatusRequestURITooLong { + t.Error(resp.StatusCode) } } -func TestHealthCheckServer_ReturnsValidHTTPCodeOnValidHostnameLength(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectHTTPForValidHostnameLength(t *testing.T) { t.Parallel() - hostname := generateStringOfLength(254) - url := fmt.Sprintf("/probe/%s", hostname) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXAllUp - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, url, nil) + hostname := generateStringOfLength(254) + resp, err := http.Get(hs.URL + "probe/" + hostname) //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() - - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) - h.ServeHTTP(resp, req) + defer resp.Body.Close() //nolint:errcheck - if !cmp.Equal(http.StatusNotFound, resp.Code) { - t.Error(cmp.Diff(http.StatusNotFound, resp.Code)) + if resp.StatusCode != http.StatusNotFound { + t.Error(resp.StatusCode) } } -func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersUpOnValidHostname(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsForHostnameForAllPeersUp(t *testing.T) { t.Parallel() - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXAllUp + + resp, err := http.Get(hs.URL + "probe/bar.tea.com") //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() - - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUp) - h.ServeHTTP(resp, req) + defer resp.Body.Close() //nolint:errcheck - if !cmp.Equal(http.StatusOK, resp.Code) { - t.Error(cmp.Diff(http.StatusOK, resp.Code)) + if resp.StatusCode != http.StatusOK { + t.Fatal(resp.StatusCode) } want := healthcheck.HostStats{ @@ -104,7 +137,7 @@ func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersUpOnValid Unhealthy: 0, } var got healthcheck.HostStats - if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + if err := json.NewDecoder(resp.Body).Decode(&got); err != nil { t.Fatal(err) } if !cmp.Equal(want, got) { @@ -112,20 +145,21 @@ func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersUpOnValid } } -func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersDownOnValidHostname(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnAllPeersDown(t *testing.T) { t.Parallel() - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXAllUnhealthy + + resp, err := http.Get(hs.URL + "probe/bar.tea.com") //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() + defer resp.Body.Close() //nolint:errcheck - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXAllUnhealthy) - h.ServeHTTP(resp, req) - - if !cmp.Equal(http.StatusServiceUnavailable, resp.Code) { - t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) + if resp.StatusCode != http.StatusServiceUnavailable { + t.Fatal(resp.StatusCode) } want := healthcheck.HostStats{ @@ -135,7 +169,7 @@ func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersDownOnVal } var got healthcheck.HostStats - if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + if err := json.NewDecoder(resp.Body).Decode(&got); err != nil { t.Fatal(err) } if !cmp.Equal(want, got) { @@ -143,20 +177,21 @@ func TestHealthCheckServer_ReturnsValidStatsAndValidHTTPCodeForAllPeersDownOnVal } } -func TestHealthCheckServer_ReturnsValidStatsAndCorrectHTTPStatusCodeForPartOfPeersDownOnValidHostname(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnPartOfPeersDown(t *testing.T) { t.Parallel() - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/bar.tea.com", nil) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXPartiallyUp + + resp, err := http.Get(hs.URL + "probe/bar.tea.com") //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() + defer resp.Body.Close() //nolint:errcheck - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXPartiallyUp) - h.ServeHTTP(resp, req) - - if !cmp.Equal(http.StatusOK, resp.Code) { - t.Error(cmp.Diff(http.StatusOK, resp.Code)) + if resp.StatusCode != http.StatusOK { + t.Fatal(resp.StatusCode) } want := healthcheck.HostStats{ @@ -166,7 +201,7 @@ func TestHealthCheckServer_ReturnsValidStatsAndCorrectHTTPStatusCodeForPartOfPee } var got healthcheck.HostStats - if err := json.Unmarshal(resp.Body.Bytes(), &got); err != nil { + if err := json.NewDecoder(resp.Body).Decode(&got); err != nil { t.Fatal(err) } if !cmp.Equal(want, got) { @@ -177,35 +212,36 @@ func TestHealthCheckServer_ReturnsValidStatsAndCorrectHTTPStatusCodeForPartOfPee func TestHealthCheckServer_RespondsWithHTTPErrCodeOnNotExistingHostname(t *testing.T) { t.Parallel() - // 'foo.mocha.org' represents not existing hostname - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/foo.mocha.org", nil) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXNotExistingHost + + resp, err := http.Get(hs.URL + "probe/foo.mocha.com") //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() + defer resp.Body.Close() //nolint:errcheck - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXNotExistingHost) - h.ServeHTTP(resp, req) - - if !cmp.Equal(http.StatusNotFound, resp.Code) { - t.Error(cmp.Diff(http.StatusServiceUnavailable, resp.Code)) + if resp.StatusCode != http.StatusNotFound { + t.Error(resp.StatusCode) } } func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnErrorFromNGINXAPI(t *testing.T) { t.Parallel() - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/probe/foo.tea.com", nil) + hs := newTestHealthServer(t) + hs.UpstreamsForHost = getUpstreamsForHost + hs.NginxUpstreams = getUpstreamsFromNGINXErrorFromAPI + + resp, err := http.Get(hs.URL + "probe/foo.tea.com") //nolint:noctx if err != nil { t.Fatal(err) } - resp := httptest.NewRecorder() - - h := healthcheck.API(getUpstreamsForHost, getUpstreamsFromNGINXErrorFromAPI) - h.ServeHTTP(resp, req) + defer resp.Body.Close() //nolint:errcheck - if !cmp.Equal(http.StatusInternalServerError, resp.Code) { - t.Error(cmp.Diff(http.StatusInternalServerError, resp.Code)) + if resp.StatusCode != http.StatusInternalServerError { + t.Error(resp.StatusCode) } } From 3982350e393ebfc0efead49c69b5b039956005b7 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 21 Nov 2022 11:14:21 +0000 Subject: [PATCH 30/66] Update dependencies --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index e4b9527736..751688206a 100644 --- a/go.mod +++ b/go.mod @@ -3,10 +3,10 @@ module github.com/nginxinc/kubernetes-ingress go 1.19 require ( - github.com/go-chi/chi v1.5.4 github.com/aws/aws-sdk-go-v2/config v1.18.2 github.com/aws/aws-sdk-go-v2/service/marketplacemetering v1.13.23 github.com/cert-manager/cert-manager v1.10.1 + github.com/go-chi/chi v1.5.4 github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang/glog v1.0.0 github.com/google/go-cmp v0.5.9 From 0b33646cfee28a7a362cc35f2047c81aacf6e693 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 23 Nov 2022 16:01:28 +0000 Subject: [PATCH 31/66] Remove hostname validation --- internal/healthcheck/healthcheck.go | 25 +--------- internal/healthcheck/healthcheck_test.go | 59 ++---------------------- 2 files changed, 6 insertions(+), 78 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 120a1bd601..c9639d75f2 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -2,7 +2,6 @@ package healthcheck import ( - "bytes" "context" "crypto/tls" "encoding/json" @@ -71,9 +70,7 @@ func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configura // ListenAndServe starts healthcheck server. func (hs *HealthServer) ListenAndServe() error { mux := chi.NewRouter() - mux.Route("/probe", func(r chi.Router) { - r.With(hostnameLengthVerifier).Get("/{hostname}", hs.Retrieve) - }) + mux.Get("/probe/{hostname}", hs.Retrieve) hs.Server.Handler = mux if hs.Server.TLSConfig != nil { return hs.Server.ListenAndServeTLS("", "") @@ -146,26 +143,6 @@ func makeCert(s *v1.Secret) (tls.Certificate, error) { return tls.X509KeyPair(cert, key) } -// MaxHostnameLength is the max number of bytes -// that the healthcheck service accepts for hostname. -// -// ref: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DomainNameFormat.html -const MaxHostnameLength = 255 - -// hostnameLengthVerifier is a middleware that checks max -// length of the hostname param passed in the probe url. -func hostnameLengthVerifier(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - name := chi.URLParam(r, "hostname") - length := bytes.NewReader([]byte(name)).Len() - if length > MaxHostnameLength { - w.WriteHeader(http.StatusRequestURITooLong) - return - } - h.ServeHTTP(w, r) - }) -} - // HostStats holds information about total, up and // unhealthy number of 'peers' associated with the // given host. diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go index a3e1e8e81f..24ab5aa81b 100644 --- a/internal/healthcheck/healthcheck_test.go +++ b/internal/healthcheck/healthcheck_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "log" - "math/rand" "net" "net/http" "testing" @@ -66,55 +65,7 @@ func TestHealthCheckServer_Returns404OnMissingHostname(t *testing.T) { } } -//nolint:gosec -func generateStringOfLength(n int) string { - const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" - b := make([]byte, n) - for i := range b { - b[i] = letterBytes[rand.Intn(len(letterBytes))] - } - return string(b) -} - -func TestHealthCheckServer_Returns414OnTooLongHostname(t *testing.T) { - t.Parallel() - - hs := newTestHealthServer(t) - hs.UpstreamsForHost = getUpstreamsForHost - hs.NginxUpstreams = getUpstreamsFromNGINXAllUp - - hostname := generateStringOfLength(256) - resp, err := http.Get(hs.URL + "probe/" + hostname) //nolint:noctx - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() //nolint:errcheck - - if resp.StatusCode != http.StatusRequestURITooLong { - t.Error(resp.StatusCode) - } -} - -func TestHealthCheckServer_ReturnsCorrectHTTPForValidHostnameLength(t *testing.T) { - t.Parallel() - - hs := newTestHealthServer(t) - hs.UpstreamsForHost = getUpstreamsForHost - hs.NginxUpstreams = getUpstreamsFromNGINXAllUp - - hostname := generateStringOfLength(254) - resp, err := http.Get(hs.URL + "probe/" + hostname) //nolint:noctx - if err != nil { - t.Fatal(err) - } - defer resp.Body.Close() //nolint:errcheck - - if resp.StatusCode != http.StatusNotFound { - t.Error(resp.StatusCode) - } -} - -func TestHealthCheckServer_ReturnsCorrectStatsForHostnameForAllPeersUp(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsForHostnameOnAllPeersUp(t *testing.T) { t.Parallel() hs := newTestHealthServer(t) @@ -145,7 +96,7 @@ func TestHealthCheckServer_ReturnsCorrectStatsForHostnameForAllPeersUp(t *testin } } -func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnAllPeersDown(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsForHostnameOnAllPeersDown(t *testing.T) { t.Parallel() hs := newTestHealthServer(t) @@ -177,7 +128,7 @@ func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnAll } } -func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnPartOfPeersDown(t *testing.T) { +func TestHealthCheckServer_ReturnsCorrectStatsForValidHostnameOnPartOfPeersDown(t *testing.T) { t.Parallel() hs := newTestHealthServer(t) @@ -209,7 +160,7 @@ func TestHealthCheckServer_ReturnsCorrectStatsAndCorrectHTTPCodeForHostnameOnPar } } -func TestHealthCheckServer_RespondsWithHTTPErrCodeOnNotExistingHostname(t *testing.T) { +func TestHealthCheckServer_RespondsWith404OnNotExistingHostname(t *testing.T) { t.Parallel() hs := newTestHealthServer(t) @@ -227,7 +178,7 @@ func TestHealthCheckServer_RespondsWithHTTPErrCodeOnNotExistingHostname(t *testi } } -func TestHealthCheckServer_RespondsWithCorrectHTTPStatusCodeOnErrorFromNGINXAPI(t *testing.T) { +func TestHealthCheckServer_RespondsWith500OnErrorFromNGINXAPI(t *testing.T) { t.Parallel() hs := newTestHealthServer(t) From c146e699e105eb9ff8e2849d50a457b7874736bc Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 23 Nov 2022 18:19:05 +0000 Subject: [PATCH 32/66] Rename healthcheck to service insight --- cmd/nginx-ingress/flags.go | 22 +++++++++++----------- cmd/nginx-ingress/main.go | 16 ++++++++-------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 50acbc780a..b7808d1b3b 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -135,14 +135,14 @@ var ( prometheusMetricsListenPort = flag.Int("prometheus-metrics-listen-port", 9113, "Set the port where the Prometheus metrics are exposed. [1024 - 65535]") - enableHealthProbe = flag.Bool("enable-health-probe", false, - `Enable health probe for external load balancers. Requires -nginx-plus`) + enableServiceInsight = flag.Bool("enable-service-insight", false, + `Enable service insight for external load balancers. Requires -nginx-plus`) - healthProbeTLSSecretName = flag.String("health-probe-tls-secret", "", - `A Secret with a TLS certificate and key for TLS termination of the health probe.`) + serviceInsightTLSSecretName = flag.String("service-insight-tls-secret", "", + `A Secret with a TLS certificate and key for TLS termination of the service insight.`) - healthProbeListenPort = flag.Int("health-probe-listen-port", 9000, - "Enable health probe and set the port where the health probe endpoint are exposed. Requires -nginx-plus. [1024 - 65535]") + serviceInsightListenPort = flag.Int("service-insight-listen-port", 9114, + "Enable service insight and set the port where the service insight endpoint are exposed. Requires -nginx-plus. [1024 - 65535]") enableCustomResources = flag.Bool("enable-custom-resources", true, "Enable custom resources") @@ -265,9 +265,9 @@ func parseFlags() { *enableLatencyMetrics = false } - if *enableHealthProbe && !*nginxPlus { - glog.Warning("enable-health-probe flag support is for NGINX Plus, health probe endpoint will not be exposed") - *enableHealthProbe = false + if *enableServiceInsight && !*nginxPlus { + glog.Warning("enable-service-insight flag support is for NGINX Plus, service insight endpoint will not be exposed") + *enableServiceInsight = false } if *enableCertManager && !*enableCustomResources { @@ -346,9 +346,9 @@ func validationChecks() { glog.Fatalf("Invalid value for ready-status-port: %v", readyStatusPortValidationError) } - healthProbePortValidationError := validatePort(*healthProbeListenPort) + healthProbePortValidationError := validatePort(*serviceInsightListenPort) if healthProbePortValidationError != nil { - glog.Fatalf("Invalid value for health-probe-listen-port: %v", metricsPortValidationError) + glog.Fatalf("Invalid value for service-insight-listen-port: %v", metricsPortValidationError) } var err error diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index eb544b01cc..128e57b8f6 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -431,8 +431,8 @@ func createGlobalConfigurationValidator() *cr_validation.GlobalConfigurationVali forbiddenListenerPorts[*prometheusMetricsListenPort] = true } - if *enableHealthProbe { - forbiddenListenerPorts[*healthProbeListenPort] = true + if *enableServiceInsight { + forbiddenListenerPorts[*serviceInsightListenPort] = true } return cr_validation.NewGlobalConfigurationValidator(forbiddenListenerPorts) @@ -664,18 +664,18 @@ func createPlusAndLatencyCollectors( } func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) { - var healthProbeSecret *api_v1.Secret + var serviceInsightSecret *api_v1.Secret var err error - if *healthProbeTLSSecretName != "" { - healthProbeSecret, err = getAndValidateSecret(kubeClient, *healthProbeTLSSecretName) + if *serviceInsightTLSSecretName != "" { + serviceInsightSecret, err = getAndValidateSecret(kubeClient, *serviceInsightTLSSecretName) if err != nil { - glog.Fatalf("Error trying to get the health probe TLS secret %v: %v", *healthProbeTLSSecretName, err) + glog.Fatalf("Error trying to get the service insight TLS secret %v: %v", *serviceInsightTLSSecretName, err) } } - if *enableHealthProbe { - go healthcheck.RunHealthCheck(*healthProbeListenPort, plusClient, cnf, healthProbeSecret) + if *enableServiceInsight { + go healthcheck.RunHealthCheck(*serviceInsightListenPort, plusClient, cnf, serviceInsightSecret) } } From bab4d7dcbe3552b5335444f3c7b822095a7bb871 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 23 Nov 2022 18:21:38 +0000 Subject: [PATCH 33/66] Update docs for Service Insight feature --- .../command-line-arguments.md | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 5d192acc57..c92cf95c28 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -326,6 +326,29 @@ A Secret with a TLS certificate and key for TLS termination of the Prometheus me * If the argument is not set, the prometheus endpoint will not use 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-service-insight + +Enables exposing Service Insight enpoint for Ingress Controller. +  + + +### -service-insight-listen-port `` + +Sets the port where the Service Insight is exposed. + +Format: `[1024 - 65535]` (default `9114`) +  + + +### -service-insight-tls-secret `` + +A Secret with a TLS certificate and key for TLS termination of the Service Insight enpoint. + +* If the argument is not set, the service insight endpoint will not use 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. Format: `/`   From 2c9c40e4a6eae6b92586ce996a16418630eb3396 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 24 Nov 2022 16:15:12 +0000 Subject: [PATCH 34/66] Add log info staring healthcheck --- cmd/nginx-ingress/flags.go | 2 +- internal/healthcheck/healthcheck.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index b7808d1b3b..9bcb514712 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -142,7 +142,7 @@ var ( `A Secret with a TLS certificate and key for TLS termination of the service insight.`) serviceInsightListenPort = flag.Int("service-insight-listen-port", 9114, - "Enable service insight and set the port where the service insight endpoint are exposed. Requires -nginx-plus. [1024 - 65535]") + "Set the port where the Service Insight stats are exposed. Requires -nginx-plus. [1024 - 65535]") enableCustomResources = flag.Bool("enable-custom-resources", true, "Enable custom resources") diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index c9639d75f2..27a95d220d 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -23,10 +23,12 @@ import ( // RunHealthCheck starts the deep healthcheck service. func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *v1.Secret) { - hs, err := NewHealthServer(strconv.Itoa(port), plusClient, cnf, healthProbeTLSSecret) + healthport := fmt.Sprintf(":%s", strconv.Itoa(port)) + hs, err := NewHealthServer(healthport, plusClient, cnf, healthProbeTLSSecret) if err != nil { glog.Fatal(err) } + glog.Infof("Starting Service Insight listener on: %v%v", healthport, "/probe") glog.Fatal(hs.ListenAndServe()) } From fb74e1222ef961f47ab313262fbe03b905acf884 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 24 Nov 2022 17:23:32 +0000 Subject: [PATCH 35/66] Use more meaningful var name --- internal/healthcheck/healthcheck.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 27a95d220d..688fe32a10 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -23,12 +23,12 @@ import ( // RunHealthCheck starts the deep healthcheck service. func RunHealthCheck(port int, plusClient *client.NginxClient, cnf *configs.Configurator, healthProbeTLSSecret *v1.Secret) { - healthport := fmt.Sprintf(":%s", strconv.Itoa(port)) - hs, err := NewHealthServer(healthport, plusClient, cnf, healthProbeTLSSecret) + addr := fmt.Sprintf(":%s", strconv.Itoa(port)) + hs, err := NewHealthServer(addr, plusClient, cnf, healthProbeTLSSecret) if err != nil { glog.Fatal(err) } - glog.Infof("Starting Service Insight listener on: %v%v", healthport, "/probe") + glog.Infof("Starting Service Insight listener on: %v%v", addr, "/probe") glog.Fatal(hs.ListenAndServe()) } From aca4d38ed01cb8412f6a6f4b978c46c8a7242d0a Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Thu, 24 Nov 2022 22:57:14 +0000 Subject: [PATCH 36/66] Update log verbosity --- internal/configs/configurator.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index ffc2210552..13eb71410f 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -277,25 +277,23 @@ func (cnf *Configurator) GetVirtualServer(hostname string) *conf_v1.VirtualServe // GetUpstreamsforVirtualServer takes VS and returns a slice of upstreams. func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) []string { - glog.Infof("Get upstreamName for vs: %s", vs.Spec.Host) - + glog.V(3).Infof("Get upstreamName for vs: %s", vs.Spec.Host) upstreamNames := make([]string, 0, len(vs.Spec.Upstreams)) virtualServerUpstreamNamer := NewUpstreamNamerForVirtualServer(vs) for _, u := range vs.Spec.Upstreams { upstreamName := virtualServerUpstreamNamer.GetNameForUpstream(u.Name) - glog.Infof("upstream: %s, upstreamName: %s", u.Name, upstreamName) + glog.V(3).Infof("upstream: %s, upstreamName: %s", u.Name, upstreamName) upstreamNames = append(upstreamNames, upstreamName) } - return upstreamNames } // GetUpstreamsforHost takes a hostname and returns a slice of upstreams // for the given hostname. func (cnf *Configurator) GetUpstreamsforHost(hostname string) []string { - glog.Infof("Get upstream for host: %s", hostname) + glog.V(3).Infof("Get upstream for host: %s", hostname) vs := cnf.GetVirtualServer(hostname) if vs != nil { From 2a3fc28c2508763aacd30f93f1acb494713b3b3b Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 25 Nov 2022 16:31:01 +0000 Subject: [PATCH 37/66] Initial integration test for healthcheck service --- .../nodeport-with-additional-ports.yaml | 4 ++ tests/data/healthcheck/secret.yaml | 9 ++++ tests/suite/fixtures/fixtures.py | 7 ++- .../suite/test_healthcheck_virtual_server.py | 47 +++++++++++++++++++ tests/suite/utils/resources_utils.py | 6 ++- 5 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 tests/data/healthcheck/secret.yaml create mode 100644 tests/suite/test_healthcheck_virtual_server.py diff --git a/tests/data/common/service/nodeport-with-additional-ports.yaml b/tests/data/common/service/nodeport-with-additional-ports.yaml index 92918b2617..01abf73013 100644 --- a/tests/data/common/service/nodeport-with-additional-ports.yaml +++ b/tests/data/common/service/nodeport-with-additional-ports.yaml @@ -30,5 +30,9 @@ spec: targetPort: 3334 protocol: UDP name: udp-server + - port: 9114 + targetPort: 9114 + protocol: TCP + name: service_insight selector: app: nginx-ingress diff --git a/tests/data/healthcheck/secret.yaml b/tests/data/healthcheck/secret.yaml new file mode 100644 index 0000000000..78396dddf5 --- /dev/null +++ b/tests/data/healthcheck/secret.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: Secret +metadata: + name: healthcheck-test-secret + namespace: nginx-ingress +type: kubernetes.io/tls +data: + tls.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURMakNDQWhZQ0NRREFPRjl0THNhWFdqQU5CZ2txaGtpRzl3MEJBUXNGQURCYU1Rc3dDUVlEVlFRR0V3SlYKVXpFTE1Ba0dBMVVFQ0F3Q1EwRXhJVEFmQmdOVkJBb01HRWx1ZEdWeWJtVjBJRmRwWkdkcGRITWdVSFI1SUV4MApaREViTUJrR0ExVUVBd3dTWTJGbVpTNWxlR0Z0Y0d4bExtTnZiU0FnTUI0WERURTRNRGt4TWpFMk1UVXpOVm9YCkRUSXpNRGt4TVRFMk1UVXpOVm93V0RFTE1Ba0dBMVVFQmhNQ1ZWTXhDekFKQmdOVkJBZ01Ba05CTVNFd0h3WUQKVlFRS0RCaEpiblJsY201bGRDQlhhV1JuYVhSeklGQjBlU0JNZEdReEdUQVhCZ05WQkFNTUVHTmhabVV1WlhoaApiWEJzWlM1amIyMHdnZ0VpTUEwR0NTcUdTSWIzRFFFQkFRVUFBNElCRHdBd2dnRUtBb0lCQVFDcDZLbjdzeTgxCnAwanVKL2N5ayt2Q0FtbHNmanRGTTJtdVpOSzBLdGVjcUcyZmpXUWI1NXhRMVlGQTJYT1N3SEFZdlNkd0kyaloKcnVXOHFYWENMMnJiNENaQ0Z4d3BWRUNyY3hkam0zdGVWaVJYVnNZSW1tSkhQUFN5UWdwaW9iczl4N0RsTGM2SQpCQTBaalVPeWwwUHFHOVNKZXhNVjczV0lJYTVyRFZTRjJyNGtTa2JBajREY2o3TFhlRmxWWEgySTVYd1hDcHRDCm42N0pDZzQyZitrOHdnemNSVnA4WFprWldaVmp3cTlSVUtEWG1GQjJZeU4xWEVXZFowZXdSdUtZVUpsc202OTIKc2tPcktRajB2a29QbjQxRUUvK1RhVkVwcUxUUm9VWTNyemc3RGtkemZkQml6Rk8yZHNQTkZ4MkNXMGpYa05MdgpLbzI1Q1pyT2hYQUhBZ01CQUFFd0RRWUpLb1pJaHZjTkFRRUxCUUFEZ2dFQkFLSEZDY3lPalp2b0hzd1VCTWRMClJkSEliMzgzcFdGeW5acS9MdVVvdnNWQTU4QjBDZzdCRWZ5NXZXVlZycTVSSWt2NGxaODFOMjl4MjFkMUpINnIKalNuUXgrRFhDTy9USkVWNWxTQ1VwSUd6RVVZYVVQZ1J5anNNL05VZENKOHVIVmhaSitTNkZBK0NuT0Q5cm4yaQpaQmVQQ0k1ckh3RVh3bm5sOHl3aWozdnZRNXpISXV5QmdsV3IvUXl1aTlmalBwd1dVdlVtNG52NVNNRzl6Q1Y3ClBwdXd2dWF0cWpPMTIwOEJqZkUvY1pISWc4SHc5bXZXOXg5QytJUU1JTURFN2IvZzZPY0s3TEdUTHdsRnh2QTgKN1dqRWVxdW5heUlwaE1oS1JYVmYxTjM0OWVOOThFejM4Zk9USFRQYmRKakZBL1BjQytHeW1lK2lHdDVPUWRGaAp5UkU9Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K + tls.key: LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFb3dJQkFBS0NBUUVBcWVpcCs3TXZOYWRJN2lmM01wUHJ3Z0pwYkg0N1JUTnBybVRTdENyWG5LaHRuNDFrCkcrZWNVTldCUU5semtzQndHTDBuY0NObzJhN2x2S2wxd2k5cTIrQW1RaGNjS1ZSQXEzTVhZNXQ3WGxZa1YxYkcKQ0pwaVJ6ejBza0lLWXFHN1BjZXc1UzNPaUFRTkdZMURzcGRENmh2VWlYc1RGZTkxaUNHdWF3MVVoZHErSkVwRwp3SStBM0kreTEzaFpWVng5aU9WOEZ3cWJRcCt1eVFvT05uL3BQTUlNM0VWYWZGMlpHVm1WWThLdlVWQ2cxNWhRCmRtTWpkVnhGbldkSHNFYmltRkNaYkp1dmRySkRxeWtJOUw1S0Q1K05SQlAvazJsUkthaTAwYUZHTjY4NE93NUgKYzMzUVlzeFR0bmJEelJjZGdsdEkxNURTN3lxTnVRbWF6b1Z3QndJREFRQUJBb0lCQVFDUFNkU1luUXRTUHlxbApGZlZGcFRPc29PWVJoZjhzSStpYkZ4SU91UmF1V2VoaEp4ZG01Uk9ScEF6bUNMeUw1VmhqdEptZTIyM2dMcncyCk45OUVqVUtiL1ZPbVp1RHNCYzZvQ0Y2UU5SNThkejhjbk9SVGV3Y290c0pSMXBuMWhobG5SNUhxSkpCSmFzazEKWkVuVVFmY1hackw5NGxvOUpIM0UrVXFqbzFGRnM4eHhFOHdvUEJxalpzVjdwUlVaZ0MzTGh4bndMU0V4eUZvNApjeGI5U09HNU9tQUpvelN0Rm9RMkdKT2VzOHJKNXFmZHZ5dGdnOXhiTGFRTC94MGtwUTYyQm9GTUJEZHFPZVBXCktmUDV6WjYvMDcvdnBqNDh5QTFRMzJQem9idWJzQkxkM0tjbjMyamZtMUU3cHJ0V2wrSmVPRmlPem5CUUZKYk4KNHFQVlJ6NWhBb0dCQU50V3l4aE5DU0x1NFArWGdLeWNrbGpKNkY1NjY4Zk5qNUN6Z0ZScUowOXpuMFRsc05ybwpGVExaY3hEcW5SM0hQWU00MkpFUmgySi9xREZaeW5SUW8zY2czb2VpdlVkQlZHWTgrRkkxVzBxZHViL0w5K3l1CmVkT1pUUTVYbUdHcDZyNmpleHltY0ppbS9Pc0IzWm5ZT3BPcmxEN1NQbUJ2ek5MazRNRjZneGJYQW9HQkFNWk8KMHA2SGJCbWNQMHRqRlhmY0tFNzdJbUxtMHNBRzR1SG9VeDBlUGovMnFyblRuT0JCTkU0TXZnRHVUSnp5K2NhVQprOFJxbWRIQ2JIelRlNmZ6WXEvOWl0OHNaNzdLVk4xcWtiSWN1YytSVHhBOW5OaDFUanNSbmU3NFowajFGQ0xrCmhIY3FIMHJpN1BZU0tIVEU4RnZGQ3haWWRidUI4NENtWmlodnhicFJBb0dBSWJqcWFNWVBUWXVrbENkYTVTNzkKWVNGSjFKelplMUtqYS8vdER3MXpGY2dWQ0thMzFqQXdjaXowZi9sU1JxM0hTMUdHR21lemhQVlRpcUxmZVpxYwpSMGlLYmhnYk9jVlZrSkozSzB5QXlLd1BUdW14S0haNnpJbVpTMGMwYW0rUlk5WUdxNVQ3WXJ6cHpjZnZwaU9VCmZmZTNSeUZUN2NmQ21mb09oREN0enVrQ2dZQjMwb0xDMVJMRk9ycW40M3ZDUzUxemM1em9ZNDR1QnpzcHd3WU4KVHd2UC9FeFdNZjNWSnJEakJDSCtULzZzeXNlUGJKRUltbHpNK0l3eXRGcEFOZmlJWEV0LzQ4WGY2ME54OGdXTQp1SHl4Wlp4L05LdER3MFY4dlgxUE9ucTJBNWVpS2ErOGpSQVJZS0pMWU5kZkR1d29seHZHNmJaaGtQaS80RXRUCjNZMThzUUtCZ0h0S2JrKzdsTkpWZXN3WEU1Y1VHNkVEVXNEZS8yVWE3ZlhwN0ZjanFCRW9hcDFMU3crNlRYcDAKWmdybUtFOEFSek00NytFSkhVdmlpcS9udXBFMTVnMGtKVzNzeWhwVTl6WkxPN2x0QjBLSWtPOVpSY21Vam84UQpjcExsSE1BcWJMSjhXWUdKQ2toaVd4eWFsNmhZVHlXWTRjVmtDMHh0VGwvaFVFOUllTktvCi0tLS0tRU5EIFJTQSBQUklWQVRFIEtFWS0tLS0tCg== diff --git a/tests/suite/fixtures/fixtures.py b/tests/suite/fixtures/fixtures.py index 00d5b5f480..fcf238cc67 100644 --- a/tests/suite/fixtures/fixtures.py +++ b/tests/suite/fixtures/fixtures.py @@ -85,6 +85,7 @@ def __init__( metrics_port=9113, tcp_server_port=3333, udp_server_port=3334, + service_insight_port=9114, ): self.public_ip = public_ip self.port = port @@ -93,6 +94,8 @@ def __init__( self.metrics_port = metrics_port self.tcp_server_port = tcp_server_port self.udp_server_port = udp_server_port + self.service_insight_port = service_insight_port + class IngressControllerPrerequisites: @@ -171,10 +174,10 @@ def ingress_controller_endpoint(cli_arguments, kube_apis, ingress_controller_pre namespace, f"{TEST_DATA}/common/service/nodeport-with-additional-ports.yaml", ) - port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port = get_service_node_ports( + port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port, service_insight_port = get_service_node_ports( kube_apis.v1, service_name, namespace ) - return PublicEndpoint(public_ip, port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port) + return PublicEndpoint(public_ip, port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port, service_insight_port) else: create_service_from_yaml( kube_apis.v1, diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_healthcheck_virtual_server.py new file mode 100644 index 0000000000..e019a0f0a3 --- /dev/null +++ b/tests/suite/test_healthcheck_virtual_server.py @@ -0,0 +1,47 @@ +import pytest +import requests +from settings import DEPLOYMENTS, TEST_DATA +from suite.utils.custom_assertions import wait_and_assert_status_code +from suite.utils.custom_resources_utils import create_crd_from_yaml, delete_crd +from suite.utils.resources_utils import ( + create_service_from_yaml, + delete_service, + patch_rbac, + read_service, + replace_service, + wait_before_test, +) +from suite.utils.vs_vsr_resources_utils import ( + create_virtual_server_from_yaml, + delete_virtual_server, + patch_virtual_server_from_yaml, +) +from suite.utils.yaml_utils import get_first_host_from_yaml, get_name_from_yaml, get_paths_from_vs_yaml + + +@pytest.mark.vs +@pytest.mark.test_jakub +@pytest.mark.parametrize( + "crd_ingress_controller, virtual_server_setup", + [ + ( + {"type": "complete", "extra_args": [f"-enable-custom-resources", f"-enable-service-insight"]}, + {"example": "virtual-server", "app_type": "simple"}, + ) + ], + indirect=True, +) +class TestHealtcheckVS: + def test_responses_after_setup(self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint): + print("\nStep 1: initial check") + wait_before_test(1) + wait_and_assert_status_code(200, virtual_server_setup.backend_1_url, virtual_server_setup.vs_host) + wait_and_assert_status_code(200, virtual_server_setup.backend_2_url, virtual_server_setup.vs_host) + + vs_source = f"{TEST_DATA}/{request.param['example']}/standard/virtual-server.yaml" + host = get_first_host_from_yaml(vs_source) + req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" + + resp = requests.get(req_url) + assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" + print(resp.json()) diff --git a/tests/suite/utils/resources_utils.py b/tests/suite/utils/resources_utils.py index 692ef4e058..85773932b9 100644 --- a/tests/suite/utils/resources_utils.py +++ b/tests/suite/utils/resources_utils.py @@ -424,7 +424,7 @@ def create_service_with_name(v1: CoreV1Api, namespace, name) -> str: return create_service(v1, namespace, dep) -def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, int, int, int): +def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, int,int, int, int): """ Get service allocated node_ports. @@ -434,10 +434,11 @@ def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, in :return: (plain_port, ssl_port, api_port, exporter_port) """ resp = v1.read_namespaced_service(name, namespace) - if len(resp.spec.ports) == 6: + if len(resp.spec.ports) == 7: print("An unexpected amount of ports in a service. Check the configuration") print(f"Service with an API port: {resp.spec.ports[2].node_port}") print(f"Service with an Exporter port: {resp.spec.ports[3].node_port}") + print(f"Service with an Service Insight port: {resp.spec.ports[6].node_port}") return ( resp.spec.ports[0].node_port, resp.spec.ports[1].node_port, @@ -445,6 +446,7 @@ def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, in resp.spec.ports[3].node_port, resp.spec.ports[4].node_port, resp.spec.ports[5].node_port, + resp.spec.ports[6].node_port, ) From 18cad3a2b3768382a19d5ba736551a4dd4e04b3d Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 25 Nov 2022 16:31:35 +0000 Subject: [PATCH 38/66] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/suite/fixtures/fixtures.py | 15 +++++++++++---- tests/suite/test_healthcheck_virtual_server.py | 4 +++- tests/suite/utils/resources_utils.py | 2 +- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/tests/suite/fixtures/fixtures.py b/tests/suite/fixtures/fixtures.py index fcf238cc67..7833643c4c 100644 --- a/tests/suite/fixtures/fixtures.py +++ b/tests/suite/fixtures/fixtures.py @@ -97,7 +97,6 @@ def __init__( self.service_insight_port = service_insight_port - class IngressControllerPrerequisites: """ Encapsulate shared items. @@ -174,10 +173,18 @@ def ingress_controller_endpoint(cli_arguments, kube_apis, ingress_controller_pre namespace, f"{TEST_DATA}/common/service/nodeport-with-additional-ports.yaml", ) - port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port, service_insight_port = get_service_node_ports( - kube_apis.v1, service_name, namespace + ( + port, + port_ssl, + api_port, + metrics_port, + tcp_server_port, + udp_server_port, + service_insight_port, + ) = get_service_node_ports(kube_apis.v1, service_name, namespace) + return PublicEndpoint( + public_ip, port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port, service_insight_port ) - return PublicEndpoint(public_ip, port, port_ssl, api_port, metrics_port, tcp_server_port, udp_server_port, service_insight_port) else: create_service_from_yaml( kube_apis.v1, diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_healthcheck_virtual_server.py index e019a0f0a3..a05220e23d 100644 --- a/tests/suite/test_healthcheck_virtual_server.py +++ b/tests/suite/test_healthcheck_virtual_server.py @@ -32,7 +32,9 @@ indirect=True, ) class TestHealtcheckVS: - def test_responses_after_setup(self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint): + def test_responses_after_setup( + self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint + ): print("\nStep 1: initial check") wait_before_test(1) wait_and_assert_status_code(200, virtual_server_setup.backend_1_url, virtual_server_setup.vs_host) diff --git a/tests/suite/utils/resources_utils.py b/tests/suite/utils/resources_utils.py index 85773932b9..330e5eb11c 100644 --- a/tests/suite/utils/resources_utils.py +++ b/tests/suite/utils/resources_utils.py @@ -424,7 +424,7 @@ def create_service_with_name(v1: CoreV1Api, namespace, name) -> str: return create_service(v1, namespace, dep) -def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, int,int, int, int): +def get_service_node_ports(v1: CoreV1Api, name, namespace) -> (int, int, int, int, int, int, int): """ Get service allocated node_ports. From 5604792cb45a1dd8ec23e8835d29cfdb0a24b7ed Mon Sep 17 00:00:00 2001 From: Venktesh Date: Mon, 28 Nov 2022 13:25:52 +0000 Subject: [PATCH 39/66] add service-insight port to loadbalancer svc yaml --- .../service/loadbalancer-with-additional-ports.yaml | 4 ++++ .../common/service/nodeport-with-additional-ports.yaml | 2 +- tests/suite/test_healthcheck_virtual_server.py | 8 +++----- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/data/common/service/loadbalancer-with-additional-ports.yaml b/tests/data/common/service/loadbalancer-with-additional-ports.yaml index 669b50b2a1..57b311a835 100644 --- a/tests/data/common/service/loadbalancer-with-additional-ports.yaml +++ b/tests/data/common/service/loadbalancer-with-additional-ports.yaml @@ -23,5 +23,9 @@ spec: targetPort: 9113 protocol: TCP name: exporter + - port: 9114 + targetPort: 9114 + protocol: TCP + name: service-insight selector: app: nginx-ingress diff --git a/tests/data/common/service/nodeport-with-additional-ports.yaml b/tests/data/common/service/nodeport-with-additional-ports.yaml index 01abf73013..9d845b2ed4 100644 --- a/tests/data/common/service/nodeport-with-additional-ports.yaml +++ b/tests/data/common/service/nodeport-with-additional-ports.yaml @@ -33,6 +33,6 @@ spec: - port: 9114 targetPort: 9114 protocol: TCP - name: service_insight + name: service-insight selector: app: nginx-ingress diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_healthcheck_virtual_server.py index a05220e23d..fdae3f381d 100644 --- a/tests/suite/test_healthcheck_virtual_server.py +++ b/tests/suite/test_healthcheck_virtual_server.py @@ -20,7 +20,6 @@ @pytest.mark.vs -@pytest.mark.test_jakub @pytest.mark.parametrize( "crd_ingress_controller, virtual_server_setup", [ @@ -36,14 +35,13 @@ def test_responses_after_setup( self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint ): print("\nStep 1: initial check") - wait_before_test(1) + wait_before_test() wait_and_assert_status_code(200, virtual_server_setup.backend_1_url, virtual_server_setup.vs_host) wait_and_assert_status_code(200, virtual_server_setup.backend_2_url, virtual_server_setup.vs_host) - vs_source = f"{TEST_DATA}/{request.param['example']}/standard/virtual-server.yaml" + vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" host = get_first_host_from_yaml(vs_source) - req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" - + req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" resp = requests.get(req_url) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" print(resp.json()) From 094d93f9331be7dbdc7055097de22cc00327134c Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 28 Nov 2022 13:46:39 +0000 Subject: [PATCH 40/66] Use TLS v1.0 as a min version --- internal/healthcheck/healthcheck.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 688fe32a10..d9d192f4c0 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -43,6 +43,8 @@ type HealthServer struct { // NewHealthServer creates Health Server. If secret is provided, // the server is configured with TLS Config. +// +//nolint:gosec func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) (*HealthServer, error) { hs := HealthServer{ Server: &http.Server{ @@ -62,7 +64,7 @@ func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configura } hs.Server.TLSConfig = &tls.Config{ Certificates: []tls.Certificate{tlsCert}, - MinVersion: tls.VersionTLS12, + MinVersion: tls.VersionTLS10, } hs.URL = fmt.Sprintf("https://%s/", addr) } From 9ffc97d73342a2a2700d0e48f2ea7a16ae463a4f Mon Sep 17 00:00:00 2001 From: Venktesh Date: Mon, 28 Nov 2022 14:28:51 +0000 Subject: [PATCH 41/66] mark test as N+ only --- tests/suite/test_healthcheck_virtual_server.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_healthcheck_virtual_server.py index fdae3f381d..df53ca31af 100644 --- a/tests/suite/test_healthcheck_virtual_server.py +++ b/tests/suite/test_healthcheck_virtual_server.py @@ -20,6 +20,7 @@ @pytest.mark.vs +@pytest.mark.skip_for_nginx_oss @pytest.mark.parametrize( "crd_ingress_controller, virtual_server_setup", [ @@ -30,7 +31,7 @@ ], indirect=True, ) -class TestHealtcheckVS: +class TestHealthCheckVs: def test_responses_after_setup( self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint ): From df4ae663a8e48a4e5b21ab595360752551f88897 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 28 Nov 2022 15:38:05 +0000 Subject: [PATCH 42/66] Update err message --- internal/healthcheck/healthcheck.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index d9d192f4c0..9286201280 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -60,11 +60,11 @@ func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configura if secret != nil { tlsCert, err := makeCert(secret) if err != nil { - return nil, fmt.Errorf("creating tls sert: %w", err) + return nil, fmt.Errorf("unable to create TLS cert: %w", err) } hs.Server.TLSConfig = &tls.Config{ Certificates: []tls.Certificate{tlsCert}, - MinVersion: tls.VersionTLS10, + MinVersion: tls.VersionTLS10, //nolint:gosec } hs.URL = fmt.Sprintf("https://%s/", addr) } From 38d9781793bea2320ca78b0a3d7ca21fe8886893 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 29 Nov 2022 13:52:23 +0000 Subject: [PATCH 43/66] Add service insight options to IC manifest --- deployments/deployment/nginx-plus-ingress.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/deployments/deployment/nginx-plus-ingress.yaml b/deployments/deployment/nginx-plus-ingress.yaml index 97a0b08dc1..1d95b856f8 100644 --- a/deployments/deployment/nginx-plus-ingress.yaml +++ b/deployments/deployment/nginx-plus-ingress.yaml @@ -32,6 +32,8 @@ spec: containerPort: 8081 - name: prometheus containerPort: 9113 + - name: service-insight + containerPort: 9114 readinessProbe: httpGet: path: /nginx-ready @@ -75,4 +77,5 @@ spec: #- -report-ingress-status #- -external-service=nginx-ingress #- -enable-prometheus-metrics + #- -enable-service-insight #- -global-configuration=$(POD_NAMESPACE)/nginx-configuration From 384cc553f7d3ab6c333df7b1650fe69e4001838b Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 29 Nov 2022 14:04:43 +0000 Subject: [PATCH 44/66] Add service-insight documentation page --- .../logging-and-monitoring/service-insight.md | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/content/logging-and-monitoring/service-insight.md diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md new file mode 100644 index 0000000000..3aa0f103bb --- /dev/null +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -0,0 +1,58 @@ +--- +title: Service Insight + +description: "The Ingress Controller exposes the Service Insight endpoint." +weight: 2100 +doctypes: [""] +aliases: + - /service-insight/ +toc: true +docs: "" +--- + + +The Ingress Controller exposes an endpoint and provides host statistics for Virtual Servers. +It exposes data in JSON format and returns HTTP status codes. +The response body holds information about the total, down and the unhealthy number of +upstreams associated with the hostname. +Returned HTTP codes indicate the health of the upstreams (service). + +The service is not healthy (HTTP response code different than 200 OK) if all upstreams are unhealthy. +The service is healthy if at least one upstream is healthy. In this case, the endpoint returns HTTP code 200 OK. + + + +## Enabling Service Insight Endpoint + +If you're using *Kubernetes manifests* (Deployment or DaemonSet) to install the Ingress Controller, to enable the Service Insight endpoint: +1. Run the Ingress Controller with the `-enable-service-insight` [command-line argument](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments). As a result, the Ingress Controller will expose the enpoint via the path `/probe/{hostname}` on port `9114` (customizable via the `-service-insight-listen-port` command-line argument). +1. To enable TLS for the Service Insight endpoint, configure the `-service-insight-tls-secret` cli argument with the namespace and name of a TLS Secret. +1. Add the Service Insight port to the list of the ports of the Ingress Controller container in the template of the Ingress Controller pod: + ```yaml + - name: service-insight + containerPort: 9114 + ``` + +If you're using *Helm* to install the Ingress Controller, to enable Service Insight enpoint, configure the `serviceinsight.*` parameters of the Helm chart. See the [Installation with Helm](/nginx-ingress-controller/installation/installation-with-helm) doc. + +## Available Statistics and HTTP Response Codes + +The Service Insight provides the following statistics: + +* Total number of VS +* Number of VS in 'Down' state +* Number of VS in 'Healthy' state + +The stats are returned as a JSON body: + +```json +{ "Total": , "Up": , "Unhealthy": } +``` + +Response codes: + +* HTTP 200 OK - service is healthy +* HTTP 404 - no upstreams/VS found for the requested hostname +* HTTP 503 Service Unavailable - the service is down (all upstreams/VS are "Unhealthy") + +**Note**: wildcards in hostnames are not supported at the moment. \ No newline at end of file From 91a45d5b68cc7ef2093ae835ac583476cca677a3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 29 Nov 2022 14:08:55 +0000 Subject: [PATCH 45/66] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index 3aa0f103bb..1ce3937339 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -55,4 +55,4 @@ Response codes: * HTTP 404 - no upstreams/VS found for the requested hostname * HTTP 503 Service Unavailable - the service is down (all upstreams/VS are "Unhealthy") -**Note**: wildcards in hostnames are not supported at the moment. \ No newline at end of file +**Note**: wildcards in hostnames are not supported at the moment. From e6defb307b2fc943e883828f10bda8d911b32cdb Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 29 Nov 2022 15:38:29 +0000 Subject: [PATCH 46/66] Update Helm chart for Service Insight --- .../templates/controller-daemonset.yaml | 7 +++ .../templates/controller-deployment.yaml | 7 +++ deployments/helm-chart/values.schema.json | 57 +++++++++++++++++++ deployments/helm-chart/values.yaml | 13 +++++ 4 files changed, 84 insertions(+) diff --git a/deployments/helm-chart/templates/controller-daemonset.yaml b/deployments/helm-chart/templates/controller-daemonset.yaml index 7b311fa2d8..cc3ef518cb 100644 --- a/deployments/helm-chart/templates/controller-daemonset.yaml +++ b/deployments/helm-chart/templates/controller-daemonset.yaml @@ -94,6 +94,10 @@ spec: - name: prometheus containerPort: {{ .Values.prometheus.port }} {{- end }} +{{- if .Values.serviceInsight.create }} + - name: service-insight + containerPort: {{ .Values.serviceInsight.port }} +{{- end }} {{- if .Values.controller.readyStatus.enable }} - name: readiness-port containerPort: {{ .Values.controller.readyStatus.port }} @@ -196,6 +200,9 @@ spec: - -enable-prometheus-metrics={{ .Values.prometheus.create }} - -prometheus-metrics-listen-port={{ .Values.prometheus.port }} - -prometheus-tls-secret={{ .Values.prometheus.secret }} + - -enable-service-insight={{ .Values.serviceInsight.create }} + - -service-insight-listen-port={{ .Values.serviceInsight.port }} + - -service-insight-tls-secret={{ .Values.serviceInsight.secret }} - -enable-custom-resources={{ .Values.controller.enableCustomResources }} - -enable-snippets={{ .Values.controller.enableSnippets }} - -include-year={{ .Values.controller.includeYear }} diff --git a/deployments/helm-chart/templates/controller-deployment.yaml b/deployments/helm-chart/templates/controller-deployment.yaml index 9ec53bf972..caa29cc533 100644 --- a/deployments/helm-chart/templates/controller-deployment.yaml +++ b/deployments/helm-chart/templates/controller-deployment.yaml @@ -97,6 +97,10 @@ spec: - name: prometheus containerPort: {{ .Values.prometheus.port }} {{- end }} +{{- if .Values.serviceInsight.create }} + - name: service-insight + containerPort: {{ .Values.serviceInsight.port }} +{{- end }} {{- if .Values.controller.readyStatus.enable }} - name: readiness-port containerPort: {{ .Values.controller.readyStatus.port }} @@ -199,6 +203,9 @@ spec: - -enable-prometheus-metrics={{ .Values.prometheus.create }} - -prometheus-metrics-listen-port={{ .Values.prometheus.port }} - -prometheus-tls-secret={{ .Values.prometheus.secret }} + - -enable-service-insight={{ .Values.serviceInsight.create }} + - -service-insight-listen-port={{ .Values.serviceInsight.port }} + - -service-insight-tls-secret={{ .Values.serviceInsight.secret }} - -enable-custom-resources={{ .Values.controller.enableCustomResources }} - -enable-snippets={{ .Values.controller.enableSnippets }} - -include-year={{ .Values.controller.includeYear }} diff --git a/deployments/helm-chart/values.schema.json b/deployments/helm-chart/values.schema.json index 9b1a4a0d8e..b37baec07b 100644 --- a/deployments/helm-chart/values.schema.json +++ b/deployments/helm-chart/values.schema.json @@ -7,6 +7,7 @@ "controller", "rbac", "prometheus", + "serviceInsight", "nginxServiceMesh" ], "properties": { @@ -1436,6 +1437,56 @@ } ] }, + "serviceInsight": { + "type": "object", + "default": {}, + "title": "The Service Insight Schema", + "required": [ + "create" + ], + "properties": { + "create": { + "type": "boolean", + "default": false, + "title": "The create", + "examples": [ + true + ] + }, + "port": { + "type": "integer", + "default": 9114, + "title": "The port", + "examples": [ + 9114 + ] + }, + "secret": { + "type": "string", + "default": "", + "title": "The secret", + "examples": [ + "" + ] + }, + "scheme": { + "type": "string", + "default": "http", + "title": "The scheme", + "examples": [ + "http" + ] + } + }, + "examples": [ + { + "create": true, + "port": 9114, + "secret": "", + "scheme": "http" + } + ] + }, "nginxServiceMesh": { "type": "object", "default": {}, @@ -1622,6 +1673,12 @@ "secret": "", "scheme": "http" }, + "serviceInsight": { + "create": true, + "port": 9114, + "secret": "", + "scheme": "http" + }, "nginxServiceMesh": { "enable": false, "enableEgress": false diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml index 1a0dd650f9..c1330d44ae 100644 --- a/deployments/helm-chart/values.yaml +++ b/deployments/helm-chart/values.yaml @@ -430,6 +430,19 @@ prometheus: ## Configures the HTTP scheme used. scheme: http +serviceInsight: + ## Expose NGINX Plus Service Insight endpoint. + create: true + + ## Configures the port to expose endpoint. + port: 9114 + + ## Specifies the namespace/name of a Kubernetes TLS Secret which will be used to protect the Service Insight endpoint. + secret: "" + + ## Configures the HTTP scheme used. + scheme: http + nginxServiceMesh: ## Enables integration with NGINX Service Mesh. ## Requires controller.nginxplus From 682026f72a7fda165d527ddf34434aea361b6245 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 29 Nov 2022 16:41:17 +0000 Subject: [PATCH 47/66] Apply PR feedback --- cmd/nginx-ingress/main.go | 12 +++++++----- internal/configs/configurator.go | 6 +++--- internal/healthcheck/healthcheck.go | 4 +--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 128e57b8f6..b0244e7c2b 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -123,7 +123,9 @@ func main() { transportServerValidator := cr_validation.NewTransportServerValidator(*enableTLSPassthrough, *enableSnippets, *nginxPlus) virtualServerValidator := cr_validation.NewVirtualServerValidator(cr_validation.IsPlus(*nginxPlus), cr_validation.IsDosEnabled(*appProtectDos), cr_validation.IsCertManagerEnabled(*enableCertManager), cr_validation.IsExternalDNSEnabled(*enableExternalDNS)) - createHealthProbeEndpoint(kubeClient, plusClient, cnf) + if *enableServiceInsight { + createHealthProbeEndpoint(kubeClient, plusClient, cnf) + } lbcInput := k8s.NewLoadBalancerControllerInput{ KubeClient: kubeClient, @@ -664,6 +666,9 @@ func createPlusAndLatencyCollectors( } func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *client.NginxClient, cnf *configs.Configurator) { + if !*enableServiceInsight { + return + } var serviceInsightSecret *api_v1.Secret var err error @@ -673,10 +678,7 @@ func createHealthProbeEndpoint(kubeClient *kubernetes.Clientset, plusClient *cli glog.Fatalf("Error trying to get the service insight TLS secret %v: %v", *serviceInsightTLSSecretName, err) } } - - if *enableServiceInsight { - go healthcheck.RunHealthCheck(*serviceInsightListenPort, plusClient, cnf, serviceInsightSecret) - } + go healthcheck.RunHealthCheck(*serviceInsightListenPort, plusClient, cnf, serviceInsightSecret) } func processGlobalConfiguration() { diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 13eb71410f..807b94a3c1 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -265,8 +265,8 @@ func (cnf *Configurator) AddOrUpdateIngress(ingEx *IngressEx) (Warnings, error) return warnings, nil } -// GetVirtualServer takes a hostname and returns a VS for the given hostname. -func (cnf *Configurator) GetVirtualServer(hostname string) *conf_v1.VirtualServer { +// GetVirtualServerForHost takes a hostname and returns a VS for the given hostname. +func (cnf *Configurator) GetVirtualServerForHost(hostname string) *conf_v1.VirtualServer { for _, vsEx := range cnf.virtualServers { if vsEx.VirtualServer.Spec.Host == hostname { return vsEx.VirtualServer @@ -294,7 +294,7 @@ func (cnf *Configurator) GetUpstreamsforVirtualServer(vs *conf_v1.VirtualServer) // for the given hostname. func (cnf *Configurator) GetUpstreamsforHost(hostname string) []string { glog.V(3).Infof("Get upstream for host: %s", hostname) - vs := cnf.GetVirtualServer(hostname) + vs := cnf.GetVirtualServerForHost(hostname) if vs != nil { return cnf.GetUpstreamsforVirtualServer(vs) diff --git a/internal/healthcheck/healthcheck.go b/internal/healthcheck/healthcheck.go index 9286201280..b25fe58bf7 100644 --- a/internal/healthcheck/healthcheck.go +++ b/internal/healthcheck/healthcheck.go @@ -43,8 +43,6 @@ type HealthServer struct { // NewHealthServer creates Health Server. If secret is provided, // the server is configured with TLS Config. -// -//nolint:gosec func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configurator, secret *v1.Secret) (*HealthServer, error) { hs := HealthServer{ Server: &http.Server{ @@ -64,7 +62,7 @@ func NewHealthServer(addr string, nc *client.NginxClient, cnf *configs.Configura } hs.Server.TLSConfig = &tls.Config{ Certificates: []tls.Certificate{tlsCert}, - MinVersion: tls.VersionTLS10, //nolint:gosec + MinVersion: tls.VersionTLS12, } hs.URL = fmt.Sprintf("https://%s/", addr) } From d93c00765f1058235f783efa5c5977d90b47fce7 Mon Sep 17 00:00:00 2001 From: Venktesh Date: Tue, 29 Nov 2022 17:26:19 +0000 Subject: [PATCH 48/66] Add test for https endpoint --- .../secret.yaml | 2 +- .../suite/test_healthcheck_virtual_server.py | 80 ++++++++++++++----- 2 files changed, 60 insertions(+), 22 deletions(-) rename tests/data/{healthcheck => service-insight}/secret.yaml (99%) diff --git a/tests/data/healthcheck/secret.yaml b/tests/data/service-insight/secret.yaml similarity index 99% rename from tests/data/healthcheck/secret.yaml rename to tests/data/service-insight/secret.yaml index 78396dddf5..dd72533ed6 100644 --- a/tests/data/healthcheck/secret.yaml +++ b/tests/data/service-insight/secret.yaml @@ -1,7 +1,7 @@ apiVersion: v1 kind: Secret metadata: - name: healthcheck-test-secret + name: test-secret namespace: nginx-ingress type: kubernetes.io/tls data: diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_healthcheck_virtual_server.py index df53ca31af..4e17020116 100644 --- a/tests/suite/test_healthcheck_virtual_server.py +++ b/tests/suite/test_healthcheck_virtual_server.py @@ -1,22 +1,13 @@ import pytest import requests -from settings import DEPLOYMENTS, TEST_DATA -from suite.utils.custom_assertions import wait_and_assert_status_code -from suite.utils.custom_resources_utils import create_crd_from_yaml, delete_crd +from settings import TEST_DATA from suite.utils.resources_utils import ( - create_service_from_yaml, - delete_service, - patch_rbac, - read_service, - replace_service, + create_secret_from_yaml, + delete_secret, + ensure_response_from_backend, wait_before_test, ) -from suite.utils.vs_vsr_resources_utils import ( - create_virtual_server_from_yaml, - delete_virtual_server, - patch_virtual_server_from_yaml, -) -from suite.utils.yaml_utils import get_first_host_from_yaml, get_name_from_yaml, get_paths_from_vs_yaml +from suite.utils.yaml_utils import get_first_host_from_yaml @pytest.mark.vs @@ -31,18 +22,65 @@ ], indirect=True, ) -class TestHealthCheckVs: - def test_responses_after_setup( +class TestHealthCheckVsHttp: + def test_responses_svc_insight_http( self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint ): - print("\nStep 1: initial check") - wait_before_test() - wait_and_assert_status_code(200, virtual_server_setup.backend_1_url, virtual_server_setup.vs_host) - wait_and_assert_status_code(200, virtual_server_setup.backend_2_url, virtual_server_setup.vs_host) - + """test responses from service insight endpoint with http""" vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" host = get_first_host_from_yaml(vs_source) req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" + ensure_response_from_backend(req_url, virtual_server_setup.vs_host) resp = requests.get(req_url) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" print(resp.json()) + + +@pytest.fixture(scope="class") +def https_secret_setup(request, kube_apis, test_namespace): + print("------------------------- Deploy Secret -----------------------------------") + secret_name = create_secret_from_yaml(kube_apis.v1, "nginx-ingress", f"{TEST_DATA}/service-insight/secret.yaml") + + def fin(): + delete_secret(kube_apis.v1, secret_name, "nginx-ingress") + + request.addfinalizer(fin) + + +@pytest.mark.vs +@pytest.mark.skip_for_nginx_oss +@pytest.mark.parametrize( + "crd_ingress_controller, virtual_server_setup", + [ + ( + { + "type": "complete", + "extra_args": [ + f"-enable-custom-resources", + f"-enable-service-insight", + f"-service-insight-tls-secret=nginx-ingress/test-secret", + ], + }, + {"example": "virtual-server", "app_type": "simple"}, + ) + ], + indirect=True, +) +class TestHealthCheckVsHttps: + def test_responses_svc_insight_https( + self, + request, + kube_apis, + https_secret_setup, + ingress_controller_endpoint, + crd_ingress_controller, + virtual_server_setup, + ): + """test responses from service insight endpoint with https""" + vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" + host = get_first_host_from_yaml(vs_source) + req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" + ensure_response_from_backend(req_url, virtual_server_setup.vs_host) + resp = requests.get(req_url, verify=False) + assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" + print(resp.json()) From ab945e890982e362825a864fec75b8c0c230664b Mon Sep 17 00:00:00 2001 From: Venktesh Date: Wed, 30 Nov 2022 13:23:56 +0000 Subject: [PATCH 49/66] add test for replica change --- tests/data/service-insight/app.yaml | 20 +++++++++++++ ...=> test_virtual_server_service_insight.py} | 29 +++++++++++++++++-- 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 tests/data/service-insight/app.yaml rename tests/suite/{test_healthcheck_virtual_server.py => test_virtual_server_service_insight.py} (71%) diff --git a/tests/data/service-insight/app.yaml b/tests/data/service-insight/app.yaml new file mode 100644 index 0000000000..d7bf5f5a7e --- /dev/null +++ b/tests/data/service-insight/app.yaml @@ -0,0 +1,20 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: backend1 +spec: + replicas: 5 + selector: + matchLabels: + app: backend1 + template: + metadata: + labels: + app: backend1 + spec: + containers: + - name: backend1 + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 + \ No newline at end of file diff --git a/tests/suite/test_healthcheck_virtual_server.py b/tests/suite/test_virtual_server_service_insight.py similarity index 71% rename from tests/suite/test_healthcheck_virtual_server.py rename to tests/suite/test_virtual_server_service_insight.py index 4e17020116..e1f3947b82 100644 --- a/tests/suite/test_healthcheck_virtual_server.py +++ b/tests/suite/test_virtual_server_service_insight.py @@ -5,6 +5,7 @@ create_secret_from_yaml, delete_secret, ensure_response_from_backend, + patch_deployment_from_yaml, wait_before_test, ) from suite.utils.yaml_utils import get_first_host_from_yaml @@ -33,8 +34,7 @@ def test_responses_svc_insight_http( ensure_response_from_backend(req_url, virtual_server_setup.vs_host) resp = requests.get(req_url) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" - print(resp.json()) - + assert resp.json() == {'Total': 3, 'Up': 3, 'Unhealthy': 0} @pytest.fixture(scope="class") def https_secret_setup(request, kube_apis, test_namespace): @@ -83,4 +83,27 @@ def test_responses_svc_insight_https( ensure_response_from_backend(req_url, virtual_server_setup.vs_host) resp = requests.get(req_url, verify=False) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" - print(resp.json()) + assert resp.json() == {'Total': 3, 'Up': 3, 'Unhealthy': 0} + + def test_responses_svc_insight_update_pods( + self, + request, + kube_apis, + https_secret_setup, + ingress_controller_endpoint, + test_namespace, + crd_ingress_controller, + virtual_server_setup, + ): + vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" + host = get_first_host_from_yaml(vs_source) + req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" + ensure_response_from_backend(req_url, virtual_server_setup.vs_host) + + # patch backend1 deployment with 5 replicas + patch_deployment_from_yaml(kube_apis.apps_v1_api, test_namespace, f"{TEST_DATA}/service-insight/app.yaml") + ensure_response_from_backend(req_url, virtual_server_setup.vs_host) + wait_before_test() + resp = requests.get(req_url, verify=False) + assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" + assert resp.json() == {'Total': 6, 'Up': 6, 'Unhealthy': 0} \ No newline at end of file From 6c04db464a573ae7fd659be69d73b7234f51f699 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 30 Nov 2022 13:26:04 +0000 Subject: [PATCH 50/66] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/data/service-insight/app.yaml | 1 - tests/suite/test_virtual_server_service_insight.py | 7 ++++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/data/service-insight/app.yaml b/tests/data/service-insight/app.yaml index d7bf5f5a7e..7a6beecb9e 100644 --- a/tests/data/service-insight/app.yaml +++ b/tests/data/service-insight/app.yaml @@ -17,4 +17,3 @@ spec: image: nginxdemos/nginx-hello:plain-text ports: - containerPort: 8080 - \ No newline at end of file diff --git a/tests/suite/test_virtual_server_service_insight.py b/tests/suite/test_virtual_server_service_insight.py index e1f3947b82..8b1f969626 100644 --- a/tests/suite/test_virtual_server_service_insight.py +++ b/tests/suite/test_virtual_server_service_insight.py @@ -34,7 +34,8 @@ def test_responses_svc_insight_http( ensure_response_from_backend(req_url, virtual_server_setup.vs_host) resp = requests.get(req_url) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" - assert resp.json() == {'Total': 3, 'Up': 3, 'Unhealthy': 0} + assert resp.json() == {"Total": 3, "Up": 3, "Unhealthy": 0} + @pytest.fixture(scope="class") def https_secret_setup(request, kube_apis, test_namespace): @@ -83,7 +84,7 @@ def test_responses_svc_insight_https( ensure_response_from_backend(req_url, virtual_server_setup.vs_host) resp = requests.get(req_url, verify=False) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" - assert resp.json() == {'Total': 3, 'Up': 3, 'Unhealthy': 0} + assert resp.json() == {"Total": 3, "Up": 3, "Unhealthy": 0} def test_responses_svc_insight_update_pods( self, @@ -106,4 +107,4 @@ def test_responses_svc_insight_update_pods( wait_before_test() resp = requests.get(req_url, verify=False) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" - assert resp.json() == {'Total': 6, 'Up': 6, 'Unhealthy': 0} \ No newline at end of file + assert resp.json() == {"Total": 6, "Up": 6, "Unhealthy": 0} From 2387e1dd43e43b93ad2c81af1a16a68d4348d8ea Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:54:56 +0000 Subject: [PATCH 51/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index 1ce3937339..77878ef1b1 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -43,7 +43,7 @@ The Service Insight provides the following statistics: * Number of VS in 'Down' state * Number of VS in 'Healthy' state -The stats are returned as a JSON body: +These statistics are returned as JSON: ```json { "Total": , "Up": , "Unhealthy": } From 1a449fce50bf48cd8f29dd2992a4f7b5019ca102 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:55:20 +0000 Subject: [PATCH 52/66] Update docs/content/configuration/global-configuration/command-line-arguments.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- .../global-configuration/command-line-arguments.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 55f09b4bfb..47d69ef6e2 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -343,7 +343,7 @@ A Secret with a TLS certificate and key for TLS termination of the Prometheus me ### -enable-service-insight -Enables exposing Service Insight enpoint for Ingress Controller. +Exposes the Service Insight endpoint for Ingress Controller.   From 167fbfc894a38666ad635c51c8ec844de33f6205 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:55:36 +0000 Subject: [PATCH 53/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index 77878ef1b1..a291ec3747 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -25,7 +25,7 @@ The service is healthy if at least one upstream is healthy. In this case, the en ## Enabling Service Insight Endpoint If you're using *Kubernetes manifests* (Deployment or DaemonSet) to install the Ingress Controller, to enable the Service Insight endpoint: -1. Run the Ingress Controller with the `-enable-service-insight` [command-line argument](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments). As a result, the Ingress Controller will expose the enpoint via the path `/probe/{hostname}` on port `9114` (customizable via the `-service-insight-listen-port` command-line argument). +1. Run the Ingress Controller with the `-enable-service-insight` [command-line argument](/nginx-ingress-controller/configuration/global-configuration/command-line-arguments). This will expose the Ingress Controller endpoint via the path `/probe/{hostname}` on port `9114` (customizable with the `-service-insight-listen-port` command-line argument). 1. To enable TLS for the Service Insight endpoint, configure the `-service-insight-tls-secret` cli argument with the namespace and name of a TLS Secret. 1. Add the Service Insight port to the list of the ports of the Ingress Controller container in the template of the Ingress Controller pod: ```yaml From 923ccb19d642fc300bf7bef16c93f1648dfb93aa Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:55:49 +0000 Subject: [PATCH 54/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index a291ec3747..62b696557f 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -7,7 +7,7 @@ doctypes: [""] aliases: - /service-insight/ toc: true -docs: "" +docs: "DOCS-000" --- From 46ac908afb927924ac86fd59ad52084a2a2f3011 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:56:03 +0000 Subject: [PATCH 55/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index 62b696557f..c6765ffeb1 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -51,7 +51,7 @@ These statistics are returned as JSON: Response codes: -* HTTP 200 OK - service is healthy +* HTTP 200 OK - Service is healthy * HTTP 404 - no upstreams/VS found for the requested hostname * HTTP 503 Service Unavailable - the service is down (all upstreams/VS are "Unhealthy") From 9d08f94394942d4a8153fd95e714b2c23c1b2893 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:56:17 +0000 Subject: [PATCH 56/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index c6765ffeb1..c48f6ce5f3 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -33,7 +33,7 @@ If you're using *Kubernetes manifests* (Deployment or DaemonSet) to install the containerPort: 9114 ``` -If you're using *Helm* to install the Ingress Controller, to enable Service Insight enpoint, configure the `serviceinsight.*` parameters of the Helm chart. See the [Installation with Helm](/nginx-ingress-controller/installation/installation-with-helm) doc. +If you're using *Helm* to install the Ingress Controller, to enable Service Insight endpoint, configure the `serviceinsight.*` parameters of the Helm chart. See the [Installation with Helm](/nginx-ingress-controller/installation/installation-with-helm) doc. ## Available Statistics and HTTP Response Codes From ee0abb0ee58aaef21dbe3d1597b74766704f844a Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:56:32 +0000 Subject: [PATCH 57/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index c48f6ce5f3..39ac4ee139 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -11,7 +11,7 @@ docs: "DOCS-000" --- -The Ingress Controller exposes an endpoint and provides host statistics for Virtual Servers. +The Ingress Controller exposes an endpoint and provides host statistics for Virtual Servers (VS). It exposes data in JSON format and returns HTTP status codes. The response body holds information about the total, down and the unhealthy number of upstreams associated with the hostname. From bd256352ce1a4c2976d3165a7c4774c2a2259b96 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:56:43 +0000 Subject: [PATCH 58/66] Update docs/content/configuration/global-configuration/command-line-arguments.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- .../global-configuration/command-line-arguments.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 47d69ef6e2..67fa275204 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -336,7 +336,7 @@ Format: `[1024 - 65535]` (default `9113`) A Secret with a TLS certificate and key for TLS termination of the Prometheus metrics endpoint. -* If the argument is not set, the prometheus endpoint will not use a TLS connection. +* If the argument is not set, the Prometheus endpoint will not use 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.   From 0aafb4162abc661480dc2279a2031d80a5015375 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:56:56 +0000 Subject: [PATCH 59/66] Update docs/content/configuration/global-configuration/command-line-arguments.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- .../global-configuration/command-line-arguments.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 67fa275204..9f0e07e402 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -359,7 +359,7 @@ Format: `[1024 - 65535]` (default `9114`) A Secret with a TLS certificate and key for TLS termination of the Service Insight enpoint. -* If the argument is not set, the service insight endpoint will not use a TLS connection. +* If the argument is not set, the Service Insight endpoint will not use 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. Format: `/` From 20751500aa56d4e259cedba0ff67b75621f33806 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:57:14 +0000 Subject: [PATCH 60/66] Update docs/content/configuration/global-configuration/command-line-arguments.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- .../global-configuration/command-line-arguments.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/configuration/global-configuration/command-line-arguments.md b/docs/content/configuration/global-configuration/command-line-arguments.md index 9f0e07e402..a31e1cd6d5 100644 --- a/docs/content/configuration/global-configuration/command-line-arguments.md +++ b/docs/content/configuration/global-configuration/command-line-arguments.md @@ -357,7 +357,7 @@ Format: `[1024 - 65535]` (default `9114`) ### -service-insight-tls-secret `` -A Secret with a TLS certificate and key for TLS termination of the Service Insight enpoint. +A Secret with a TLS certificate and key for TLS termination of the Service Insight endpoint. * If the argument is not set, the Service Insight endpoint will not use 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. From 86903f4a2989d4a94d1b57c657dec8945fd4e14c Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 09:57:55 +0000 Subject: [PATCH 61/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index 39ac4ee139..c846e7b819 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -52,7 +52,7 @@ These statistics are returned as JSON: Response codes: * HTTP 200 OK - Service is healthy -* HTTP 404 - no upstreams/VS found for the requested hostname +* HTTP 404 - No upstreams/VS found for the requested hostname * HTTP 503 Service Unavailable - the service is down (all upstreams/VS are "Unhealthy") **Note**: wildcards in hostnames are not supported at the moment. From faf82458e3c2d890ec4397efce80b0fb96982414 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Thu, 1 Dec 2022 10:06:46 +0000 Subject: [PATCH 62/66] Update docs/content/logging-and-monitoring/service-insight.md Co-authored-by: Alan Dooley Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- docs/content/logging-and-monitoring/service-insight.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/content/logging-and-monitoring/service-insight.md b/docs/content/logging-and-monitoring/service-insight.md index c846e7b819..f55ee5a5f8 100644 --- a/docs/content/logging-and-monitoring/service-insight.md +++ b/docs/content/logging-and-monitoring/service-insight.md @@ -53,6 +53,6 @@ Response codes: * HTTP 200 OK - Service is healthy * HTTP 404 - No upstreams/VS found for the requested hostname -* HTTP 503 Service Unavailable - the service is down (all upstreams/VS are "Unhealthy") +* HTTP 503 Service Unavailable - The service is down (All upstreams/VS are "Unhealthy") **Note**: wildcards in hostnames are not supported at the moment. From aec41bc21f1a0dc00589aae00b0d23890a46c63c Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Fri, 2 Dec 2022 11:13:10 +0000 Subject: [PATCH 63/66] Increase wait time in integration test --- tests/suite/test_virtual_server_service_insight.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suite/test_virtual_server_service_insight.py b/tests/suite/test_virtual_server_service_insight.py index 8b1f969626..82029eaa67 100644 --- a/tests/suite/test_virtual_server_service_insight.py +++ b/tests/suite/test_virtual_server_service_insight.py @@ -104,7 +104,7 @@ def test_responses_svc_insight_update_pods( # patch backend1 deployment with 5 replicas patch_deployment_from_yaml(kube_apis.apps_v1_api, test_namespace, f"{TEST_DATA}/service-insight/app.yaml") ensure_response_from_backend(req_url, virtual_server_setup.vs_host) - wait_before_test() + wait_before_test(15) resp = requests.get(req_url, verify=False) assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" assert resp.json() == {"Total": 6, "Up": 6, "Unhealthy": 0} From cbf43e681940ca1278a283ecd651e1ac068aa6c4 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> Date: Fri, 2 Dec 2022 14:50:20 +0000 Subject: [PATCH 64/66] Update deployments/helm-chart/values.yaml Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> Signed-off-by: Jakub Jarosz <99677300+jjngx@users.noreply.github.com> --- deployments/helm-chart/values.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deployments/helm-chart/values.yaml b/deployments/helm-chart/values.yaml index 4ca164513b..8353523267 100644 --- a/deployments/helm-chart/values.yaml +++ b/deployments/helm-chart/values.yaml @@ -435,7 +435,7 @@ prometheus: serviceInsight: ## Expose NGINX Plus Service Insight endpoint. - create: true + create: false ## Configures the port to expose endpoint. port: 9114 From 8ba10294aff1d7e50fae98ea3b2cd5ce5236aa41 Mon Sep 17 00:00:00 2001 From: Venktesh Date: Fri, 2 Dec 2022 16:39:43 +0000 Subject: [PATCH 65/66] add conditional check on waitTime for api response --- .../test_virtual_server_service_insight.py | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/suite/test_virtual_server_service_insight.py b/tests/suite/test_virtual_server_service_insight.py index 82029eaa67..bbda23c107 100644 --- a/tests/suite/test_virtual_server_service_insight.py +++ b/tests/suite/test_virtual_server_service_insight.py @@ -1,3 +1,5 @@ +from unittest import mock + import pytest import requests from settings import TEST_DATA @@ -28,11 +30,19 @@ def test_responses_svc_insight_http( self, request, kube_apis, crd_ingress_controller, virtual_server_setup, ingress_controller_endpoint ): """test responses from service insight endpoint with http""" + retry = 0 + resp = mock.Mock() + resp.json.return_value = {} + resp.status_code == 502 vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" host = get_first_host_from_yaml(vs_source) req_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" ensure_response_from_backend(req_url, virtual_server_setup.vs_host) - resp = requests.get(req_url) + while (resp.json() != {"Total": 3, "Up": 3, "Unhealthy": 0}) and retry < 5: + resp = requests.get(req_url) + wait_before_test() + retry = +1 + assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" assert resp.json() == {"Total": 3, "Up": 3, "Unhealthy": 0} @@ -78,11 +88,18 @@ def test_responses_svc_insight_https( virtual_server_setup, ): """test responses from service insight endpoint with https""" + retry = 0 + resp = mock.Mock() + resp.json.return_value = {} + resp.status_code == 502 vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" host = get_first_host_from_yaml(vs_source) req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" ensure_response_from_backend(req_url, virtual_server_setup.vs_host) - resp = requests.get(req_url, verify=False) + while (resp.json() != {"Total": 3, "Up": 3, "Unhealthy": 0}) and retry < 5: + resp = requests.get(req_url, verify=False) + wait_before_test() + retry = +1 assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" assert resp.json() == {"Total": 3, "Up": 3, "Unhealthy": 0} @@ -96,6 +113,11 @@ def test_responses_svc_insight_update_pods( crd_ingress_controller, virtual_server_setup, ): + """test responses from service insight endpoint with https and update number of replicas""" + retry = 0 + resp = mock.Mock() + resp.json.return_value = {} + resp.status_code == 502 vs_source = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" host = get_first_host_from_yaml(vs_source) req_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.service_insight_port}/probe/{host}" @@ -104,7 +126,9 @@ def test_responses_svc_insight_update_pods( # patch backend1 deployment with 5 replicas patch_deployment_from_yaml(kube_apis.apps_v1_api, test_namespace, f"{TEST_DATA}/service-insight/app.yaml") ensure_response_from_backend(req_url, virtual_server_setup.vs_host) - wait_before_test(15) - resp = requests.get(req_url, verify=False) + while (resp.json() != {"Total": 6, "Up": 6, "Unhealthy": 0}) and retry < 5: + resp = requests.get(req_url, verify=False) + wait_before_test() + retry = +1 assert resp.status_code == 200, f"Expected 200 code for /probe/{host} but got {resp.status_code}" assert resp.json() == {"Total": 6, "Up": 6, "Unhealthy": 0} From 48b3d72653814b8e529ad0a717574b37f593e4f8 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 5 Dec 2022 14:26:40 +0000 Subject: [PATCH 66/66] Add docs for Helm installation --- deployments/helm-chart/README.md | 4 ++++ docs/content/installation/installation-with-helm.md | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/deployments/helm-chart/README.md b/deployments/helm-chart/README.md index 84353f5fae..d8d78e583d 100644 --- a/deployments/helm-chart/README.md +++ b/deployments/helm-chart/README.md @@ -262,6 +262,10 @@ Parameter | Description | Default `prometheus.port` | Configures the port to scrape the metrics. | 9113 `prometheus.scheme` | Configures the HTTP scheme to use for connections to the Prometheus endpoint. | http `prometheus.secret` | The namespace / name of a Kubernetes TLS Secret. If specified, this secret is used to secure the Prometheus endpoint with TLS connections. | "" +`serviceInsight.create` | Expose NGINX Plus Service Insight endpoint. | false +`serviceInsight.port` | Configures the port to expose endpoints. | 9114 +`serviceInsight.scheme` | Configures the HTTP scheme to use for connections to the Service Insight endpoint. | http +`serviceInsight.secret` | The namespace / name of a Kubernetes TLS Secret. If specified, this secret is used to secure the Service Insight endpoint with TLS connections. | "" `nginxServiceMesh.enable` | Enable integration with NGINX Service Mesh. See the NGINX Service Mesh [docs](https://docs.nginx.com/nginx-service-mesh/tutorials/kic/deploy-with-kic/) for more details. Requires `controller.nginxplus`. | false `nginxServiceMesh.enableEgress` | Enable NGINX Service Mesh workloads to route egress traffic through the Ingress Controller. See the NGINX Service Mesh [docs](https://docs.nginx.com/nginx-service-mesh/tutorials/kic/deploy-with-kic/#enabling-egress) for more details. Requires `nginxServiceMesh.enable`. | false diff --git a/docs/content/installation/installation-with-helm.md b/docs/content/installation/installation-with-helm.md index e598bcb0fd..d6d1a5359a 100644 --- a/docs/content/installation/installation-with-helm.md +++ b/docs/content/installation/installation-with-helm.md @@ -250,6 +250,10 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont |``prometheus.port`` | Configures the port to scrape the metrics. | 9113 | |``prometheus.scheme`` | Configures the HTTP scheme that requests must use to connect to the Prometheus endpoint. | http | |``prometheus.secret`` | Specifies the namespace/name of a Kubernetes TLS secret which can be used to establish a secure HTTPS connection with the Prometheus endpoint. | "" | +|``serviceInsight.create`` | Expose NGINX Plus Service Insight endpoint. | false | +|``serviceInsight.port`` | Configures the port to scrape the metrics. | 9114 | +|``serviceInsight.scheme`` | Configures the HTTP scheme to use for connections to the Service Insight endpoint. | http | +|``serviceInsight.secret`` | The namespace / name of a Kubernetes TLS Secret. If specified, this secret is used to secure the Service Insight endpoint with TLS connections. | "" | {{% /table %}} ## Notes