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

build: add rbac for default sa #13917

Closed
wants to merge 1 commit into from
Closed

Conversation

parth-gr
Copy link
Member

rook csv doesnt contain the default
service account

recently we added default sa for most
of the ceph daemons but it didnt have the
rbacs, so added the rbacs to it
so rook csv can generate default sa

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@parth-gr parth-gr requested a review from travisn March 12, 2024 11:07
@parth-gr
Copy link
Member Author

Now we can see the service account.

[root@192 rook]# docker run --rm -it --platform=linux/amd64 --entrypoint=cat  quay.io/paarora/rook-operator:v471 /etc/ceph-csv-templates/rook-ceph.clusterserviceversion.yaml  | grep rook-ceph-default
          serviceAccountName: rook-ceph-default

@parth-gr parth-gr requested a review from iamniting March 12, 2024 11:08
@parth-gr parth-gr force-pushed the sa-rgw1 branch 2 times, most recently from c658d7f to 1ce172b Compare March 12, 2024 13:50
@parth-gr parth-gr requested a review from travisn March 12, 2024 13:51
namespace: {{ .Release.Namespace }} # namespace:cluster
rules:
- apiGroups: [""]
resources: ["pod"]
Copy link
Member

@travisn travisn Mar 12, 2024

Choose a reason for hiding this comment

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

Why is this pod get role added? I don't expect any new privileges for the default service account.

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 remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

That was just for testing

rook csv doesnt contain the default
service account

recently we added default sa for most
of the ceph daemons but it didnt have the
rbacs, so added the rbacs to it
so rook csv can generate default sa

Signed-off-by: parth-gr <partharora1010@gmail.com>
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

If this is only needed for the csv, could we just add this during the csv generation script? I don't see why we need it in the rook repo

@parth-gr
Copy link
Member Author

If this is only needed for the csv, could we just add this during the csv generation script? I don't see why we need it in the rook repo

at the end it will be in the rook image so doesn't matter, but to keep the flow same, lets discuss

@iamniting
Copy link
Member

If this is only needed for the csv, could we just add this during the csv generation script? I don't see why we need it in the rook repo

It will be difficult to do such a hack during CSV generation. I would +1 for this PR. Maybe we can add a comment why do we need it.

@parth-gr parth-gr requested a review from travisn March 12, 2024 14:42
@travisn
Copy link
Member

travisn commented Mar 12, 2024

Please sync up with @subhamkrai on related PR #13906

@subhamkrai
Copy link
Contributor

Please sync up with @subhamkrai on related PR #13906

for now, we can add this in downstream repo only no need to add this in upstream and ocs-operator or build can use the downstream repo(which it does I guess)

@parth-gr
Copy link
Member Author

Please sync up with @subhamkrai on related PR #13906

for now, we can add this in downstream repo only no need to add this in upstream and ocs-operator or build can use the downstream repo(which it does I guess)

Opened a PR red-hat-storage#589 hope it is easy to maintain, and from where red-hat-storage/rook:master this is genrated

@parth-gr
Copy link
Member Author

closing as fixed in downstream

@parth-gr parth-gr closed this Mar 14, 2024
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.

None yet

4 participants