Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upExtend namespace filtering to all operations on namespaced resources #1668
Conversation
f060e01
to
752d7b2
e3f9e83
to
dda60d7
This comment has been minimized.
This comment has been minimized.
I ran into a problem when filtering resources by the namespace in their identifier. Namespace filtering cannot distinguish cluster-wide resources and resources in the When the namespace of a resource identifier comes from ...
(2) is slightly less problematic because, AFAIU, the only cluster resources we currently read are controllers, which are all namespaced. My suggestion would be to create the following invariant for namespaces in resource identifiers:
More specifically, the changes needed are: when constructing resource identifiers ...
|
This comment has been minimized.
This comment has been minimized.
|
This comment has been minimized.
This comment has been minimized.
For those two cases you've put above: in (2), I think the API server will always give you a namespace if the resource is namespaced, and never if it's not. So an empty namespace is a reliable guide to cluster-scoped resources. But for 1), the only way to know is to check with the API server whether the particular GroupVersionKind is namespaced or not; something which I was able to dodge in #1442 since I don't actually care what the namespace is, so long I can use it to relate resources to manifests (though it would be cleaner if I could give things accurate IDs). (CODA: #1442 did end up querying the API for resource namespacedness) |
00570c5
to
84a3850
18cbe9f
to
a936989
This comment has been minimized.
This comment has been minimized.
I am done with the code. I want to do some further high-level testing on a Kubernetes cluster, but it's ready to review. |
This comment has been minimized.
This comment has been minimized.
This PR is blocked by #1442 (they have a lot in common and I agreed with @squaremo to get #1442 merged first) |
759d954
to
e71b52c
c5706cc
to
cb4ad60
This comment has been minimized.
This comment has been minimized.
@squaremo this is ready again, PTAL |
I need to have a think about this, so only comments for now -- sorry |
1ea6920
to
47d75cf
This comment has been minimized.
This comment has been minimized.
@2opremio That commit is what I had in mind (hopefully clearer in code than in comments!). See what you think .. |
This comment has been minimized.
This comment has been minimized.
My initial proposal was very similar (to filter out disallowed namespaces when loading the manifests) but it was discarded, see #1668 (comment) . I recall you ended up coming with a scenario in which it could be a problem. Now I don't recall which one and I can't find it, maybe you shared it privately. At the very least, it will lead to confusing error messages when trying to edit policies from workload belonging to disallowed namespaces. If we remove the manifests when loading, the error will be PS: We should also merge the code of |
This comment has been minimized.
This comment has been minimized.
Where we ended up on that (in #1442) was that it was an acceptable mid-way point to leave the parsing as generic (that's
That's a fair objection. It's the usual trade-off of reliable enforcement versus being helpful to users. In this case, I don't think it's important to hide from a user the fact of namespaces being blocked, and I do think we can do better at closing down opportunities for accidentally side-stepping the check. What about if the check against the allowed namespaces happened in I'll have another go at my amendment .. |
5bf61b4
to
92db72a
This comment has been minimized.
This comment has been minimized.
Rebased on master, and removed my extra commit. I've tried out a few scenarios:
So all good so far! Will think of some tricksier tests, next .. |
This comment has been minimized.
This comment has been minimized.
I did some tests myself, but it's great that you are being thorough. Thanks!! |
This comment has been minimized.
This comment has been minimized.
@squaremo I think I've dealt with the comments (except for #1668 (comment) which I don't know how to address). PTAL |
This comment has been minimized.
This comment has been minimized.
@squaremo I tested this PR against https://github.com/2opremio/locked-down-flux to see how it behaved with a namespace-locking RBAC and everything worked as expected (no errors in the logs and the example workload was created as expected)
|
This comment has been minimized.
This comment has been minimized.
Lovely! What happens if you tell it to allow a namespace it doesn't have RBAC access to? |
This comment has been minimized.
This comment has been minimized.
Good suggestion. After creating a namespace forbidden by RBAC and included in the whitelist I noticed that flux was creating everything in the namespace anyways. After quite some digging, it turns out that the Kubernetes installation provided by Docker For Mac makes all service accounts
I will retest tomorrow after removing that role binding. PS: It's really frustrating that the RBAC groups are second-class citizens and are only documented through examples. |
This comment has been minimized.
This comment has been minimized.
I bit the bullet and everything works as expected:
That said, although we give a sensible error about not being able to access the
That said, that's something unrelated to this PR. |
0696022
to
a61e228
f2272f7
to
0aaca0b
This is looking good to me
|
2opremio commentedJan 17, 2019
•
edited
Addresses part of #1471
--kubernetes-namespace-whitelist
to--kubernetes-allow-namespace
--k8s-allow-namespace
--k8s-allow-namespace
for namespaced kubernetes resources in all flux operations.