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

Jump to bottom

Automate prometheususer creation for ob metrics collector #1336

Merged
merged 1 commit into from Dec 9, 2021
Merged

Automate prometheususer creation for ob metrics collector #1336

merged 1 commit into from Dec 9, 2021

Conversation

thotz
Copy link
Contributor

@thotz thotz commented Sep 14, 2021

As a prerequisite for object bucket metrics collector, it is expected to have a user named prometheus-user in the object store with certain permissions . Otherwise, it won't able to fetch the stat info from backend. This PR automates that creation

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 14, 2021

Hi @thotz. Thanks for your PR.

I'm waiting for a red-hat-storage member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@thotz
Copy link
Contributor Author

thotz commented Sep 14, 2021

@umangachapagain @jarrpa @anmolsachan PTAL

@nbalacha
Copy link
Contributor

This PR includes a commit to update the rook version to 1.7.3. Is that required in order to create the prometheus user?
If yes, please do that in a separate PR as that is a big change.

@thotz
Copy link
Contributor Author

thotz commented Sep 17, 2021 •
edited

This PR includes a commit to update the rook version to 1.7.3. Is that required in order to create the prometheus user?
If yes, please do that in a separate PR as that is a big change.

Yes, I will send separate PR for rook update #1340

umangachapagain
umangachapagain reviewed Sep 22, 2021
View reviewed changes
controllers/storagecluster/cephobjectstoreusers.go
@@ -29,6 +31,20 @@ func (r *StorageClusterReconciler) newCephObjectStoreUserInstances(initData *ocs
Store: generateNameForCephObjectStore(initData),
},
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to create such a user? any user who can access this resource could use it for previlage escalation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same security concern applies for the noobaa-ceph-objectstore-user and ocs-storagecluster-cephobjectstoreuser, even if we create this user manually, this concern issue will be present

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021
@thotz thotz force-pushed the automate-prometheususer-creation branch from 4f368d3 to 7ea88da Compare November 26, 2021 10:02
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2021
umangachapagain
umangachapagain reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

lgtm

/cc @leseb Do you think there could be any issues with creating such a user?

@umangachapagain
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 6, 2021
@thotz thotz force-pushed the automate-prometheususer-creation branch from 7ea88da to 88cd57f Compare December 7, 2021 06:08
@thotz thotz force-pushed the automate-prometheususer-creation branch from 88cd57f to ee39f11 Compare December 8, 2021 12:34
leseb
leseb suggested changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@leseb leseb left a comment

Choose a reason for hiding this comment

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

One nit

controllers/storagecluster/cephobjectstoreusers.go Outdated Show resolved Hide resolved
@thotz
automate creation of prometheus-user for ob metrics collection
e836837 
Signed-off-by: Jiffin Tony Thottan <thottanjiffin@gmail.com>
@thotz thotz force-pushed the automate-prometheususer-creation branch from ee39f11 to e836837 Compare December 8, 2021 15:17
@leseb
Copy link
Contributor

leseb commented Dec 8, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 8, 2021

@thotz: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws e836837 link false /test red-hat-storage-ocs-ci-e2e-aws

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leseb, thotz, umangachapagain

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2021
@openshift-merge-robot openshift-merge-robot merged commit 25cbaa8 into red-hat-storage:main Dec 9, 2021
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umangachapagain umangachapagain umangachapagain approved these changes

@leseb leseb Awaiting requested review from leseb

Assignees

@leseb leseb

@umangachapagain umangachapagain

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@thotz @nbalacha @umangachapagain @leseb @openshift-merge-robot