feat: option to disable create default userGroup on ODH and self-managed #1278

zdtsw · 2024-10-07T07:57:32Z

Description

user env variable ODH_USE_EXTERNAL_AUTH to disable user group creation.
this works by manually create subscritpon and set value to "true"
by install operator from Operatorhub will still ,by default, to create group

(a different soltuion than #1276)

https://issues.redhat.com/browse/RHOAIENG-14214

How Has This Been Tested?

delete odh-admin group from cluster
create

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: rhoai-catalog-dev
  namespace: openshift-marketplace
spec:
  displayName: usergroup
  image: 'quay.io/wenzhou/opendatahub-operator-catalog:v2.14.1007'
  publisher: wen
  sourceType: grpc

and

apiVersion: operators.coreos.com/v1alpha1
kind: Subscription 
metadata:
  name: opendatahub-operator
  namespace: openshift-operators
spec:
  name: opendatahub-operator
  channel: fast
  source: rhoai-catalog-dev
  sourceNamespace: openshift-marketplace
  config:
     env:
      - name: "ODH_DISABLE_USERGROUP"
        value: "true"

see operator installed
manually create DSCI
check no group created afterwards

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
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

- this only works if user create subscription and pass in variable Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci · 2024-10-07T07:57:43Z

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

openshift-ci · 2024-10-07T07:57:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from zdtsw. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

lburgazzoli · 2024-10-07T08:07:25Z

controllers/dscinitialization/dscinitialization_controller.go

-				return reconcile.Result{}, err
+			// Check if user opted for disabling creating user groups
+			disableUserGroup, exist := os.LookupEnv("DISABLE_USERGROUP")
+			if exist && disableUserGroup != "false" {


nit: since we don't need to distinguish between an empty value and an unset value, the check can probably simplified with os.Getenv("DISABLE_USERGROUP") == "true"

lburgazzoli · 2024-10-07T08:07:32Z

controllers/dscinitialization/dscinitialization_controller.go

-				return reconcile.Result{}, err
+			// Check if user opted for disabling creating user groups
+			disableUserGroup, exist := os.LookupEnv("DISABLE_USERGROUP")
+			if exist && disableUserGroup != "false" {


nit: since we don't need to distinguish between an empty value and an unset value, the check can probably simplified with os.Getenv("DISABLE_USERGROUP") == "true"

lburgazzoli · 2024-10-07T08:08:21Z

controllers/dscinitialization/dscinitialization_controller.go

-			if err != nil {
-				return reconcile.Result{}, err
+			// Check if user opted for disabling creating user groups
+			disableUserGroup, exist := os.LookupEnv("DISABLE_USERGROUP")


Ideally the env var name should be defined as a constant and better to add a prefix i.e. ODH_ to avoid any risk with env vars automatically injected in the pod or defined by the container.

- update README Signed-off-by: Wen Zhou <wenzhou@redhat.com>

lphiri

@zdtsw @lburgazzoli i wonder if we should make this config closer to what we want to things to be in future. Disabling group is only one aspect, but things like secret generation is also broken with external oidc because it uses OauthClient CR. Would renaming the environment to point to why we are making this change help, for example ODH_USE_EXTERNAL_AUTH? This would be closer to what we eventually want to add to the DSCI API like Luca was suggesting.

lburgazzoli · 2024-10-07T14:51:18Z

@lphiri yep that make sense

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw · 2024-10-07T17:18:24Z

/test opendatahub-operator-e2e

VaishnaviHire · 2024-10-07T19:15:18Z

controllers/dscinitialization/dscinitialization_controller.go

-			if err != nil {
-				return reconcile.Result{}, err
+			// Check if user opted for disabling creating user groups
+			if os.Getenv("ODH_USE_EXTERNAL_AUTH") == "true" {


When upgrading, what is the default value for this env variable?

If e value is set in the subscription, it will persist even later upgrade to new Operator version. Subscription should not get changed.

VaishnaviHire · 2024-10-08T19:57:48Z

/lgtm

Just a small request to keep the env name platform agnostic.

adelton · 2024-10-09T09:36:07Z

@zdtsw @lburgazzoli i wonder if we should make this config closer to what we want to things to be in future. Disabling group is only one aspect, but things like secret generation is also broken with external oidc because it uses OauthClient CR. Would renaming the environment to point to why we are making this change help, for example ODH_USE_EXTERNAL_AUTH? This would be closer to what we eventually want to add to the DSCI API like Luca was suggesting.

I might actually prefer the environment variable name to be very explicit about what effect it has on the functionally.

If at the very end of some revamp we say "now adding ODH_USE_EXTERNAL_AUTH which acts as ODH_DISABLE_USERGROUP, ODH_DISABLE_SECRETS, and ODH_DISABLE_SOMETHING_ELSE all being set to true", that is fine. But currently it seems we are overloading that ODH_USE_EXTERNAL_AUTH with multiple "automagic" functional behaviours which are bound to change/evolve in the future, which might be confusing people.

zdtsw · 2024-10-11T14:07:47Z

close it in favor of #1297

feat: option to disable create default userGroup on ODH and self-managed

9491db9

- this only works if user create subscription and pass in variable Signed-off-by: Wen Zhou <wenzhou@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress label Oct 7, 2024

zdtsw requested review from lburgazzoli, lphiri and VaishnaviHire October 7, 2024 07:58

lburgazzoli reviewed Oct 7, 2024

View reviewed changes

update: change env variable name + only check if value is set to true

1cb6612

- update README Signed-off-by: Wen Zhou <wenzhou@redhat.com>

lphiri reviewed Oct 7, 2024

View reviewed changes

update: rename variable to make it for broad use.

a918fed

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw marked this pull request as ready for review October 7, 2024 15:42

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 7, 2024

openshift-ci bot requested a review from grdryn October 7, 2024 15:42

VaishnaviHire reviewed Oct 7, 2024

View reviewed changes

openshift-ci bot assigned VaishnaviHire Oct 8, 2024

openshift-ci bot added the lgtm label Oct 8, 2024

zdtsw mentioned this pull request Oct 11, 2024

feat: Operator disable create usergroup if detect user enabled external auth #1297

Merged

5 tasks

zdtsw marked this pull request as draft October 11, 2024 09:10

openshift-ci bot added the do-not-merge/work-in-progress label Oct 11, 2024

zdtsw closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: option to disable create default userGroup on ODH and self-managed #1278

feat: option to disable create default userGroup on ODH and self-managed #1278

zdtsw commented Oct 7, 2024 •

edited

Loading

openshift-ci bot commented Oct 7, 2024

openshift-ci bot commented Oct 7, 2024

lburgazzoli Oct 7, 2024 •

edited

Loading

lburgazzoli Oct 7, 2024 •

edited

Loading

lburgazzoli Oct 7, 2024 •

edited

Loading

lphiri left a comment

lburgazzoli commented Oct 7, 2024

zdtsw commented Oct 7, 2024

VaishnaviHire Oct 7, 2024

zdtsw Oct 8, 2024

VaishnaviHire commented Oct 8, 2024

adelton commented Oct 9, 2024 •

edited

Loading

zdtsw commented Oct 11, 2024

feat: option to disable create default userGroup on ODH and self-managed #1278

feat: option to disable create default userGroup on ODH and self-managed #1278

Conversation

zdtsw commented Oct 7, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

openshift-ci bot commented Oct 7, 2024

openshift-ci bot commented Oct 7, 2024

lburgazzoli Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

lburgazzoli Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

lburgazzoli Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

lphiri left a comment

Choose a reason for hiding this comment

lburgazzoli commented Oct 7, 2024

zdtsw commented Oct 7, 2024

VaishnaviHire Oct 7, 2024

Choose a reason for hiding this comment

zdtsw Oct 8, 2024

Choose a reason for hiding this comment

VaishnaviHire commented Oct 8, 2024

adelton commented Oct 9, 2024 • edited Loading

zdtsw commented Oct 11, 2024

zdtsw commented Oct 7, 2024 •

edited

Loading

lburgazzoli Oct 7, 2024 •

edited

Loading

lburgazzoli Oct 7, 2024 •

edited

Loading

lburgazzoli Oct 7, 2024 •

edited

Loading

adelton commented Oct 9, 2024 •

edited

Loading