Skip to content

Commit

Permalink
feat: Operator disable create usergroup if detect users enabled exter…
Browse files Browse the repository at this point in the history
…nal 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 <[email protected]>
  • Loading branch information
zdtsw committed Oct 11, 2024
1 parent a918fed commit 2d4f726
Show file tree
Hide file tree
Showing 9 changed files with 228 additions and 28 deletions.
21 changes: 1 addition & 20 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ spec:
- apiGroups:
- config.openshift.io
resources:
- authentications
- clusterversions
verbs:
- get
Expand Down
175 changes: 175 additions & 0 deletions config/crd/external/config.openshift.io_authentications.yaml
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ rules:
- apiGroups:
- config.openshift.io
resources:
- authentications
- clusterversions
verbs:
- get
Expand Down
17 changes: 13 additions & 4 deletions controllers/dscinitialization/dscinitialization_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package dscinitialization

import (
"context"
"os"
"path/filepath"
"reflect"

Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
14 changes: 12 additions & 2 deletions controllers/dscinitialization/dscinitialization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 2 additions & 0 deletions controllers/dscinitialization/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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})
Expand Down
5 changes: 5 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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},
},
Expand Down
Loading

0 comments on commit 2d4f726

Please sign in to comment.