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

zdtsw · 2024-10-11T09:10:32Z

Description

feat: Operator disable create usergroup if detect users enabled external auth

user internal Authentication CR Type indicate if Operator should create or not
only grant "get, watch , list" as least permission
remove duplicated rbac for "ingress"
add object into cache
(a different soltuion than feat: option to disable create default userGroup on ODH and self-managed #1278)

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

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.14214.8
1.

on a clean cluster, without odh-admins group, create

apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  name: opendatahub-operator
  namespace: openshift-operators
spec:
  channel: fast
  name: usergroup
  source: usergroup
  sourceNamespace: openshift-marketplace
  InstallPlanApproval: Manaual
---
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: usergroup
  namespace: openshift-marketplace
spec:
  displayName: usergroup
  image: 'quay.io/wenzhou/opendatahub-operator-catalog:v2.14214.8'
  publisher: wen
  sourceType: grpc

check Auth CR cluster has type set to ""
create DSCI CR
check odh-admins group created

manually update Auth CR to set "IntegratedOAuth" as Type
delete odh-admins group
restart Operator pod
odh-admins group created

test on a cluster with external-auth setup

check Auth CR cluster has type set to "OIDC"
check no odh-admins group and cannot see Group form UI (user management)
install Opreator
create DSCI without servicemesh enabled
no user group created

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

lburgazzoli · 2024-10-11T12:40:06Z

pkg/cluster/cluster_config.go

+		return true, fmt.Errorf("failed to get Authentication CR cluster: %w", err)
+	}
+
+	return (authenticationobj.Spec.Type == "IntegratedOAuth" || authenticationobj.Spec.Type == ""), nil


it appears there is a constant for this configv1.AuthenticationTypeIntegratedOAuth

i was thinking to make it:
return (authenticationobj.Spec.Type != "OIDC"), nil

or should we only create if it is AuthenticationTypeIntegratedOAuth ?

I don't have any strong opinion but sinceI don't know what are all the possible values (i.e. behind a feature flag like the OIDC one) I would check if the type is among the supported values IntegratedOAuth or "" to be on the safe side

lburgazzoli · 2024-10-11T12:51:06Z

controllers/dscinitialization/dscinitialization_controller.go

@@ -83,6 +84,11 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	// Set platform
 	platform := currentOperatorRelease.Name

+	createUsergroup, err := cluster.IsDefaultAuthMethod(ctx, r.Client)


nit: this can probably be moved closed where it is actually in use so i.e. if there's no DSCI, then there's no even need to retrieve the auth config. Same as if the reconcile request is not about the DSCI.

codecov-commenter · 2024-10-11T13:20:46Z

Codecov Report

Attention: Patch coverage is 13.63636% with 19 lines in your changes missing coverage. Please review.

Please upload report for BASE (incubation@fa63b4d). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
.../dscinitialization/dscinitialization_controller.go	20.00%	10 Missing and 2 partials ⚠️
pkg/cluster/cluster_config.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             incubation    #1297   +/-   ##
=============================================
  Coverage              ?   18.59%           
=============================================
  Files                 ?       30           
  Lines                 ?     2699           
  Branches              ?        0           
=============================================
  Hits                  ?      502           
  Misses                ?     2135           
  Partials              ?       62

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lburgazzoli · 2024-10-14T07:57:32Z

controllers/dscinitialization/dscinitialization_controller.go

@@ -212,11 +214,23 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re

 		return ctrl.Result{}, nil
 	default:
