v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags #276

ibihim · 2024-01-15T17:23:59Z

No description provided.

stlaz · 2024-01-16T10:08:57Z

single bump commit, single commit with changes that accommodate that bump, please

stlaz · 2024-01-23T09:35:07Z

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

@@ -451,7 +451,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)


This does not appear to be a bump commit at all? So many test changes, too.

klog.Errorf -> klog.ErrorS, it was necessary after the bump.

I moved the validation from the completed config to the options, like in apiserver, such that I don't need to add the disabled options to the config.

Why? klog.Errorf is still present.

The test changes below don't belong to the bump commit.

Dunno what was wrong with my linter. Reverting it.

stlaz · 2024-01-23T09:45:55Z

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

@@ -122,5 +147,101 @@ 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


mark all hidded with flagset.MarkHidden(name)

ujo-trackunit · 2024-02-07T06:55:39Z

Is it ready to "go"?

ibihim · 2024-02-07T09:33:47Z

Well, I try to find a contributor to review it.

This is serious software and I don't want to have a single-contributor bias in it.

stlaz · 2024-02-07T09:38:54Z

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

@@ -451,7 +451,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)


Why? klog.Errorf is still present.

The test changes below don't belong to the bump commit.

stlaz · 2024-02-07T09:41:08Z

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

-	flagset.IntVar(&o.disabled.stderrthreshold, "stderrthreshold", 2, "[DISABLED] logs at or above this threshold go to stderr")
+	o.flagSet = flagset // reference used for validation
+	flagset.BoolVar(&o.disabled.logtostderr, "logtostderr", false, "[DISABLED]")
+	if err := flagset.MarkHidden("logtostderr"); err != nil {


keep the flag names in a slice, handle hiding all in one place

stlaz · 2024-02-07T09:42:25Z

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

-}
-
-func Complete(o *options.ProxyRunOptions) (*completedProxyRunOptions, error) {
+func Complete(o *options.ProxyRunOptions) (*proxyConfig, error) {


make it a method of ProxyRunOptions

I would need to pull completedProxyRunOptions then into the options pkg to avoid a circular dependency.

I see, leave it be, then. I was wondering why I did it this way back then :)

stlaz · 2024-02-07T09:45:54Z

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

 	var err error
-	completed := &completedProxyRunOptions{
+	config := &proxyConfig{


if you plan to rebase the devel branch on top of this, the rename does not bring enough value to justify the merge conflicts headache

stlaz · 2024-02-07T09:47:05Z

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

+
+	// Removed upstream flags shouldn't be use
+	for _, disabledOpt := range []string{
+		"logtostderr",


this is a second spot where you're constructing this list, maybe it would make sense to keep it somewhere where you can refer to it

stlaz · 2024-02-07T22:52:22Z

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

@@ -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))


klog.Errorf, no fmt.Errorf should be necessary I believe

stlaz · 2024-02-07T22:53:26Z

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

+	// disabled flags
+	o.flagSet = flagset // reference used for validation
+	for _, disabledOpt := range disabledFlags {
+		_ = flagset.String(disabledOpt, "", "[DISABLED]")


not sure converting all opts to sting is safe

dpuniaredhat · 2024-02-08T06:03:47Z

/label cherry-pick-approved

dpuniaredhat · 2024-02-08T06:03:59Z

/label qe-approved

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.

stlaz · 2024-02-08T09:27:49Z

/lgtm

ibihim force-pushed the bump-deps-v0.15.1 branch 3 times, most recently from 9a17d0e to aa80caf Compare January 23, 2024 09:30

ibihim changed the title ~~[wip] v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags~~ v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags Jan 23, 2024

ibihim force-pushed the bump-deps-v0.15.1 branch from aa80caf to 62b87fe Compare January 23, 2024 09:33

stlaz reviewed Jan 23, 2024

View reviewed changes

ibihim force-pushed the bump-deps-v0.15.1 branch 4 times, most recently from 8b599c0 to d226575 Compare January 23, 2024 10:52

This was referenced Feb 7, 2024

CVE vulnerabilities found for golang-runtime:1.20.4 (usr/local/bin/kube-rbac-proxy) #274

Closed

CVE high security vulnerabilities found in image: quay.io/brancz/kube-rbac-proxy:v0.15.0 #271

Closed

stlaz requested changes Feb 7, 2024

View reviewed changes

ibihim force-pushed the bump-deps-v0.15.1 branch from 2e94961 to 25b0cee Compare February 7, 2024 14:26

go.*: bump deps and k8s.io to v1.28.6

c6e521e

ibihim force-pushed the bump-deps-v0.15.1 branch from 25b0cee to b9c9d44 Compare February 7, 2024 14:39

stlaz reviewed Feb 7, 2024

View reviewed changes

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

0566550

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.

ibihim force-pushed the bump-deps-v0.15.1 branch from b9c9d44 to 0566550 Compare February 8, 2024 09:22

ibihim merged commit 1d8bd9a into brancz:master Feb 8, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags #276

v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags #276

ibihim commented Jan 15, 2024

stlaz commented Jan 16, 2024

stlaz Jan 23, 2024

ibihim Jan 23, 2024

stlaz Feb 7, 2024

ibihim Feb 7, 2024

stlaz Jan 23, 2024

ujo-trackunit commented Feb 7, 2024

ibihim commented Feb 7, 2024 •

edited

Loading

stlaz Feb 7, 2024

stlaz Feb 7, 2024

stlaz Feb 7, 2024

ibihim Feb 7, 2024

stlaz Feb 7, 2024

stlaz Feb 7, 2024

stlaz Feb 7, 2024

stlaz Feb 7, 2024

stlaz Feb 7, 2024

dpuniaredhat commented Feb 8, 2024

dpuniaredhat commented Feb 8, 2024

stlaz commented Feb 8, 2024

v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags #276

v0.16.0: Bump deps, Bump Kubernetes, Disabled Log Flags #276

Conversation

ibihim commented Jan 15, 2024

stlaz commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujo-trackunit commented Feb 7, 2024

ibihim commented Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpuniaredhat commented Feb 8, 2024

dpuniaredhat commented Feb 8, 2024

stlaz commented Feb 8, 2024

ibihim commented Feb 7, 2024 •

edited

Loading