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

charts: allow cross namespace metrics scraping #854

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

tennix
Copy link
Member

@tennix tennix commented Aug 30, 2019

What problem does this PR solve?

Allow Prometheus to scrape cross namespace pods. The tidb-lightning chart may be deployed in the upstream tidb cluster to use the adhoc backup PVC. So Prometheus needs to scrape metrics cross namespace.

What is changed and how does it work?

The rbac rule of Prometheus changed to use cluster role and cluster role binding in order scrape cross namespace.

Check List

Tests

Manual test

Code changes

  • Has Helm charts change

Side effects

None

Related changes

  • Need to cherry-pick to the release branch release-1.1

Does this PR introduce a user-facing change?:

None

Copy link
Contributor

@aylei aylei left a comment

Choose a reason for hiding this comment

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

LGTM

@weekface
Copy link
Contributor

/run-e2e-in-kind

Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix tennix merged commit 4c6c104 into pingcap:master Aug 30, 2019
@tennix tennix deleted the cross-namespace-metrics-scraping branch August 30, 2019 12:30
@gregwebs
Copy link
Contributor

All of K8s security comes down to namespaces, so this is a big grant.
For the marketplace install this would fail because cluster roles must be passed in.
Note that for tidb-operator/scheduler we have the cluster scoped option so that it doesn't require cross-namespace permission.

I think we should come up with some alternative models of deployment here. I think we should definitely have an option to not support cross-namespace requests here (and thus this type of lightning deployment). In the cloud one will want to stream to and from object storage and not retain a backup on the PVC.
Another approach to this issue is to have an option to pass in the ClusterRole instead of defining it here, but I think we can avoid this for now.

yahonda pushed a commit that referenced this pull request Dec 27, 2021
* update conflicts check and add test file

* delete test file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants