Rhoaieng 14232 add auth crd #1452

StevenTobin · 2024-12-13T15:53:07Z

Description

Add Auth controller and CRD.

Functionality

Create Auth singleton instance if none exists.
Copy groups from odhDashboardConfig into auth cr.
Create Roles for admin and allowed groups.
Create rolebinding binding required role to each group in list.
Delete the rolebinding for any group removed from the list.

RHOAIENG-14231

How Has This Been Tested?

Creation of CR and Rolebindings

Deploy operator and create a DSCi CR.
Verify the auth CR is created automatically.
Verify the Auth CR contains the default groups of odh-admins and system:authenticated.
Verify the roles admingroups-role and allowedgroups-role exist in the applications namespace.
Verify the rolebinding admingroups-rolebinding and allowedgroups-rolebinding exist in the applications namespace.
Add a group to the Auth CR and verify the role and rolebinding are created for the new group.
Delete a group and confirm the rolebinding is deleted for that group.

Copying info from dashboardConfig

Delete auth CR.
Create dashboardConfig with different groups defined.
Stop - start operator to force a reconciliation.
Verify the groups from the dashboardConfig are copied into the Auth CR

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

openshift-ci · 2024-12-13T15:53:18Z

[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 steventobin. For more information see the 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

github-actions · 2024-12-13T15:54:26Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12318824711

github-actions · 2024-12-13T15:56:34Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12318856499

controllers/dscinitialization/dscinitialization_controller.go

controllers/services/auth/auth_controller_actions.go

pkg/cluster/gvk/gvk.go

tests/e2e/controller_test.go

apis/services/v1alpha1/auth_types.go

config/samples/services_v1_auth.yaml

controllers/services/auth/auth_controller_actions.go

github-actions · 2024-12-16T09:21:43Z

This PR can't be merged just yet 😢

Please run make generate manifests api-docs and commit the changes.

For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/12349540685

StevenTobin · 2024-12-16T16:12:49Z

/retest

controllers/services/auth/auth_controller.go

controllers/services/auth/auth_controller_actions.go

controllers/dscinitialization/dscinitialization_controller.go

StevenTobin · 2024-12-17T10:07:40Z

@lburgazzoli as per the slack conversation I added a watch in the DSCI so if the Auth CR is deleted the DSCI will recreate it.

StevenTobin · 2024-12-17T11:14:46Z

/retest

StevenTobin · 2024-12-17T11:39:34Z

/retest

lburgazzoli · 2024-12-17T13:28:24Z

/retest

controllers/services/auth/resources/admingroup-role.tmpl.yaml

StevenTobin · 2024-12-19T11:51:37Z

/retest

controllers/services/auth/auth_controller_actions.go

ykaliuta · 2024-12-20T04:28:20Z

controllers/services/auth/auth_controller_actions.go

+	if newGroups {
+		err := rr.Client.Update(ctx, ai)
+		if err != nil {
+			return errors.New("error adding groups to Auth CR")


Should such errors be wrapped?

controllers/services/auth/auth_controller_actions.go

ykaliuta · 2024-12-20T04:45:06Z

I doubt commit message with progress commits makes any sense. Would it be possible to put PR's description there?

codecov · 2024-12-20T10:44:23Z

Codecov Report

Attention: Patch coverage is 12.56281% with 174 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@326e4fc). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...ntrollers/services/auth/auth_controller_actions.go	0.00%	143 Missing ⚠️
controllers/services/auth/auth_controller.go	0.00%	24 Missing ⚠️
.../dscinitialization/dscinitialization_controller.go	76.66%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1452   +/-   ##
=======================================
  Coverage        ?   18.92%           
=======================================
  Files           ?      161           
  Lines           ?    10603           
  Branches        ?        0           
=======================================
  Hits            ?     2007           
  Misses          ?     8370           
  Partials        ?      226

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

StevenTobin · 2025-01-06T14:46:51Z

/retest

config/manifests/bases/opendatahub-operator.clusterserviceversion.yaml

VaishnaviHire · 2025-01-06T17:22:44Z

When only DSCI is created, without DSC, we see following error

{"level":"error","ts":"2025-01-06T17:06:09Z","msg":"Reconciler error","controller":"auth","controllerGroup":"services.platform.opendatahub.io","controllerKind":"Auth","Auth":{"name":"auth"},"namespace":"","name":"auth","reconcileID":"c88a3850-579a-463f-82cd-c3ddcd257c00","error":"no matches for kind \"OdhDashboardConfig\" in version \"opendatahub.io/v1alpha\"","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227"}

Should we have default values instead or update error message for users stating they need to enable dashboard?

VaishnaviHire · 2025-01-06T17:25:08Z

apis/services/v1alpha1/auth_types.go

+
+const (
+	AuthServiceName  = "auth"
+	AuthInstanceName = "auth"


nit: we have been using the naming format default-*(e.g default-dsc, default-dsci, default-monitoring) , can we update the name here as well?

I initially used that naming of default-auth, but @andrewballantyne made the point that it seems strange to use default-auth when it's intended to be a singleton and I agree. There will only ever be one, the system doesn't allow two. A prefix of default seemed strange. It implies that there might be other non-default auth instances or that we are creating a default because the user didn't provide an auth instance.

@VaishnaviHire wonder if the same case should be made against components ?

Yeah Andrew's and Steven's points make sense. I do not remember the reason why we went with default- prefix. I am okay with removing the default, but we will have to have upgrade logic to update the names for components and dsc/dsci in ODH releases

i guess: we have default-dsc/default-dsci, we carried this "default-" to the component CRs.
and it is easier to have the pattern as filter.

VaishnaviHire · 2025-01-06T17:26:52Z

When I delete DSCI and DSC the Auth CR remains, is this expected?

lburgazzoli · 2025-01-07T08:23:26Z

When only DSCI is created, without DSC, we see following error

{"level":"error","ts":"2025-01-06T17:06:09Z","msg":"Reconciler error","controller":"auth","controllerGroup":"services.platform.opendatahub.io","controllerKind":"Auth","Auth":{"name":"auth"},"namespace":"","name":"auth","reconcileID":"c88a3850-579a-463f-82cd-c3ddcd257c00","error":"no matches for kind \"OdhDashboardConfig\" in version \"opendatahub.io/v1alpha\"","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\t/opt/app-root/src/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227"}

Should we have default values instead or update error message for users stating they need to enable dashboard?

Seems a case for dynamic watching since the OdhDashboardConfig won't be available till the dashboard component is enabled. Thinking a little bit more, we may also need to check if the dahboard is enabled or not ? i.e. the OdhDashboardConfig may be present, but the component may be marked as removed, in such case I guess we should ignore the OdhDashboardConfig.

The other option (that I don't know if it is feasible at thist stage) would be to do the migration only once at startup time

StevenTobin · 2025-01-07T08:56:03Z

When only DSCI is created, without DSC, we see following error
Should we have default values instead or update error message for users stating they need to enable dashboard?

Seems a case for dynamic watching since the OdhDashboardConfig won't be available till the dashboard component is enabled. Thinking a little bit more, we may also need to check if the dahboard is enabled or not

I'll add a check for the kind existing to catch that. If the dashboardConfig doesn't exist we shouldn't do anything but continue. We don't need it to exist we just need to read the content if it does.

lburgazzoli · 2025-01-07T09:01:15Z

When only DSCI is created, without DSC, we see following error
Should we have default values instead or update error message for users stating they need to enable dashboard?

Seems a case for dynamic watching since the OdhDashboardConfig won't be available till the dashboard component is enabled. Thinking a little bit more, we may also need to check if the dahboard is enabled or not

I'll add a check for the kind existing to catch that. If the dashboardConfig doesn't exist we shouldn't do anything but continue. We don't need it to exist we just need to read the content if it does.

Wonder if we should also watch the OdhDashboardConfig type ? my understanding is that as today, the Auth reconcile won't be triggered in case something changes on the OdhDashboardConfig or if it is created/removed.

StevenTobin · 2025-01-07T09:26:26Z

Wonder if we should also watch the OdhDashboardConfig type ? my understanding is that as today, the Auth reconcile won't be triggered in case something changes on the OdhDashboardConfig or if it is created/removed.

Yeah great point. I'll add that.

apis/services/v1alpha1/auth_types.go

config/manifests/bases/opendatahub-operator.clusterserviceversion.yaml

controllers/datasciencecluster/kubebuilder_rbac.go

controllers/dscinitialization/dscinitialization_controller.go

controllers/services/auth/auth_controller_actions.go

controllers/services/auth/resources/admingroup-role.tmpl.yaml

controllers/services/auth/auth_controller_support.go

controllers/services/auth/auth_controller.go

StevenTobin · 2025-01-08T11:08:32Z

/retest

StevenTobin · 2025-01-08T13:41:02Z

When I delete DSCI and DSC the Auth CR remains, is this expected?

@VaishnaviHire I hadn't really considered it, but in the future the Auth CR may handle Auth concerns beyond the DSC and DSCi so I think it sticking around isn't a big issue. It's similar behaviour to the dashboardConfig and is similarly expected to be user configuration.

StevenTobin · 2025-01-08T14:02:53Z

/retest

Functionality Create Auth singleton instance if none exists. Copy groups from odhDashboardConfig into auth cr. Create Roles for admin and allowed groups. Create CLusterRole for admin groups. Create required rolebindings for each group in both lists. Create requires clusterROlebinding for each admin group. Delete the rolebinding or clusterrolebinding for any group removed from either list. add dynamic watch for dashboardConfig, fix labels and move functions into seperate file remove gvk watch for dashboardconfig

openshift-ci · 2025-01-08T19:32:04Z

@StevenTobin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/opendatahub-operator-e2e	`0f87a2c`	link	true	`/test opendatahub-operator-e2e`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

zdtsw · 2025-01-09T08:10:41Z

controllers/services/auth/auth_controller.go

+		WatchesGVK(
+			gvk.CustomResourceDefinition,
+			reconciler.WithEventHandler(handlers.ToNamed(serviceApi.AuthInstanceName)),
+			reconciler.WithPredicates(predicate.NewPredicateFuncs(func(object client.Object) bool {
+				return object.GetName() == odhDashboardConfigCRDName
+			}))).


wonder: why we need to watch this CRD.
dashboard can update this CRD, but Auth CR only care about value from adminGroups and allowedGroups.
so the change to other part of the CRD should not trigger reconcile for Auth CR,
right?

This is to trigger the dynamic watching when the OdhDashbaordConfig CRD gets deployed as consequence of the dashboard installation. Without this, the controller won't notice the existence of the OdhDashbaordConfig resource.

zdtsw · 2025-01-09T08:13:36Z

controllers/services/auth/auth_controller.go

+			}))).
+		WatchesGVK(
+			gvk.OdhDashboardConfig,
+			reconciler.Dynamic(shouldWatchDashboardConfig),


if we are locking down changes in odh-dashboard-config (as read-only) for groupsConfig block, do we still need to watch this object? as if the change is done for .spec.dashboardConfig.disalbeModelMesh should reconcile be triggered.

We have two options:

we do sync OdhDashbaordConfig only once when creating the Auth instance

we do sync OdhDashbaordConfig as part of the reconcile loop.

As today, it is done as part of the reconcile loop (2), so we must watch the resource to be consistent, otherwise the sync would happen by chance i.e, if the controller gets restarted. I guess that when the auth stanza in the OdhDashbaordConfig will be made read-only, we can probably switch to option (1), but that's something for @StevenTobin and @csams to clarify.

openshift-ci bot requested review from lburgazzoli and ugiordan December 13, 2024 15:53