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

Introduce rbac controller to handle RoleBindings #434

Merged
merged 7 commits into from
Nov 7, 2024

Conversation

atheo89
Copy link
Member

@atheo89 atheo89 commented Oct 29, 2024

Related to: https://issues.redhat.com/browse/RHOAIENG-580

Description

The PR introduce a new controller under odh-controller that handles the RoleBindings

How Has This Been Tested?

Created new test cases on the notebook_controller_test.go for RoleBinding creation, delete and update.

The video bellow demonstrates three different scenarios

  • Create a notebook on a DSP without the existence on a pipeline server, in that case should not create any Rolebinding because the specific Role
  • Setup a pipeline server, once is done then create a new notebook, on this new notebook we shouuld see the rolebinding named elyra-pipeline-<notebook-name>
  • Last, delete the notebook, and eventually delete the rolebinding as well.

https://drive.google.com/file/d/1uJJumxBiZukyVNakQ0uULZ68syKpq_3o/view?usp=sharing

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Copy link

openshift-ci bot commented Oct 29, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@atheo89
Copy link
Member Author

atheo89 commented Oct 30, 2024

/retest-required

1 similar comment
@atheo89
Copy link
Member Author

atheo89 commented Oct 30, 2024

/retest-required

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

Thank you for the great changes.
One additional request, we would like to keep this feature behind devflag and default keep it false, in future, once the bits are cleaned from dashboard , we would make it true, in this way this feature doesn't need to be stalled on dev side.

@atheo89
Copy link
Member Author

atheo89 commented Oct 31, 2024

The GHA tests had a great catch on the new changes! So since the configMap was false the operator didn't create any rolebinding and that why had the failure https://github.com/opendatahub-io/kubeflow/actions/runs/11611201427/job/32332040147?pr=434#step:4:540

Need to update the tests, in case that found SET_PIPELINE_RBAC=false to don't run the tests

@jiridanek
Copy link
Member

/lgtm overall, if I understand this correctly it just moves the rolebinding creation that dashboard used to do into our controller, but I'd want harshad to check it too ;p

@openshift-merge-robot openshift-merge-robot added the needs-rebase The PR needs a rebase or there are conflicts label Nov 4, 2024
@openshift-merge-robot
Copy link

PR needs rebase.

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.

@atheo89
Copy link
Member Author

atheo89 commented Nov 6, 2024

/retest-required

@atheo89
Copy link
Member Author

atheo89 commented Nov 6, 2024

/test odh-notebook-controller-unit

1 similar comment
@atheo89
Copy link
Member Author

atheo89 commented Nov 6, 2024

/test odh-notebook-controller-unit

@jiridanek
Copy link
Member

/lgtm

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

lgtm

i have tested the os.getenv, seems to be working as expected.
perhaps, we should take a closer look.

anyhow have few more comments

@openshift-ci openshift-ci bot removed the lgtm label Nov 7, 2024
@atheo89 atheo89 force-pushed the RHOAIENG-580 branch 2 times, most recently from 4d7146a to 57e275f Compare November 7, 2024 08:33
@jiridanek
Copy link
Member

/lgtm
looked it over and gha is passing

@openshift-ci openshift-ci bot added the lgtm label Nov 7, 2024
Copy link

openshift-ci bot commented Nov 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

@atheo89
Copy link
Member Author

atheo89 commented Nov 7, 2024

Thank you all for you time to review this and make it better!
I move on to merge this in order to start the release.

/override ci/prow/odh-notebook-controller-e2e

Copy link

openshift-ci bot commented Nov 7, 2024

@atheo89: Overrode contexts on behalf of atheo89: ci/prow/odh-notebook-controller-e2e

In response to this:

Thank you all for you time to review this and make it better!
I move on to merge this in order to start the release.

/override ci/prow/odh-notebook-controller-e2e

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.

@openshift-merge-bot openshift-merge-bot bot merged commit 3f931d2 into opendatahub-io:main Nov 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants