Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

reuse apiserver serving logic #213

Merged
merged 3 commits into from
Jan 20, 2023
Merged

reuse apiserver serving logic #213

merged 3 commits into from
Jan 20, 2023

Conversation

stlaz
Copy link
Collaborator

@stlaz stlaz commented Dec 1, 2022 •
edited
Loading

This PR reuses the serving logic used by kube-apiservers.

Related to #169

This PR is not bringing the graceful termination logic kube-apiserver is using. If that is required, it should be done as a feature follow up that's based on https://github.com/kubernetes/kubernetes/blob/0e54bd294237e8fc3e0f60f3195353f7c25e8a4c/staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go#L534

@stlaz stlaz force-pushed the apiserver_serving branch 15 times, most recently from 446b305 to 0960e48 Compare December 6, 2022 13:01
@stlaz stlaz changed the title wip: reuse apiserver serving logic reuse apiserver serving logic Dec 6, 2022
@stlaz stlaz force-pushed the apiserver_serving branch 3 times, most recently from 7075969 to c90f90b Compare December 6, 2022 14:14
stlaz
stlaz commented Dec 7, 2022
View reviewed changes
README.md Show resolved Hide resolved
stlaz
stlaz commented Dec 7, 2022
View reviewed changes
cmd/kube-rbac-proxy/app/options/legacyoptions.go Outdated
}

if o.TLS.ReloadInterval != time.Minute {
klog.Warning("--tls-reload-interval no longer has any effect and will be removed in the next version")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous deprecation message was removed in favor of this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message might give the impression, that there is not TLS reload interval anymore. Would it be possible to change the message slightly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was called "TLS reload interval" no longer exists. Since we have feature branch now and don't need to care as much about backward compatibility, I'm going to remove the option instead.

ibihim
ibihim reviewed Dec 8, 2022
View reviewed changes
pkg/config/config.go Outdated Show resolved Hide resolved
cmd/kube-rbac-proxy/app/options/legacyoptions.go Outdated Show resolved Hide resolved
cmd/kube-rbac-proxy/app/options/legacyoptions.go Outdated Show resolved Hide resolved
cmd/kube-rbac-proxy/app/options/legacyoptions.go
so.CipherSuites = o.TLS.CipherSuites
}

if so.BindAddress.Equal(netutils.ParseIPSloppy("0.0.0.0")) && len(o.SecureListenAddress) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is quite a long line.

cmd/kube-rbac-proxy/app/options/proxyoptions.go Show resolved Hide resolved
cmd/kube-rbac-proxy/app/kube-rbac-proxy.go Outdated Show resolved Hide resolved
cmd/kube-rbac-proxy/app/options/legacyoptions.go Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated
return nil
}

func (i *KubeRBACProxyConfig) GetClientCAProvider() (dynamiccertificates.CAContentProvider, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at this point I am curious if it wouldn't be better to name it something like DownstreamClientCA. In reference to the explicit UpstreamClientCA.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should rename the UpstreamClientCA to just UpstreamCA instead

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do so as well. But Client itself is not specific enough as we receive client connections and we create client connections.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not notice this originally. I think the name Client CA matches its use - it's a CA for client certs that we use in order to perform mTLS for the incoming client requests.

@ibihim
Copy link
Collaborator

ibihim commented Dec 8, 2022

Comparing the CLI output, I see the following flags being dropped:

  • --add_dir_header
  • --log_backtrace_at
  • --one_output
  • --skip_log_headers
  • --vmodule.

But they are listed as deprecated when used.

@stlaz stlaz mentioned this pull request Dec 9, 2022
Merged
@ibihim
Copy link
Collaborator

ibihim commented Dec 13, 2022

lgtm, approve, I will make a release without it first.

@stlaz
Copy link
Collaborator Author

stlaz commented Dec 13, 2022

Thanks. I squashed the changes into two commits only

@stlaz
Copy link
Collaborator Author

stlaz commented Dec 14, 2022

/hold
after recent proxy-endpoints change, I need to figure out how to copy the serving options for the proxy port

@stlaz stlaz changed the base branch from master to sig-auth-acceptance December 15, 2022 11:22
@stlaz
Copy link
Collaborator Author

stlaz commented Dec 16, 2022

/hold cancel
done

ibihim
ibihim reviewed Jan 19, 2023
View reviewed changes
Copy link
Collaborator

@ibihim ibihim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I am sorry for giving it another review. If it bothers you too much, we could create issues for it and do it in another PR.

It would be also lovely to get a comment on every exporting function 😅

cmd/kube-rbac-proxy/app/options/legacyoptions.go Outdated
}

if o.TLS.ReloadInterval != time.Minute {
klog.Warning("--tls-reload-interval no longer has any effect and will be removed in the next version")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message might give the impression, that there is not TLS reload interval anymore. Would it be possible to change the message slightly?

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go Outdated
}
func prepareSecureServer(
ctx context.Context,
runGroup *run.Group,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handing something like a run.Group down is not a good thing to do. it would be better to return the func.

@ibihim
Copy link
Collaborator

ibihim commented Jan 20, 2023

/lgtm

@ibihim ibihim merged commit 9d45c10 into brancz:sig-auth-acceptance Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibihim ibihim ibihim left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stlaz @ibihim