From 2d4f7269de3343dedc8191a46f90136acd178dff Mon Sep 17 00:00:00 2001
From: Wen Zhou <wenzhou@redhat.com>
Date: Fri, 11 Oct 2024 09:32:14 +0200
Subject: [PATCH] feat: Operator disable create usergroup if detect users
 enabled external auth

- use internal Authentication CR Type 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 <wenzhou@redhat.com>
---
 README.md                                     |  21 +--
 ...atahub-operator.clusterserviceversion.yaml |   1 +
 .../config.openshift.io_authentications.yaml  | 175 ++++++++++++++++++
 config/rbac/role.yaml                         |   1 +
 .../dscinitialization_controller.go           |  17 +-
 .../dscinitialization_test.go                 |  14 +-
 controllers/dscinitialization/suite_test.go   |   2 +
 main.go                                       |   5 +
 pkg/cluster/cluster_config.go                 |  20 +-
 9 files changed, 228 insertions(+), 28 deletions(-)
 create mode 100644 config/crd/external/config.openshift.io_authentications.yaml

diff --git a/README.md b/README.md
index cbbd737b852..e9860a1b0bb 100644
--- a/README.md
+++ b/README.md
@@ -66,26 +66,7 @@ Additionally installing `Authorino operator` & `Service Mesh operator` enhances
         sourceNamespace: openshift-marketplace
       EOF
       ```
-      If user would prefer skipping group "odh-admin" to be created by DSCI CR automatically, explicitly set env variable ODH_USE_EXTERNAL_AUTH to "true". example:
-
-    ```console
-      cat <<EOF | oc create -f -
-      apiVersion: operators.coreos.com/v1alpha1
-      kind: Subscription
-      metadata:
-        name: opendatahub-operator
-        namespace: openshift-operators
-      spec:
-        channel: fast
-        name: opendatahub-operator
-        source: community-operators
-        sourceNamespace: openshift-marketplace
-        config:
-          env:
-            - name: "ODH_USE_EXTERNAL_AUTH"
-              value: "true"
-      EOF
-
+      
   2. Create [DSCInitialization](#example-dscinitialization) CR manually.
     You can also use operator to create default DSCI CR by removing env variable DISABLE_DSC_CONFIG from CSV or changing the value to "false", followed by restarting the operator pod.
 
diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
index 81b13c83f14..21561dfc6b4 100644
--- a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
+++ b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
@@ -357,6 +357,7 @@ spec:
         - apiGroups:
           - config.openshift.io
           resources:
+          - authentications
           - clusterversions
           verbs:
           - get
diff --git a/config/crd/external/config.openshift.io_authentications.yaml b/config/crd/external/config.openshift.io_authentications.yaml
new file mode 100644
index 00000000000..86ad306c7fd
--- /dev/null
+++ b/config/crd/external/config.openshift.io_authentications.yaml
@@ -0,0 +1,175 @@
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  annotations:
+    controller-gen.kubebuilder.io/version: v0.16.1
+  name: authentications.config.openshift.io
+spec:
+  group: config.openshift.io
+  names:
+    kind: Authentication
+    listKind: AuthenticationList
+    plural: authentications
+    singular: authentication
+  scope: Cluster
+  versions:
+  - name: v1
+    schema:
+      openAPIV3Schema:
+        description: |-
+          Authentication specifies cluster-wide settings for authentication (like OAuth and
+          webhook token authenticators). The canonical name of an instance is `cluster`.
+
+          Compatibility level 1: Stable within a major release for a minimum of 12 months or 3 minor releases (whichever is longer).
+        properties:
+          apiVersion:
+            description: |-
+              APIVersion defines the versioned schema of this representation of an object.
+              Servers should convert recognized schemas to the latest internal value, and
+              may reject unrecognized values.
+              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
+            type: string
+          kind:
+            description: |-
+              Kind is a string value representing the REST resource this object represents.
+              Servers may infer this from the endpoint the client submits requests to.
+              Cannot be updated.
+              In CamelCase.
+              More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
+            type: string
+          metadata:
+            type: object
+          spec:
+            description: spec holds user settable values for configuration
+            properties:
+              oauthMetadata:
+                description: |-
+                  oauthMetadata contains the discovery endpoint data for OAuth 2.0
+                  Authorization Server Metadata for an external OAuth server.
+                  This discovery document can be viewed from its served location:
+                  oc get --raw '/.well-known/oauth-authorization-server'
+                  For further details, see the IETF Draft:
+                  https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
+                  If oauthMetadata.name is non-empty, this value has precedence
+                  over any metadata reference stored in status.
+                  The key "oauthMetadata" is used to locate the data.
+                  If specified and the config map or expected key is not found, no metadata is served.
+                  If the specified metadata is not valid, no metadata is served.
+                  The namespace for this config map is openshift-config.
+                properties:
+                  name:
+                    description: name is the metadata.name of the referenced config
+                      map
+                    type: string
+                required:
+                - name
+                type: object
+              serviceAccountIssuer:
+                description: |-
+                  serviceAccountIssuer is the identifier of the bound service account token
+                  issuer.
+                  The default is https://kubernetes.default.svc
+                  WARNING: Updating this field will not result in immediate invalidation of all bound tokens with the
+                  previous issuer value. Instead, the tokens issued by previous service account issuer will continue to
+                  be trusted for a time period chosen by the platform (currently set to 24h).
+                  This time period is subject to change over time.
+                  This allows internal components to transition to use new service account issuer without service distruption.
+                type: string
+              type:
+                description: |-
+                  type identifies the cluster managed, user facing authentication mode in use.
+                  Specifically, it manages the component that responds to login attempts.
+                  The default is IntegratedOAuth.
+                type: string
+              webhookTokenAuthenticator:
+                description: |-
+                  webhookTokenAuthenticator configures a remote token reviewer.
+                  These remote authentication webhooks can be used to verify bearer tokens
+                  via the tokenreviews.authentication.k8s.io REST API. This is required to
+                  honor bearer tokens that are provisioned by an external authentication service.
+                properties:
+                  kubeConfig:
+                    description: |-
+                      kubeConfig references a secret that contains kube config file data which
+                      describes how to access the remote webhook service.
+                      The namespace for the referenced secret is openshift-config.
+
+                      For further details, see:
+
+                      https://kubernetes.io/docs/reference/access-authn-authz/authentication/#webhook-token-authentication
+
+                      The key "kubeConfig" is used to locate the data.
+                      If the secret or expected key is not found, the webhook is not honored.
+                      If the specified kube config data is not valid, the webhook is not honored.
+                    properties:
+                      name:
+                        description: name is the metadata.name of the referenced secret
+                        type: string
+                    required:
+                    - name
+                    type: object
+                required:
+                - kubeConfig
+                type: object
+              webhookTokenAuthenticators:
+                description: webhookTokenAuthenticators is DEPRECATED, setting it
+                  has no effect.
+                items:
+                  description: |-
+                    deprecatedWebhookTokenAuthenticator holds the necessary configuration options for a remote token authenticator.
+                    It's the same as WebhookTokenAuthenticator but it's missing the 'required' validation on KubeConfig field.
+                  properties:
+                    kubeConfig:
+                      description: |-
+                        kubeConfig contains kube config file data which describes how to access the remote webhook service.
+                        For further details, see:
+                        https://kubernetes.io/docs/reference/access-authn-authz/authentication/#webhook-token-authentication
+                        The key "kubeConfig" is used to locate the data.
+                        If the secret or expected key is not found, the webhook is not honored.
+                        If the specified kube config data is not valid, the webhook is not honored.
+                        The namespace for this secret is determined by the point of use.
+                      properties:
+                        name:
+                          description: name is the metadata.name of the referenced
+                            secret
+                          type: string
+                      required:
+                      - name
+                      type: object
+                  type: object
+                type: array
+            type: object
+          status:
+            description: status holds observed values from the cluster. They may not
+              be overridden.
+            properties:
+              integratedOAuthMetadata:
+                description: |-
+                  integratedOAuthMetadata contains the discovery endpoint data for OAuth 2.0
+                  Authorization Server Metadata for the in-cluster integrated OAuth server.
+                  This discovery document can be viewed from its served location:
+                  oc get --raw '/.well-known/oauth-authorization-server'
+                  For further details, see the IETF Draft:
+                  https://tools.ietf.org/html/draft-ietf-oauth-discovery-04#section-2
+                  This contains the observed value based on cluster state.
+                  An explicitly set value in spec.oauthMetadata has precedence over this field.
+                  This field has no meaning if authentication spec.type is not set to IntegratedOAuth.
+                  The key "oauthMetadata" is used to locate the data.
+                  If the config map or expected key is not found, no metadata is served.
+                  If the specified metadata is not valid, no metadata is served.
+                  The namespace for this config map is openshift-config-managed.
+                properties:
+                  name:
+                    description: name is the metadata.name of the referenced config
+                      map
+                    type: string
+                required:
+                - name
+                type: object
+            type: object
+        required:
+        - spec
+        type: object
+    served: true
+    storage: true
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index d81b97e1555..01ab8982756 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -171,6 +171,7 @@ rules:
 - apiGroups:
   - config.openshift.io
   resources:
+  - authentications
   - clusterversions
   verbs:
   - get
diff --git a/controllers/dscinitialization/dscinitialization_controller.go b/controllers/dscinitialization/dscinitialization_controller.go
index 39e064e10a4..d5b2f0de999 100644
--- a/controllers/dscinitialization/dscinitialization_controller.go
+++ b/controllers/dscinitialization/dscinitialization_controller.go
@@ -19,7 +19,6 @@ package dscinitialization
 
 import (
 	"context"
-	"os"
 	"path/filepath"
 	"reflect"
 
@@ -30,6 +29,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	k8serr "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
@@ -74,6 +74,7 @@ type DSCInitializationReconciler struct {
 // +kubebuilder:rbac:groups="dscinitialization.opendatahub.io",resources=dscinitializations,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="features.opendatahub.io",resources=featuretrackers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups="features.opendatahub.io",resources=featuretrackers/status,verbs=get;update;patch;delete
+// +kubebuilder:rbac:groups="config.openshift.io",resources=authentications,verbs=get;watch;list
 
 // Reconcile contains controller logic specific to DSCInitialization instance updates.
 func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { //nolint:funlen,gocyclo,maintidx
@@ -84,6 +85,14 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	// Set platform
 	platform := currentOperatorRelease.Name
 
+	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
+			return ctrl.Result{}, err
+		}
+	}
+
 	instances := &dsciv1.DSCInitializationList{}
 	if err := r.Client.List(ctx, instances); err != nil {
 		log.Error(err, "Failed to retrieve DSCInitialization resource.", "DSCInitialization Request.Name", req.Name)
@@ -155,7 +164,7 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 
 	// Check namespace is not exist, then create
 	namespace := instance.Spec.ApplicationsNamespace
-	err := r.createOdhNamespace(ctx, instance, namespace, platform)
+	err = r.createOdhNamespace(ctx, instance, namespace, platform)
 	if err != nil {
 		// no need to log error as it was already logged in createOdhNamespace
 		return reconcile.Result{}, err
@@ -202,7 +211,7 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		switch platform {
 		case cluster.SelfManagedRhods:
 			// Check if user opted for disabling creating user groups
-			if os.Getenv("ODH_USE_EXTERNAL_AUTH") == "true" {
+			if !createUsergroup {
 				log.Info("DSCI disabled usergroup creation")
 			} else {
 				err := r.createUserGroup(ctx, instance, "rhods-admins")
@@ -239,7 +248,7 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re
 			}
 		default:
 			// Check if user opted for disabling creating user groups
-			if os.Getenv("ODH_USE_EXTERNAL_AUTH") == "true" {
+			if !createUsergroup {
 				log.Info("DSCI disabled usergroup creation")
 			} else {
 				err := r.createUserGroup(ctx, instance, "odh-admins")
diff --git a/controllers/dscinitialization/dscinitialization_test.go b/controllers/dscinitialization/dscinitialization_test.go
index d380375ef63..d9afb51deb4 100644
--- a/controllers/dscinitialization/dscinitialization_test.go
+++ b/controllers/dscinitialization/dscinitialization_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
+	userv1 "github.com/openshift/api/user/v1"
 	corev1 "k8s.io/api/core/v1"
 	networkingv1 "k8s.io/api/networking/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -21,6 +22,7 @@ const (
 	workingNamespace     = "test-operator-ns"
 	applicationName      = "default-dsci"
 	applicationNamespace = "test-application-ns"
+	usergroupName        = "odh-admins"
 	configmapName        = "odh-common-config"
 	monitoringNamespace  = "test-monitoring-ns"
 	readyPhase           = "Ready"
@@ -109,6 +111,14 @@ var _ = Describe("DataScienceCluster initialization", func() {
 			Expect(foundConfigMap.Data).To(Equal(expectedConfigmapData))
 		})
 
+		It("Should not create user group when we do not have authentications CR in the cluster", func(ctx context.Context) {
+			userGroup := &userv1.Group{}
+			Eventually(objectExists(usergroupName, "", userGroup)).
+				WithContext(ctx).
+				WithTimeout(timeout).
+				WithPolling(interval).
+				Should(BeFalse())
+		})
 	})
 
 	Context("Monitoring Resource", func() {
@@ -341,9 +351,9 @@ func namespaceExists(ns string, obj client.Object) func(ctx context.Context) boo
 	}
 }
 
-func objectExists(ns string, name string, obj client.Object) func(ctx context.Context) bool { //nolint:unparam
+func objectExists(name string, namespace string, obj client.Object) func(ctx context.Context) bool {
 	return func(ctx context.Context) bool {
-		err := k8sClient.Get(ctx, client.ObjectKey{Name: ns, Namespace: name}, obj)
+		err := k8sClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, obj)
 
 		return err == nil
 	}
diff --git a/controllers/dscinitialization/suite_test.go b/controllers/dscinitialization/suite_test.go
index f3ef428f878..985618bacf8 100644
--- a/controllers/dscinitialization/suite_test.go
+++ b/controllers/dscinitialization/suite_test.go
@@ -22,6 +22,7 @@ import (
 	"testing"
 	"time"
 
+	configv1 "github.com/openshift/api/config/v1"
 	routev1 "github.com/openshift/api/route/v1"
 	userv1 "github.com/openshift/api/user/v1"
 	ofapi "github.com/operator-framework/api/pkg/operators/v1alpha1"
@@ -116,6 +117,7 @@ var _ = BeforeSuite(func() {
 	utilruntime.Must(routev1.Install(testScheme))
 	utilruntime.Must(userv1.Install(testScheme))
 	utilruntime.Must(monitoringv1.AddToScheme(testScheme))
+	utilruntime.Must(configv1.Install(testScheme))
 	// +kubebuilder:scaffold:scheme
 
 	k8sClient, err = client.New(cfg, client.Options{Scheme: testScheme})
diff --git a/main.go b/main.go
index 62c31edec0b..8ac8821c01b 100644
--- a/main.go
+++ b/main.go
@@ -25,6 +25,7 @@ import (
 	addonv1alpha1 "github.com/openshift/addon-operator/apis/addons/v1alpha1"
 	ocappsv1 "github.com/openshift/api/apps/v1" //nolint:importas //reason: conflicts with appsv1 "k8s.io/api/apps/v1"
 	buildv1 "github.com/openshift/api/build/v1"
+	configv1 "github.com/openshift/api/config/v1"
 	imagev1 "github.com/openshift/api/image/v1"
 	oauthv1 "github.com/openshift/api/oauth/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
@@ -198,6 +199,10 @@ func main() { //nolint:funlen,maintidx
 			&operatorv1.IngressController{}: {
 				Field: fields.Set{"metadata.name": "default"}.AsSelector(),
 			},
+			// For authentication CR "cluster"
+			&configv1.Authentication{}: {
+				Field: fields.Set{"metadata.name": "cluster"}.AsSelector(),
+			},
 			// for prometheus and black-box deployment and ones we owns
 			&appsv1.Deployment{}: {Namespaces: deploymentCache},
 		},
diff --git a/pkg/cluster/cluster_config.go b/pkg/cluster/cluster_config.go
index 8a09e3450cc..5efc7f99f0f 100644
--- a/pkg/cluster/cluster_config.go
+++ b/pkg/cluster/cluster_config.go
@@ -9,10 +9,12 @@ import (
 
 	"github.com/blang/semver/v4"
 	"github.com/go-logr/logr"
+	configv1 "github.com/openshift/api/config/v1"
 	"github.com/operator-framework/api/pkg/lib/version"
 	ofapiv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
 	k8serr "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -21,8 +23,6 @@ import (
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk"
 )
 
-// +kubebuilder:rbac:groups="config.openshift.io",resources=ingresses,verbs=get
-
 type Platform string
 
 // Release includes information on operator version and platform
@@ -215,3 +215,19 @@ func getRelease(ctx context.Context, cli client.Client) (Release, error) {
 	initRelease.Version = csv.Spec.Version
 	return initRelease, nil
 }
+
+// IsDefaultAuthMethod returns true if the default authentication method is IntegratedOAuth or empty.
+// This will give indication that Operator should create userGroups or not in the cluster.
+func IsDefaultAuthMethod(ctx context.Context, cli client.Client) (bool, error) {
+	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")
+		}
+		return false, err
+	}
+
+	// for now, HPC support "" "None" "IntegratedOAuth"(default) "OIDC"
+	// other offering support "" "None" "IntegratedOAuth"(default)
+	return (authenticationobj.Spec.Type != "OIDC"), nil
+}