pkg/tls,cmd/krp/app/options: fix breaking changes

Some logging were removed. We introduce them in a disabled, but
non-breaking way.
Updating the deprecated functions in TLS and using the new ones.

README.md

@@ -90,19 +90,8 @@ Kube-rbac-proxy flags:
 
 Global flags:
 
-      --add-dir-header                   If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --alsologtostderr                  log to standard error as well as files (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-  -h, --help                             help for kube-rbac-proxy
-      --log-backtrace-at traceLocation   when logging hits line file:N, emit a stack trace (default :0) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --log-dir string                   If non-empty, write log files in this directory (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --log-file string                  If non-empty, use this log file (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --log-file-max-size uint           Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --logtostderr                      log to standard error instead of files (default true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --one-output                       If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --skip-headers                     If true, avoid header prefixes in the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --skip-log-headers                 If true, avoid headers when opening log files (no effect when -logtostderr=true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --stderrthreshold severity         logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false) (default 2) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
-      --version version[=true]           Print version information and quit
+  -h, --help                     help for kube-rbac-proxy
+      --version version[=true]   --version, --version=raw prints version information and quits; --version=vX.Y.Z... sets the reported version
 ```
 
 

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go

@@ -40,7 +40,6 @@ import (
 	"golang.org/x/net/http2"
 	"golang.org/x/net/http2/h2c"
 
-	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authorization/union"
 	"k8s.io/client-go/kubernetes"
@@ -84,17 +83,16 @@ that can perform RBAC authorization against the Kubernetes API using SubjectAcce
 
 			k8sapiflag.PrintFlags(fs)
 
+			if err := o.Validate(); err != nil {
+				return err
+			}
+
 			// set default options
 			completedOptions, err := Complete(o)
 			if err != nil {
 				return err
 			}
 
-			// validate options
-			if errs := completedOptions.Validate(); len(errs) != 0 {
-				return utilerrors.NewAggregate(errs)
-			}
-
 			return Run(completedOptions)
 		},
 		Args: func(cmd *cobra.Command, args []string) error {
@@ -146,63 +144,6 @@ type completedProxyRunOptions struct {
 	ignorePaths []string
 }
 
-func (o *completedProxyRunOptions) Validate() []error {
-	var errs []error
-
-	hasCerts := !(o.tls.CertFile == "") && !(o.tls.KeyFile == "")
-	hasInsecureListenAddress := o.insecureListenAddress != ""
-	if !hasCerts || hasInsecureListenAddress {
-		klog.Warning(`
-==== Deprecation Warning ======================
-
-Insecure listen address will be removed.
-Using --insecure-listen-address won't be possible!
-
-The ability to run kube-rbac-proxy without TLS certificates will be removed.
-Not using --tls-cert-file and --tls-private-key-file won't be possible!
-
-For more information, please go to https://github.com/brancz/kube-rbac-proxy/issues/187
-
-===============================================
-
-		`)
-	}
-
-	if o.tls.ReloadInterval != time.Minute {
-		klog.Warning(`
-==== Deprecation Warning ======================
-
-tls-reload-interval will be removed.
-Using --tls-reload-interval won't be possible!
-
-For more information, please go to https://github.com/brancz/kube-rbac-proxy/issues/196
-
-===============================================
-		`)
-
-	}
-
-	if len(o.allowPaths) > 0 && len(o.ignorePaths) > 0 {
-		errs = append(errs, fmt.Errorf("cannot use --allow-paths and --ignore-paths together"))
-	}
-
-	for _, pathAllowed := range o.allowPaths {
-		_, err := path.Match(pathAllowed, "")
-		if err != nil {
-			errs = append(errs, fmt.Errorf("failed to verify allow path: %s", pathAllowed))
-		}
-	}
-
-	for _, pathIgnored := range o.ignorePaths {
-		_, err := path.Match(pathIgnored, "")
-		if err != nil {
-			errs = append(errs, fmt.Errorf("failed to verify ignored path: %s", pathIgnored))
-		}
-	}
-
-	return errs
-}
-
 func Complete(o *options.ProxyRunOptions) (*completedProxyRunOptions, error) {
 	var err error
 	completed := &completedProxyRunOptions{
@@ -451,7 +392,7 @@ func Run(cfg *completedProxyRunOptions) error {
 				return srv.Serve(tlsListener)
 			}, func(err error) {
 				if err := srv.Shutdown(context.Background()); err != nil {
-					klog.Errorf("failed to gracefully shutdown server: %w", err)
+					klog.Error(fmt.Errorf("failed to gracefully shutdown server: %w", err))
 				}
 			})
 
@@ -499,7 +440,7 @@ func Run(cfg *completedProxyRunOptions) error {
 					return proxyEndpointsSrv.Serve(tlsListener)
 				}, func(err error) {
 					if err := proxyEndpointsSrv.Shutdown(context.Background()); err != nil {
-						klog.Errorf("failed to gracefully shutdown proxy endpoints server: %w", err)
+						klog.Error(fmt.Errorf("failed to gracefully shutdown proxy endpoints server: %w", err))
 					}
 				})
 			}
@@ -524,10 +465,10 @@ func Run(cfg *completedProxyRunOptions) error {
 				return srv.Serve(l)
 			}, func(err error) {
 				if err := srv.Shutdown(context.Background()); err != nil {
-					klog.Errorf("failed to gracefully shutdown server: %w", err)
+					klog.Error(fmt.Errorf("failed to gracefully shutdown server: %w", err))
 				}
 				if err := l.Close(); err != nil {
-					klog.Errorf("failed to gracefully close listener: %w", err)
+					klog.Error(fmt.Errorf("failed to gracefully close listener: %w", err))
 				}
 			})
 		}

cmd/kube-rbac-proxy/app/options/options.go

@@ -17,13 +17,18 @@ limitations under the License.
 package options
 
 import (
+	"fmt"
+	"path"
 	"time"
 
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	k8sapiflag "k8s.io/component-base/cli/flag"
+	"k8s.io/klog/v2"
 
 	"github.com/brancz/kube-rbac-proxy/pkg/authn"
 	"github.com/brancz/kube-rbac-proxy/pkg/authz"
 	"github.com/brancz/kube-rbac-proxy/pkg/proxy"
+	"github.com/spf13/pflag"
 )
 
 type ProxyRunOptions struct {
@@ -45,6 +50,22 @@ type ProxyRunOptions struct {
 	HTTP2Disable              bool
 	HTTP2MaxConcurrentStreams uint32
 	HTTP2MaxSize              uint32
+
+	flagSet *pflag.FlagSet
+}
+
+var disabledFlags = []string{
+	"logtostderr",
+	"add-dir-header",
+	"alsologtostderr",
+	"log-backtrace-at",
+	"log-dir",
+	"log-file",
+	"log-file-max-size",
+	"one-output",
+	"skip-headers",
+	"skip-log-headers",
+	"stderrthreshold",
 }
 
 type TLSConfig struct {
@@ -122,5 +143,84 @@ func (o *ProxyRunOptions) Flags() k8sapiflag.NamedFlagSets {
 	flagset.Uint32Var(&o.HTTP2MaxConcurrentStreams, "http2-max-concurrent-streams", 100, "The maximum number of concurrent streams per HTTP/2 connection.")
 	flagset.Uint32Var(&o.HTTP2MaxSize, "http2-max-size", 256*1024, "The maximum number of bytes that the server will accept for frame size and buffer per stream in a HTTP/2 request.")
 
+	// disabled flags
+	o.flagSet = flagset // reference used for validation
+	for _, disabledOpt := range disabledFlags {
+		_ = flagset.String(disabledOpt, "", "[DISABLED]")
+		if err := flagset.MarkHidden(disabledOpt); err != nil {
+			panic(err)
+		}
+	}
+
 	return namedFlagSets
 }
+
+func (o *ProxyRunOptions) Validate() error {
+	var errs []error
+
+	hasCerts := !(o.TLS.CertFile == "") && !(o.TLS.KeyFile == "")
+	hasInsecureListenAddress := o.InsecureListenAddress != ""
+	if !hasCerts || hasInsecureListenAddress {
+		klog.Warning(`
+==== Deprecation Warning ======================
+
+Insecure listen address will be removed.
+Using --insecure-listen-address won't be possible!
+
+The ability to run kube-rbac-proxy without TLS certificates will be removed.
+Not using --tls-cert-file and --tls-private-key-file won't be possible!
+
+For more information, please go to https://github.com/brancz/kube-rbac-proxy/issues/187
+
+===============================================
+
+		`)
+	}
+
+	if o.TLS.ReloadInterval != time.Minute {
+		klog.Warning(`
+==== Deprecation Warning ======================
+
+tls-reload-interval will be removed.
+Using --tls-reload-interval won't be possible!
+
+For more information, please go to https://github.com/brancz/kube-rbac-proxy/issues/196
+
+===============================================
+		`)
+
+	}
+
+	if len(o.AllowPaths) > 0 && len(o.IgnorePaths) > 0 {
+		errs = append(errs, fmt.Errorf("cannot use --allow-paths and --ignore-paths together"))
+	}
+
+	for _, pathAllowed := range o.AllowPaths {
+		_, err := path.Match(pathAllowed, "")
+		if err != nil {
+			errs = append(errs, fmt.Errorf("failed to verify allow path: %s", pathAllowed))
+		}
+	}
+
+	for _, pathIgnored := range o.IgnorePaths {
+		_, err := path.Match(pathIgnored, "")
+		if err != nil {
+			errs = append(errs, fmt.Errorf("failed to verify ignored path: %s", pathIgnored))
+		}
+	}
+
+	// Removed upstream flags shouldn't be use
+	for _, disabledOpt := range disabledFlags {
+		if flag := o.flagSet.Lookup(disabledOpt); flag.Changed {
+			klog.Warningf(`
+==== Removed Flag Warning ======================
+
+%s is removed in the k8s upstream and has no effect any more.
+
+===============================================
+		`, disabledOpt)
+		}
+	}
+
+	return utilerrors.NewAggregate(errs)
+}

pkg/tls/reloader_test.go

@@ -335,7 +335,8 @@ func writeTempFile(pattern string, data []byte) (string, error) {
 func poll(interval, timeout time.Duration, f func() error) error {
 	var lastErr error
 
-	err := wait.Poll(interval, timeout, func() (bool, error) {
+	ctx := context.Background()
+	err := wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(_ context.Context) (bool, error) {
 		lastErr = f()
 
 		if lastErr != nil {
@@ -346,7 +347,7 @@ func poll(interval, timeout time.Duration, f func() error) error {
 		return true, nil
 	})
 
-	if err != nil && err == wait.ErrWaitTimeout && lastErr != nil {
+	if err != nil && wait.Interrupted(err) && lastErr != nil {
 		err = fmt.Errorf("%v: %v", err, lastErr)
 	}
 

test/e2e/allowpaths/deployment.yaml

@@ -22,7 +22,6 @@ spec:
             - "--proxy-endpoints-port=8643"
             - "--upstream=http://127.0.0.1:8081/"
             - "--allow-paths=/metrics,/api/v1/label/*/values"
-            - "--logtostderr=true"
             - "--v=10"
           ports:
             - containerPort: 8443

test/e2e/basics.go

@@ -108,6 +108,92 @@ func testBasics(client kubernetes.Interface) kubetest.TestSuite {
 	}
 }
 
+func testFlags(client kubernetes.Interface) kubetest.TestSuite {
+	return func(t *testing.T) {
+		command := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" https://kube-rbac-proxy.default.svc.cluster.local:8443/metrics`
+
+		kubetest.Scenario{
+			Name: "WithAllOtherDisabledFlags",
+			Description: `
+				This should succeed. Even though all flags are set for kube-rbac-proxy.
+				This implies deprecated flags that got disabled.
+			`,
+
+			Given: kubetest.Actions(
+				kubetest.CreatedManifests(
+					client,
+					"flags/clusterRole.yaml",
+					"flags/clusterRoleBinding.yaml",
+					"flags/deployment-other-flags.yaml",
+					"flags/service.yaml",
+					"flags/serviceAccount.yaml",
+					"flags/clusterRole-client.yaml",
+					"flags/clusterRoleBinding-client.yaml",
+				),
+			),
+			When: kubetest.Actions(
+				kubetest.PodsAreReady(
+					client,
+					1,
+					"app=kube-rbac-proxy",
+				),
+				kubetest.ServiceIsReady(
+					client,
+					"kube-rbac-proxy",
+				),
+			),
+			Then: kubetest.Actions(
+				kubetest.ClientSucceeds(
+					client,
+					command,
+					nil,
+				),
+			),
+		}.Run(t)
+
+		kubetest.Scenario{
+			Name: "WithDisabledLogToStdErr",
+			Description: `
+				This should succeed. Even though logtostderr flag is set for
+				kube-rbac-proxy.
+				It is complementary to the other flags above.
+			`,
+
+			Given: kubetest.Actions(
+				kubetest.CreatedManifests(
+					client,
+					"flags/clusterRole.yaml",
+					"flags/clusterRoleBinding.yaml",
+					"flags/deployment-logtostderr.yaml",
+					"flags/service.yaml",
+					"flags/serviceAccount.yaml",
+					// This adds the clients cluster role to succeed
+					"flags/clusterRole-client.yaml",
+					"flags/clusterRoleBinding-client.yaml",
+				),
+			),
+			When: kubetest.Actions(
+				kubetest.PodsAreReady(
+					client,
+					1,
+					"app=kube-rbac-proxy",
+				),
+				kubetest.ServiceIsReady(
+					client,
+					"kube-rbac-proxy",
+				),
+			),
+			Then: kubetest.Actions(
+				kubetest.ClientSucceeds(
+					client,
+					command,
+					nil,
+				),
+			),
+		}.Run(t)
+	}
+}
+
 func testTokenAudience(client kubernetes.Interface) kubetest.TestSuite {
 	return func(t *testing.T) {
 		command := `curl --connect-timeout 5 -v -s -k --fail -H "Authorization: Bearer $(cat /var/run/secrets/tokens/requestedtoken)" https://kube-rbac-proxy.default.svc.cluster.local:8443/metrics`