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.

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

Get individual namespaces when given whitelist #1298

Merged
merged 3 commits into from Aug 23, 2018

Conversation

@squaremo
Copy link
Member

squaremo commented Aug 21, 2018

Asking for the full list of namespaces requires a cluster-scoped permission of listing namespaces; however, a common scenario for using the whitelist is that you want to restrict permissions.

If we simply Get the whitelisted namespaces, ignoring those we're forbidden to see (or that don't exist, as before), we don't need the cluster-scoped permission and can just be given permissions per namespace.

The trade is that we do an API request per whitelisted namespace. I expect there to be relatively few, though, so I don't think this is a huge deal.

@squaremo squaremo force-pushed the dont-require-list-ns branch from abaa1b6 to c5bfdd1 Aug 21, 2018
@squaremo

This comment has been minimized.

Copy link
Member Author

squaremo commented Aug 21, 2018

Asking for the full list of namespaces requires a cluster-scoped
permission of listing namespaces; however, a common scenario for using
the whitelist is that you want to restrict permissions.

If we simply Get the whitelisted namespaces, ignoring those we're
forbidden to see (or that don't exist, as before), we don't need the
cluster-scoped permission and can just be given permissions per
namespace.

The trade is that we do an API request per whitelisted namespace. I
expect there to be relatively few, though, so I don't think this is a
huge deal.
@squaremo squaremo force-pushed the dont-require-list-ns branch from c5bfdd1 to 9b7aee1 Aug 21, 2018
@petervandenabeele

This comment has been minimized.

Copy link
Contributor

petervandenabeele commented Aug 21, 2018

I presume this would be part of a fix for the question I raised recently:
https://weave-community.slack.com/archives/C2ND76PAA/p1534778698000100
(the other part being to use an explicit whitelist of namespaces).

@squaremo

This comment has been minimized.

Copy link
Member Author

squaremo commented Aug 21, 2018

I presume this would be part of a fix for the question I raised recently:
https://weave-community.slack.com/messages/C2ND76PAA/convo/C2ND76PAA-1534779886.000100/
(the other part being to use an explicit whitelist of namespaces).

This changes how whitelisting is implemented, so you need to give flux slightly fewer permissions. I'll do a follow-up that documents exactly what you do need, but with this PR, it should be a little as:

  • A role that can patch the SSH key secret in flux's namespace (if you don't populate it yourself)
  • A role that can list, get, create and update resources you want managed, for each namespace in the whitelist
@justinbarrick

This comment has been minimized.

Copy link
Collaborator

justinbarrick commented Aug 21, 2018

Awesome!

for _, name := range c.nsWhitelist {
if ns, err := c.client.CoreV1().Namespaces().Get(name, meta_v1.GetOptions{}); err == nil {
nsList = append(nsList, *ns)
} else if !(apierrors.IsNotFound(err) || apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err)) {

This comment has been minimized.

Copy link
@rade

rade Aug 21, 2018

Contributor

Do we really want to be silent here? Won't that make it rather hard for users to diagnose why a namespace they specified in their whitelist is getting ignored by flux?

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 22, 2018

Author Member

It's a tricky one. If the namespace doesn't exist (yet), we'll get either NotFound (if the permissions allow listing namespaces) or Forbidden (if they don't). Failing the whole operation, which underlies much of the API, seems a bit punitive.

We could log it as a warning, I suppose -- that way at least it's flagged up to anyone investigating problems.

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 22, 2018

Author Member

Now logs it -- could be pretty verbose, but no straight-forward way to avoid that.

@squaremo squaremo force-pushed the dont-require-list-ns branch from d0a846e to 8600d06 Aug 22, 2018
@justinbarrick

This comment has been minimized.

Copy link
Collaborator

justinbarrick commented Aug 22, 2018

If you wanted to avoid spamming the logs you could cache in memory which namespaces you have logged errors for and rate limit it.

@squaremo squaremo requested a review from rade Aug 23, 2018
@rade
rade approved these changes Aug 23, 2018
Copy link
Contributor

rade left a comment

I really think we should log the actual ns access failure reasons. But it's not a blocker.

nsList = append(nsList, *ns)
case apierrors.IsUnauthorized(err) || apierrors.IsForbidden(err) || apierrors.IsNotFound(err):
if !c.nsWhitelistLogged[name] {
c.logger.Log("warning", "whitelisted namespace unauthorized, forbidden, or not found", "namespace", name)

This comment has been minimized.

Copy link
@rade

rade Aug 23, 2018

Contributor

It would be rather nice to know which of the three errors it is. Why not simply log err? AFAICT (from looking at https://github.com/kubernetes/apimachinery/blob/master/pkg/api/errors/errors.go) that should produce something reasonably intelligible.

This comment has been minimized.

Copy link
@rade

rade Aug 23, 2018

Contributor

I suppose one issue here is that the error may change. If we care enough, we could stash the last error message (well, ideally code, but getting hold of that requires some casting) in the map.

This comment has been minimized.

Copy link
@squaremo

squaremo Aug 23, 2018

Author Member

I changed it to log the error, so that will give people an indication of what's wrong. Logging transitions between the different errors is overkill I reckon.

If we log a warning every time a whitelisted is missing, there may be
an awful lot of repeated warnings. Instead, keep track of which
namespaces have been seen to be missing (resetting when they are seen
again), and log only when the namespace was not known to be missing.
@squaremo squaremo force-pushed the dont-require-list-ns branch from 2e9ad51 to 2302abf Aug 23, 2018
@squaremo squaremo merged commit 4269545 into master Aug 23, 2018
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@squaremo squaremo deleted the dont-require-list-ns branch Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.