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.

Sign up for GitHub

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

Jump to bottom

Give apiserver full access to kubelet API #43265

Merged
merged 1 commit into from
Mar 18, 2017
Merged

Give apiserver full access to kubelet API #43265

merged 1 commit into from
Mar 18, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 17, 2017 •
edited

the kubelet stats API calls use both GET and POST. POST calls proxied through the API server were getting forbidden because only get was allowed.

more broadly, the apiserver is responsible for proxying authorized API calls to the kubelet API... I think this means the apiserver should have access to all verbs on the kubelet subresources.

Fixes #42045

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 17, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2017
@liggitt liggitt mentioned this pull request Mar 17, 2017
Closed
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed release-note-label-needed labels Mar 17, 2017
@liggitt liggitt added this to the v1.6 milestone Mar 17, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 17, 2017

cc @kubernetes/sig-auth-pr-reviews

@liggitt
Copy link
Member Author

liggitt commented Mar 17, 2017

#39975 flake
@k8s-bot bazel test this

@liggitt
Copy link
Member Author

liggitt commented Mar 17, 2017

@mikedanese @cjcullen PTAL

@deads2k
Copy link
Contributor

deads2k commented Mar 17, 2017

I think this means the apiserver should have access to all verbs on the kubelet subresources.

Why does this follow? All verbs on proxy, sure, but why the others?

deads2k
deads2k reviewed Mar 17, 2017
View reviewed changes
cluster/addons/rbac/node-proxy-role.yaml
- get
- apiGroups:
- ""
resources:
- nodes/log
- nodes/stats
- nodes/metrics
- nodes/spec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect a role called "node-proxy" to have mutation rights to endpoints other than proxy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename the role if that helps... this thing's job is to proxy to the kubelet API... to do that, it needs full access to the kubelet API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rename the role if that helps... this thing's job is to proxy to the kubelet API... to do that, it needs full access to the kubelet API.

Ok, so its more akin to our system:node-admin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this is the role given to the credential the apiserver uses to reach the kubelet. some deployments use a superuser credential for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this is the role given to the credential the apiserver uses to reach the kubelet. some deployments use a superuser credential for that.

Ok. I think a better name would help. It makes sense for the apiserver to be a nodeadmin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikedanese @cjcullen do you know what the addon manager does with the old object if we rename these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll delete the old one. No guarantees about 0 vs. 1 vs. both running.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node-admin sounds to me like an ops role that would be provisioning/updating node objects...

Copy link
Member Author

@liggitt liggitt Mar 17, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on second thought, I agree. I think the node-proxy name is ok with a comment. the nodes/proxy resource is the most powerful one... the others were split out from nodes/proxy to allow granting those lesser permissions to specific users.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 17, 2017
@liggitt liggitt force-pushed the node-proxy-role branch 2 times, most recently from 0d8d2a7 to 54798e8 Compare March 17, 2017 21:36
@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2017
@cjcullen
Copy link
Member

@roberthbailey for approval

@roberthbailey
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, liggitt, roberthbailey

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2017
@liggitt liggitt added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 17, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 17, 2017

just realized the role for kube-proxy is system:node-proxier... we might want to make the name of this one a little scarier to avoid confusion

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 17, 2017

how about kubelet-api-admin, going once...

@cjcullen
Copy link
Member

Still lgtm

@liggitt liggitt added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Mar 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit bb0c81a into kubernetes:master Mar 18, 2017
@liggitt liggitt deleted the node-proxy-role branch March 18, 2017 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcullen cjcullen cjcullen left review comments

@deads2k deads2k deads2k left review comments

Assignees

@mikedanese mikedanese

@cjcullen cjcullen

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@liggitt @k8s-reviewable @deads2k @cjcullen @roberthbailey @k8s-github-robot @mikedanese @k8s-ci-robot