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

Allow for listing & watching individual secrets from nodes #63469

Merged
merged 2 commits into from May 17, 2018

Conversation

@wojtek-t
Copy link
Member

wojtek-t commented May 6, 2018

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
Copy link
Member

liggitt left a comment

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?

@@ -57,6 +60,9 @@ type RequestInfo struct {
Name string
// Parts are the path parts for the request, always starting with /{resource}/{name}
Parts []string

// FIXME: Comment
SingleName string

This comment has been minimized.

@liggitt

liggitt May 7, 2018

Member

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

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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

glog.Errorf("Couldn't parse fieldSelector: %q", values)
} else {
if name, ok := selector.RequiresExactMatch("metadata.name"); ok {
requestInfo.SingleName = name

This comment has been minimized.

@liggitt

liggitt May 7, 2018

Member
  1. set the existing requestInfo.Name field, drop SingleName
  2. 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
  3. 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.

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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?

  1. not yet done (though you also added a dedicated comment about that, so will rely on that) for this purpose)
@@ -91,7 +91,7 @@ func (r *NodeAuthorizer) Authorize(attrs authorizer.Attributes) (authorizer.Deci
requestResource := schema.GroupResource{Group: attrs.GetAPIGroup(), Resource: attrs.GetResource()}
switch requestResource {
case secretResource:
return r.authorizeGet(nodeName, secretVertexType, attrs)
return r.authorizeGetAndSingleNameListWatch(nodeName, secretVertexType, attrs)

This comment has been minimized.

@liggitt

liggitt May 7, 2018

Member
  1. can make this authorizeReadNamespacedObject(...), authorize get/list/watch uniformly (must have name, namespace, no subresource)
  2. treat configmaps the same way

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

done


// FIXME: For list and watch requests check if there is field selector matching single object.
if values := req.URL.Query()["fieldSelector"]; len(values) > 0 {
selector, err := fields.ParseSelector(values[0])

This comment has been minimized.

@liggitt

liggitt May 7, 2018

Member

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:

  1. ensure namespaced resources enforce having a non-empty namespace edit: after consideration, you should be able to authorize watching a name across all namespaces
  2. allow a nil fieldSelector or a non-nil fieldSelector that returns true and a matching value for RequiresExactMatch("metadata.name")

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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.

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 7, 2018

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 wojtek-t force-pushed the wojtek-t:allow_list_and_watch_secrets branch 4 times, most recently from 046655c to cf38e49 May 8, 2018

@wojtek-t
Copy link
Member Author

wojtek-t left a comment

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

@@ -207,18 +207,26 @@ func ListResource(r rest.Lister, rw rest.Watcher, scope RequestScope, forceWatch
}

if hasName {
// FIXME: Check if namespaced resources have namespace set.
// TODO: How to check if the resource is namespaced here?

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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

This comment has been minimized.

@liggitt

liggitt May 14, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt May 14, 2018

Member

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)

This comment has been minimized.

@wojtek-t

wojtek-t May 14, 2018

Author Member

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

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

@@ -104,6 +104,21 @@ func TestAuthorizer(t *testing.T) {
attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: "ns0"},
expect: authorizer.DecisionAllow,
},
{
name: "list allowed secret via pod",

This comment has been minimized.

@liggitt

liggitt May 8, 2018

Member

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

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

done

// Note that fieldSelector setting explicitly the "metadata.name"
// will result in reaching this branch (as the value of that field
// is propagated to requestInfo as the name parameter.
// Since it doesn't make sense to ask for both specific name and

This comment has been minimized.

@liggitt

liggitt May 8, 2018

Member

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

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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?

This comment has been minimized.

@liggitt

liggitt May 8, 2018

Member

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?

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

done

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

@wojtek-t
Copy link
Member Author

wojtek-t left a comment

PTAL

// Note that fieldSelector setting explicitly the "metadata.name"
// will result in reaching this branch (as the value of that field
// is propagated to requestInfo as the name parameter.
// Since it doesn't make sense to ask for both specific name and

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

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?

@@ -104,6 +104,21 @@ func TestAuthorizer(t *testing.T) {
attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "secrets", Name: "secret0-pod0-node0", Namespace: "ns0"},
expect: authorizer.DecisionAllow,
},
{
name: "list allowed secret via pod",

This comment has been minimized.

@wojtek-t

wojtek-t May 8, 2018

Author Member

done

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

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented May 15, 2018

I did a quick scan and didn't see discussion. Is a list for a specific item a list or a get? Did you guys already hash that out?

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 15, 2018

I did a quick scan and didn't see discussion. Is a list for a specific item a list or a get? Did you guys already hash that out?

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

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented May 15, 2018

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 wojtek-t force-pushed the wojtek-t:allow_list_and_watch_secrets branch from 53a8c5c to b2500d4 May 15, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 15, 2018

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

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 15, 2018

/test pull-kubernetes-e2e-gce

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 15, 2018

@deads2k - tests gree, comments addressed. PTAL

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented May 15, 2018

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

This comment has been minimized.

Copy link
Member

liggitt commented May 15, 2018

@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

This comment has been minimized.

Copy link
Member

liggitt commented May 15, 2018

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 15, 2018

@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

This comment has been minimized.

Copy link
Contributor

deads2k commented May 15, 2018

lgtm

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented May 15, 2018

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

This comment has been minimized.

Copy link
Member

liggitt commented May 17, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 17, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 17, 2018

[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

Needs approval from an approver in each of these files:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 17, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 17, 2018

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

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

15 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation wojtek-t authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@enj

This comment has been minimized.

Copy link
Member

enj commented May 17, 2018

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

@wojtek-t

This comment has been minimized.

Copy link
Member Author

wojtek-t commented May 17, 2018

@enj - yes, thanks for pointing. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.