+		createUsergroup, err := cluster.IsDefaultAuthMethod(ctx, r.Client)
+		if err != nil {
+			if !k8serr.IsNotFound(err) { // only keep reconcile if real error but not missing CRD or missing CR


nit: it can probably be a single line if err != nil && !k8serr.IsNotFound(err)

lburgazzoli · 2024-10-14T08:02:47Z

pkg/cluster/cluster_config.go

+	// for now, HPC support "" "None" "IntegratedOAuth"(default) "OIDC"
+	// other offering support "" "None" "IntegratedOAuth"(default)
+	// we only create userGroups for "IntegratedOAuth" or "" and leave other or new supported type value in the future
+	return (authenticationobj.Spec.Type == configv1.AuthenticationTypeIntegratedOAuth || authenticationobj.Spec.Type == ""), nil


nit: parentheses seems to be redundant

…nal auth - use internal Authentication CR Type "" or IntegratedOAuth to indicate if Operator should create usergroup or not CRD has validation to only allow "IntegratedOAuth", "", "None" or "OIDC" - only grant "get, watch , list" as least permission - remove duplicated rbac for "ingress", has been defined in other places - add object into cache - add CRD into unit-test - add unit-test: since we dont have auth CR, it should not create usergroup Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

lphiri · 2024-10-14T14:25:56Z

pkg/cluster/cluster_config.go

+	authenticationobj := &configv1.Authentication{}
+	if err := cli.Get(ctx, client.ObjectKey{Name: "cluster", Namespace: ""}, authenticationobj); err != nil {
+		if errors.Is(err, &meta.NoKindMatchError{}) { // when CRD is missing, conver error type
+			return false, k8serr.NewNotFound(configv1.Resource("authentications"), "cluster")


nit: suggest using a constant for "cluster" , since its repeated a few times.

Signed-off-by: Wen Zhou <[email protected]>

openshift-ci · 2024-10-15T07:14:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lburgazzoli

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:

~~OWNERS~~ [lburgazzoli]

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

… external auth - manual cherry-pick from opendatahub-io#1297 Signed-off-by: Wen Zhou <[email protected]>

… external auth - manual cherry-pick from opendatahub-io#1297 Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 5bed921)

… external auth - manual cherry-pick from opendatahub-io#1297 Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit 5bed921) Signed-off-by: Wen Zhou <[email protected]>

openshift-ci bot requested review from etirelli and VaishnaviHire October 11, 2024 09:10

zdtsw requested review from csams, lburgazzoli and lphiri October 11, 2024 09:11

lburgazzoli reviewed Oct 11, 2024

View reviewed changes

zdtsw force-pushed the chore_groups3 branch from e4ffeb6 to 2d4f726 Compare October 11, 2024 13:17

zdtsw force-pushed the chore_groups3 branch 3 times, most recently from 9d2a57c to 17f8fae Compare October 11, 2024 13:36

This was referenced Oct 11, 2024

feat: skip create usergroups if group CRD does not exsit in cluster #1276

Closed

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

Closed

lburgazzoli reviewed Oct 14, 2024

View reviewed changes

zdtsw added 2 commits October 14, 2024 10:31

update: review comments

2d7737b

Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the chore_groups3 branch from 17f8fae to 2d7737b Compare October 14, 2024 08:39

lphiri reviewed Oct 14, 2024

View reviewed changes

update: review comments use const

61f43f8

Signed-off-by: Wen Zhou <[email protected]>

zdtsw added the rhoai-2.13.1 label Oct 15, 2024

zdtsw requested review from lphiri and lburgazzoli October 15, 2024 06:45

lburgazzoli approved these changes Oct 15, 2024

View reviewed changes

openshift-ci bot assigned lburgazzoli Oct 15, 2024

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

openshift-ci bot added the approved label Oct 15, 2024

openshift-merge-bot bot merged commit a5388ad into opendatahub-io:incubation Oct 15, 2024
10 checks passed

zdtsw added a commit to zdtsw-forking/rhods-operator that referenced this pull request Oct 15, 2024

[sync]:feat: Operator disable create usergroup if detect user enabled…

6f6e586

… external auth - manual cherry-pick from opendatahub-io#1297 Signed-off-by: Wen Zhou <[email protected]>

zdtsw mentioned this pull request Oct 15, 2024

[sync]:feat: Operator disable create usergroup if detect user enable external auth red-hat-data-services/rhods-operator#380

Merged

5 tasks

zdtsw added a commit to zdtsw-forking/rhods-operator that referenced this pull request Oct 15, 2024

[sync]:feat: Operator disable create usergroup if detect user enabled…

5bed921

… external auth - manual cherry-pick from opendatahub-io#1297 Signed-off-by: Wen Zhou <[email protected]>

zdtsw added the rhoai-2.15 label Oct 15, 2024

zdtsw mentioned this pull request Oct 16, 2024

[sync]:feat: Operator disable create usergroup if detect user enabl external auth red-hat-data-services/rhods-operator#383

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zdtsw commented Oct 11, 2024 •

edited

Loading

lburgazzoli Oct 11, 2024

zdtsw Oct 11, 2024

lburgazzoli Oct 11, 2024

lburgazzoli Oct 11, 2024

codecov-commenter commented Oct 11, 2024 •

edited

Loading

lburgazzoli Oct 14, 2024

lburgazzoli Oct 14, 2024 •

edited

Loading

lphiri Oct 14, 2024

zdtsw Oct 14, 2024

openshift-ci bot commented Oct 15, 2024

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

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

Conversation

zdtsw commented Oct 11, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

lburgazzoli Oct 11, 2024

Choose a reason for hiding this comment

zdtsw Oct 11, 2024

Choose a reason for hiding this comment

lburgazzoli Oct 11, 2024

Choose a reason for hiding this comment

lburgazzoli Oct 11, 2024

Choose a reason for hiding this comment

codecov-commenter commented Oct 11, 2024 • edited Loading

Codecov Report

lburgazzoli Oct 14, 2024

Choose a reason for hiding this comment

lburgazzoli Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

lphiri Oct 14, 2024

Choose a reason for hiding this comment

zdtsw Oct 14, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Oct 15, 2024

zdtsw commented Oct 11, 2024 •

edited

Loading

codecov-commenter commented Oct 11, 2024 •

edited

Loading

lburgazzoli Oct 14, 2024 •

edited

Loading