config: allow multiple configurations #164
Conversation
This commit fixes issue brancz#154 by enabling the user to repeat the `--config-file` flag multiple times to specify multiple configurations. Doing so, allows the user to declare that the krp should, e.g., enforce multiple resource attributes. Signed-off-by: Lucas Servén Marín <[email protected]>
Now that KRP can take multiple config files, it is possible that it will need to make multiple outbout SubjectAccessReview requests to the Kubernetes API. In order to keep authorization fast, this commit modifies the proxy so that all authorizations are concurrent. As soon as one authorization fails, the context is cancelled and all other in-flight calls are stopped. Signed-off-by: Lucas Servén Marín <[email protected]>
|
cc @ibihim |
|
Thx a lot for the PR. I will try to take a look into it next week. Currently the focus is to make the release and support the transition of this project into a k8s sig-auth maintained project. So please bear with me 😅 |
|
Thanks for the ACK, @ibihim For completeness, it looks like @s-urbaniak is happy with this general approach [0], and it seems like other projects, both in- and outside of Red Hat would be glad for such a feature. [0] #154 (comment) |
|
hey @squat 👋 yes, I recall discussions around this in the past and we concluded that this is a non-breaking useful change 👍 let us add this once we are done with the current release woes :-) |
Signed-off-by: Lucas Servén Marín <[email protected]>
|
ping @s-urbaniak |
|
ok, sorry. I got pretty busy. I will give it a shot this week. |
|
Looks like your recent PR introduced some conflicts with this branch. I'll rebase this ASAP. Hopefully you have some time to take a look soon. Cc @ibihim @s-urbaniak |
| Subresource: c.Authorization.ResourceAttributes.Subresource, | ||
| Name: c.Authorization.ResourceAttributes.Name, | ||
| }, | ||
| if len(c.Authorizations) != 0 { |
There was a problem hiding this comment.
We don't need this any more. DeepCopy was unused and removed.
| params = append(params, ps...) | ||
| for _, ac := range n.authzConfigs { | ||
| if ac.ResourceAttributes == nil { | ||
| allAttrs = append(allAttrs, defaultAttributesRecord) |
There was a problem hiding this comment.
Is there a benefit to having the defaultAttributesRecord several times in the allAttrs?
| Path: r.URL.Path, | ||
| }) | ||
| return allAttrs | ||
| // Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy |
There was a problem hiding this comment.
Maybe I am lazy, but I would've map'ed the old GetRequestAttributes to the new n.authzConfigs list.
func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http.Request) []authorizer.Attributes {
allAttrs := []authorizer.Attributes{}
for _, ac := range n.authzConfigs{
allAttrs = append(allAttrs, getRequestAttributes(u, r, ac)...)
}
return allAttrs
}| } | ||
| wg.Wait() | ||
| if authzDone == 1 { | ||
| return false |
There was a problem hiding this comment.
The solution is pretty effective. And this part is the reason, why it took me ages to make a PR review. I have huge respect for concurrency and the problems it brings.
I would've tried to use something well known like a fan out pattern or tried to separate the threading logic from the rest as much as possible.
There is a very low level of abstraction in the code base and the hierarchy is pretty flat with lengthy functions, so I won't object as it fits existing code.
| @@ -37,7 +37,7 @@ Usage of _output/kube-rbac-proxy: | |||
| --auth-header-user-field-name string The name of the field inside a http(2) request header to tell the upstream server about the user's name (default "x-remote-user") | |||
| --auth-token-audiences strings Comma-separated list of token audiences to accept. By default a token does not have to have any specific audience. It is recommended to set a specific audience. | |||
| --client-ca-file string 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. | |||
| --config-file string Configuration file to configure kube-rbac-proxy. | |||
| --config-file strings Configuration file to configure kube-rbac-proxy. Can be specified multiple times to configure multiple resource authorizations. | |||
There was a problem hiding this comment.
Would string be more appropriate as it is not a comma-separated list, but it can be specified multiple times? In comparison to --ignore-paths below.
|
Thank you for your contribution and your patience!!! 🎉 |
|
I am sorry that there is now even more rebasing issues. I would close this issue as it is stale. Feel free to reopen it, once you work on it again. |
|
The PR is pretty stale, please reopen it and rebase it, if you want to continue to work on it. |
This commit fixes issue #154 by enabling the user to repeat the
--config-fileflag multiple times to specify multiple configurations.Doing so, allows the user to declare that the krp should, e.g., enforce
multiple resource attributes.
fixes: #154
cc @simonpasquier @s-urbaniak
Signed-off-by: Lucas Servén Marín [email protected]