Skip to content

fix(RELEASE-1645): allow serviceaccounts to see cm #6450

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
Closed
wants to merge 1 commit into from
Closed

fix(RELEASE-1645): allow serviceaccounts to see cm #6450

wants to merge 1 commit into from

Conversation

scoheb
Copy link
Member

@scoheb scoheb commented May 27, 2025 •
edited
Loading

  • Grant access to authenticated users to be able to view openshift-pipelines configMaps.
  • The use case involves service accounts running automated tests being able to discover the custom URL for the Konflux UI

@openshift-ci openshift-ci bot requested review from enarha and xinredhat May 27, 2025 16:11
@scoheb scoheb requested review from hugares and aThorp96 and removed request for xinredhat May 27, 2025 16:11
aThorp96
aThorp96 approved these changes May 27, 2025
View reviewed changes
Copy link
Contributor

@aThorp96 aThorp96 left a comment

Choose a reason for hiding this comment

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

/lgtm

One optional nitpick

components/pipeline-service/base/rbac/openshift-pipelines/configmap-viewer.yaml Outdated
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: configmap-viewer-role
Copy link
Contributor

@aThorp96 aThorp96 May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nitpick: is it necessary to include the resource kind in the resource name? Any objection to naming the role and rolebinding configmap-viewer? It's not the end of the world to leave the kind in the name, just a little verbose, like type FooStruct struct {} 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed!

@openshift-ci openshift-ci bot added the lgtm label May 27, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 27, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aThorp96, scoheb

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

@aThorp96
Copy link
Contributor

/retest failed

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 27, 2025

@aThorp96: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test appstudio-e2e-tests

/test appstudio-load-test

The following commands are available to trigger optional jobs:

/test appstudio-hac-e2e-tests

/test appstudio-upgrade-tests

/test konflux-e2e-v416-optional

Use /test all to run the following jobs that were automatically triggered:

pull-ci-redhat-appstudio-infra-deployments-main-appstudio-e2e-tests

pull-ci-redhat-appstudio-infra-deployments-main-appstudio-upgrade-tests

In response to this:

/retest failed

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-sigs/prow repository.

@aThorp96
Copy link
Contributor

/retest-failed

@aThorp96
Copy link
Contributor

/test appstudio-e2e-tests

@scoheb
fix(RELEASE-1645): allow serviceaccounts to see cm
f00bc34 
- Grant access to authenticated users to be able to view
  openshift-pipelines configMaps. The user case involves
  service accounts running automated tests being able to
  discover the custom URL for the Konflux UI

Signed-off-by: Scott Hebert <[email protected]>
@scoheb scoheb force-pushed the get-cm-openshift-pipelines branch from 4918a73 to f00bc34 Compare May 28, 2025 14:56
@openshift-ci openshift-ci bot removed the lgtm label May 28, 2025
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented May 28, 2025

New changes are detected. LGTM label has been removed.

@scoheb scoheb requested a review from aThorp96 May 28, 2025 14:57
aThorp96
aThorp96 reviewed May 28, 2025
View reviewed changes
components/pipeline-service/base/rbac/openshift-pipelines/configmap-viewer.yaml
Comment on lines +2 to +5
# Grant access to authenticated users to be able to view
# openshift-pipelines configMaps. The user case involves
# service accounts running automated tests being able to
# discover the custom URL for the Konflux UI
Copy link
Contributor

Choose a reason for hiding this comment

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

I really appreciate the "why" comment here. Since this is going to apply to all users, is there any risk in all users having read access to configmaps? I'm wondering if the the automated testing serviceaccounts have a shared group which this rolebinding can use as its subject instead of all authenticated users

@scoheb
Copy link
Member Author

scoheb commented May 28, 2025

no longer needed!

@scoheb scoheb closed this May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aThorp96 aThorp96 aThorp96 approved these changes

@enarha enarha Awaiting requested review from enarha

@hugares hugares Awaiting requested review from hugares

Assignees

@aThorp96 aThorp96

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scoheb @aThorp96