From 19d557c8be083e0f1b415835f156fe4991bab66b Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Sat, 4 May 2024 11:08:09 -0300 Subject: [PATCH] add ssl-always-follow-redirect option Controller v0.11 and older had an incorrect behavior compared with ingress spec, regarding HTTPS configuration and SSL redirect. v0.12 added ssl-always-add-https option that adds the host in the HTTPS map when enabled, regardless of the tls attribute configuration. This was a v0.11 and older behavior. This update adds the ability to avoid SSL redirect when TLS was added automatically. This was also a v0.11 and older behavior. --- docs/content/en/docs/configuration/keys.md | 14 +++++-- pkg/converters/ingress/annotations/backend.go | 2 +- pkg/converters/ingress/annotations/updater.go | 1 + .../ingress/annotations/updater_test.go | 2 +- pkg/converters/ingress/defaults.go | 13 ++++--- pkg/converters/ingress/types/annotations.go | 39 ++++++++++--------- pkg/haproxy/types/host.go | 20 +++++++--- pkg/haproxy/types/types.go | 7 ++-- tests/framework/framework.go | 14 +++++-- tests/integration/integration_test.go | 6 +-- 10 files changed, 71 insertions(+), 47 deletions(-) diff --git a/docs/content/en/docs/configuration/keys.md b/docs/content/en/docs/configuration/keys.md index fee52cb78..fcadb2b3a 100644 --- a/docs/content/en/docs/configuration/keys.md +++ b/docs/content/en/docs/configuration/keys.md @@ -456,6 +456,7 @@ The table below describes all supported configuration keys. | [`slots-min-free`](#dynamic-scaling) | minimum number of free slots | Backend | `0` | | [`source-address-intf`](#source-address-intf) | `[,...]` | Backend | | | [`ssl-always-add-https`](#ssl-always-add-https) | [true\|false] | Host | `false` | +| [`ssl-always-follow-redirect`](#ssl-always-add-https) | [true\|false] | Host | `true` | | [`ssl-cipher-suites`](#ssl-ciphers) | colon-separated list | Host | [see description](#ssl-ciphers) | | [`ssl-cipher-suites-backend`](#ssl-ciphers) | colon-separated list | Backend | [see description](#ssl-ciphers) | | [`ssl-ciphers`](#ssl-ciphers) | colon-separated list | Host | [see description](#ssl-ciphers) | @@ -2488,15 +2489,20 @@ See also: ## SSL always add HTTPS -| Configuration key | Scope | Default | Since | -|------------------------|-------|---------|---------| -| `ssl-always-add-https` | Host | `false` | v0.12.4 | +| Configuration key | Scope | Default | Since | +|------------------------------|-------|---------|---------| +| `ssl-always-add-https` | Host | `false` | v0.12.4 | +| `ssl-always-follow-redirect` | Host | `true` | v0.14.7 | Every hostname declared on an Ingress resource is added to an internal HTTP map. If at least one Ingress adds the hostname in the `tls` attribute, the hostname is also added to an internal HTTPS map and does ssl offload using the default certificate. A secret name can also be added in the `tls` attribute, overriding the certificate used in the TLS handshake. `ssl-always-add-https` asks the controller to always add the domain in the internal HTTP and HTTPS maps, even if the `tls` attribute isn't declared. If `false`, a missing `tls` attribute will only declare the domain in the HTTP map and `ssl-redirect` is ignored. If `true`, a missing `tls` attribute adds the domain in the HTTPS map, and the TLS handshake will use the default certificate. If `tls` attribute is used, this configuration is ignored. -The default value is `false` since v0.13 to correctly implement Ingress spec. The default value can be globally changed in the global ConfigMap. +`ssl-always-follow-redirect` configures how the `ssl-redirect` option should be used when the `tls` attribute is missing, but the host is added in the HTTPS map. When `false`, it makes the controller to mimic a v0.11 and older behavior by not redirecting to HTTPS if the ingress does not declare the `tls` attribute. When `true`, SSL redirect will happen if configured, regardless the presence of the `tls` attribute. This option is ignored if `ssl-always-add-https` is false. + +The default value for `ssl-always-add-https` is `false` since v0.13 to correctly implement Ingress spec. The default value can be globally changed in the global ConfigMap. + +These options are implemented to help teams upgrade from older controller versions without disruptions. It is suggested not to be changed, and if so, it is also suggested to evolve ingress resources to a state that does not depend on it in the mid term. --- diff --git a/pkg/converters/ingress/annotations/backend.go b/pkg/converters/ingress/annotations/backend.go index b1910f506..8037c3c01 100644 --- a/pkg/converters/ingress/annotations/backend.go +++ b/pkg/converters/ingress/annotations/backend.go @@ -977,7 +977,7 @@ func (c *updater) buildBackendSSL(d *backData) { func (c *updater) buildBackendSSLRedirect(d *backData) { noTLSRedir := utils.Split(d.mapper.Get(ingtypes.GlobalNoTLSRedirectLocations).Value, ",") for _, path := range d.backend.Paths { - redir := path.Host != nil && path.Host.HasTLS() && + redir := path.Host != nil && path.Host.UseTLS() && d.mapper.GetConfig(path.Link).Get(ingtypes.BackSSLRedirect).Bool() if redir { for _, noredir := range noTLSRedir { diff --git a/pkg/converters/ingress/annotations/updater.go b/pkg/converters/ingress/annotations/updater.go index 4ad05b84b..ab4bebe31 100644 --- a/pkg/converters/ingress/annotations/updater.go +++ b/pkg/converters/ingress/annotations/updater.go @@ -213,6 +213,7 @@ func (c *updater) UpdateHostConfig(host *hatypes.Host, mapper *Mapper) { host.Alias.AliasName = mapper.Get(ingtypes.HostServerAlias).Value host.Alias.AliasRegex = mapper.Get(ingtypes.HostServerAliasRegex).Value host.TLS.UseDefaultCrt = mapper.Get(ingtypes.HostSSLAlwaysAddHTTPS).Bool() + host.TLS.FollowRedirect = mapper.Get(ingtypes.HostSSLAlwaysFollowRedirect).Bool() host.VarNamespace = mapper.Get(ingtypes.HostVarNamespace).Bool() c.buildHostAuthExternal(data) c.buildHostAuthTLS(data) diff --git a/pkg/converters/ingress/annotations/updater_test.go b/pkg/converters/ingress/annotations/updater_test.go index 7f529dec2..a17fa9e22 100644 --- a/pkg/converters/ingress/annotations/updater_test.go +++ b/pkg/converters/ingress/annotations/updater_test.go @@ -170,7 +170,7 @@ const testingHostname = "host.local" type hostResolver struct{} -func (h *hostResolver) HasTLS() bool { +func (h *hostResolver) UseTLS() bool { return true } diff --git a/pkg/converters/ingress/defaults.go b/pkg/converters/ingress/defaults.go index aa68b1b75..0f9d0fe18 100644 --- a/pkg/converters/ingress/defaults.go +++ b/pkg/converters/ingress/defaults.go @@ -32,12 +32,13 @@ func createDefaults() map[string]string { return map[string]string{ types.TCPTCPServiceLogFormat: "default", // - types.HostAuthTLSStrict: "true", - types.HostSSLAlwaysAddHTTPS: "false", - types.HostSSLCiphers: defaultSSLCiphers, - types.HostSSLCipherSuites: defaultSSLCipherSuites, - types.HostSSLOptionsHost: "", - types.HostTLSALPN: "h2,http/1.1", + types.HostAuthTLSStrict: "true", + types.HostSSLAlwaysAddHTTPS: "false", + types.HostSSLAlwaysFollowRedirect: "true", + types.HostSSLCiphers: defaultSSLCiphers, + types.HostSSLCipherSuites: defaultSSLCipherSuites, + types.HostSSLOptionsHost: "", + types.HostTLSALPN: "h2,http/1.1", // types.BackAuthExternalPlacement: "backend", types.BackAuthHeadersFail: "*", diff --git a/pkg/converters/ingress/types/annotations.go b/pkg/converters/ingress/types/annotations.go index 795286f5e..52803273f 100644 --- a/pkg/converters/ingress/types/annotations.go +++ b/pkg/converters/ingress/types/annotations.go @@ -36,25 +36,26 @@ var ( // Host Annotations const ( - HostAcmePreferredChain = "acme-preferred-chain" - HostAppRoot = "app-root" - HostAuthTLSErrorPage = "auth-tls-error-page" - HostAuthTLSSecret = "auth-tls-secret" - HostAuthTLSStrict = "auth-tls-strict" - HostAuthTLSVerifyClient = "auth-tls-verify-client" - HostCertSigner = "cert-signer" - HostRedirectFrom = "redirect-from" - HostRedirectFromRegex = "redirect-from-regex" - HostServerAlias = "server-alias" - HostServerAliasRegex = "server-alias-regex" - HostSSLAlwaysAddHTTPS = "ssl-always-add-https" - HostSSLCiphers = "ssl-ciphers" - HostSSLCipherSuites = "ssl-cipher-suites" - HostSSLOptionsHost = "ssl-options-host" - HostSSLPassthrough = "ssl-passthrough" - HostSSLPassthroughHTTPPort = "ssl-passthrough-http-port" - HostTLSALPN = "tls-alpn" - HostVarNamespace = "var-namespace" + HostAcmePreferredChain = "acme-preferred-chain" + HostAppRoot = "app-root" + HostAuthTLSErrorPage = "auth-tls-error-page" + HostAuthTLSSecret = "auth-tls-secret" + HostAuthTLSStrict = "auth-tls-strict" + HostAuthTLSVerifyClient = "auth-tls-verify-client" + HostCertSigner = "cert-signer" + HostRedirectFrom = "redirect-from" + HostRedirectFromRegex = "redirect-from-regex" + HostServerAlias = "server-alias" + HostServerAliasRegex = "server-alias-regex" + HostSSLAlwaysAddHTTPS = "ssl-always-add-https" + HostSSLAlwaysFollowRedirect = "ssl-always-follow-redirect" + HostSSLCiphers = "ssl-ciphers" + HostSSLCipherSuites = "ssl-cipher-suites" + HostSSLOptionsHost = "ssl-options-host" + HostSSLPassthrough = "ssl-passthrough" + HostSSLPassthroughHTTPPort = "ssl-passthrough-http-port" + HostTLSALPN = "tls-alpn" + HostVarNamespace = "var-namespace" ) var ( diff --git a/pkg/haproxy/types/host.go b/pkg/haproxy/types/host.go index 38fb440e7..dced4ab71 100644 --- a/pkg/haproxy/types/host.go +++ b/pkg/haproxy/types/host.go @@ -260,8 +260,9 @@ func (h *Host) AddRedirect(path string, match MatchType, redirTo string) { } type hostResolver struct { - useDefaultCrt *bool - crtFilename *string + useDefaultCrt *bool + followRedirect *bool + crtFilename *string } func (h *Host) addPath(path string, match MatchType, backend *Backend, redirTo string) *HostPath { @@ -281,8 +282,9 @@ func (h *Host) addLink(backend *Backend, link *PathLink, redirTo string) *HostPa } bpath := backend.AddBackendPath(link) bpath.Host = &hostResolver{ - useDefaultCrt: &h.TLS.UseDefaultCrt, - crtFilename: &h.TLS.TLSFilename, + useDefaultCrt: &h.TLS.UseDefaultCrt, + followRedirect: &h.TLS.FollowRedirect, + crtFilename: &h.TLS.TLSFilename, } } else if redirTo == "" { hback = HostBackend{ID: "_error404"} @@ -327,8 +329,14 @@ func (h *Host) HasTLS() bool { return h.TLS.UseDefaultCrt || h.TLS.TLSHash != "" } -func (h *hostResolver) HasTLS() bool { - return *h.useDefaultCrt || *h.crtFilename != "" +func (h *hostResolver) UseTLS() bool { + // whether the ingress resource has the `tls:` entry for the host + hasTLSEntry := *h.crtFilename != "" + + // whether the user has globally or locally configured auto TLS for the host + autoTLSEnabled := *h.useDefaultCrt && *h.followRedirect + + return hasTLSEntry || autoTLSEnabled } // HasTLSAuth ... diff --git a/pkg/haproxy/types/types.go b/pkg/haproxy/types/types.go index 60d6f3efa..30641b7c9 100644 --- a/pkg/haproxy/types/types.go +++ b/pkg/haproxy/types/types.go @@ -572,8 +572,9 @@ type HostRedirectConfig struct { // HostTLSConfig ... type HostTLSConfig struct { TLSConfig - CAErrorPage string - UseDefaultCrt bool + CAErrorPage string + UseDefaultCrt bool + FollowRedirect bool } // EndpointNaming ... @@ -693,7 +694,7 @@ type BackendPathItem struct { // HostResolver ... type HostResolver interface { - HasTLS() bool + UseTLS() bool } // BackendPath ... diff --git a/tests/framework/framework.go b/tests/framework/framework.go index b65e35ebc..7c2f471bb 100644 --- a/tests/framework/framework.go +++ b/tests/framework/framework.go @@ -46,6 +46,11 @@ const ( PublishSvcName = "default/publish" PublishAddress = "10.0.1.1" PublishHostname = "ingress.local" + + TestPortHTTP = 28080 + TestPortHTTPS = 28443 + TestPortStat = 21936 + TestPortTCPService = 25432 ) func NewFramework(ctx context.Context, t *testing.T, o ...options.Framework) *framework { @@ -164,8 +169,9 @@ func (f *framework) StartController(ctx context.Context, t *testing.T) { global.Namespace = "default" global.Name = "ingress-controller" global.Data = map[string]string{ - "http-port": "18080", - "https-port": "18443", + "http-port": strconv.Itoa(TestPortHTTP), + "https-port": strconv.Itoa(TestPortHTTPS), + "stats-port": strconv.Itoa(TestPortStat), "max-connections": "20", } err = f.cli.Create(ctx, &global) @@ -218,9 +224,9 @@ func (f *framework) Request(ctx context.Context, t *testing.T, method, host, pat t.Logf("request method=%s host=%s path=%s\n", method, host, path) opt := options.ParseRequestOptions(o...) - url := "http://127.0.0.1:18080" + url := fmt.Sprintf("http://127.0.0.1:%d", TestPortHTTP) if opt.HTTPS { - url = "https://127.0.0.1:18443" + url = fmt.Sprintf("https://127.0.0.1:%d", TestPortHTTPS) } req, err := http.NewRequestWithContext(ctx, method, url, nil) require.NoError(t, err) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 23238ffc1..fdbcb8d03 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -304,12 +304,12 @@ func TestIntegrationGateway(t *testing.T) { t.Run("expose TCPRoute", func(t *testing.T) { t.Parallel() - gw := f.CreateGatewayV1(ctx, t, gc, options.Listener("pgserver", "TCP", 15432)) + gw := f.CreateGatewayV1(ctx, t, gc, options.Listener("pgserver", "TCP", framework.TestPortTCPService)) svc := f.CreateService(ctx, t, tcpServerPort) _ = f.CreateTCPRouteA2(ctx, t, gw, svc) - res1 := f.TCPRequest(ctx, t, 15432, "ping") + res1 := f.TCPRequest(ctx, t, framework.TestPortTCPService, "ping") assert.Equal(t, "ping", res1) - res2 := f.TCPRequest(ctx, t, 15432, "reply") + res2 := f.TCPRequest(ctx, t, framework.TestPortTCPService, "reply") assert.Equal(t, "reply", res2) }) })