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

Add permisions for Metrics Server to read resources on cluster level #53330

Merged
merged 1 commit into from
Oct 3, 2017
Merged

Add permisions for Metrics Server to read resources on cluster level #53330

merged 1 commit into from
Oct 3, 2017

Conversation

kawych
Copy link
Contributor

@kawych kawych commented Oct 2, 2017

What this PR does / why we need it:
Add permisions for Metrics Server to read resources on cluster level.

Which issue this PR fixes:
fixes kubernetes-sigs/metrics-server#16

Release note:

Fix permissions for Metrics Server.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 2, 2017
@kawych kawych mentioned this pull request Oct 2, 2017
Merged
piosz
piosz reviewed Oct 2, 2017
View reviewed changes
Copy link
Member

@piosz piosz left a comment

Choose a reason for hiding this comment

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

@kubernetes/sig-auth-pr-reviews could you please take a look?

cluster/addons/metrics-server/resource-reader.yaml Outdated
@@ -0,0 +1,32 @@
apiVersion: rbac.authorization.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

This should be v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cluster/addons/metrics-server/resource-reader.yaml Outdated
- list
- watch
---
apiVersion: rbac.authorization.k8s.io/v1beta1
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cluster/addons/metrics-server/resource-reader.yaml Outdated
@@ -0,0 +1,32 @@
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRole
Copy link
Member

Choose a reason for hiding this comment

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

How this is handled for Heapster? It requires exactly the same permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my comment below

@piosz
Copy link
Member

piosz commented Oct 2, 2017

Once this is merged, could you please apply the same changes in https://github.com/kubernetes-incubator/metrics-server/tree/master/deploy?

liggitt
liggitt reviewed Oct 2, 2017
View reviewed changes
cluster/addons/metrics-server/resource-reader.yaml Outdated
- apiGroups:
- ""
resources:
- "*"
Copy link
Member

Choose a reason for hiding this comment

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

why does the metrics server require the ability to read every secret in the system?

Copy link
Member

Choose a reason for hiding this comment

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

It requires ability to list/watch pods, nodes and namespaces.

Copy link
Member

Choose a reason for hiding this comment

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

then enumerate pods, nodes, and namespaces

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@kawych kawych force-pushed the auth branch 3 times, most recently from 0139b86 to 1b1ea7f Compare October 3, 2017 08:22
@kawych
Copy link
Contributor Author

kawych commented Oct 3, 2017

@piosz @liggitt
I've changed permissions to the same as Heapster has (events and deployments on top of what you mentioned). The cluster role for heapster is defined here: https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go#L263

Created PR: kubernetes-sigs/metrics-server#20 for changes in metrics-server repo.

piosz
piosz reviewed Oct 3, 2017
View reviewed changes
Copy link
Member

@piosz piosz left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor nit

cluster/addons/metrics-server/resource-reader.yaml Outdated
- apiGroups:
- ""
resources:
- events
Copy link
Member

Choose a reason for hiding this comment

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

we don't need events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed events

@piosz
Copy link
Member

piosz commented Oct 3, 2017

It has to be cherry-picked to release-1.8

@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@piosz piosz added this to the v1.8 milestone Oct 3, 2017
@kawych
Copy link
Contributor Author

kawych commented Oct 3, 2017

/retest

@mikedanese
Copy link
Member

/approve

@piosz
Copy link
Member

piosz commented Oct 3, 2017

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kawych, mikedanese, piosz

Associated issue requirement bypassed by: piosz

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

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 Oct 3, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 53280, 53330). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f11a551 into kubernetes:master Oct 3, 2017
@kawych kawych mentioned this pull request Oct 4, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 4, 2017
Merge pull request #53429 from kawych/automated-cherry-pick-of-#53330…
10d3f97 
…-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #53330

Cherry pick of #53330 on release-1.8.

#53330: Add permisions for Metrics Server to read resources on cluster level.

```release-note
Fix permissions for Metrics Server.
```
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.8" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piosz piosz piosz left review comments

@liggitt liggitt liggitt left review comments

Assignees

@piosz piosz

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

Problem with auth scopes in metrics-server
8 participants
@kawych @piosz @k8s-cherrypick-bot @mikedanese @k8s-github-robot @liggitt @DirectXMan12 @k8s-ci-robot