Allow for listing & watching individual secrets from nodes by wojtek-t · Pull Request #63469 · kubernetes/kubernetes

wojtek-t · 2018-05-06T13:53:44Z

This PR:

propagates value of metadata.name field from fieldSelector to name field in RequestInfo (for list and watch requests)
authorizes list/watch for requests for single secrets/configmaps coming from nodes

As an example:

/api/v1/secrets/namespaces/ns?fieldSelector=metadata.name=foo =>
  requestInfo.Name = "foo",
  requestInfo.Verb = "list"
/api/v1/secrets/namespaces/ns?fieldSelector=metadata.name=foo&watch=true =>
  requestInfo.Name = "foo",
  requestInfo.Verb = "watch"

list/watch API requests with a fieldSelector that specifies `metadata.name` can now be authorized as requests for an individual named resource

liggitt

The context for this change is to allow switching secret manager to be watch-based

does opening hundreds/thousands of distinct single-item watches actually scale on the apiserver side? did you check how the watch event multiplexer handles that case?

liggitt · 2018-05-06T14:59:23Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go

No need for a new attribute, Name already holds this information

I initially didn't want to mix information coming from two different places into single field.
But I'm fine with merging those two.

liggitt · 2018-05-07T13:45:33Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go

set the existing requestInfo.Name field, drop SingleName

this assumes the storage backing this resource honors a fieldSelector of metadata.name, which is very likely to be true, but is not guaranteed. I think that's probably an ok assumption... anyone writing policy based on this should be aware of the capabilities of the resource they are policying

this does introduce the possibility for name to be set and namespace to be empty for a namespaced resource, which doesn't seem good (/api/v1/pods?fieldSelector=metadata.name=foo). Updating ListResource to ensure a namespace is present if a name is specified in the request attributes would cover this.

1 - done
2 - yes, this introduces such assumption (added a comment about that).
That said, won't we fail the validation later if we specify the field that is not supported?

not yet done (though you also added a dedicated comment about that, so will rely on that) for this purpose)

liggitt · 2018-05-07T13:48:46Z

plugin/pkg/auth/authorizer/node/node_authorizer.go

can make this authorizeReadNamespacedObject(...), authorize get/list/watch uniformly (must have name, namespace, no subresource)

treat configmaps the same way

liggitt · 2018-05-07T14:00:29Z

staging/src/k8s.io/apiserver/pkg/endpoints/request/requestinfo.go

this bakes in assumptions about []string -> string conversion for parameters. the worst-case scenario is that this code determines there is a fieldSelector that limits to a single name, authorization allows that, and then the actual rest handler doesn't see or make use of that fieldSelector.

I'd like to see ListResource to get updated with the following when hasName is true:

~~ensure namespaced resources enforce having a non-empty namespace~~ edit: after consideration, you should be able to authorize watching a name across all namespaces

allow a nil fieldSelector or a non-nil fieldSelector that returns true and a matching value for RequiresExactMatch("metadata.name")

IIUC, "hasName" is true only if the name is passed explicitly (not via fieldSelector).

So while I agree that we need to increase validation, I'm not sure that's the correct place to do this.

Apparently I was wrong - hasName is computed based on requestInfo:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/namer.go#L73

and that will be set in this PR. So this will work. Changing.

wojtek-t · 2018-05-07T14:15:33Z

does opening hundreds/thousands of distinct single-item watches actually scale on the apiserver side? did you check how the watch event multiplexer handles that case?

I didn't do enough tests to make it default soon. But it's completely fine to open 200.000 watches on a single apiserver - that works fine.

@liggitt - thanks for comments, will take a look and address later this week

wojtek-t

@liggitt - thanks a lot for comments - they should be now applied (except from one where I added a questions). Tests are coming...

wojtek-t · 2018-05-08T12:48:30Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go

@liggitt - how can we check here is the resource is namespaced or cluster-scoped?

I'm actually rethinking whether we need to do that check... this works today:

$ kubectl get serviceaccounts --all-namespaces --field-selector=metadata.name=default --v=6 I0513 21:12:31.976525 16635 round_trippers.go:405] GET https://localhost:6443/api/v1/serviceaccounts?fieldSelector=metadata.name%3Ddefault&limit=500 200 OK in 10 milliseconds NAMESPACE NAME SECRETS AGE default default 1 14m kube-public default 1 14m kube-system default 1 14m

let's ensure we have test coverage of the following API calls:

CoreV1().Secrets("").List("metadata.name=foo") only returns secrets named "foo"

CoreV1().Secrets("").List("metadata.name=foo,metadata.name=other") returns no secrets (AND of conflicting name field selectors)

CoreV1().Secrets("").List("metadata.name=foo,metadata.namespace=bar") only returns the secret named "foo" in the "bar" namespace

CoreV1().Secrets("bar").List("metadata.name=foo,metadata.namespace=bar") only returns the secret named "foo" in the "bar" namespace

CoreV1().Secrets("bar").List("metadata.name=foo,metadata.namespace=baz") returns no secrets (namespace field selector doesn't match namespace in path)

CoreV1().Secrets("bar").List("metadata.name=foo,metadata.namespace=") returns no secrets (namespace field selector doesn't match namespace in path)

OK. So I left this code as it was and added integration test to check those things.

liggitt · 2018-05-08T16:17:50Z

plugin/pkg/auth/authorizer/node/node_authorizer_test.go

add cases to make sure a list and watch without a name is not allowed

liggitt · 2018-05-08T16:20:20Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go

it doesn't make sense to ask for both specific name and a different field selector

that's not really accurate... listing fieldSelector=metadata.name=foo,otherfield=bar isn't unreasonable. imagine you are setting up a lister/watcher to get events for a particular object in a particular state

I'm fine with changing it, but that would be relaxing compared what we had previously - previously if the name was specified we didn't allow for any fieldSelector. WDYT?

that would be relaxing compared what we had previously

I don't think so... the only way hasName was set before was via a single-item watch, which doesn't take a fieldSelector parameter, right?

Ahh ok - of course you're right. Changing to make logic as you expect.

wojtek-t

PTAL

wojtek-t · 2018-05-08T19:53:23Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/get.go

I'm fine with changing it, but that would be relaxing compared what we had previously - previously if the name was specified we didn't allow for any fieldSelector. WDYT?

wojtek-t · 2018-05-08T20:00:11Z

plugin/pkg/auth/authorizer/node/node_authorizer_test.go

deads2k · 2018-05-15T12:19:37Z

already hash that out?

It's a list (i.e. it returns SecretList for example), but with just a single element in Items.

I meant for the purposes of authorization, not REST handling. I guess they could grant get, list, watch for a name and this would fall through correctly.

wojtek-t · 2018-05-15T12:21:50Z

I meant for the purposes of authorization, not REST handling. I guess they could grant get, list, watch for a name and this would fall through correctly.

I think that treating it as "list" is cleaner - treating it as "get" would introduce confusion in the code in my opinion.

wojtek-t · 2018-05-15T13:33:57Z

/test pull-kubernetes-e2e-gce

wojtek-t · 2018-05-15T15:28:56Z

@deads2k - tests gree, comments addressed. PTAL

liggitt · 2018-05-15T15:36:21Z

I meant for the purposes of authorization, not REST handling. I guess they could grant get, list, watch for a name and this would fall through correctly.

I think that treating it as "list" is cleaner - treating it as "get" would introduce confusion in the code in my opinion.

I agree, I'd rather see this represented as a list/watch filtered to a single named object.

liggitt · 2018-05-15T15:37:34Z

@wojtek-t can you update the PR description with the scope of the change being made, some example requests and resulting RequestInfo, and a release note that list/watch can now authorize named resources?

liggitt · 2018-05-15T15:37:51Z

cc @kubernetes/sig-auth-pr-reviews

wojtek-t · 2018-05-15T15:47:06Z

@wojtek-t can you update the PR description with the scope of the change being made, some example requests and resulting RequestInfo, and a release note that list/watch can now authorize named resources?

@liggitt - done PTAL

deads2k · 2018-05-15T21:36:28Z

lgtm

deads2k · 2018-05-15T21:36:40Z

I meant for the purposes of authorization, not REST handling. I guess they could grant get, list, watch for a name and this would fall through correctly.

I think that treating it as "list" is cleaner - treating it as "get" would introduce confusion in the code in my opinion.

ok, no strong opinion

liggitt · 2018-05-17T13:05:28Z

/lgtm

k8s-ci-robot · 2018-05-17T13:05:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~plugin/pkg/auth/OWNERS~~ [liggitt,wojtek-t]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt,wojtek-t]
~~test/OWNERS~~ [liggitt,wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-05-17T13:07:32Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-05-17T14:09:42Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

enj · 2018-05-17T16:09:13Z

@wojtek-t your second example in the first comment has the wrong verb (should be watch).

wojtek-t · 2018-05-17T16:11:39Z

@enj - yes, thanks for pointing. Fixed.

wojtek-t added the release-note-none Denotes a PR that doesn't merit a release note. label May 6, 2018

wojtek-t assigned liggitt May 6, 2018

k8s-ci-robot requested review from deads2k, enj, ericchiang, liggitt and sttts May 6, 2018 13:53

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 6, 2018

wojtek-t added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 6, 2018

wojtek-t force-pushed the allow_list_and_watch_secrets branch from b2bbe8e to bf6c249 Compare May 6, 2018 14:25

liggitt reviewed May 7, 2018

View reviewed changes

wojtek-t force-pushed the allow_list_and_watch_secrets branch 4 times, most recently from 046655c to cf38e49 Compare May 8, 2018 12:45

wojtek-t commented May 8, 2018

View reviewed changes

wojtek-t force-pushed the allow_list_and_watch_secrets branch 2 times, most recently from e10a52c to 64a477e Compare May 8, 2018 13:52

liggitt reviewed May 8, 2018

View reviewed changes

wojtek-t force-pushed the allow_list_and_watch_secrets branch from 64a477e to 5171841 Compare May 8, 2018 20:00

wojtek-t commented May 8, 2018

View reviewed changes

wojtek-t force-pushed the allow_list_and_watch_secrets branch from 5171841 to 5623098 Compare May 8, 2018 20:19

wojtek-t added 2 commits May 15, 2018 14:19

Requires single name for list and watch

f344c5c

Fix bootstrap roles to allow list/watch secrets/configmaps from nodes

b2500d4

wojtek-t force-pushed the allow_list_and_watch_secrets branch from 53a8c5c to b2500d4 Compare May 15, 2018 12:19

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 15, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2018

k8s-github-robot merged commit b3837d0 into kubernetes:master May 17, 2018

wojtek-t deleted the allow_list_and_watch_secrets branch July 3, 2018 12:56

This was referenced Dec 28, 2018

document /watch prefix deprecation #65147

Merged

get --watch specifying a resource uses "watch list" instead of "watch" API kubernetes/kubectl#305

Closed

tallclair mentioned this pull request May 15, 2019

List & watch can be authorized on named resources kubernetes/website#14339

Merged

liggitt mentioned this pull request Apr 2, 2020

Describe how resource namespace and name are determined for get/list/watch requests, in authorization and API Priority and Fairness #89772

Closed

liggitt mentioned this pull request Apr 23, 2020

RBAC policy for watching a single object? #54079

Closed

Conversation

wojtek-t commented May 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented May 7, 2018

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented May 15, 2018

Uh oh!

wojtek-t commented May 15, 2018

Uh oh!

wojtek-t commented May 15, 2018

Uh oh!

wojtek-t commented May 15, 2018

Uh oh!

liggitt commented May 15, 2018

Uh oh!

liggitt commented May 15, 2018

Uh oh!

liggitt commented May 15, 2018

Uh oh!

wojtek-t commented May 15, 2018

wojtek-t commented May 6, 2018 •

edited

Loading

liggitt May 7, 2018 •

edited

Loading

liggitt May 14, 2018 •

edited

Loading

liggitt May 8, 2018 •

edited

Loading