From 277cc8735ebd9d858d7af9c956c64ee469331a7c Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Mon, 7 Oct 2024 09:54:59 +0200 Subject: [PATCH 1/3] feat: Operator disable create usergroup if detect users enabled external 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 --- ...atahub-operator.clusterserviceversion.yaml | 1 + .../config.openshift.io_authentications.yaml | 175 ++++++++++++++++++ config/rbac/role.yaml | 1 + .../dscinitialization_controller.go | 31 +++- .../dscinitialization_test.go | 14 +- controllers/dscinitialization/suite_test.go | 2 + main.go | 5 + pkg/cluster/cluster_config.go | 21 ++- 8 files changed, 240 insertions(+), 10 deletions(-) create mode 100644 config/crd/external/config.openshift.io_authentications.yaml diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml index 1519a9ef8ca..7ec954e1a03 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 37991131cb5..187cabefd25 100644 --- a/controllers/dscinitialization/dscinitialization_controller.go +++ b/controllers/dscinitialization/dscinitialization_controller.go @@ -29,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" @@ -73,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 @@ -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 + return ctrl.Result{}, err + } + } + switch platform { case cluster.SelfManagedRhods: - err := r.createUserGroup(ctx, instance, "rhods-admins") - if err != nil { - return reconcile.Result{}, err + // Check if user opted for disabling creating user groups + if !createUsergroup { + log.Info("DSCI disabled usergroup creation") + } else { + err := r.createUserGroup(ctx, instance, "rhods-admins") + if err != nil { + return reconcile.Result{}, err + } } if instance.Spec.Monitoring.ManagementState == operatorv1.Managed { log.Info("Monitoring enabled, won't apply changes", "cluster", "Self-Managed RHODS Mode") @@ -246,9 +260,14 @@ func (r *DSCInitializationReconciler) Reconcile(ctx context.Context, req ctrl.Re } } default: - err := r.createUserGroup(ctx, instance, "odh-admins") - if err != nil { - return reconcile.Result{}, err + // Check if user opted for disabling creating user groups + if !createUsergroup { + log.Info("DSCI disabled usergroup creation") + } else { + err := r.createUserGroup(ctx, instance, "odh-admins") + if err != nil { + return reconcile.Result{}, err + } } if instance.Spec.Monitoring.ManagementState == operatorv1.Managed { log.Info("Monitoring enabled, won't apply changes", "cluster", "ODH Mode") 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 c7d38cfb1c3..207da12e001 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" @@ -201,6 +202,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 442878baf2f..15cdd79cce9 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 @@ -226,3 +226,20 @@ 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) + // 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 +} From 2d7737bc1e00bade807a9673ab413f0d1acbf86c Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Mon, 14 Oct 2024 10:39:04 +0200 Subject: [PATCH 2/3] update: review comments Signed-off-by: Wen Zhou --- .../dscinitialization/dscinitialization_controller.go | 6 ++---- pkg/cluster/cluster_config.go | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/controllers/dscinitialization/dscinitialization_controller.go b/controllers/dscinitialization/dscinitialization_controller.go index 187cabefd25..845418ac843 100644 --- a/controllers/dscinitialization/dscinitialization_controller.go +++ b/controllers/dscinitialization/dscinitialization_controller.go @@ -215,10 +215,8 @@ 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 - return ctrl.Result{}, err - } + if err != nil && !k8serr.IsNotFound(err) { // only keep reconcile if real error but not missing CRD or missing CR + return ctrl.Result{}, err } switch platform { diff --git a/pkg/cluster/cluster_config.go b/pkg/cluster/cluster_config.go index 15cdd79cce9..d2ca3bd2ff9 100644 --- a/pkg/cluster/cluster_config.go +++ b/pkg/cluster/cluster_config.go @@ -241,5 +241,5 @@ func IsDefaultAuthMethod(ctx context.Context, cli client.Client) (bool, error) { // 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 + return authenticationobj.Spec.Type == configv1.AuthenticationTypeIntegratedOAuth || authenticationobj.Spec.Type == "", nil } From 61f43f893fb6ef255bc3fffe635516a28bcc09b7 Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Mon, 14 Oct 2024 16:38:31 +0200 Subject: [PATCH 3/3] update: review comments use const Signed-off-by: Wen Zhou --- main.go | 2 +- pkg/cluster/cluster_config.go | 4 ++-- pkg/cluster/const.go | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 207da12e001..6eb6cffe420 100644 --- a/main.go +++ b/main.go @@ -204,7 +204,7 @@ func main() { //nolint:funlen,maintidx }, // For authentication CR "cluster" &configv1.Authentication{}: { - Field: fields.Set{"metadata.name": "cluster"}.AsSelector(), + Field: fields.Set{"metadata.name": cluster.ClusterAuthenticationObj}.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 d2ca3bd2ff9..e0e31fa199c 100644 --- a/pkg/cluster/cluster_config.go +++ b/pkg/cluster/cluster_config.go @@ -231,9 +231,9 @@ func getRelease(ctx context.Context, cli client.Client) (Release, error) { // 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 err := cli.Get(ctx, client.ObjectKey{Name: ClusterAuthenticationObj, 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, k8serr.NewNotFound(configv1.Resource("authentications"), ClusterAuthenticationObj) } return false, err } diff --git a/pkg/cluster/const.go b/pkg/cluster/const.go index 5e218430db7..de5a27de29d 100644 --- a/pkg/cluster/const.go +++ b/pkg/cluster/const.go @@ -12,4 +12,7 @@ const ( // DefaultNotebooksNamespace defines default namespace for notebooks. DefaultNotebooksNamespace = "rhods-notebooks" + + // Default cluster-scope Authentication CR name. + ClusterAuthenticationObj = "cluster" )