From 20bb8e8b196a94bd5f5e55c606758c255b7a347d Mon Sep 17 00:00:00 2001 From: Adel Salakh Date: Tue, 27 May 2025 20:19:12 +0200 Subject: [PATCH] fix review comments Signed-off-by: Jan Larwig Signed-off-by: Adel Salakh --- CHANGELOG.md | 1 + docs/docs/configuration/alpha_config.md | 1 + docs/docs/configuration/overview.md | 2 ++ oauthproxy.go | 2 +- pkg/apis/options/legacy_options.go | 23 +++++++++++++---------- pkg/apis/options/options.go | 4 ---- pkg/apis/options/server.go | 7 +++++++ pkg/middleware/readynesscheck.go | 2 ++ pkg/proxyhttp/server.go | 2 -- 9 files changed, 27 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0a800f1..02b10442 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - [#3304](https://github.com/oauth2-proxy/oauth2-proxy/pull/3304) fix: added conditional so default is not always set and env vars are honored fixes 3303 (@pixeldrew) - [#3264](https://github.com/oauth2-proxy/oauth2-proxy/pull/3264) fix: more aggressively truncate logged access_token (@MartinNowak / @tuunit) - [#3267](https://github.com/oauth2-proxy/oauth2-proxy/pull/3267) fix: Session refresh handling in OIDC provider (@gysel) +- [#3068](https://github.com/oauth2-proxy/oauth2-proxy/pull/3068) feat: graceful shutdown to prevent errors when oauth2-proxy is load balanced (@adelsz) # V7.13.0 diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 495bc206..efcacc7e 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -495,6 +495,7 @@ Server represents the configuration for an HTTP(S) server | `bindAddress` | _string_ | BindAddress is the address on which to serve traffic.
Leave blank or set to "-" to disable. | | `secureBindAddress` | _string_ | SecureBindAddress is the address on which to serve secure traffic.
Leave blank or set to "-" to disable. | | `tls` | _[TLS](#tls)_ | TLS contains the information for loading the certificate and key for the
secure traffic and further configuration for the TLS server. | +| `shutdownDuration` | _duration_ | Duration of time to continue serving traffic after receiving an exit signal.
During this time the readiness endpoint will be returning HTTP 503 errors.
Leave blank to disable. | ### TLS diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md index b159df09..2bc795c9 100644 --- a/docs/docs/configuration/overview.md +++ b/docs/docs/configuration/overview.md @@ -235,6 +235,8 @@ Provider specific options can be found on their respective subpages. | flag: `--tls-key-file`
toml: `tls_key_file` | string | path to private key file | | | flag: `--tls-cipher-suite`
toml: `tls_cipher_suites` | string \| list | Restricts TLS cipher suites used by server to those listed (e.g. TLS_RSA_WITH_RC4_128_SHA) (may be given multiple times). If not specified, the default Go safe cipher list is used. List of valid cipher suites can be found in the [crypto/tls documentation](https://pkg.go.dev/crypto/tls#pkg-constants). | | | flag: `--tls-min-version`
toml: `tls_min_version` | string | minimum TLS version that is acceptable, either `"TLS1.2"` or `"TLS1.3"` | `"TLS1.2"` | +| flag: `--shutdown-duration`
toml: `shutdown_duration` | duration | Duration of time to continue serving traffic after receiving an exit signal. During this duration the readiness endpoint will be returning HTTP 503 errors. | `"0s"` | + ### Session Options diff --git a/oauthproxy.go b/oauthproxy.go index 827cc8d8..67d19104 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -271,7 +271,7 @@ func (p *OAuthProxy) setupServer(opts *options.Options) error { BindAddress: opts.Server.BindAddress, SecureBindAddress: opts.Server.SecureBindAddress, TLS: opts.Server.TLS, - ShutdownDuration: opts.ShutdownDuration, + ShutdownDuration: opts.Server.ShutdownDuration, } // Option: AllowQuerySemicolons diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 6ce730fd..e0ef0716 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -471,16 +471,17 @@ func getXAuthRequestAccessTokenHeader() Header { } type LegacyServer struct { - MetricsAddress string `flag:"metrics-address" cfg:"metrics_address"` - MetricsSecureAddress string `flag:"metrics-secure-address" cfg:"metrics_secure_address"` - MetricsTLSCertFile string `flag:"metrics-tls-cert-file" cfg:"metrics_tls_cert_file"` - MetricsTLSKeyFile string `flag:"metrics-tls-key-file" cfg:"metrics_tls_key_file"` - HTTPAddress string `flag:"http-address" cfg:"http_address"` - HTTPSAddress string `flag:"https-address" cfg:"https_address"` - TLSCertFile string `flag:"tls-cert-file" cfg:"tls_cert_file"` - TLSKeyFile string `flag:"tls-key-file" cfg:"tls_key_file"` - TLSMinVersion string `flag:"tls-min-version" cfg:"tls_min_version"` - TLSCipherSuites []string `flag:"tls-cipher-suite" cfg:"tls_cipher_suites"` + MetricsAddress string `flag:"metrics-address" cfg:"metrics_address"` + MetricsSecureAddress string `flag:"metrics-secure-address" cfg:"metrics_secure_address"` + MetricsTLSCertFile string `flag:"metrics-tls-cert-file" cfg:"metrics_tls_cert_file"` + MetricsTLSKeyFile string `flag:"metrics-tls-key-file" cfg:"metrics_tls_key_file"` + HTTPAddress string `flag:"http-address" cfg:"http_address"` + HTTPSAddress string `flag:"https-address" cfg:"https_address"` + TLSCertFile string `flag:"tls-cert-file" cfg:"tls_cert_file"` + TLSKeyFile string `flag:"tls-key-file" cfg:"tls_key_file"` + TLSMinVersion string `flag:"tls-min-version" cfg:"tls_min_version"` + TLSCipherSuites []string `flag:"tls-cipher-suite" cfg:"tls_cipher_suites"` + ShutdownDuration time.Duration `flag:"shutdown-duration" cfg:"shutdown_duration"` } func legacyServerFlagset() *pflag.FlagSet { @@ -496,6 +497,7 @@ func legacyServerFlagset() *pflag.FlagSet { flagSet.String("tls-key-file", "", "path to private key file") flagSet.String("tls-min-version", "", "minimal TLS version for HTTPS clients (either \"TLS1.2\" or \"TLS1.3\")") flagSet.StringSlice("tls-cipher-suite", []string{}, "restricts TLS cipher suites to those listed (e.g. TLS_RSA_WITH_RC4_128_SHA) (may be given multiple times)") + flagSet.Duration("shutdown-duration", 0, "Amount of time to continue serving traffic after receiving an exit signal with readiness endpoint set to false.") return flagSet } @@ -650,6 +652,7 @@ func (l LegacyServer) convert() (Server, Server) { appServer := Server{ BindAddress: l.HTTPAddress, SecureBindAddress: l.HTTPSAddress, + ShutdownDuration: l.ShutdownDuration, } if l.TLSKeyFile != "" || l.TLSCertFile != "" { appServer.TLS = &TLS{ diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 38d48a37..b57d5aed 100644 --- a/pkg/apis/options/options.go +++ b/pkg/apis/options/options.go @@ -3,7 +3,6 @@ package options import ( "crypto" "net/url" - "time" ipapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/ip" internaloidc "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/providers/oidc" @@ -30,8 +29,6 @@ type Options struct { RawRedirectURL string `flag:"redirect-url" cfg:"redirect_url"` RelativeRedirectURL bool `flag:"relative-redirect-url" cfg:"relative_redirect_url"` - ShutdownDuration time.Duration `flag:"shutdown-duration" cfg:"shutdown_duration"` - AuthenticatedEmailsFile string `flag:"authenticated-emails-file" cfg:"authenticated_emails_file"` EmailDomains []string `flag:"email-domain" cfg:"email_domains"` WhitelistDomains []string `flag:"whitelist-domain" cfg:"whitelist_domains"` @@ -164,7 +161,6 @@ func NewFlagSet() *pflag.FlagSet { flagSet.Int("redis-connection-idle-timeout", 0, "Redis connection idle timeout seconds, if Redis timeout option is non-zero, the --redis-connection-idle-timeout must be less then Redis timeout option") flagSet.String("signature-key", "", "GAP-Signature request signature key (algorithm:secretkey)") flagSet.Bool("gcp-healthchecks", false, "Enable GCP/GKE healthcheck endpoints") - flagSet.Duration("shutdown-duration", 0, "Amount of time to continue serving traffic after receiving an exit signal with readiness endpoint set to false.") flagSet.AddFlagSet(cookieFlagSet()) flagSet.AddFlagSet(loggingFlagSet()) diff --git a/pkg/apis/options/server.go b/pkg/apis/options/server.go index 8fa41af8..a26d3a61 100644 --- a/pkg/apis/options/server.go +++ b/pkg/apis/options/server.go @@ -1,5 +1,7 @@ package options +import "time" + // Server represents the configuration for an HTTP(S) server type Server struct { // BindAddress is the address on which to serve traffic. @@ -13,6 +15,11 @@ type Server struct { // TLS contains the information for loading the certificate and key for the // secure traffic and further configuration for the TLS server. TLS *TLS `yaml:"tls,omitempty"` + + // Duration of time to continue serving traffic after receiving an exit signal. + // During this time the readiness endpoint will be returning HTTP 503 errors. + // Leave blank to disable. + ShutdownDuration time.Duration `yaml:"shutdownDuration,omitempty"` } // TLS contains the information for loading a TLS certificate and key diff --git a/pkg/middleware/readynesscheck.go b/pkg/middleware/readynesscheck.go index 6977cf2e..2179454a 100644 --- a/pkg/middleware/readynesscheck.go +++ b/pkg/middleware/readynesscheck.go @@ -24,6 +24,8 @@ func NewReadynessCheck(ctx context.Context, path string, verifiable Verifiable) func readynessCheck(ctx context.Context, path string, verifiable Verifiable, next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { + // Check the server context (not request). + // Has the context been canceled because of SIGTERM? if ctx.Err() != nil { rw.WriteHeader(http.StatusServiceUnavailable) fmt.Fprintf(rw, "Shutting down") diff --git a/pkg/proxyhttp/server.go b/pkg/proxyhttp/server.go index 5372d1c3..6bbebd77 100644 --- a/pkg/proxyhttp/server.go +++ b/pkg/proxyhttp/server.go @@ -222,9 +222,7 @@ func (s *server) startServer(ctx context.Context, listener net.Listener) error { g.Go(func() error { <-groupCtx.Done() logger.Printf("Context canceled. Waiting %s before shutting down the listeners.", s.shutdownDuration) - time.Sleep(s.shutdownDuration) - logger.Printf("Shutting down listener.") if err := srv.Shutdown(context.Background()); err != nil {