-
Notifications
You must be signed in to change notification settings - Fork 189
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
Initial support for http/2(h2, h2c) and gRPC clients/upstream severs #2
Conversation
@brancz Would you like me to rebase this onto the master branch? |
yes, rebasing would be great, thanks! |
73d85c5
to
7596d40
Compare
@brancz Done 👍 |
7596d40
to
8d58cd2
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.
Sorry I had the review standing around and forgot to submit. This looks great! Just some nits, and we should probably start adding tests, but let's do that in separate PRs.
main.go
Outdated
@@ -84,6 +89,9 @@ func main() { | |||
|
|||
// Auth flags | |||
flagset.StringVar(&cfg.auth.Authentication.X509.ClientCAFile, "client-ca-file", "", "If set, any request presenting a client certificate signed by one of the authorities in the client-ca-file is authenticated with an identity corresponding to the CommonName of the client certificate.") | |||
flagset.StringVar(&cfg.auth.Authentication.Header.UserFieldName, "user-header-field-name", "x-remote-user", "The name of the field inside a http(2) request header to tell the upstream server about the user's name") |
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.
This will need an additional flag to enable that user information should be augmented onto the request, and it should probably be turned off by default.
auth.go
Outdated
// BuildAuth creates an authenticator, an authorizer, and a matching authorizer attributes getter compatible with the kube-rbac-proxy | ||
func BuildAuth(client clientset.Interface, config AuthConfig) (AuthInterface, error) { | ||
// BuildAuthHandler creates an authenticator, an authorizer, and a matching authorizer attributes getter compatible with the kube-rbac-proxy | ||
func BuildAuthHandler(client clientset.Interface, config AuthConfig) (*Handler, error) { |
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.
It was good to have an interface here I think. Maybe we can instead add a "Handle" function to the AuthInterface
?
main.go
Outdated
// which would handle HTTP UPGRADE from http1.1 to http/2, especially in case | ||
// what you wanted kube-rbac-proxy to authn/authz was gRPC over h2c calls. | ||
// | ||
// // Note that golang's http server requires a client(including gRPC) to send HTTP Upgrade req to |
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.
double //
Thanks for your review @brancz! |
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.
Almost there! Thanks again for putting so much work into this! :)
auth.go
Outdated
type Handler struct { | ||
AuthInterface | ||
Config AuthConfig | ||
type Handler interface { |
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.
This would be an AuthHandler
as it returns whether authN and authZ was successful.
auth.go
Outdated
} | ||
|
||
func (h *Handler) Handle(w http.ResponseWriter, req *http.Request) bool { | ||
func (h *kubeRBACProxyAuth) Handle(w http.ResponseWriter, req *http.Request) bool { |
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.
Doc string missing.
@brancz Thanks again! I've addressed the two comments just now. |
Awesome, thanks! lgtm 👍 (if you're interested it would be good to add some testing 🙂, happy to discuss on Kubernetes slack if you're interested, I'm brancz there as well) |
@brancz Yes, I'm willing to write some tests! |
Nice! Yes I'll come up with some first tests for the other existing functionality, then we can see how we can add tests for this functionality. |
make minimal command worth trying
Resolves #1