From 38a03fa3c183b0229b250ed586e92a1b4ca9e41c Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 5 Jan 2024 10:52:24 +0000 Subject: [PATCH] Add use-cluster-ip annotation for ingress resources (#4862) * add use cluter ip annotation * Update docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md Co-authored-by: Alan Dooley Signed-off-by: Jim Ryan * annotation validation * remove duplicate test * small docs fix * go ingress tests * use fqdn to support cross namespace ingresses --------- Signed-off-by: Jim Ryan Co-authored-by: Alan Dooley --- ...advanced-configuration-with-annotations.md | 1 + ...server-and-virtualserverroute-resources.md | 2 +- docs/content/tutorials/nginx-ingress-istio.md | 2 +- .../mergeable-ingress-types/cafe.yaml | 1 - internal/configs/annotations.go | 12 + internal/configs/config_params.go | 1 + internal/configs/ingress.go | 27 +- internal/configs/ingress_test.go | 247 ++++++++++++++++++ internal/k8s/validation.go | 4 + internal/k8s/validation_test.go | 38 +++ 10 files changed, 325 insertions(+), 10 deletions(-) diff --git a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md index a7e67204bd..2d89fb33d9 100644 --- a/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md +++ b/docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md @@ -172,6 +172,7 @@ The table below summarizes the available annotations. |``nginx.com/health-checks-mandatory`` | N/A | Configures active health checks as mandatory. | ``False`` | [Support for Active Health Checks](https://github.com/nginxinc/kubernetes-ingress/tree/v3.4.0/examples/ingress-resources/health-checks). | |``nginx.com/health-checks-mandatory-queue`` | N/A | When active health checks are mandatory, creates a queue where incoming requests are temporarily stored while NGINX Plus is checking the health of the endpoints after a configuration reload. | ``0`` | [Support for Active Health Checks](https://github.com/nginxinc/kubernetes-ingress/tree/v3.4.0/examples/ingress-resources/health-checks). | |``nginx.com/slow-start`` | N/A | Sets the upstream server [slow-start period](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#server-slow-start). By default, slow-start is activated after a server becomes [available](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-health-check/#passive-health-checks) or [healthy](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-health-check/#active-health-checks). To enable slow-start for newly-added servers, configure [mandatory active health checks](https://github.com/nginxinc/kubernetes-ingress/tree/v3.4.0/examples/ingress-resources/health-checks). | ``"0s"`` | | +|``nginx.org/use-cluster-ip`` | N/A | Enables using the Cluster IP and port of the service instead of the default behavior of using the IP and port of the pods. When this field is enabled, the fields that configure NGINX behavior related to multiple upstream servers (like ``lb-method`` and ``next-upstream``) will have no effect, as NGINX Ingress Controller will configure NGINX with only one upstream server that will match the service Cluster IP. | ``False`` | | {{% /table %}} ### Snippets and Custom Templates diff --git a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md index 831709c81f..2e8d178ecc 100644 --- a/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md +++ b/docs/content/configuration/virtualserver-and-virtualserverroute-resources.md @@ -349,7 +349,7 @@ tls: |``name`` | The name of the upstream. Must be a valid DNS label as defined in RFC 1035. For example, ``hello`` and ``upstream-123`` are valid. The name must be unique among all upstreams of the resource. | ``string`` | Yes | |``service`` | The name of a [service](https://kubernetes.io/docs/concepts/services-networking/service/). The service must belong to the same namespace as the resource. If the service doesn't exist, NGINX will assume the service has zero endpoints and return a ``502`` response for requests for this upstream. For NGINX Plus only, services of type [ExternalName](https://kubernetes.io/docs/concepts/services-networking/service/#externalname) are also supported (check the [prerequisites](https://github.com/nginxinc/kubernetes-ingress/tree/v3.4.0/examples/ingress-resources/externalname-services#prerequisites) ). | ``string`` | Yes | |``subselector`` | Selects the pods within the service using label keys and values. By default, all pods of the service are selected. Note: the specified labels are expected to be present in the pods when they are created. If the pod labels are updated, the Ingress Controller will not see that change until the number of the pods is changed. | ``map[string]string`` | No | -|``use-cluster-ip`` | Enables using the Cluster IP and port of the service instead of the default behavior of using the IP and port of the pods. When this field is enabled, the fields that configure NGINX behavior related to multiple upstream servers (like ``lb-method`` and ``next-upstream``) will have no effect, as the Ingress Controller will configure NGINX with only one upstream server that will match the service Cluster IP. | ``boolean`` | No | +|``use-cluster-ip`` | Enables using the Cluster IP and port of the service instead of the default behavior of using the IP and port of the pods. When this field is enabled, the fields that configure NGINX behavior related to multiple upstream servers (like ``lb-method`` and ``next-upstream``) will have no effect, as NGINX Ingress Controller will configure NGINX with only one upstream server that will match the service Cluster IP. | ``boolean`` | No | |``port`` | The port of the service. If the service doesn't define that port, NGINX will assume the service has zero endpoints and return a ``502`` response for requests for this upstream. The port must fall into the range ``1..65535``. | ``uint16`` | Yes | |``lb-method`` | The load [balancing method](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method). To use the round-robin method, specify ``round_robin``. The default is specified in the ``lb-method`` ConfigMap key. | ``string`` | No | |``fail-timeout`` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the ``fail-timeout`` ConfigMap key. | ``string`` | No | diff --git a/docs/content/tutorials/nginx-ingress-istio.md b/docs/content/tutorials/nginx-ingress-istio.md index 6bb752ae04..b6eb8a1d55 100644 --- a/docs/content/tutorials/nginx-ingress-istio.md +++ b/docs/content/tutorials/nginx-ingress-istio.md @@ -214,7 +214,7 @@ We can see in the above output that our curl request is sent and received by NGI By default, for NGINX Ingress Controller, we populate the upstream server addresses with the endpoint IPs of the pods. -When using the new `use-cluster-ip` feature, we will no populate the upstream with the `service` IP address, instead of the endpoint IP addresses. +When using the new `use-cluster-ip` feature, we will now populate the upstream with the `service` IP address, instead of the endpoint IP addresses. In the 1.11 release, NGINX Ingress controller will only send one host header, depending on how you configure Ingress. By default NGINX Ingress Controller will send `proxy_set_header $host`. If Ingress has been configured with `action.proxy.requestHeaders` this ensures that only one set of headers will be sent to the upstream server. In summary, by setting `action.proxy.requestHeaders` in the `VirtualServer` CRD, NGINX Ingress will only send the specified headers that have been defined. diff --git a/examples/ingress-resources/mergeable-ingress-types/cafe.yaml b/examples/ingress-resources/mergeable-ingress-types/cafe.yaml index cda0f23573..4127392147 100644 --- a/examples/ingress-resources/mergeable-ingress-types/cafe.yaml +++ b/examples/ingress-resources/mergeable-ingress-types/cafe.yaml @@ -55,7 +55,6 @@ apiVersion: v1 kind: Service metadata: name: tea-svc - labels: spec: ports: - port: 80 diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 5dbcd662f0..a73dbf3d6a 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -13,6 +13,9 @@ const BasicAuthSecretAnnotation = "nginx.org/basic-auth-secret" // #nosec G101 // PathRegexAnnotation is the annotation where the regex location (path) modifier is specified. const PathRegexAnnotation = "nginx.org/path-regex" +// UseClusterIPAnnotation is the annotation where the use-cluster-ip boolean is specified. +const UseClusterIPAnnotation = "nginx.org/use-cluster-ip" + // AppProtectPolicyAnnotation is where the NGINX App Protect policy is specified const AppProtectPolicyAnnotation = "appprotect.f5.com/app-protect-policy" @@ -37,6 +40,7 @@ var masterBlacklist = map[string]bool{ "nginx.com/health-checks": true, "nginx.com/health-checks-mandatory": true, "nginx.com/health-checks-mandatory-queue": true, + UseClusterIPAnnotation: true, } var minionBlacklist = map[string]bool{ @@ -401,6 +405,14 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool glog.Errorf("Ingress %s/%s: Invalid value nginx.org/path-regex: got %q. Allowed values: 'case_sensitive', 'case_insensitive', 'exact'", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName(), pathRegex) } } + + if useClusterIP, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, UseClusterIPAnnotation, ingEx.Ingress); exists { + if err != nil { + glog.Error(err) + } else { + cfgParams.UseClusterIP = useClusterIP + } + } return cfgParams } diff --git a/internal/configs/config_params.go b/internal/configs/config_params.go index 908463024e..1028035cca 100644 --- a/internal/configs/config_params.go +++ b/internal/configs/config_params.go @@ -83,6 +83,7 @@ type ConfigParams struct { SlowStart string SSLRedirect bool UpstreamZoneSize string + UseClusterIP bool VariablesHashBucketSize uint64 VariablesHashMaxSize uint64 diff --git a/internal/configs/ingress.go b/internal/configs/ingress.go index 799624a6fc..ff09975fba 100644 --- a/internal/configs/ingress.go +++ b/internal/configs/ingress.go @@ -511,21 +511,34 @@ func createUpstream(ingEx *IngressEx, name string, backend *networking.IngressBa endps = []string{} } - for _, endp := range endps { + if cfg.UseClusterIP { + fqdn := fmt.Sprintf("%s.%s.svc.cluster.local:%d", backend.Service.Name, ingEx.Ingress.Namespace, backend.Service.Port.Number) upsServers = append(upsServers, version1.UpstreamServer{ - Address: endp, + Address: fqdn, MaxFails: cfg.MaxFails, MaxConns: cfg.MaxConns, FailTimeout: cfg.FailTimeout, SlowStart: cfg.SlowStart, Resolve: isExternalNameSvc, }) - } - if len(upsServers) > 0 { - sort.Slice(upsServers, func(i, j int) bool { - return upsServers[i].Address < upsServers[j].Address - }) ups.UpstreamServers = upsServers + } else { + for _, endp := range endps { + upsServers = append(upsServers, version1.UpstreamServer{ + Address: endp, + MaxFails: cfg.MaxFails, + MaxConns: cfg.MaxConns, + FailTimeout: cfg.FailTimeout, + SlowStart: cfg.SlowStart, + Resolve: isExternalNameSvc, + }) + } + if len(upsServers) > 0 { + sort.Slice(upsServers, func(i, j int) bool { + return upsServers[i].Address < upsServers[j].Address + }) + ups.UpstreamServers = upsServers + } } } diff --git a/internal/configs/ingress_test.go b/internal/configs/ingress_test.go index 29911e0977..decf9f4a2f 100644 --- a/internal/configs/ingress_test.go +++ b/internal/configs/ingress_test.go @@ -663,6 +663,253 @@ func TestGenerateNginxCfgForMergeableIngressesForBasicAuth(t *testing.T) { } } +func TestGenerateNginxCfgForMergeableIngressesWithUseClusterIP(t *testing.T) { + t.Parallel() + mergeableIngresses := createMergeableCafeIngress() + mergeableIngresses.Minions[0].Ingress.Annotations["nginx.org/use-cluster-ip"] = "true" + + isPlus := false + + expected := createExpectedConfigForMergeableCafeIngressWithUseClusterIP() + configParams := NewDefaultConfigParams(isPlus) + + result, warnings := generateNginxCfgForMergeableIngresses(NginxCfgParams{ + mergeableIngs: mergeableIngresses, + apResources: nil, + dosResource: nil, + baseCfgParams: configParams, + isPlus: isPlus, + isResolverConfigured: false, + staticParams: &StaticConfigParams{}, + isWildcardEnabled: false, + }) + + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfgForMergeableIngresses() returned unexpected result (-want +got):\n%s", diff) + } + if len(warnings) != 0 { + t.Errorf("generateNginxCfgForMergeableIngresses() returned warnings: %v", warnings) + } +} + +func createExpectedConfigForMergeableCafeIngressWithUseClusterIP() version1.IngressNginxConfig { + upstreamZoneSize := "256k" + coffeeUpstream := version1.Upstream{ + Name: "default-cafe-ingress-coffee-minion-cafe.example.com-coffee-svc-80", + LBMethod: "random two least_conn", + UpstreamZoneSize: upstreamZoneSize, + UpstreamServers: []version1.UpstreamServer{ + { + Address: "coffee-svc.default.svc.cluster.local:80", + MaxFails: 1, + MaxConns: 0, + FailTimeout: "10s", + }, + }, + } + teaUpstream := version1.Upstream{ + Name: "default-cafe-ingress-tea-minion-cafe.example.com-tea-svc-80", + LBMethod: "random two least_conn", + UpstreamZoneSize: upstreamZoneSize, + UpstreamServers: []version1.UpstreamServer{ + { + Address: "10.0.0.2:80", + MaxFails: 1, + MaxConns: 0, + FailTimeout: "10s", + }, + }, + } + expected := version1.IngressNginxConfig{ + Upstreams: []version1.Upstream{ + coffeeUpstream, + teaUpstream, + }, + Servers: []version1.Server{ + { + Name: "cafe.example.com", + ServerTokens: "on", + Locations: []version1.Location{ + { + Path: "/coffee", + ServiceName: "coffee-svc", + Upstream: coffeeUpstream, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + MinionIngress: &version1.Ingress{ + Name: "cafe-ingress-coffee-minion", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "minion", + "nginx.org/use-cluster-ip": "true", + }, + }, + ProxySSLName: "coffee-svc.default.svc", + }, + { + Path: "/tea", + ServiceName: "tea-svc", + Upstream: teaUpstream, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + MinionIngress: &version1.Ingress{ + Name: "cafe-ingress-tea-minion", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "minion", + }, + }, + ProxySSLName: "tea-svc.default.svc", + }, + }, + SSL: true, + SSLCertificate: "/etc/nginx/secrets/default-cafe-secret", + SSLCertificateKey: "/etc/nginx/secrets/default-cafe-secret", + StatusZone: "cafe.example.com", + HSTSMaxAge: 2592000, + Ports: []int{80}, + SSLPorts: []int{443}, + SSLRedirect: true, + HealthChecks: make(map[string]version1.HealthCheck), + }, + }, + Ingress: version1.Ingress{ + Name: "cafe-ingress-master", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/mergeable-ingress-type": "master", + }, + }, + } + + return expected +} + +func createExpectedConfigForCafeIngressWithUseClusterIP() version1.IngressNginxConfig { + upstreamZoneSize := "256k" + + coffeeUpstream := version1.Upstream{ + Name: "default-cafe-ingress-cafe.example.com-coffee-svc-80", + LBMethod: "random two least_conn", + UpstreamZoneSize: upstreamZoneSize, + UpstreamServers: []version1.UpstreamServer{ + { + Address: "coffee-svc.default.svc.cluster.local:80", + MaxFails: 1, + MaxConns: 0, + FailTimeout: "10s", + }, + }, + } + + teaUpstream := version1.Upstream{ + Name: "default-cafe-ingress-cafe.example.com-tea-svc-80", + LBMethod: "random two least_conn", + UpstreamZoneSize: upstreamZoneSize, + UpstreamServers: []version1.UpstreamServer{ + { + Address: "tea-svc.default.svc.cluster.local:80", + MaxFails: 1, + MaxConns: 0, + FailTimeout: "10s", + }, + }, + } + + expected := version1.IngressNginxConfig{ + Upstreams: []version1.Upstream{ + coffeeUpstream, + teaUpstream, + }, + Servers: []version1.Server{ + { + Name: "cafe.example.com", + ServerTokens: "on", + Locations: []version1.Location{ + { + Path: "/coffee", + ServiceName: "coffee-svc", + Upstream: coffeeUpstream, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + ProxySSLName: "coffee-svc.default.svc", + }, + { + Path: "/tea", + ServiceName: "tea-svc", + Upstream: teaUpstream, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + ProxySSLName: "tea-svc.default.svc", + }, + }, + SSL: true, + SSLCertificate: "/etc/nginx/secrets/default-cafe-secret", + SSLCertificateKey: "/etc/nginx/secrets/default-cafe-secret", + StatusZone: "cafe.example.com", + HSTSMaxAge: 2592000, + Ports: []int{80}, + SSLPorts: []int{443}, + SSLRedirect: true, + HealthChecks: make(map[string]version1.HealthCheck), + }, + }, + Ingress: version1.Ingress{ + Name: "cafe-ingress", + Namespace: "default", + Annotations: map[string]string{ + "kubernetes.io/ingress.class": "nginx", + "nginx.org/use-cluster-ip": "true", + }, + }, + } + return expected +} + +func TestGenerateNginxCfgWithUseClusterIP(t *testing.T) { + t.Parallel() + cafeIngressEx := createCafeIngressEx() + cafeIngressEx.Ingress.Annotations["nginx.org/use-cluster-ip"] = "true" + isPlus := false + configParams := NewDefaultConfigParams(isPlus) + + expected := createExpectedConfigForCafeIngressWithUseClusterIP() + + result, warnings := generateNginxCfg(NginxCfgParams{ + staticParams: &StaticConfigParams{}, + ingEx: &cafeIngressEx, + apResources: nil, + dosResource: nil, + isMinion: false, + isPlus: false, + baseCfgParams: configParams, + isResolverConfigured: false, + isWildcardEnabled: false, + }) + + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("generateNginxCfg() returned unexpected result (-want +got):\n%s", diff) + } + if len(warnings) != 0 { + t.Errorf("generateNginxCfg() returned warnings: %v", warnings) + } +} + func createMergeableCafeIngress() *MergeableIngresses { master := networking.Ingress{ ObjectMeta: meta_v1.ObjectMeta{ diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index f242895270..9ae802bf02 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -69,6 +69,7 @@ const ( rewritesAnnotation = "nginx.org/rewrites" stickyCookieServicesAnnotation = "nginx.com/sticky-cookie-services" pathRegexAnnotation = "nginx.org/path-regex" + useClusterIPAnnotation = "nginx.org/use-cluster-ip" ) const ( @@ -332,6 +333,9 @@ var ( pathRegexAnnotation: { validatePathRegex, }, + useClusterIPAnnotation: { + validateBoolAnnotation, + }, } annotationNames = sortedAnnotationNames(annotationValidations) ) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 1e57722028..ee1ab174ac 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -3046,6 +3046,44 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.com/sticky-cookie-services annotation", }, + { + annotations: map[string]string{ + "nginx.org/use-cluster-ip": "not_a_boolean", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/use-cluster-ip: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/use-cluster-ip annotation", + }, + { + annotations: map[string]string{ + "nginx.org/use-cluster-ip": "true", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/use-cluster-ip annotation", + }, + { + annotations: map[string]string{ + "nginx.org/use-cluster-ip": "false", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: false, + appProtectDosEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/use-cluster-ip annotation", + }, } for _, test := range tests {