-
Notifications
You must be signed in to change notification settings - Fork 183
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.
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
Minor improvements in cmd/kube-rbac-proxy #243
Minor improvements in cmd/kube-rbac-proxy #243
Conversation
@@ -207,6 +198,7 @@ func Run(opts *completedProxyRunOptions) error { | |||
handler := identityheaders.WithAuthHeaders(proxy, cfg.KubeRBACProxyInfo.UpstreamHeaders) | |||
handler = kubefilters.WithAuthorization(handler, authz, scheme.Codecs) | |||
handler = kubefilters.WithAuthentication(handler, authenticator, http.HandlerFunc(filters.UnauthorizedHandler), cfg.DelegatingAuthentication.APIAudiences) | |||
// passing an empty RequestInfoFactory results in attaching a non-resource RequestInfo to the context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibihim I'm not sure if I'm missing any other implication of passing an empty RequestInfoFactory
, please advise :)
Added (11) to the list of addressed issues; converted to Draft while this is WIP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first contribution, thank you a lot!
6245f7d
to
9398674
Compare
LGTM, leaving final ack to @ibihim |
…g within Complete()
…Info and different listeners/ports used
…ter for shutdown signals and use the signal context
9398674
to
c8fc660
Compare
@ibihim after our discussions I've opted for a simple approach to handling the wait groups -- PTAL. I've dropped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 comment and 2 nits.
if err := opts.SecureServing.ApplyTo(&proxyConfig.SecureServing); err != nil { | ||
return nil, err | ||
if err := wg.Add(1); err != nil { | ||
cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move that into line 242 like so: defer cancel()
? Like so:
serverCtx, cancel := context.WithCancel(ctx)
defer cancel()
c8fc660
to
8953fe1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks for the patience. Looking forward for more PRs 😄
This PR addresses some of the issues from #238; in particular, the ones listed in this comment.
In particular: