fix(oauth-dashboard): update APIversion when patch oauth-client (#136)

add more comment and error message

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

components/component.go

@@ -80,7 +80,7 @@ type ManifestsConfig struct {
 }
 
 type ComponentInterface interface {
-	ReconcileComponent(ctx context.Context, cli client.Client, resConf *rest.Config, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, currentComponentStatus bool) error
+	ReconcileComponent(ctx context.Context, cli client.Client, resConf *rest.Config, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, currentComponentExist bool) error
 	Cleanup(cli client.Client, DSCISpec *dsciv1.DSCInitializationSpec) error
 	GetComponentName() string
 	GetManagementState() operatorv1.ManagementState

components/dashboard/dashboard.go

@@ -84,7 +84,7 @@ func (d *Dashboard) ReconcileComponent(ctx context.Context,
 	resConf *rest.Config,
 	owner metav1.Object,
 	dscispec *dsciv1.DSCInitializationSpec,
-	currentComponentStatus bool,
+	currentComponentExist bool,
 ) error {
 	var imageParamMap = map[string]string{
 		"odh-dashboard-image": "RELATED_IMAGE_ODH_DASHBOARD_IMAGE",
@@ -99,9 +99,11 @@ func (d *Dashboard) ReconcileComponent(ctx context.Context,
 
 	// Update Default rolebinding
 	if enabled {
-		if err := d.cleanOauthClientSecrets(cli, dscispec, currentComponentStatus); err != nil {
+		// cleanup OAuth client related secret and CR if dashboard is in 'installed falas' status
+		if err := d.cleanOauthClient(cli, dscispec, currentComponentExist); err != nil {
 			return err
 		}
+
 		// Download manifests and update paths
 		if err := d.OverrideManifests(string(platform)); err != nil {
 			return err
@@ -276,27 +278,27 @@ func (d *Dashboard) deployConsoleLink(cli client.Client, owner metav1.Object, na
 	return nil
 }
 
-func (d *Dashboard) cleanOauthClientSecrets(cli client.Client, dscispec *dsciv1.DSCInitializationSpec, currentComponentStatus bool) error {
+func (d *Dashboard) cleanOauthClient(cli client.Client, dscispec *dsciv1.DSCInitializationSpec, currentComponentExist bool) error {
 	// Remove previous oauth-client secrets
 	// Check if component is going from state of `Not Installed --> Installed`
 	// Assumption: Component is currently set to enabled
-	if !currentComponentStatus {
+	name := "dashboard-oauth-client"
+	if !currentComponentExist {
+		fmt.Println("Cleanup any left secret")
 		// Delete client secrets from previous installation
 		oauthClientSecret := &v1.Secret{}
 		err := cli.Get(context.TODO(), client.ObjectKey{
 			Namespace: dscispec.ApplicationsNamespace,
-			Name:      "dashboard-oauth-client",
+			Name:      name,
 		}, oauthClientSecret)
 		if err != nil {
 			if !apierrs.IsNotFound(err) {
-				return err
-			}
-		} else {
-			err := cli.Delete(context.TODO(), oauthClientSecret)
-			if err != nil {
-				return fmt.Errorf("error deleting oauth client secret: %v", err)
+				return fmt.Errorf("error getting secret %s: %w", name, err)
 			}
 		}
+		if err := cli.Delete(context.TODO(), oauthClientSecret); err != nil {
+			return fmt.Errorf("error deleting secret %s in namespace %s : %w", name, dscispec.ApplicationsNamespace, err)
+		}
 	}
 	return nil
 }

controllers/secretgenerator/secretgenerator_controller.go

@@ -75,6 +75,8 @@ func (r *SecretGeneratorReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		GenericFunc: func(e event.GenericEvent) bool {
 			return false
 		},
+		// this only watch for secret deletion if has with annotation
+		// e.g dashboard-oauth-client but not dashboard-oauth-client-generated
 		DeleteFunc: func(e event.DeleteEvent) bool {
 			if _, found := e.Object.GetAnnotations()[SECRET_NAME_ANNOTATION]; found {
 				return true
@@ -137,7 +139,7 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
 
 			secret, err := NewSecretFrom(foundSecret.GetAnnotations())
 			if err != nil {
-				secGenLog.Error(err, "error creating secret")
+				secGenLog.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace)
 				return ctrl.Result{}, err
 			}
 
@@ -149,15 +151,18 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
 			if err != nil {
 				return ctrl.Result{}, err
 			}
+			secGenLog.Info("Done generating secret in namespace",
+				"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)
+			// check if annotation oauth-client-route exists
 			if secret.OAuthClientRoute != "" {
 				// Get OauthClient Route
 				oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, request.Namespace)
 				if err != nil {
-					secGenLog.Error(err, "Unable to retrieve route", "route-name", secret.OAuthClientRoute)
+					secGenLog.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute)
 					return ctrl.Result{}, err
 				}
 				// Generate OAuthClient for the generated secret
-				secGenLog.Info("Generating an oauth client resource for route", "route-name", oauthClientRoute.Name)
+				secGenLog.Info("Generating an OAuthClient CR for route", "route-name", oauthClientRoute.Name)
 				err = r.createOAuthClient(ctx, foundSecret.Name, secret.Value, oauthClientRoute.Spec.Host)
 				if err != nil {
 					secGenLog.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name",
@@ -206,7 +211,7 @@ func (r *SecretGeneratorReconciler) createOAuthClient(ctx context.Context, name
 	oauthClient := &ocv1.OAuthClient{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "OAuthClient",
-			APIVersion: "v1",
+			APIVersion: "oauth.openshift.io/v1",
 		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name: name,
@@ -222,12 +227,11 @@ func (r *SecretGeneratorReconciler) createOAuthClient(ctx context.Context, name
 			secGenLog.Info("OAuth client resource already exists, patch it", "name", oauthClient.Name)
 			data, err := json.Marshal(oauthClient)
 			if err != nil {
-				return fmt.Errorf("failed to get DataScienceCluster custom resource data: %v", err)
+				return fmt.Errorf("failed to get DataScienceCluster CR data: %w", err)
 			}
-			err = r.Client.Patch(context.TODO(), oauthClient, client.RawPatch(types.ApplyPatchType, data),
-				client.ForceOwnership, client.FieldOwner("opendatahub-operator"))
-			if err != nil {
-				return err
+			if err = r.Client.Patch(context.TODO(), oauthClient, client.RawPatch(types.ApplyPatchType, data),
+				client.ForceOwnership, client.FieldOwner("rhods-operator")); err != nil {
+				return fmt.Errorf("failed to patch existing OAuthClient CR: %w", err)
 			}
 			return nil
 		}
@@ -248,9 +252,8 @@ func (r *SecretGeneratorReconciler) deleteOAuthClient(ctx context.Context, secre
 		return err
 	}
 
-	err = r.Client.Delete(ctx, oauthClient)
-	if err != nil {
-		return fmt.Errorf("error deleting OAuthClient %v", oauthClient.Name)
+	if err = r.Client.Delete(ctx, oauthClient); err != nil {
+		return fmt.Errorf("error deleting OAuthClient %s: %w", oauthClient.Name, err)
 	}
 
 	return nil

pkg/upgrade/upgrade.go

@@ -245,7 +245,7 @@ func CreateDefaultDSCI(cli client.Client, platform deploy.Platform, appNamespace
 			}
 			existingDSCI := &instances.Items[0]
 			err = cli.Patch(context.TODO(), existingDSCI, client.RawPatch(types.ApplyPatchType, data),
-				client.ForceOwnership, client.FieldOwner("opendatahub-operator"))
+				client.ForceOwnership, client.FieldOwner("rhods-operator"))
 			if err != nil {
 				return err
 			}