From 128f46f736257a3985f3b120d31ce7986657bb5c Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Tue, 23 Aug 2022 17:04:53 -0400 Subject: [PATCH 01/14] Add WorkspaceWithConfig struct, use it in controller Use config from workspaceWithConfig instead of global config (almost) everywhere Signed-off-by: Andrew Obuchowicz --- .../workspace/devworkspace_controller.go | 154 +++++++++--------- controllers/workspace/finalize.go | 16 +- controllers/workspace/status.go | 14 +- pkg/common/types.go | 27 +++ pkg/config/common_test.go | 2 +- pkg/config/sync.go | 43 ++--- pkg/config/sync_test.go | 24 +-- pkg/library/container/container.go | 6 +- pkg/library/container/container_test.go | 2 +- pkg/library/container/conversion.go | 4 +- pkg/library/defaults/helper.go | 19 +-- pkg/library/env/workspaceenv.go | 7 +- pkg/library/projects/clone.go | 10 +- pkg/library/status/check.go | 22 +-- pkg/provision/storage/asyncStorage.go | 41 ++--- .../storage/asyncstorage/deployment.go | 9 +- pkg/provision/storage/cleanup.go | 34 ++-- pkg/provision/storage/commonStorage.go | 24 +-- pkg/provision/storage/commonStorage_test.go | 11 +- pkg/provision/storage/ephemeralStorage.go | 12 +- .../storage/ephemeralStorage_test.go | 5 +- pkg/provision/storage/perWorkspaceStorage.go | 23 ++- .../storage/perWorkspaceStorage_test.go | 3 +- pkg/provision/storage/provisioner.go | 8 +- pkg/provision/storage/shared.go | 10 +- pkg/provision/workspace/deployment.go | 18 +- 26 files changed, 292 insertions(+), 256 deletions(-) create mode 100644 pkg/common/types.go diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index eab63771d..96c3b1f3a 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -109,8 +109,9 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Fetch the Workspace instance - workspace := &dw.DevWorkspace{} - err = r.Get(ctx, req.NamespacedName, workspace) + workspaceWithConfig := &common.DevWorkspaceWithConfig{} + workspaceWithConfig.Config = *config.InternalConfig + err = r.Get(ctx, req.NamespacedName, workspaceWithConfig) if err != nil { if k8sErrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. @@ -121,35 +122,35 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Error reading the object - requeue the request. return reconcile.Result{}, err } - reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspace.Status.DevWorkspaceId) + reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspaceWithConfig.Status.DevWorkspaceId) reqLogger.Info("Reconciling Workspace") // Check if the DevWorkspaceRouting instance is marked to be deleted, which is // indicated by the deletion timestamp being set. - if workspace.GetDeletionTimestamp() != nil { + if workspaceWithConfig.GetDeletionTimestamp() != nil { reqLogger.Info("Finalizing DevWorkspace") - return r.finalize(ctx, reqLogger, workspace) + return r.finalize(ctx, reqLogger, workspaceWithConfig) } // Ensure workspaceID is set. - if workspace.Status.DevWorkspaceId == "" { - workspaceId, err := r.getWorkspaceId(ctx, workspace) + if workspaceWithConfig.Status.DevWorkspaceId == "" { + workspaceId, err := r.getWorkspaceId(ctx, &workspaceWithConfig.DevWorkspace) if err != nil { - workspace.Status.Phase = dw.DevWorkspaceStatusFailed - workspace.Status.Message = fmt.Sprintf("Failed to set DevWorkspace ID: %s", err.Error()) - return reconcile.Result{}, r.Status().Update(ctx, workspace) + workspaceWithConfig.Status.Phase = dw.DevWorkspaceStatusFailed + workspaceWithConfig.Status.Message = fmt.Sprintf("Failed to set DevWorkspace ID: %s", err.Error()) + return reconcile.Result{}, r.Status().Update(ctx, workspaceWithConfig) } - workspace.Status.DevWorkspaceId = workspaceId - err = r.Status().Update(ctx, workspace) + workspaceWithConfig.Status.DevWorkspaceId = workspaceId + err = r.Status().Update(ctx, workspaceWithConfig) return reconcile.Result{Requeue: true}, err } // Stop failed workspaces - if workspace.Status.Phase == devworkspacePhaseFailing && workspace.Spec.Started { + if workspaceWithConfig.Status.Phase == devworkspacePhaseFailing && workspaceWithConfig.Spec.Started { // If debug annotation is present, leave the deployment in place to let users // view logs. - if workspace.Annotations[constants.DevWorkspaceDebugStartAnnotation] == "true" { - if isTimeout, err := checkForFailingTimeout(workspace); err != nil { + if workspaceWithConfig.Annotations[constants.DevWorkspaceDebugStartAnnotation] == "true" { + if isTimeout, err := checkForFailingTimeout(workspaceWithConfig); err != nil { return reconcile.Result{}, err } else if !isTimeout { return reconcile.Result{}, nil @@ -157,7 +158,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } patch := []byte(`{"spec":{"started": false}}`) - err := r.Client.Patch(context.Background(), workspace, client.RawPatch(types.MergePatchType, patch)) + err := r.Client.Patch(context.Background(), workspaceWithConfig, client.RawPatch(types.MergePatchType, patch)) if err != nil { return reconcile.Result{}, err } @@ -167,20 +168,20 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Handle stopped workspaces - if !workspace.Spec.Started { - timing.ClearAnnotations(workspace) - r.removeStartedAtFromCluster(ctx, workspace, reqLogger) - r.syncTimingToCluster(ctx, workspace, map[string]string{}, reqLogger) - return r.stopWorkspace(ctx, workspace, reqLogger) + if !workspaceWithConfig.Spec.Started { + timing.ClearAnnotations(&workspaceWithConfig.DevWorkspace) + r.removeStartedAtFromCluster(ctx, &workspaceWithConfig.DevWorkspace, reqLogger) + r.syncTimingToCluster(ctx, &workspaceWithConfig.DevWorkspace, map[string]string{}, reqLogger) + return r.stopWorkspace(ctx, workspaceWithConfig, reqLogger) } // If this is the first reconcile for a starting workspace, mark it as starting now. This is done outside the regular // updateWorkspaceStatus function to ensure it gets set immediately - if workspace.Status.Phase != dw.DevWorkspaceStatusStarting && workspace.Status.Phase != dw.DevWorkspaceStatusRunning { + if workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusStarting && workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusRunning { // Set 'Started' condition as early as possible to get accurate timing metrics - workspace.Status.Phase = dw.DevWorkspaceStatusStarting - workspace.Status.Message = "Initializing DevWorkspace" - workspace.Status.Conditions = []dw.DevWorkspaceCondition{ + workspaceWithConfig.Status.Phase = dw.DevWorkspaceStatusStarting + workspaceWithConfig.Status.Message = "Initializing DevWorkspace" + workspaceWithConfig.Status.Conditions = []dw.DevWorkspaceCondition{ { Type: conditions.Started, Status: corev1.ConditionTrue, @@ -188,9 +189,9 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request Message: "DevWorkspace is starting", }, } - err = r.Status().Update(ctx, workspace) + err = r.Status().Update(ctx, workspaceWithConfig) if err == nil { - metrics.WorkspaceStarted(workspace, reqLogger) + metrics.WorkspaceStarted(&workspaceWithConfig.DevWorkspace, reqLogger) } return reconcile.Result{}, err } @@ -198,7 +199,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Prepare handling workspace status and condition reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting} reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting") - clusterWorkspace := workspace.DeepCopy() + clusterWorkspace := workspaceWithConfig.DevWorkspace.DeepCopy() timingInfo := map[string]string{} timing.SetTime(timingInfo, timing.DevWorkspaceStarted) @@ -208,22 +209,22 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Don't accidentally suppress errors by overwriting here; only check for timeout when no error // encountered in main reconcile loop. if err == nil { - if timeoutErr := checkForStartTimeout(clusterWorkspace); timeoutErr != nil { - reconcileResult, err = r.failWorkspace(workspace, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + if timeoutErr := checkForStartTimeout(clusterWorkspace, workspaceWithConfig.Config); timeoutErr != nil { + reconcileResult, err = r.failWorkspace(&workspaceWithConfig.DevWorkspace, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } } if reconcileStatus.phase == dw.DevWorkspaceStatusRunning { - metrics.WorkspaceRunning(clusterWorkspace, reqLogger) + metrics.WorkspaceRunning(&workspaceWithConfig.DevWorkspace, reqLogger) r.syncStartedAtToCluster(ctx, clusterWorkspace, reqLogger) } return r.updateWorkspaceStatus(clusterWorkspace, reqLogger, &reconcileStatus, reconcileResult, err) }() - if workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { + if workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { msg, err := r.validateCreatorLabel(clusterWorkspace) if err != nil { - return r.failWorkspace(workspace, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) } } @@ -235,74 +236,74 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request timing.SetTime(timingInfo, timing.ComponentsCreated) flattenHelpers := flatten.ResolverTools{ - WorkspaceNamespace: workspace.Namespace, + WorkspaceNamespace: workspaceWithConfig.Namespace, Context: ctx, K8sClient: r.Client, HttpClient: httpClient, } - if wsDefaults.NeedsDefaultTemplate(workspace) { - wsDefaults.ApplyDefaultTemplate(workspace) + if wsDefaults.NeedsDefaultTemplate(workspaceWithConfig) { + wsDefaults.ApplyDefaultTemplate(workspaceWithConfig) } - flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspace.Spec.Template, flattenHelpers) + flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) if err != nil { - return r.failWorkspace(workspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } if warnings != nil { reconcileStatus.setConditionTrue(conditions.DevWorkspaceWarning, flatten.FormatVariablesWarning(warnings)) } else { reconcileStatus.setConditionFalse(conditions.DevWorkspaceWarning, "No warnings in processing DevWorkspace") } - workspace.Spec.Template = *flattenedWorkspace + workspaceWithConfig.Spec.Template = *flattenedWorkspace reconcileStatus.setConditionTrue(conditions.DevWorkspaceResolved, "Resolved plugins and parents from DevWorkspace") // Verify that the devworkspace components are valid after flattening - components := workspace.Spec.Template.Components + components := workspaceWithConfig.Spec.Template.Components if components != nil { eventErrors := devfilevalidation.ValidateComponents(components) if eventErrors != nil { - return r.failWorkspace(workspace, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } } - storageProvisioner, err := storage.GetProvisioner(workspace) + storageProvisioner, err := storage.GetProvisioner(&workspaceWithConfig.DevWorkspace) if err != nil { - return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Set finalizer on DevWorkspace if necessary // Note: we need to check the flattened workspace to see if a finalizer is needed, as plugins could require storage - if storageProvisioner.NeedsStorage(&workspace.Spec.Template) { + if storageProvisioner.NeedsStorage(&workspaceWithConfig.Spec.Template) { coputil.AddFinalizer(clusterWorkspace, constants.StorageCleanupFinalizer) if err := r.Update(ctx, clusterWorkspace); err != nil { return reconcile.Result{}, err } } - devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(&workspace.Spec.Template) + devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(&workspaceWithConfig.Spec.Template, workspaceWithConfig.Config) if err != nil { - return r.failWorkspace(workspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add common environment variables and env vars defined via workspaceEnv attribute - if err := env.AddCommonEnvironmentVariables(devfilePodAdditions, clusterWorkspace, &workspace.Spec.Template); err != nil { - return r.failWorkspace(workspace, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + if err := env.AddCommonEnvironmentVariables(devfilePodAdditions, clusterWorkspace, &workspaceWithConfig.Spec.Template, workspaceWithConfig.Config); err != nil { + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add init container to clone projects - if projectClone, err := projects.GetProjectCloneInitContainer(&workspace.Spec.Template); err != nil { - return r.failWorkspace(workspace, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + if projectClone, err := projects.GetProjectCloneInitContainer(workspaceWithConfig); err != nil { + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } else if projectClone != nil { devfilePodAdditions.InitContainers = append(devfilePodAdditions.InitContainers, *projectClone) } // Add automount resources into devfile containers - if err := automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspace.Namespace); err != nil { + if err := automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspaceWithConfig.Namespace); err != nil { var autoMountErr *automount.AutoMountError if errors.As(err, &autoMountErr) { if autoMountErr.IsFatal { - return r.failWorkspace(workspace, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info(autoMountErr.Error()) return reconcile.Result{Requeue: true}, nil @@ -311,7 +312,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } } - err = storageProvisioner.ProvisionStorage(devfilePodAdditions, workspace, clusterAPI) + err = storageProvisioner.ProvisionStorage(devfilePodAdditions, workspaceWithConfig, clusterAPI) if err != nil { switch storageErr := err.(type) { case *storage.NotReadyError: @@ -319,7 +320,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus.setConditionFalse(conditions.StorageReady, fmt.Sprintf("Provisioning storage: %s", storageErr.Message)) return reconcile.Result{Requeue: true, RequeueAfter: storageErr.RequeueAfter}, nil case *storage.ProvisioningError: - return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, storageErr } @@ -328,17 +329,17 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request timing.SetTime(timingInfo, timing.ComponentsReady) - rbacStatus := wsprovision.SyncRBAC(workspace, clusterAPI) + rbacStatus := wsprovision.SyncRBAC(&workspaceWithConfig.DevWorkspace, clusterAPI) if rbacStatus.Err != nil || !rbacStatus.Continue { return reconcile.Result{Requeue: true}, rbacStatus.Err } // Step two: Create routing, and wait for routing to be ready timing.SetTime(timingInfo, timing.RoutingCreated) - routingStatus := wsprovision.SyncRoutingToCluster(workspace, clusterAPI) + routingStatus := wsprovision.SyncRoutingToCluster(&workspaceWithConfig.DevWorkspace, clusterAPI) if !routingStatus.Continue { if routingStatus.FailStartup { - return r.failWorkspace(workspace, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on routing to be ready") message := "Preparing networking" @@ -363,17 +364,17 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{Requeue: true}, nil } - annotate.AddURLAttributesToEndpoints(&workspace.Spec.Template, routingStatus.ExposedEndpoints) + annotate.AddURLAttributesToEndpoints(&workspaceWithConfig.Spec.Template, routingStatus.ExposedEndpoints) // Step three: provision a configmap on the cluster to mount the flattened devfile in deployment containers - err = metadata.ProvisionWorkspaceMetadata(devfilePodAdditions, clusterWorkspace, workspace, clusterAPI) + err = metadata.ProvisionWorkspaceMetadata(devfilePodAdditions, clusterWorkspace, &workspaceWithConfig.DevWorkspace, clusterAPI) if err != nil { switch provisionErr := err.(type) { case *metadata.NotReadyError: reqLogger.Info(provisionErr.Message) return reconcile.Result{Requeue: true, RequeueAfter: provisionErr.RequeueAfter}, nil case *metadata.ProvisioningError: - return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, provisionErr } @@ -391,10 +392,10 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if routingPodAdditions != nil { saAnnotations = routingPodAdditions.ServiceAccountAnnotations } - serviceAcctStatus := wsprovision.SyncServiceAccount(workspace, saAnnotations, clusterAPI) + serviceAcctStatus := wsprovision.SyncServiceAccount(&workspaceWithConfig.DevWorkspace, saAnnotations, clusterAPI) if !serviceAcctStatus.Continue { if serviceAcctStatus.FailStartup { - return r.failWorkspace(workspace, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting for workspace ServiceAccount") reconcileStatus.setConditionFalse(dw.DevWorkspaceServiceAccountReady, "Waiting for DevWorkspace ServiceAccount") @@ -403,7 +404,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } return reconcile.Result{Requeue: serviceAcctStatus.Requeue}, serviceAcctStatus.Err } - if wsprovision.NeedsServiceAccountFinalizer(&workspace.Spec.Template) { + if wsprovision.NeedsServiceAccountFinalizer(&workspaceWithConfig.Spec.Template) { coputil.AddFinalizer(clusterWorkspace, constants.ServiceAccountCleanupFinalizer) if err := r.Update(ctx, clusterWorkspace); err != nil { return reconcile.Result{}, err @@ -413,7 +414,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request serviceAcctName := serviceAcctStatus.ServiceAccountName reconcileStatus.setConditionTrue(dw.DevWorkspaceServiceAccountReady, "DevWorkspace serviceaccount ready") - pullSecretStatus := wsprovision.PullSecrets(clusterAPI, serviceAcctName, workspace.GetNamespace()) + pullSecretStatus := wsprovision.PullSecrets(clusterAPI, serviceAcctName, workspaceWithConfig.GetNamespace()) if !pullSecretStatus.Continue { reconcileStatus.setConditionFalse(conditions.PullSecretsReady, "Waiting for DevWorkspace pull secrets") if !pullSecretStatus.Requeue && pullSecretStatus.Err == nil { @@ -426,11 +427,11 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Step six: Create deployment and wait for it to be ready timing.SetTime(timingInfo, timing.DeploymentCreated) - deploymentStatus := wsprovision.SyncDeploymentToCluster(workspace, allPodAdditions, serviceAcctName, clusterAPI) + deploymentStatus := wsprovision.SyncDeploymentToCluster(&workspaceWithConfig.DevWorkspace, allPodAdditions, serviceAcctName, clusterAPI, &workspaceWithConfig.Config) if !deploymentStatus.Continue { if deploymentStatus.FailStartup { failureReason := metrics.DetermineProvisioningFailureReason(deploymentStatus) - return r.failWorkspace(workspace, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) + return r.failWorkspace(&workspaceWithConfig.DevWorkspace, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on deployment to be ready") reconcileStatus.setConditionFalse(conditions.DeploymentReady, "Waiting for workspace deployment") @@ -457,7 +458,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{}, nil } -func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *dw.DevWorkspace, logger logr.Logger) (reconcile.Result, error) { +func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *common.DevWorkspaceWithConfig, logger logr.Logger) (reconcile.Result, error) { status := currentStatus{phase: dw.DevWorkspaceStatusStopping} if workspace.Status.Phase == devworkspacePhaseFailing || workspace.Status.Phase == dw.DevWorkspaceStatusFailed { status.phase = workspace.Status.Phase @@ -485,14 +486,14 @@ func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *d if stoppedBy, ok := workspace.Annotations[constants.DevWorkspaceStopReasonAnnotation]; ok { logger.Info("Workspace stopped with reason", "stopped-by", stoppedBy) } - return r.updateWorkspaceStatus(workspace, logger, &status, reconcile.Result{}, nil) + return r.updateWorkspaceStatus(&workspace.DevWorkspace, logger, &status, reconcile.Result{}, nil) } -func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspace *dw.DevWorkspace, logger logr.Logger) (stopped bool, err error) { +func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig *common.DevWorkspaceWithConfig, logger logr.Logger) (stopped bool, err error) { workspaceDeployment := &appsv1.Deployment{} namespaceName := types.NamespacedName{ - Name: common.DeploymentName(workspace.Status.DevWorkspaceId), - Namespace: workspace.Namespace, + Name: common.DeploymentName(workspaceWithConfig.Status.DevWorkspaceId), + Namespace: workspaceWithConfig.Namespace, } err = r.Get(ctx, namespaceName, workspaceDeployment) if err != nil { @@ -505,8 +506,8 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspace *dw.DevWo // Update DevWorkspaceRouting to have `devworkspace-started` annotation "false" routing := &controllerv1alpha1.DevWorkspaceRouting{} routingRef := types.NamespacedName{ - Name: common.DevWorkspaceRoutingName(workspace.Status.DevWorkspaceId), - Namespace: workspace.Namespace, + Name: common.DevWorkspaceRoutingName(workspaceWithConfig.Status.DevWorkspaceId), + Namespace: workspaceWithConfig.Namespace, } err = r.Get(ctx, routingRef, routing) if err != nil { @@ -525,11 +526,11 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspace *dw.DevWo } // CleanupOnStop should never be nil as a default is always set - if config.Workspace.CleanupOnStop == nil || !*config.Workspace.CleanupOnStop { + if workspaceWithConfig.Config.Workspace.CleanupOnStop == nil || !*workspaceWithConfig.Config.Workspace.CleanupOnStop { replicas := workspaceDeployment.Spec.Replicas if replicas == nil || *replicas > 0 { logger.Info("Stopping workspace") - err = wsprovision.ScaleDeploymentToZero(ctx, workspace, r.Client) + err = wsprovision.ScaleDeploymentToZero(ctx, &workspaceWithConfig.DevWorkspace, r.Client) if err != nil && !k8sErrors.IsConflict(err) { return false, err } @@ -539,7 +540,7 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspace *dw.DevWo return workspaceDeployment.Status.Replicas == 0, nil } else { logger.Info("Cleaning up workspace-owned objects") - requeue, err := r.deleteWorkspaceOwnedObjects(ctx, workspace) + requeue, err := r.deleteWorkspaceOwnedObjects(ctx, &workspaceWithConfig.DevWorkspace) return !requeue, err } } @@ -676,6 +677,7 @@ func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Req } } + // TODO: Address this usage of global config? // Otherwise, check if common PVC is deleted to make sure all DevWorkspaces see it happen if obj.GetName() != config.Workspace.PVCName || obj.GetDeletionTimestamp() == nil { // We're looking for a deleted common PVC diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index 201c176f1..15a7f280b 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -18,10 +18,10 @@ package controllers import ( "context" + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/conditions" "github.com/devfile/devworkspace-operator/pkg/constants" - - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/provision/sync" "github.com/go-logr/logr" @@ -46,7 +46,7 @@ func (r *DevWorkspaceReconciler) workspaceNeedsFinalize(workspace *dw.DevWorkspa return false } -func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace) (finalizeResult reconcile.Result, finalizeErr error) { +func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, workspace *common.DevWorkspaceWithConfig) (finalizeResult reconcile.Result, finalizeErr error) { // Tracked state for the finalize process; we update the workspace status in a deferred function (and pass the // named return value for finalize()) to update the workspace's status with whatever is in finalizeStatus // when this function returns. @@ -60,7 +60,7 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, // client.Update() call that removes the last finalizer. return finalizeResult, finalizeErr } - return r.updateWorkspaceStatus(workspace, log, finalizeStatus, finalizeResult, finalizeErr) + return r.updateWorkspaceStatus(&workspace.DevWorkspace, log, finalizeStatus, finalizeResult, finalizeErr) }() for _, finalizer := range workspace.Finalizers { @@ -68,15 +68,15 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, case constants.StorageCleanupFinalizer: return r.finalizeStorage(ctx, log, workspace, finalizeStatus) case constants.ServiceAccountCleanupFinalizer: - return r.finalizeServiceAccount(ctx, log, workspace, finalizeStatus) + return r.finalizeServiceAccount(ctx, log, &workspace.DevWorkspace, finalizeStatus) } } return reconcile.Result{}, nil } -func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace, finalizeStatus *currentStatus) (reconcile.Result, error) { +func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.Logger, workspace *common.DevWorkspaceWithConfig, finalizeStatus *currentStatus) (reconcile.Result, error) { // Need to make sure Deployment is cleaned up before starting job to avoid mounting issues for RWO PVCs - wait, err := wsprovision.DeleteWorkspaceDeployment(ctx, workspace, r.Client) + wait, err := wsprovision.DeleteWorkspaceDeployment(ctx, &workspace.DevWorkspace, r.Client) if err != nil { return reconcile.Result{}, err } @@ -94,7 +94,7 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L return reconcile.Result{}, r.Update(ctx, workspace) } - storageProvisioner, err := storage.GetProvisioner(workspace) + storageProvisioner, err := storage.GetProvisioner(&workspace.DevWorkspace) if err != nil { log.Error(err, "Failed to clean up DevWorkspace storage") finalizeStatus.phase = dw.DevWorkspaceStatusError diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 2eb36ec89..7544712ae 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -23,8 +23,8 @@ import ( "time" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/provision/sync" - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" @@ -33,9 +33,9 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/controllers/workspace/metrics" "github.com/devfile/devworkspace-operator/pkg/conditions" - "github.com/devfile/devworkspace-operator/pkg/config" ) const ( @@ -241,7 +241,7 @@ func updateMetricsForPhase(workspace *dw.DevWorkspace, oldPhase, newPhase dw.Dev // startup timeout. This is determined by checking to see if the last condition transition time is more // than [timeout] duration ago. Workspaces that are not in the "Starting" phase cannot timeout. Returns // an error with message when timeout is reached. -func checkForStartTimeout(workspace *dw.DevWorkspace) error { +func checkForStartTimeout(workspace *dw.DevWorkspace, config controllerv1alpha1.OperatorConfiguration) error { if workspace.Status.Phase != dw.DevWorkspaceStatusStarting { return nil } @@ -267,17 +267,17 @@ func checkForStartTimeout(workspace *dw.DevWorkspace) error { // configured progress timeout. If the workspace is not in the Failing state or does not have a DevWorkspaceFailed // condition set, returns false. Otherwise, returns true if the workspace has timed out. Returns an error if // timeout is configured with an unparsable duration. -func checkForFailingTimeout(workspace *dw.DevWorkspace) (isTimedOut bool, err error) { - if workspace.Status.Phase != devworkspacePhaseFailing { +func checkForFailingTimeout(workspaceWithConfig *common.DevWorkspaceWithConfig) (isTimedOut bool, err error) { + if workspaceWithConfig.Status.Phase != devworkspacePhaseFailing { return false, nil } - timeout, err := time.ParseDuration(config.Workspace.ProgressTimeout) + timeout, err := time.ParseDuration(workspaceWithConfig.Config.Workspace.ProgressTimeout) if err != nil { return false, fmt.Errorf("invalid duration specified for timeout: %w", err) } currTime := clock.Now() failedTime := time.Time{} - for _, condition := range workspace.Status.Conditions { + for _, condition := range workspaceWithConfig.Status.Conditions { if condition.Type == dw.DevWorkspaceFailedStart { failedTime = condition.LastTransitionTime.Time } diff --git a/pkg/common/types.go b/pkg/common/types.go new file mode 100644 index 000000000..852e9fd02 --- /dev/null +++ b/pkg/common/types.go @@ -0,0 +1,27 @@ +// Copyright (c) 2019-2022 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package common + +import ( + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" +) + +//TODO: Rename? +// TODO: Move +type DevWorkspaceWithConfig struct { + dw.DevWorkspace `json:",inline"` + Config controllerv1alpha1.OperatorConfiguration +} diff --git a/pkg/config/common_test.go b/pkg/config/common_test.go index f05ead396..d76b02409 100644 --- a/pkg/config/common_test.go +++ b/pkg/config/common_test.go @@ -54,7 +54,7 @@ func setupForTest(t *testing.T) { configNamespace = testNamespace originalDefaultConfig := defaultConfig.DeepCopy() t.Cleanup(func() { - internalConfig = nil + InternalConfig = nil defaultConfig = originalDefaultConfig }) } diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 4ee83a752..76d7f3eed 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -41,25 +41,26 @@ const ( var ( Routing *controller.RoutingConfig Workspace *controller.WorkspaceConfig - internalConfig *controller.OperatorConfiguration + InternalConfig *controller.OperatorConfiguration // TODO: Make this unexported again configMutex sync.Mutex configNamespace string log = ctrl.Log.WithName("operator-configuration") ) +// TODO: Refactor this to return the modified config, so test classes don't acces the InternalConfig func SetConfigForTesting(config *controller.OperatorConfiguration) { configMutex.Lock() defer configMutex.Unlock() - internalConfig = defaultConfig.DeepCopy() - mergeConfig(config, internalConfig) + InternalConfig = defaultConfig.DeepCopy() + mergeConfig(config, InternalConfig) updatePublicConfig() } func SetupControllerConfig(client crclient.Client) error { - if internalConfig != nil { + if InternalConfig != nil { return fmt.Errorf("internal controller configuration is already set up") } - internalConfig = &controller.OperatorConfiguration{} + InternalConfig = &controller.OperatorConfiguration{} namespace, err := infrastructure.GetNamespace() if err != nil { @@ -72,7 +73,7 @@ func SetupControllerConfig(client crclient.Client) error { return err } if config == nil { - internalConfig = defaultConfig.DeepCopy() + InternalConfig = defaultConfig.DeepCopy() } else { syncConfigFrom(config) } @@ -82,8 +83,8 @@ func SetupControllerConfig(client crclient.Client) error { return err } defaultConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix - if internalConfig.Routing.ClusterHostSuffix == "" { - internalConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix + if InternalConfig.Routing.ClusterHostSuffix == "" { + InternalConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix } clusterProxy, err := proxy.GetClusterProxyConfig(client) @@ -91,21 +92,21 @@ func SetupControllerConfig(client crclient.Client) error { return err } defaultConfig.Routing.ProxyConfig = clusterProxy - internalConfig.Routing.ProxyConfig = proxy.MergeProxyConfigs(clusterProxy, internalConfig.Routing.ProxyConfig) + InternalConfig.Routing.ProxyConfig = proxy.MergeProxyConfigs(clusterProxy, InternalConfig.Routing.ProxyConfig) updatePublicConfig() return nil } func IsSetUp() bool { - return internalConfig != nil + return InternalConfig != nil } func ExperimentalFeaturesEnabled() bool { - if internalConfig == nil || internalConfig.EnableExperimentalFeatures == nil { + if InternalConfig == nil || InternalConfig.EnableExperimentalFeatures == nil { return false } - return *internalConfig.EnableExperimentalFeatures + return *InternalConfig.EnableExperimentalFeatures } func getClusterConfig(namespace string, client crclient.Client) (*controller.DevWorkspaceOperatorConfig, error) { @@ -125,21 +126,21 @@ func syncConfigFrom(newConfig *controller.DevWorkspaceOperatorConfig) { } configMutex.Lock() defer configMutex.Unlock() - internalConfig = defaultConfig.DeepCopy() - mergeConfig(newConfig.Config, internalConfig) + InternalConfig = defaultConfig.DeepCopy() + mergeConfig(newConfig.Config, InternalConfig) updatePublicConfig() } func restoreDefaultConfig() { configMutex.Lock() defer configMutex.Unlock() - internalConfig = defaultConfig.DeepCopy() + InternalConfig = defaultConfig.DeepCopy() updatePublicConfig() } func updatePublicConfig() { - Routing = internalConfig.Routing.DeepCopy() - Workspace = internalConfig.Workspace.DeepCopy() + Routing = InternalConfig.Routing.DeepCopy() + Workspace = InternalConfig.Workspace.DeepCopy() logCurrentConfig() } @@ -262,7 +263,7 @@ func mergeConfig(from, to *controller.OperatorConfiguration) { // logCurrentConfig formats the current operator configuration as a plain string func logCurrentConfig() { - if internalConfig == nil { + if InternalConfig == nil { return } var config []string @@ -303,7 +304,7 @@ func logCurrentConfig() { config = append(config, fmt.Sprintf("workspace.defaultTemplate is set")) } } - if internalConfig.EnableExperimentalFeatures != nil && *internalConfig.EnableExperimentalFeatures { + if InternalConfig.EnableExperimentalFeatures != nil && *InternalConfig.EnableExperimentalFeatures { config = append(config, "enableExperimentalFeatures=true") } @@ -313,7 +314,7 @@ func logCurrentConfig() { log.Info(fmt.Sprintf("Updated config to [%s]", strings.Join(config, ","))) } - if internalConfig.Routing.ProxyConfig != nil { - log.Info("Resolved proxy configuration", "proxy", internalConfig.Routing.ProxyConfig) + if InternalConfig.Routing.ProxyConfig != nil { + log.Info("Resolved proxy configuration", "proxy", InternalConfig.Routing.ProxyConfig) } } diff --git a/pkg/config/sync_test.go b/pkg/config/sync_test.go index 12af19df3..94553621c 100644 --- a/pkg/config/sync_test.go +++ b/pkg/config/sync_test.go @@ -38,7 +38,7 @@ func TestSetupControllerConfigUsesDefault(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, defaultConfig, internalConfig, "Config used should be the default") + assert.Equal(t, defaultConfig, InternalConfig, "Config used should be the default") } func TestSetupControllerDefaultsAreExported(t *testing.T) { @@ -63,7 +63,7 @@ func TestSetupControllerConfigFailsWhenAlreadySetup(t *testing.T) { if !assert.Error(t, err, "Should return error when config is already setup") { return } - assert.Equal(t, defaultConfig, internalConfig, "Config used should be the default") + assert.Equal(t, defaultConfig, InternalConfig, "Config used should be the default") } func TestSetupControllerMergesClusterConfig(t *testing.T) { @@ -91,9 +91,9 @@ func TestSetupControllerMergesClusterConfig(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, expectedConfig, internalConfig, fmt.Sprintf("Processed config should merge settings from cluster: %s", cmp.Diff(internalConfig, expectedConfig))) - assert.Equal(t, internalConfig.Routing, Routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) - assert.Equal(t, internalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, expectedConfig, InternalConfig, fmt.Sprintf("Processed config should merge settings from cluster: %s", cmp.Diff(InternalConfig, expectedConfig))) + assert.Equal(t, InternalConfig.Routing, Routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, InternalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) } func TestSetupControllerAlwaysSetsDefaultClusterRoutingSuffix(t *testing.T) { @@ -139,12 +139,12 @@ func TestDetectsOpenShiftRouteSuffix(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, "test-host", internalConfig.Routing.ClusterHostSuffix, "Should determine host suffix with route on OpenShift") + assert.Equal(t, "test-host", InternalConfig.Routing.ClusterHostSuffix, "Should determine host suffix with route on OpenShift") } func TestSyncConfigFromIgnoresOtherConfigInOtherNamespaces(t *testing.T) { setupForTest(t) - internalConfig = defaultConfig.DeepCopy() + InternalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Workspace: &v1alpha1.WorkspaceConfig{ ImagePullPolicy: "IfNotPresent", @@ -152,12 +152,12 @@ func TestSyncConfigFromIgnoresOtherConfigInOtherNamespaces(t *testing.T) { }) config.Namespace = "not-test-namespace" syncConfigFrom(config) - assert.Equal(t, defaultConfig, internalConfig) + assert.Equal(t, defaultConfig, InternalConfig) } func TestSyncConfigFromIgnoresOtherConfigWithOtherName(t *testing.T) { setupForTest(t) - internalConfig = defaultConfig.DeepCopy() + InternalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Workspace: &v1alpha1.WorkspaceConfig{ ImagePullPolicy: "IfNotPresent", @@ -165,13 +165,13 @@ func TestSyncConfigFromIgnoresOtherConfigWithOtherName(t *testing.T) { }) config.Name = "testing-name" syncConfigFrom(config) - assert.Equal(t, defaultConfig, internalConfig) + assert.Equal(t, defaultConfig, InternalConfig) } func TestSyncConfigDoesNotChangeDefaults(t *testing.T) { setupForTest(t) oldDefaultConfig := defaultConfig.DeepCopy() - internalConfig = defaultConfig.DeepCopy() + InternalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Routing: &v1alpha1.RoutingConfig{ DefaultRoutingClass: "test-routingClass", @@ -183,7 +183,7 @@ func TestSyncConfigDoesNotChangeDefaults(t *testing.T) { EnableExperimentalFeatures: &trueBool, }) syncConfigFrom(config) - internalConfig.Routing.DefaultRoutingClass = "Changed after the fact" + InternalConfig.Routing.DefaultRoutingClass = "Changed after the fact" assert.Equal(t, defaultConfig, oldDefaultConfig) } diff --git a/pkg/library/container/container.go b/pkg/library/container/container.go index c55eb8aaa..4b2c6a7a9 100644 --- a/pkg/library/container/container.go +++ b/pkg/library/container/container.go @@ -43,7 +43,7 @@ import ( // rewritten as Volumes are added to PodAdditions, in order to support e.g. using one PVC to hold all volumes // // Note: Requires DevWorkspace to be flattened (i.e. the DevWorkspace contains no Parent or Components of type Plugin) -func GetKubeContainersFromDevfile(workspace *dw.DevWorkspaceTemplateSpec) (*v1alpha1.PodAdditions, error) { +func GetKubeContainersFromDevfile(workspace *dw.DevWorkspaceTemplateSpec, config v1alpha1.OperatorConfiguration) (*v1alpha1.PodAdditions, error) { if !flatten.DevWorkspaceIsFlattened(workspace) { return nil, fmt.Errorf("devfile is not flattened") } @@ -58,7 +58,7 @@ func GetKubeContainersFromDevfile(workspace *dw.DevWorkspaceTemplateSpec) (*v1al if component.Container == nil { continue } - k8sContainer, err := convertContainerToK8s(component) + k8sContainer, err := convertContainerToK8s(component, config) if err != nil { return nil, err } @@ -71,7 +71,7 @@ func GetKubeContainersFromDevfile(workspace *dw.DevWorkspaceTemplateSpec) (*v1al } for _, container := range initContainers { - k8sContainer, err := convertContainerToK8s(container) + k8sContainer, err := convertContainerToK8s(container, config) if err != nil { return nil, err } diff --git a/pkg/library/container/container_test.go b/pkg/library/container/container_test.go index a30e96fda..ffaf68118 100644 --- a/pkg/library/container/container_test.go +++ b/pkg/library/container/container_test.go @@ -88,7 +88,7 @@ func TestGetKubeContainersFromDevfile(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check that file is read correctly. assert.True(t, len(tt.Input.Components) > 0, "Input defines no components") - gotPodAdditions, err := GetKubeContainersFromDevfile(tt.Input) + gotPodAdditions, err := GetKubeContainersFromDevfile(tt.Input, *testControllerCfg) if tt.Output.ErrRegexp != nil && assert.Error(t, err) { assert.Regexp(t, *tt.Output.ErrRegexp, err.Error(), "Error message should match") } else { diff --git a/pkg/library/container/conversion.go b/pkg/library/container/conversion.go index d549ac423..e7c1d35f4 100644 --- a/pkg/library/container/conversion.go +++ b/pkg/library/container/conversion.go @@ -19,16 +19,16 @@ import ( "fmt" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" ) -func convertContainerToK8s(devfileComponent dw.Component) (*v1.Container, error) { +func convertContainerToK8s(devfileComponent dw.Component, config controllerv1alpha1.OperatorConfiguration) (*v1.Container, error) { if devfileComponent.Container == nil { return nil, fmt.Errorf("cannot get k8s container from non-container component") } diff --git a/pkg/library/defaults/helper.go b/pkg/library/defaults/helper.go index 4c5fdfbb9..32e66561b 100644 --- a/pkg/library/defaults/helper.go +++ b/pkg/library/defaults/helper.go @@ -16,23 +16,22 @@ package defaults import ( - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/pkg/config" + "github.com/devfile/devworkspace-operator/pkg/common" ) // Overwrites the content of the workspace's Template Spec with the workspace config's Template Spec, // with the exception of the workspace's projects. // If the workspace's Template Spec defined any projects, they are preserved. -func ApplyDefaultTemplate(workspace *dw.DevWorkspace) { - if config.Workspace.DefaultTemplate == nil { +func ApplyDefaultTemplate(workspaceWithConfig *common.DevWorkspaceWithConfig) { + if workspaceWithConfig.Config.Workspace.DefaultTemplate == nil { return } - defaultCopy := config.Workspace.DefaultTemplate.DeepCopy() - originalProjects := workspace.Spec.Template.Projects - workspace.Spec.Template.DevWorkspaceTemplateSpecContent = *defaultCopy - workspace.Spec.Template.Projects = append(workspace.Spec.Template.Projects, originalProjects...) + defaultCopy := workspaceWithConfig.Config.Workspace.DefaultTemplate.DeepCopy() + originalProjects := workspaceWithConfig.Spec.Template.Projects + workspaceWithConfig.Spec.Template.DevWorkspaceTemplateSpecContent = *defaultCopy + workspaceWithConfig.Spec.Template.Projects = append(workspaceWithConfig.Spec.Template.Projects, originalProjects...) } -func NeedsDefaultTemplate(workspace *dw.DevWorkspace) bool { - return len(workspace.Spec.Template.Components) == 0 && config.Workspace.DefaultTemplate != nil +func NeedsDefaultTemplate(workspaceWithConfig *common.DevWorkspaceWithConfig) bool { + return len(workspaceWithConfig.Spec.Template.Components) == 0 && workspaceWithConfig.Config.Workspace.DefaultTemplate != nil } diff --git a/pkg/library/env/workspaceenv.go b/pkg/library/env/workspaceenv.go index 37db72bef..84fa7aa20 100644 --- a/pkg/library/env/workspaceenv.go +++ b/pkg/library/env/workspaceenv.go @@ -21,6 +21,7 @@ import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" @@ -30,8 +31,8 @@ import ( // AddCommonEnvironmentVariables adds environment variables to each container in podAdditions. Environment variables added include common // info environment variables and environment variables defined by a workspaceEnv attribute in the devfile itself -func AddCommonEnvironmentVariables(podAdditions *v1alpha1.PodAdditions, clusterDW *dw.DevWorkspace, flattenedDW *dw.DevWorkspaceTemplateSpec) error { - commonEnv := commonEnvironmentVariables(clusterDW.Name, clusterDW.Status.DevWorkspaceId, clusterDW.Namespace, clusterDW.Labels[constants.DevWorkspaceCreatorLabel]) +func AddCommonEnvironmentVariables(podAdditions *v1alpha1.PodAdditions, clusterDW *dw.DevWorkspace, flattenedDW *dw.DevWorkspaceTemplateSpec, config controllerv1alpha1.OperatorConfiguration) error { + commonEnv := commonEnvironmentVariables(clusterDW.Name, clusterDW.Status.DevWorkspaceId, clusterDW.Namespace, clusterDW.Labels[constants.DevWorkspaceCreatorLabel], config) workspaceEnv, err := collectWorkspaceEnv(flattenedDW) if err != nil { return err @@ -47,7 +48,7 @@ func AddCommonEnvironmentVariables(podAdditions *v1alpha1.PodAdditions, clusterD return nil } -func commonEnvironmentVariables(workspaceName, workspaceId, namespace, creator string) []corev1.EnvVar { +func commonEnvironmentVariables(workspaceName, workspaceId, namespace, creator string, config controllerv1alpha1.OperatorConfiguration) []corev1.EnvVar { envvars := []corev1.EnvVar{ { Name: constants.DevWorkspaceNamespace, diff --git a/pkg/library/projects/clone.go b/pkg/library/projects/clone.go index ec7c806ee..04971bf4a 100644 --- a/pkg/library/projects/clone.go +++ b/pkg/library/projects/clone.go @@ -19,12 +19,12 @@ import ( "fmt" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/pkg/config" devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "github.com/devfile/devworkspace-operator/internal/images" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/constants" ) @@ -32,14 +32,16 @@ const ( projectClonerContainerName = "project-clone" ) -func GetProjectCloneInitContainer(workspace *dw.DevWorkspaceTemplateSpec) (*corev1.Container, error) { +func GetProjectCloneInitContainer(workspaceWithConfig *common.DevWorkspaceWithConfig) (*corev1.Container, error) { + + workspace := workspaceWithConfig.Spec.Template if len(workspace.Projects) == 0 { return nil, nil } if workspace.Attributes.GetString(constants.ProjectCloneAttribute, nil) == constants.ProjectCloneDisable { return nil, nil } - if !hasContainerComponents(workspace) { + if !hasContainerComponents(&workspace) { // Avoid adding project-clone init container when DevWorkspace does not define any containers return nil, nil } @@ -92,7 +94,7 @@ func GetProjectCloneInitContainer(workspace *dw.DevWorkspaceTemplateSpec) (*core MountPath: constants.DefaultProjectsSourcesRoot, }, }, - ImagePullPolicy: corev1.PullPolicy(config.Workspace.ImagePullPolicy), + ImagePullPolicy: corev1.PullPolicy(workspaceWithConfig.Config.Workspace.ImagePullPolicy), }, nil } diff --git a/pkg/library/status/check.go b/pkg/library/status/check.go index d36dc9d1f..c86bd147f 100644 --- a/pkg/library/status/check.go +++ b/pkg/library/status/check.go @@ -20,8 +20,8 @@ import ( "fmt" "strings" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/infrastructure" "github.com/devfile/devworkspace-operator/pkg/provision/sync" appsv1 "k8s.io/api/apps/v1" @@ -75,7 +75,7 @@ func CheckDeploymentConditions(deployment *appsv1.Deployment) (healthy bool, err // Returns optional message with detected unrecoverable state details // error if any happens during check func CheckPodsState(workspaceID string, namespace string, labelSelector k8sclient.MatchingLabels, - clusterAPI sync.ClusterAPI) (stateMsg string, checkFailure error) { + clusterAPI sync.ClusterAPI, config *controllerv1alpha1.OperatorConfiguration) (stateMsg string, checkFailure error) { podList := &corev1.PodList{} if err := clusterAPI.Client.List(context.TODO(), podList, k8sclient.InNamespace(namespace), labelSelector); err != nil { return "", err @@ -83,25 +83,25 @@ func CheckPodsState(workspaceID string, namespace string, labelSelector k8sclien for _, pod := range podList.Items { for _, containerStatus := range pod.Status.ContainerStatuses { - ok, reason := CheckContainerStatusForFailure(&containerStatus) + ok, reason := CheckContainerStatusForFailure(&containerStatus, config) if !ok { return fmt.Sprintf("Container %s has state %s", containerStatus.Name, reason), nil } } for _, initContainerStatus := range pod.Status.InitContainerStatuses { - ok, reason := CheckContainerStatusForFailure(&initContainerStatus) + ok, reason := CheckContainerStatusForFailure(&initContainerStatus, config) if !ok { return fmt.Sprintf("Init Container %s has state %s", initContainerStatus.Name, reason), nil } } - if msg, err := CheckPodEvents(&pod, workspaceID, clusterAPI); err != nil || msg != "" { + if msg, err := CheckPodEvents(&pod, workspaceID, clusterAPI, config); err != nil || msg != "" { return msg, err } } return "", nil } -func CheckPodEvents(pod *corev1.Pod, workspaceID string, clusterAPI sync.ClusterAPI) (msg string, err error) { +func CheckPodEvents(pod *corev1.Pod, workspaceID string, clusterAPI sync.ClusterAPI, config *controllerv1alpha1.OperatorConfiguration) (msg string, err error) { evs := &corev1.EventList{} selector, err := fields.ParseSelector(fmt.Sprintf("involvedObject.name=%s", pod.Name)) if err != nil { @@ -124,7 +124,7 @@ func CheckPodEvents(pod *corev1.Pod, workspaceID string, clusterAPI sync.Cluster } if maxCount, isUnrecoverableEvent := unrecoverablePodEventReasons[ev.Reason]; isUnrecoverableEvent { - if !checkIfUnrecoverableEventIgnored(ev.Reason) && ev.Count >= maxCount { + if !checkIfUnrecoverableEventIgnored(ev.Reason, config) && ev.Count >= maxCount { var msg string if ev.Count > 1 { msg = fmt.Sprintf("Detected unrecoverable event %s %d times: %s.", ev.Reason, ev.Count, ev.Message) @@ -138,11 +138,11 @@ func CheckPodEvents(pod *corev1.Pod, workspaceID string, clusterAPI sync.Cluster return "", nil } -func CheckContainerStatusForFailure(containerStatus *corev1.ContainerStatus) (ok bool, reason string) { +func CheckContainerStatusForFailure(containerStatus *corev1.ContainerStatus, config *controllerv1alpha1.OperatorConfiguration) (ok bool, reason string) { if containerStatus.State.Waiting != nil { for _, failureReason := range containerFailureStateReasons { if containerStatus.State.Waiting.Reason == failureReason { - return checkIfUnrecoverableEventIgnored(containerStatus.State.Waiting.Reason), containerStatus.State.Waiting.Reason + return checkIfUnrecoverableEventIgnored(containerStatus.State.Waiting.Reason, config), containerStatus.State.Waiting.Reason } } } @@ -150,14 +150,14 @@ func CheckContainerStatusForFailure(containerStatus *corev1.ContainerStatus) (ok if containerStatus.State.Terminated != nil { for _, failureReason := range containerFailureStateReasons { if containerStatus.State.Terminated.Reason == failureReason { - return checkIfUnrecoverableEventIgnored(containerStatus.State.Terminated.Reason), containerStatus.State.Terminated.Reason + return checkIfUnrecoverableEventIgnored(containerStatus.State.Terminated.Reason, config), containerStatus.State.Terminated.Reason } } } return true, "" } -func checkIfUnrecoverableEventIgnored(reason string) (ignored bool) { +func checkIfUnrecoverableEventIgnored(reason string, config *controllerv1alpha1.OperatorConfiguration) (ignored bool) { for _, ignoredReason := range config.Workspace.IgnoredUnrecoverableEvents { if ignoredReason == reason { return true diff --git a/pkg/provision/storage/asyncStorage.go b/pkg/provision/storage/asyncStorage.go index 2c457ac8a..50c4f04d5 100644 --- a/pkg/provision/storage/asyncStorage.go +++ b/pkg/provision/storage/asyncStorage.go @@ -29,6 +29,7 @@ import ( "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/internal/images" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/constants" devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants" "github.com/devfile/devworkspace-operator/pkg/provision/storage/asyncstorage" @@ -45,39 +46,39 @@ func (*AsyncStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplateS return needsStorage(workspace) } -func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { if err := checkConfigured(); err != nil { return &ProvisioningError{ Message: fmt.Sprintf("%s. Contact an administrator to resolve this issue.", err.Error()), } } - numWorkspaces, _, err := p.getAsyncWorkspaceCount(workspace.Namespace, clusterAPI) + numWorkspaces, _, err := p.getAsyncWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return err } // If there is more than one started workspace using async storage, then we fail starting additional ones // Note we need to check phase so as to not accidentally fail an already-running workspace when a second one // is created. - if numWorkspaces > 1 && workspace.Status.Phase != dw.DevWorkspaceStatusRunning { + if numWorkspaces > 1 && workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusRunning { return &ProvisioningError{ - Message: fmt.Sprintf("cannot provision storage for workspace %s", workspace.Name), + Message: fmt.Sprintf("cannot provision storage for workspace %s", workspaceWithConfig.Name), Err: fmt.Errorf("at most one workspace using async storage can be running in a namespace"), } } // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(workspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !p.NeedsStorage(&workspace.Spec.Template) { + if !p.NeedsStorage(&workspaceWithConfig.Spec.Template) { return nil } // Sync SSH keypair to cluster - secret, configmap, err := asyncstorage.GetOrCreateSSHConfig(workspace, clusterAPI) + secret, configmap, err := asyncstorage.GetOrCreateSSHConfig(&workspaceWithConfig.DevWorkspace, clusterAPI) if err != nil { if errors.Is(err, asyncstorage.NotReadyError) { return &NotReadyError{ @@ -88,12 +89,12 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd return err } - pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return err } - pvcTerminating, err := checkPVCTerminating(pvcName, workspace.Namespace, clusterAPI) + pvcTerminating, err := checkPVCTerminating(pvcName, workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) if err != nil { return err } else if pvcTerminating { @@ -105,7 +106,7 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd if pvcName != "" { // Create common PVC if needed - clusterPVC, err := syncCommonPVC(workspace.Namespace, clusterAPI) + clusterPVC, err := syncCommonPVC(workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) if err != nil { return err } @@ -113,7 +114,7 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd } // Create async server deployment - deploy, err := asyncstorage.SyncWorkspaceSyncDeploymentToCluster(workspace.Namespace, configmap, pvcName, clusterAPI) + deploy, err := asyncstorage.SyncWorkspaceSyncDeploymentToCluster(workspaceWithConfig.Namespace, configmap, pvcName, clusterAPI, workspaceWithConfig.Config) if err != nil { if errors.Is(err, asyncstorage.NotReadyError) { return &NotReadyError{ @@ -156,32 +157,32 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd return err } - volumes, err := p.addVolumesForAsyncStorage(podAdditions, workspace) + volumes, err := p.addVolumesForAsyncStorage(podAdditions, &workspaceWithConfig.DevWorkspace) if err != nil { return err } sshSecretVolume := asyncstorage.GetVolumeFromSecret(secret) - asyncSidecar := asyncstorage.GetAsyncSidecar(workspace.Status.DevWorkspaceId, sshSecretVolume.Name, volumes) + asyncSidecar := asyncstorage.GetAsyncSidecar(workspaceWithConfig.Status.DevWorkspaceId, sshSecretVolume.Name, volumes) podAdditions.Containers = append(podAdditions.Containers, *asyncSidecar) podAdditions.Volumes = append(podAdditions.Volumes, *sshSecretVolume) return nil } -func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // TODO: This approach relies on there being a maximum of one workspace running per namespace. - asyncDeploy, err := asyncstorage.GetWorkspaceSyncDeploymentCluster(workspace.Namespace, clusterAPI) + asyncDeploy, err := asyncstorage.GetWorkspaceSyncDeploymentCluster(workspaceWithConfig.Namespace, clusterAPI) if err != nil { if k8sErrors.IsNotFound(err) { - return runCommonPVCCleanupJob(workspace, clusterAPI) + return runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) } else { return err } } // Check if another workspace is currently using the async server - numWorkspaces, totalWorkspaces, err := p.getAsyncWorkspaceCount(workspace.Namespace, clusterAPI) + numWorkspaces, totalWorkspaces, err := p.getAsyncWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return err } @@ -189,7 +190,7 @@ func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWorks case 0: // no problem case 1: - if workspace.Spec.Started { + if workspaceWithConfig.Spec.Started { // This is the only workspace using the async server, we can safely stop it break } @@ -218,12 +219,12 @@ func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWorks } // Clean up PVC using usual job - err = runCommonPVCCleanupJob(workspace, clusterAPI) + err = runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) if err != nil { return err } - retry, err := asyncstorage.RemoveAuthorizedKeyFromConfigMap(workspace, clusterAPI) + retry, err := asyncstorage.RemoveAuthorizedKeyFromConfigMap(&workspaceWithConfig.DevWorkspace, clusterAPI) if err != nil { return &ProvisioningError{ Message: "Failed to remove authorized key from async storage configmap", diff --git a/pkg/provision/storage/asyncstorage/deployment.go b/pkg/provision/storage/asyncstorage/deployment.go index cafdb0195..1b6a5a8eb 100644 --- a/pkg/provision/storage/asyncstorage/deployment.go +++ b/pkg/provision/storage/asyncstorage/deployment.go @@ -16,6 +16,7 @@ package asyncstorage import ( + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/internal/images" nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config" "github.com/devfile/devworkspace-operator/pkg/provision/sync" @@ -27,13 +28,13 @@ import ( "k8s.io/apimachinery/pkg/types" ) -func SyncWorkspaceSyncDeploymentToCluster(namespace string, sshConfigMap *corev1.ConfigMap, pvcName string, clusterAPI sync.ClusterAPI) (*appsv1.Deployment, error) { +func SyncWorkspaceSyncDeploymentToCluster(namespace string, sshConfigMap *corev1.ConfigMap, pvcName string, clusterAPI sync.ClusterAPI, config controllerv1alpha1.OperatorConfiguration) (*appsv1.Deployment, error) { podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(namespace, clusterAPI) if err != nil { return nil, err } - specDeployment := getWorkspaceSyncDeploymentSpec(namespace, sshConfigMap, pvcName, podTolerations, nodeSelector) + specDeployment := getWorkspaceSyncDeploymentSpec(namespace, sshConfigMap, pvcName, podTolerations, nodeSelector, config) clusterObj, err := sync.SyncObjectWithCluster(specDeployment, clusterAPI) switch err.(type) { case nil: @@ -58,7 +59,7 @@ func getWorkspaceSyncDeploymentSpec( sshConfigMap *corev1.ConfigMap, pvcName string, tolerations []corev1.Toleration, - nodeSelector map[string]string) *appsv1.Deployment { + nodeSelector map[string]string, config controllerv1alpha1.OperatorConfiguration) *appsv1.Deployment { replicas := int32(1) terminationGracePeriod := int64(1) @@ -147,7 +148,7 @@ func getWorkspaceSyncDeploymentSpec( }, }, TerminationGracePeriodSeconds: &terminationGracePeriod, - SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(), + SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(config), AutomountServiceAccountToken: nil, }, }, diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index ac81e22be..87bb5b233 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -20,7 +20,6 @@ import ( "path" "time" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/pkg/library/status" nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config" "github.com/devfile/devworkspace-operator/pkg/provision/sync" @@ -35,7 +34,6 @@ import ( "github.com/devfile/devworkspace-operator/internal/images" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" wsprovision "github.com/devfile/devworkspace-operator/pkg/provision/workspace" ) @@ -54,7 +52,7 @@ var ( pvcCleanupPodCPURequest = resource.MustParse(constants.PVCCleanupPodCPURequest) ) -func runCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func runCommonPVCCleanupJob(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { PVCexists, err := commonPVCExists(workspace, clusterAPI) if err != nil { return err @@ -94,7 +92,7 @@ func runCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.ClusterA } } - msg, err := status.CheckPodsState(workspace.Status.DevWorkspaceId, clusterJob.Namespace, k8sclient.MatchingLabels{"job-name": common.PVCCleanupJobName(workspace.Status.DevWorkspaceId)}, clusterAPI) + msg, err := status.CheckPodsState(workspace.Status.DevWorkspaceId, clusterJob.Namespace, k8sclient.MatchingLabels{"job-name": common.PVCCleanupJobName(workspace.Status.DevWorkspaceId)}, clusterAPI, &workspace.Config) if err != nil { return &ProvisioningError{ Err: err, @@ -115,30 +113,30 @@ func runCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.ClusterA } } -func getSpecCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) (*batchv1.Job, error) { - workspaceId := workspace.Status.DevWorkspaceId +func getSpecCommonPVCCleanupJob(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*batchv1.Job, error) { + workspaceId := workspaceWithConfig.Status.DevWorkspaceId - pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return nil, err } if pvcName == "" { - pvcName = config.Workspace.PVCName + pvcName = workspaceWithConfig.Config.Workspace.PVCName } jobLabels := map[string]string{ constants.DevWorkspaceIDLabel: workspaceId, - constants.DevWorkspaceNameLabel: workspace.Name, - constants.DevWorkspaceCreatorLabel: workspace.Labels[constants.DevWorkspaceCreatorLabel], + constants.DevWorkspaceNameLabel: workspaceWithConfig.Name, + constants.DevWorkspaceCreatorLabel: workspaceWithConfig.Labels[constants.DevWorkspaceCreatorLabel], } - if restrictedAccess, needsRestrictedAccess := workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; needsRestrictedAccess { + if restrictedAccess, needsRestrictedAccess := workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; needsRestrictedAccess { jobLabels[constants.DevWorkspaceRestrictedAccessAnnotation] = restrictedAccess } job := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: common.PVCCleanupJobName(workspaceId), - Namespace: workspace.Namespace, + Namespace: workspaceWithConfig.Namespace, Labels: jobLabels, }, Spec: batchv1.JobSpec{ @@ -150,7 +148,7 @@ func getSpecCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.Clus }, Spec: corev1.PodSpec{ RestartPolicy: "Never", - SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(), + SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(workspaceWithConfig.Config), Volumes: []corev1.Volume{ { Name: pvcName, @@ -193,7 +191,7 @@ func getSpecCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.Clus }, } - podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(workspace.Namespace, clusterAPI) + podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return nil, err } @@ -204,16 +202,16 @@ func getSpecCommonPVCCleanupJob(workspace *dw.DevWorkspace, clusterAPI sync.Clus job.Spec.Template.Spec.NodeSelector = nodeSelector } - if err := controllerutil.SetControllerReference(workspace, job, clusterAPI.Scheme); err != nil { + if err := controllerutil.SetControllerReference(workspaceWithConfig, job, clusterAPI.Scheme); err != nil { return nil, err } return job, nil } -func commonPVCExists(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) (bool, error) { +func commonPVCExists(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (bool, error) { namespacedName := types.NamespacedName{ - Name: config.Workspace.PVCName, - Namespace: workspace.Namespace, + Name: workspaceWithConfig.Config.Workspace.PVCName, + Namespace: workspaceWithConfig.Namespace, } err := clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, &corev1.PersistentVolumeClaim{}) if err != nil { diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go index a1de73494..b87ef226f 100644 --- a/pkg/provision/storage/commonStorage.go +++ b/pkg/provision/storage/commonStorage.go @@ -20,9 +20,9 @@ import ( "time" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/provision/sync" + "github.com/devfile/devworkspace-operator/pkg/common" corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -42,23 +42,23 @@ func (*CommonStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplate return needsStorage(workspace) } -func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(workspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !p.NeedsStorage(&workspace.Spec.Template) { + if !p.NeedsStorage(&workspaceWithConfig.Spec.Template) { return nil } - pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return err } - pvcTerminating, err := checkPVCTerminating(pvcName, workspace.Namespace, clusterAPI) + pvcTerminating, err := checkPVCTerminating(pvcName, workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) if err != nil { return err } else if pvcTerminating { @@ -69,14 +69,14 @@ func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAd } if pvcName == "" { - commonPVC, err := syncCommonPVC(workspace.Namespace, clusterAPI) + commonPVC, err := syncCommonPVC(workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) if err != nil { return err } pvcName = commonPVC.Name } - if err := p.rewriteContainerVolumeMounts(workspace.Status.DevWorkspaceId, pvcName, podAdditions, &workspace.Spec.Template); err != nil { + if err := p.rewriteContainerVolumeMounts(workspaceWithConfig.Status.DevWorkspaceId, pvcName, podAdditions, &workspaceWithConfig.Spec.Template); err != nil { return &ProvisioningError{ Err: err, Message: "Could not rewrite container volume mounts", @@ -86,8 +86,8 @@ func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAd return nil } -func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { - totalWorkspaces, err := getSharedPVCWorkspaceCount(workspace.Namespace, clusterAPI) +func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { + totalWorkspaces, err := getSharedPVCWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return err } @@ -95,10 +95,10 @@ func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWork // If the number of common + async workspaces that exist (started or stopped) is zero, // delete common PVC instead of running cleanup job if totalWorkspaces > 0 { - return runCommonPVCCleanupJob(workspace, clusterAPI) + return runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) } else { sharedPVC := &corev1.PersistentVolumeClaim{} - namespacedName := types.NamespacedName{Name: config.Workspace.PVCName, Namespace: workspace.Namespace} + namespacedName := types.NamespacedName{Name: workspaceWithConfig.Config.Workspace.PVCName, Namespace: workspaceWithConfig.Namespace} err := clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, sharedPVC) if err != nil { diff --git a/pkg/provision/storage/commonStorage_test.go b/pkg/provision/storage/commonStorage_test.go index cfad122e2..d0c4819af 100644 --- a/pkg/provision/storage/commonStorage_test.go +++ b/pkg/provision/storage/commonStorage_test.go @@ -23,6 +23,7 @@ import ( "testing" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/provision/sync" "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -125,7 +126,7 @@ func TestProvisionStorageForCommonStorageClass(t *testing.T) { tests := loadAllTestCasesOrPanic(t, "testdata/common-storage") setupControllerCfg() commonStorage := CommonStorageProvisioner{} - commonPVC, err := getPVCSpec("claim-devworkspace", "test-namespace", resource.MustParse("10Gi")) + commonPVC, err := getPVCSpec("claim-devworkspace", "test-namespace", resource.MustParse("10Gi"), *testControllerCfg) if err != nil { t.Fatalf("Failure during setup: %s", err) } @@ -139,7 +140,8 @@ func TestProvisionStorageForCommonStorageClass(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check that file is read correctly. assert.NotNil(t, tt.Input.Workspace, "Input does not define workspace") - workspace := &dw.DevWorkspace{} + workspace := &common.DevWorkspaceWithConfig{} + workspace.Config = *config.InternalConfig workspace.Spec.Template = *tt.Input.Workspace workspace.Status.DevWorkspaceId = tt.Input.DevWorkspaceID workspace.Namespace = "test-namespace" @@ -162,7 +164,7 @@ func TestProvisionStorageForCommonStorageClass(t *testing.T) { func TestTerminatingPVC(t *testing.T) { setupControllerCfg() commonStorage := CommonStorageProvisioner{} - commonPVC, err := getPVCSpec("claim-devworkspace", "test-namespace", resource.MustParse("10Gi")) + commonPVC, err := getPVCSpec("claim-devworkspace", "test-namespace", resource.MustParse("10Gi"), *testControllerCfg) if err != nil { t.Fatalf("Failure during setup: %s", err) } @@ -175,7 +177,8 @@ func TestTerminatingPVC(t *testing.T) { } testCase := loadTestCaseOrPanic(t, "testdata/common-storage/rewrites-volumes-for-common-pvc-strategy.yaml") assert.NotNil(t, testCase.Input.Workspace, "Input does not define workspace") - workspace := &dw.DevWorkspace{} + workspace := &common.DevWorkspaceWithConfig{} + workspace.Config = *config.InternalConfig workspace.Spec.Template = *testCase.Input.Workspace workspace.Status.DevWorkspaceId = testCase.Input.DevWorkspaceID workspace.Namespace = "test-namespace" diff --git a/pkg/provision/storage/ephemeralStorage.go b/pkg/provision/storage/ephemeralStorage.go index e9e936007..89ecbcd93 100644 --- a/pkg/provision/storage/ephemeralStorage.go +++ b/pkg/provision/storage/ephemeralStorage.go @@ -17,10 +17,10 @@ package storage import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/pkg/provision/sync" - "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/library/container" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" ) // The EphemeralStorageProvisioner provisions all workspace storage as emptyDir volumes. @@ -35,8 +35,8 @@ func (e EphemeralStorageProvisioner) NeedsStorage(_ *dw.DevWorkspaceTemplateSpec return false } -func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *dw.DevWorkspace, _ sync.ClusterAPI) error { - persistent, ephemeral, projects := getWorkspaceVolumes(workspace) +func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, _ sync.ClusterAPI) error { + persistent, ephemeral, projects := getWorkspaceVolumes(&workspaceWithConfig.DevWorkspace) if _, err := addEphemeralVolumesToPodAdditions(podAdditions, persistent); err != nil { return err } @@ -48,7 +48,7 @@ func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.Pod return err } } else { - if container.AnyMountSources(workspace.Spec.Template.Components) { + if container.AnyMountSources(workspaceWithConfig.Spec.Template.Components) { projectsComponent := dw.Component{Name: "projects"} projectsComponent.Volume = &dw.VolumeComponent{} if _, err := addEphemeralVolumesToPodAdditions(podAdditions, []dw.Component{projectsComponent}); err != nil { @@ -59,6 +59,6 @@ func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.Pod return nil } -func (e EphemeralStorageProvisioner) CleanupWorkspaceStorage(_ *dw.DevWorkspace, _ sync.ClusterAPI) error { +func (e EphemeralStorageProvisioner) CleanupWorkspaceStorage(_ *common.DevWorkspaceWithConfig, _ sync.ClusterAPI) error { return nil } diff --git a/pkg/provision/storage/ephemeralStorage_test.go b/pkg/provision/storage/ephemeralStorage_test.go index 6c080bc06..5ff3bbd36 100644 --- a/pkg/provision/storage/ephemeralStorage_test.go +++ b/pkg/provision/storage/ephemeralStorage_test.go @@ -18,7 +18,7 @@ package storage import ( "testing" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/provision/sync" "github.com/stretchr/testify/assert" ) @@ -32,7 +32,8 @@ func TestRewriteContainerVolumeMountsForEphemeralStorageClass(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check that file is read correctly. assert.NotNil(t, tt.Input.Workspace, "Input does not define workspace") - workspace := &dw.DevWorkspace{} + workspace := &common.DevWorkspaceWithConfig{} + workspace.Config = *testControllerCfg workspace.Spec.Template = *tt.Input.Workspace workspace.Status.DevWorkspaceId = tt.Input.DevWorkspaceID workspace.Namespace = "test-namespace" diff --git a/pkg/provision/storage/perWorkspaceStorage.go b/pkg/provision/storage/perWorkspaceStorage.go index db6af9a01..8a6ab88eb 100644 --- a/pkg/provision/storage/perWorkspaceStorage.go +++ b/pkg/provision/storage/perWorkspaceStorage.go @@ -22,7 +22,6 @@ import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants" nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config" @@ -45,19 +44,19 @@ func (*PerWorkspaceStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTe return false } -func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(workspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !needsStorage(&workspace.Spec.Template) { + if !needsStorage(&workspaceWithConfig.Spec.Template) { return nil } // Get perWorkspace PVC spec and sync it with cluster - perWorkspacePVC, err := syncPerWorkspacePVC(workspace, clusterAPI) + perWorkspacePVC, err := syncPerWorkspacePVC(workspaceWithConfig, clusterAPI) if err != nil { return err } @@ -71,7 +70,7 @@ func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1 } // Rewrite container volume mounts - if err := p.rewriteContainerVolumeMounts(workspace.Status.DevWorkspaceId, pvcName, podAdditions, &workspace.Spec.Template); err != nil { + if err := p.rewriteContainerVolumeMounts(workspaceWithConfig.Status.DevWorkspaceId, pvcName, podAdditions, &workspaceWithConfig.Spec.Template); err != nil { return &ProvisioningError{ Err: err, Message: "Could not rewrite container volume mounts", @@ -82,7 +81,7 @@ func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1 } // We rely on Kubernetes to use the owner reference to automatically delete the PVC once the workspace is set for deletion. -func (*PerWorkspaceStorageProvisioner) CleanupWorkspaceStorage(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error { +func (*PerWorkspaceStorageProvisioner) CleanupWorkspaceStorage(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { return nil } @@ -156,14 +155,14 @@ func (p *PerWorkspaceStorageProvisioner) rewriteContainerVolumeMounts(workspaceI return nil } -func syncPerWorkspacePVC(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) { - namespacedConfig, err := nsconfig.ReadNamespacedConfig(workspace.Namespace, clusterAPI) +func syncPerWorkspacePVC(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) { + namespacedConfig, err := nsconfig.ReadNamespacedConfig(workspaceWithConfig.Namespace, clusterAPI) if err != nil { return nil, fmt.Errorf("failed to read namespace-specific configuration: %w", err) } // TODO: Determine the storage size that is needed by iterating through workspace volumes, // adding the sizes specified and figuring out overrides/defaults - pvcSize := *config.Workspace.DefaultStorageSize.PerWorkspace + pvcSize := *workspaceWithConfig.Config.Workspace.DefaultStorageSize.PerWorkspace if namespacedConfig != nil && namespacedConfig.PerWorkspacePVCSize != "" { pvcSize, err = resource.ParseQuantity(namespacedConfig.PerWorkspacePVCSize) if err != nil { @@ -171,12 +170,12 @@ func syncPerWorkspacePVC(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) } } - pvc, err := getPVCSpec(common.PerWorkspacePVCName(workspace.Status.DevWorkspaceId), workspace.Namespace, pvcSize) + pvc, err := getPVCSpec(common.PerWorkspacePVCName(workspaceWithConfig.Status.DevWorkspaceId), workspaceWithConfig.Namespace, pvcSize, workspaceWithConfig.Config) if err != nil { return nil, err } - if err := controllerutil.SetControllerReference(workspace, pvc, clusterAPI.Scheme); err != nil { + if err := controllerutil.SetControllerReference(&workspaceWithConfig.DevWorkspace, pvc, clusterAPI.Scheme); err != nil { return nil, err } diff --git a/pkg/provision/storage/perWorkspaceStorage_test.go b/pkg/provision/storage/perWorkspaceStorage_test.go index bd3699285..18ff38ca4 100644 --- a/pkg/provision/storage/perWorkspaceStorage_test.go +++ b/pkg/provision/storage/perWorkspaceStorage_test.go @@ -52,7 +52,8 @@ func TestRewriteContainerVolumeMountsForPerWorkspaceStorageClass(t *testing.T) { t.Run(tt.Name, func(t *testing.T) { // sanity check that file is read correctly. assert.NotNil(t, tt.Input.Workspace, "Input does not define workspace") - workspace := &dw.DevWorkspace{} + workspace := &common.DevWorkspaceWithConfig{} + workspace.Config = *config.InternalConfig workspace.Spec.Template = *tt.Input.Workspace workspace.Status.DevWorkspaceId = tt.Input.DevWorkspaceID workspace.Namespace = "test-namespace" diff --git a/pkg/provision/storage/provisioner.go b/pkg/provision/storage/provisioner.go index 71185a9c6..8126fec19 100644 --- a/pkg/provision/storage/provisioner.go +++ b/pkg/provision/storage/provisioner.go @@ -17,10 +17,10 @@ package storage import ( dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/pkg/provision/sync" - "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/provision/sync" ) // Provisioner is an interface for rewriting volumeMounts in a pod according to a storage policy (e.g. common PVC for all mounts, etc.) @@ -29,14 +29,14 @@ type Provisioner interface { // out-of-pod required objects to the cluster. // Returns NotReadyError to signify that storage is not ready, ProvisioningError when a fatal issue is encountered, // and other error if there is an unexpected problem. - ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error + ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error // NeedsStorage returns whether the current workspace needs a PVC to be provisioned, given this storage strategy. NeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool // CleanupWorkspaceStorage removes any objects provisioned by in the ProvisionStorage step that aren't automatically removed when a // DevWorkspace is deleted (e.g. delete subfolders in a common PVC assigned to the workspace) // Returns nil on success (DevWorkspace can be deleted), NotReadyError if additional reconciles are necessary, ProvisioningError when // a fatal issue is encountered, and any other error if an unexpected problem arises. - CleanupWorkspaceStorage(workspace *dw.DevWorkspace, clusterAPI sync.ClusterAPI) error + CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error } // GetProvisioner returns the storage provisioner that should be used for the current workspace diff --git a/pkg/provision/storage/shared.go b/pkg/provision/storage/shared.go index 31427c056..3fadaa489 100644 --- a/pkg/provision/storage/shared.go +++ b/pkg/provision/storage/shared.go @@ -29,14 +29,14 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants" containerlib "github.com/devfile/devworkspace-operator/pkg/library/container" nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config" ) -func getPVCSpec(name, namespace string, size resource.Quantity) (*corev1.PersistentVolumeClaim, error) { +// TODO: Maybe just pass in *string storageClassName? +func getPVCSpec(name, namespace string, size resource.Quantity, config v1alpha1.OperatorConfiguration) (*corev1.PersistentVolumeClaim, error) { return &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ @@ -85,7 +85,7 @@ func needsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool { return containerlib.AnyMountSources(workspace.Components) } -func syncCommonPVC(namespace string, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) { +func syncCommonPVC(namespace string, clusterAPI sync.ClusterAPI, config v1alpha1.OperatorConfiguration) (*corev1.PersistentVolumeClaim, error) { namespacedConfig, err := nsconfig.ReadNamespacedConfig(namespace, clusterAPI) if err != nil { return nil, fmt.Errorf("failed to read namespace-specific configuration: %w", err) @@ -98,7 +98,7 @@ func syncCommonPVC(namespace string, clusterAPI sync.ClusterAPI) (*corev1.Persis } } - pvc, err := getPVCSpec(config.Workspace.PVCName, namespace, pvcSize) + pvc, err := getPVCSpec(config.Workspace.PVCName, namespace, pvcSize, config) if err != nil { return nil, err } @@ -259,7 +259,7 @@ func getSharedPVCWorkspaceCount(namespace string, api sync.ClusterAPI) (total in return total, nil } -func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, error) { +func checkPVCTerminating(name, namespace string, api sync.ClusterAPI, config v1alpha1.OperatorConfiguration) (bool, error) { if name == "" { name = config.Workspace.PVCName } diff --git a/pkg/provision/workspace/deployment.go b/pkg/provision/workspace/deployment.go index bb3a35a79..6706f726b 100644 --- a/pkg/provision/workspace/deployment.go +++ b/pkg/provision/workspace/deployment.go @@ -28,10 +28,10 @@ import ( "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" maputils "github.com/devfile/devworkspace-operator/internal/map" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" @@ -51,7 +51,7 @@ func SyncDeploymentToCluster( workspace *dw.DevWorkspace, podAdditions []v1alpha1.PodAdditions, saName string, - clusterAPI sync.ClusterAPI) DeploymentProvisioningStatus { + clusterAPI sync.ClusterAPI, config *controllerv1alpha1.OperatorConfiguration) DeploymentProvisioningStatus { podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(workspace.Namespace, clusterAPI) if err != nil { @@ -66,7 +66,7 @@ func SyncDeploymentToCluster( // [design] we have to pass components and routing pod additions separately because we need mountsources from each // component. - specDeployment, err := getSpecDeployment(workspace, podAdditions, saName, podTolerations, nodeSelector, clusterAPI.Scheme) + specDeployment, err := getSpecDeployment(workspace, podAdditions, saName, podTolerations, nodeSelector, clusterAPI.Scheme, *config) if err != nil { return DeploymentProvisioningStatus{ ProvisioningStatus{ @@ -118,7 +118,7 @@ func SyncDeploymentToCluster( failureMsg, checkErr := status.CheckPodsState(workspace.Status.DevWorkspaceId, workspace.Namespace, k8sclient.MatchingLabels{ constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, - }, clusterAPI) + }, clusterAPI, config) if checkErr != nil { return DeploymentProvisioningStatus{ ProvisioningStatus: ProvisioningStatus{ @@ -172,7 +172,7 @@ func ScaleDeploymentToZero(ctx context.Context, workspace *dw.DevWorkspace, clie return nil } -func GetDevWorkspaceSecurityContext() *corev1.PodSecurityContext { +func GetDevWorkspaceSecurityContext(config controllerv1alpha1.OperatorConfiguration) *corev1.PodSecurityContext { if infrastructure.IsOpenShift() { return &corev1.PodSecurityContext{} } @@ -185,7 +185,7 @@ func getSpecDeployment( saName string, podTolerations []corev1.Toleration, nodeSelector map[string]string, - scheme *runtime.Scheme) (*appsv1.Deployment, error) { + scheme *runtime.Scheme, config controllerv1alpha1.OperatorConfiguration) (*appsv1.Deployment, error) { replicas := int32(1) terminationGracePeriod := int64(10) @@ -240,7 +240,7 @@ func getSpecDeployment( Volumes: podAdditions.Volumes, RestartPolicy: "Always", TerminationGracePeriodSeconds: &terminationGracePeriod, - SecurityContext: GetDevWorkspaceSecurityContext(), + SecurityContext: GetDevWorkspaceSecurityContext(config), ServiceAccountName: saName, AutomountServiceAccountToken: nil, }, @@ -261,7 +261,7 @@ func getSpecDeployment( } } - if needPVC, pvcName := needsPVCWorkaround(podAdditions); needPVC { + if needPVC, pvcName := needsPVCWorkaround(podAdditions, config); needPVC { // Kubernetes creates directories in a PVC to support subpaths such that only the leaf directory has g+rwx permissions. // This means that mounting the subpath e.g. /plugins will result in the directory being // created with 755 permissions, requiring the root UID to remove it. @@ -368,7 +368,7 @@ func getWorkspaceSubpathVolumeMount(workspaceId, pvcName string) corev1.VolumeMo return workspaceVolumeMount } -func needsPVCWorkaround(podAdditions *v1alpha1.PodAdditions) (needs bool, pvcName string) { +func needsPVCWorkaround(podAdditions *v1alpha1.PodAdditions, config controllerv1alpha1.OperatorConfiguration) (needs bool, pvcName string) { commonPVCName := config.Workspace.PVCName for _, vol := range podAdditions.Volumes { if vol.Name == commonPVCName { From a6d64aef0ce5a88e6c17629eff8ae451d6d7b8a1 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 17 Aug 2022 14:46:22 -0400 Subject: [PATCH 02/14] feat: Allow external DWOC to be merged with internal DWOC Allow specifying an external DWOC that gets merged with the workspace's internal DWOC. The external DWOC's name and namespace are specified with the `controller.devfile.io/devworkspace-config` DevWorkspace attribute, which has the following structure: attributes: controller.devfile.io/devworkspace-config: name: namespace: Part of eclipse/che#21405 Signed-off-by: Andrew Obuchowicz --- .../solvers/basic_solver.go | 1 + .../workspace/devworkspace_controller.go | 42 ++++ pkg/config/common_test.go | 10 + pkg/config/sync.go | 204 +++++++++++++++++- pkg/config/sync_test.go | 94 ++++++++ pkg/constants/attributes.go | 15 ++ pkg/constants/metadata.go | 4 + 7 files changed, 365 insertions(+), 5 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 23beeccc3..39de5d6a2 100644 --- a/controllers/controller/devworkspacerouting/solvers/basic_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/basic_solver.go @@ -20,6 +20,7 @@ import ( "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "k8s.io/apimachinery/pkg/types" ) var routeAnnotations = func(endpointName string) map[string]string { diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 96c3b1f3a..9db5f3bc7 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -246,6 +246,20 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request wsDefaults.ApplyDefaultTemplate(workspaceWithConfig) } + // Apply devworkspace routing annotation for external DWOC + // TODO: Cleanup + err = addExternalDWOCAnnotations(*workspaceWithConfig) + if err != nil { + reqLogger.Error(err, "Unable to apply annotations used by Devworkspace Router for external DevWorkspace-Operator configuration") + } + + // Merge workspace's DWOC with an external, one if it exists + // TODO: Rework + err = config.ApplyExternalDWOCConfig(&workspaceWithConfig.DevWorkspace, clusterAPI.Client) + if err != nil { + reqLogger.Error(err, "Unable to apply external DevWorkspace-Operator configuration") + } + flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) if err != nil { return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) @@ -458,6 +472,34 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{}, nil } +// TODO: Clean up/polish this function +func addExternalDWOCAnnotations(workspaceWithConfig common.DevWorkspaceWithConfig) error { + if !workspaceWithConfig.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { + return nil + } + + ExternalDWOCMeta := &types.NamespacedName{} + + err := workspaceWithConfig.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &ExternalDWOCMeta) + if err != nil { + return fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err) + } + + if ExternalDWOCMeta.Name == "" { + return fmt.Errorf("'name' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } + + if ExternalDWOCMeta.Namespace == "" { + return fmt.Errorf("'namespace' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } + + annotationPrefix := string(workspaceWithConfig.Spec.RoutingClass) + constants.RoutingAnnotationInfix + workspaceWithConfig.Annotations[annotationPrefix+constants.ExternalDWOCNameAnnotationSuffix] = ExternalDWOCMeta.Name + workspaceWithConfig.Annotations[annotationPrefix+constants.ExternalDWOCNamespaceAnnotationSuffix] = ExternalDWOCMeta.Namespace + return nil + +} + func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *common.DevWorkspaceWithConfig, logger logr.Logger) (reconcile.Result, error) { status := currentStatus{phase: dw.DevWorkspaceStatusStopping} if workspace.Status.Phase == devworkspacePhaseFailing || workspace.Status.Phase == dw.DevWorkspaceStatusFailed { diff --git a/pkg/config/common_test.go b/pkg/config/common_test.go index d76b02409..ad9322a2d 100644 --- a/pkg/config/common_test.go +++ b/pkg/config/common_test.go @@ -68,3 +68,13 @@ func buildConfig(config *v1alpha1.OperatorConfiguration) *v1alpha1.DevWorkspaceO Config: config, } } + +func buildExternalConfig(config *v1alpha1.OperatorConfiguration) *v1alpha1.DevWorkspaceOperatorConfig { + return &v1alpha1.DevWorkspaceOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: ExternalConfigName, + Namespace: ExternalConfigNamespace, + }, + Config: config, + } +} diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 76d7f3eed..29c1bf26b 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -18,24 +18,30 @@ package config import ( "context" "fmt" + "reflect" + "sort" "strings" "sync" + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/config/proxy" + "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" routeV1 "github.com/openshift/api/route/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" crclient "sigs.k8s.io/controller-runtime/pkg/client" - - controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/infrastructure" ) const ( - OperatorConfigName = "devworkspace-operator-config" - openShiftTestRouteName = "devworkspace-controller-test-route" + OperatorConfigName = "devworkspace-operator-config" + openShiftTestRouteName = "devworkspace-controller-test-route" + ExternalConfigName = "external-config-name" + ExternalConfigNamespace = "external-config-namespace" ) var ( @@ -56,6 +62,53 @@ func SetConfigForTesting(config *controller.OperatorConfiguration) { updatePublicConfig() } +func ApplyExternalDWOCConfig(workspace *dw.DevWorkspace, client crclient.Client) (err error) { + + if !workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { + return nil + } + + ExternalDWOCMeta := v1alpha1.ExternalConfig{} + + err = workspace.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &ExternalDWOCMeta) + if err != nil { + return fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err) + } + + if ExternalDWOCMeta.Name == "" { + return fmt.Errorf("'name' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } + + if ExternalDWOCMeta.Namespace == "" { + return fmt.Errorf("'namespace' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } + + externalDWOC := &controller.DevWorkspaceOperatorConfig{} + namespacedName := types.NamespacedName{ + Name: ExternalDWOCMeta.Name, + Namespace: ExternalDWOCMeta.Namespace, + } + + err = client.Get(context.TODO(), namespacedName, externalDWOC) + if err != nil { + return fmt.Errorf("could not fetch external DWOC with name '%s' in namespace '%s': %w", ExternalDWOCMeta.Name, ExternalDWOCMeta.Namespace, err) + } + + // TODO: It might be better to just always merge rather than suffering performance hit in configsAlreadyMerged() (or maintaing configsAlreadyMerged if we don't use reflect.DeepEqual) + if !configsAlreadyMerged(externalDWOC.Config, InternalConfig) { + mergeInteralConfigWithExternal(externalDWOC.Config) + } + + return nil +} + +func mergeInteralConfigWithExternal(externalConfig *controller.OperatorConfiguration) { + configMutex.Lock() + defer configMutex.Unlock() + mergeConfig(externalConfig, InternalConfig) + updatePublicConfig() +} + func SetupControllerConfig(client crclient.Client) error { if InternalConfig != nil { return fmt.Errorf("internal controller configuration is already set up") @@ -186,6 +239,147 @@ func discoverRouteSuffix(client crclient.Client) (string, error) { return host, nil } +// TODO: Improve variable names? +// Returns true if 'from' has already been merged with 'to'. +// The two configs are considered merged if all fields that are set in 'from' have the same value in 'to' +func configsAlreadyMerged(from, to *controller.OperatorConfiguration) bool { + if from == nil { + return true + } + if from.EnableExperimentalFeatures != nil { + + if to.EnableExperimentalFeatures == nil { + return false + } + + if *to.EnableExperimentalFeatures != *from.EnableExperimentalFeatures { + return false + } + } + if from.Routing != nil { + if to.Routing == nil { + return false + } + if from.Routing.DefaultRoutingClass != "" && to.Routing.DefaultRoutingClass != from.Routing.DefaultRoutingClass { + return false + } + if from.Routing.ClusterHostSuffix != "" && to.Routing.ClusterHostSuffix != from.Routing.ClusterHostSuffix { + return false + } + if from.Routing.ProxyConfig != nil { + if to.Routing.ProxyConfig == nil { + return false + } + + if from.Routing.ProxyConfig.HttpProxy != "" && to.Routing.ProxyConfig.HttpProxy != from.Routing.ProxyConfig.HttpProxy { + return false + } + + if from.Routing.ProxyConfig.HttpsProxy != "" && to.Routing.ProxyConfig.HttpsProxy != from.Routing.ProxyConfig.HttpsProxy { + return false + } + + if from.Routing.ProxyConfig.NoProxy != "" && to.Routing.ProxyConfig.NoProxy != from.Routing.ProxyConfig.NoProxy { + return false + } + } + } + if from.Workspace != nil { + if to.Workspace == nil { + return false + } + if from.Workspace.StorageClassName != nil { + if to.Workspace.StorageClassName == nil { + return false + } + + if *to.Workspace.StorageClassName != *from.Workspace.StorageClassName { + return false + } + } + if from.Workspace.PVCName != "" && to.Workspace.PVCName != from.Workspace.PVCName { + return false + } + if from.Workspace.ImagePullPolicy != "" && to.Workspace.ImagePullPolicy != from.Workspace.ImagePullPolicy { + return false + } + if from.Workspace.IdleTimeout != "" && to.Workspace.IdleTimeout != from.Workspace.IdleTimeout { + return false + } + if from.Workspace.ProgressTimeout != "" && to.Workspace.ProgressTimeout != from.Workspace.ProgressTimeout { + return false + } + if from.Workspace.IgnoredUnrecoverableEvents != nil && to.Workspace.IgnoredUnrecoverableEvents != nil { + + if len(from.Workspace.IgnoredUnrecoverableEvents) != len(to.Workspace.IgnoredUnrecoverableEvents) { + return false + } + + sort.Strings(to.Workspace.IgnoredUnrecoverableEvents) + sort.Strings(from.Workspace.IgnoredUnrecoverableEvents) + + for i := range from.Workspace.IgnoredUnrecoverableEvents { + if from.Workspace.IgnoredUnrecoverableEvents[i] != to.Workspace.IgnoredUnrecoverableEvents[i] { + return false + } + } + } + if from.Workspace.CleanupOnStop != nil { + if to.Workspace.CleanupOnStop == nil { + return false + } + + if *to.Workspace.CleanupOnStop != *from.Workspace.CleanupOnStop { + return false + } + } + if from.Workspace.PodSecurityContext != nil { + if to.Workspace.PodSecurityContext == nil { + return false + } + + // TODO: Using reflect.DeepEqual could potentially really degrade performance + if !reflect.DeepEqual(from.Workspace.PodSecurityContext, to.Workspace.PodSecurityContext) { + return false + } + } + if from.Workspace.DefaultStorageSize != nil { + if to.Workspace.DefaultStorageSize == nil { + return false + } + if from.Workspace.DefaultStorageSize.Common != nil { + if to.Workspace.DefaultStorageSize.Common == nil { + return false + } + if from.Workspace.DefaultStorageSize.Common.Cmp(*to.Workspace.DefaultStorageSize.Common) != 0 { + return false + } + + } + if from.Workspace.DefaultStorageSize.PerWorkspace != nil { + if to.Workspace.DefaultStorageSize.PerWorkspace == nil { + return false + } + if from.Workspace.DefaultStorageSize.PerWorkspace.Cmp(*to.Workspace.DefaultStorageSize.PerWorkspace) != 0 { + return false + } + } + + } + if from.Workspace.DefaultTemplate != nil { + if to.Workspace.DefaultTemplate == nil { + return false + } + + // TODO: Using reflect.DeepEqual could potentially really degrade performance + if !reflect.DeepEqual(from.Workspace.DefaultTemplate, to.Workspace.DefaultTemplate) { + return false + } + } + } + return true +} + func mergeConfig(from, to *controller.OperatorConfiguration) { if to == nil { to = &controller.OperatorConfiguration{} diff --git a/pkg/config/sync_test.go b/pkg/config/sync_test.go index 94553621c..ea1e70753 100644 --- a/pkg/config/sync_test.go +++ b/pkg/config/sync_test.go @@ -16,18 +16,24 @@ package config import ( + "context" "fmt" "testing" + attributes "github.com/devfile/api/v2/pkg/attributes" + + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/google/go-cmp/cmp" fuzz "github.com/google/gofuzz" routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" ) @@ -96,6 +102,94 @@ func TestSetupControllerMergesClusterConfig(t *testing.T) { assert.Equal(t, InternalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) } +func TestCatchesNonExistentExternalDWOC(t *testing.T) { + setupForTest(t) + + workspace := &dw.DevWorkspace{} + attributes := attributes.Attributes{} + externalDWOCMeta := v1alpha1.ExternalConfig{} + externalDWOCMeta.Name = "external-config-name" + externalDWOCMeta.Namespace = "external-config-namespace" + + attributes.Put(constants.ExternalDevWorkspaceConfiguration, externalDWOCMeta, nil) + workspace.Spec.Template.DevWorkspaceTemplateSpecContent = dw.DevWorkspaceTemplateSpecContent{ + Attributes: attributes, + } + + client := fake.NewClientBuilder().WithScheme(scheme).Build() + + err := ApplyExternalDWOCConfig(workspace, client) + if !assert.Error(t, err, "Error should be given if external DWOC specified in workspace spec does not exist") { + return + } +} + +func TestConfigUpdatedAfterMerge(t *testing.T) { + setupForTest(t) + + workspace := &dw.DevWorkspace{} + attributes := attributes.Attributes{} + externalDWOCMeta := v1alpha1.ExternalConfig{} + externalDWOCMeta.Name = "external-config-name" + externalDWOCMeta.Namespace = "external-config-namespace" + + attributes.Put(constants.ExternalDevWorkspaceConfiguration, externalDWOCMeta, nil) + workspace.Spec.Template.DevWorkspaceTemplateSpecContent = dw.DevWorkspaceTemplateSpecContent{ + Attributes: attributes, + } + + clusterConfig := buildConfig(&v1alpha1.OperatorConfiguration{ + Routing: &v1alpha1.RoutingConfig{ + DefaultRoutingClass: "test-routingClass", + ClusterHostSuffix: "192.168.0.1.nip.io", + }, + Workspace: &v1alpha1.WorkspaceConfig{ + ImagePullPolicy: "IfNotPresent", + }, + EnableExperimentalFeatures: &trueBool, + }) + + InternalConfig = clusterConfig.Config.DeepCopy() + + externalConfig := buildExternalConfig(&v1alpha1.OperatorConfiguration{ + Routing: &v1alpha1.RoutingConfig{ + DefaultRoutingClass: "test-routingClass", + ClusterHostSuffix: "192.168.0.1.nip.io", + }, + Workspace: &v1alpha1.WorkspaceConfig{ + ImagePullPolicy: "Always", + }, + EnableExperimentalFeatures: &trueBool, + }) + + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(clusterConfig).WithObjects(externalConfig).Build() + + err := ApplyExternalDWOCConfig(workspace, client) + if !assert.NoError(t, err, "Should not return error") { + return + } + + // Compare the internal config and external config + if !cmp.Equal(InternalConfig, externalConfig.Config) { + t.Error("Internal config and external config should match after merge") + } + + // Get the global config off cluster and ensure it hasn't changed + retrievedClusterConfig := &v1alpha1.DevWorkspaceOperatorConfig{} + namespacedName := types.NamespacedName{ + Name: OperatorConfigName, + Namespace: testNamespace, + } + err = client.Get(context.TODO(), namespacedName, retrievedClusterConfig) + if !assert.NoError(t, err, "Should not return error when fetching config from cluster") { + return + } + + if !cmp.Equal(retrievedClusterConfig.Config, clusterConfig.Config) { + t.Error("Config on cluster and global config should match after merge; global config should not have been modified from merge") + } +} + func TestSetupControllerAlwaysSetsDefaultClusterRoutingSuffix(t *testing.T) { setupForTest(t) infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) diff --git a/pkg/constants/attributes.go b/pkg/constants/attributes.go index 71994662f..f381b6315 100644 --- a/pkg/constants/attributes.go +++ b/pkg/constants/attributes.go @@ -28,6 +28,21 @@ const ( // stopped. DevWorkspaceStorageTypeAttribute = "controller.devfile.io/storage-type" + // ExternalDevWorkspaceConfiguration is an attribute that allows for specifying an (optional) external DevWorkspace-Operator configuration (DWOC) + // which will merged with the internal/global DevWorkspace-Operator configuration. The DWOC resulting from the merge will be used for the workspace. + // The fields which are set in the external DevWorkspace-Operator configuration will overwrite those existing in the + // internal/global DevWorkspace-Operator configuration + // The structure of the attribute value should contain two strings: name and namespace. + // 'name' specifies the metadata.name of the external DevWorkspace-Operator configuration + // 'namespace' specifies the metadata.namespace of the external DevWorkspace-Operator configuration + // For example: + // + // attributes: + // controller.devfile.io/devworkspace-config: + // name: external-dwoc-name + // namespace: some-namespace + ExternalDevWorkspaceConfiguration = "controller.devfile.io/devworkspace-config" + // RuntimeClassNameAttribute is an attribute added to a DevWorkspace to specify a runtimeClassName for container // components in the DevWorkspace (pod.spec.runtimeClassName). If empty, no runtimeClassName is added. RuntimeClassNameAttribute = "controller.devfile.io/runtime-class" diff --git a/pkg/constants/metadata.go b/pkg/constants/metadata.go index 4e99b4bba..11ea1989d 100644 --- a/pkg/constants/metadata.go +++ b/pkg/constants/metadata.go @@ -140,4 +140,8 @@ const ( // NamespaceNodeSelectorAnnotation is an annotation applied to a namespace to configure the node selector for all workspaces // in that namespace. Value should be json-encoded map[string]string NamespaceNodeSelectorAnnotation = "controller.devfile.io/node-selector" + + // TODO: Document these + ExternalDWOCNameAnnotationSuffix = "external-config-name" + ExternalDWOCNamespaceAnnotationSuffix = "external-config-namespace" ) From a86cd79aecb5716a00509ed5f1679f16bd6fae22 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 24 Aug 2022 12:37:46 -0400 Subject: [PATCH 03/14] Don't use workspaceWithConfig struct with clusterAPI + allow basic solver to get config from annotations Signed-off-by: Andrew Obuchowicz --- .../solvers/basic_solver.go | 9 ++-- .../devworkspacerouting/solvers/common.go | 10 ++++ .../workspace/devworkspace_controller.go | 47 ++++--------------- controllers/workspace/finalize.go | 4 +- controllers/workspace/status.go | 2 +- pkg/common/types.go | 3 +- pkg/constants/metadata.go | 9 ++-- pkg/provision/workspace/routing.go | 17 ++++--- 8 files changed, 43 insertions(+), 58 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 39de5d6a2..5b985eb70 100644 --- a/controllers/controller/devworkspacerouting/solvers/basic_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/basic_solver.go @@ -17,10 +17,8 @@ package solvers import ( controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" - "k8s.io/apimachinery/pkg/types" ) var routeAnnotations = func(endpointName string) map[string]string { @@ -58,7 +56,12 @@ func (s *BasicSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error { func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { routingObjects := RoutingObjects{} - routingSuffix := config.Routing.ClusterHostSuffix + routingAnnotationPrefix := string(routing.Spec.RoutingClass) + constants.RoutingAnnotationInfix + if _, set := routing.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix]; !set { + return routingObjects, &RoutingInvalid{"basic routing requires .config.routing.clusterHostSuffix to be set in operator config"} + } + + routingSuffix := routing.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix] if routingSuffix == "" { return routingObjects, &RoutingInvalid{"basic routing requires .config.routing.clusterHostSuffix to be set in operator config"} } diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index bd26dc8ea..e7c3be042 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -138,6 +138,16 @@ func GetServiceForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList } } +func AddDWOCRoutingAnnotations(workspaceWithConfig common.DevWorkspaceWithConfig) { + routingClass := workspaceWithConfig.Spec.RoutingClass + if routingClass == "" { + routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass + } + routingAnnotationPrefix := routingClass + constants.RoutingAnnotationInfix + // TODO: We could also just pass the entire Routing config + workspaceWithConfig.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix] = workspaceWithConfig.Config.Routing.ClusterHostSuffix +} + func getServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []corev1.Service { if len(endpoints) == 0 { return nil diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 9db5f3bc7..721cfbc98 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -24,6 +24,7 @@ import ( devfilevalidation "github.com/devfile/api/v2/pkg/validation" controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/controllers/workspace/metrics" "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/conditions" @@ -111,7 +112,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Fetch the Workspace instance workspaceWithConfig := &common.DevWorkspaceWithConfig{} workspaceWithConfig.Config = *config.InternalConfig - err = r.Get(ctx, req.NamespacedName, workspaceWithConfig) + err = r.Get(ctx, req.NamespacedName, &workspaceWithConfig.DevWorkspace) if err != nil { if k8sErrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. @@ -138,10 +139,10 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if err != nil { workspaceWithConfig.Status.Phase = dw.DevWorkspaceStatusFailed workspaceWithConfig.Status.Message = fmt.Sprintf("Failed to set DevWorkspace ID: %s", err.Error()) - return reconcile.Result{}, r.Status().Update(ctx, workspaceWithConfig) + return reconcile.Result{}, r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) } workspaceWithConfig.Status.DevWorkspaceId = workspaceId - err = r.Status().Update(ctx, workspaceWithConfig) + err = r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) return reconcile.Result{Requeue: true}, err } @@ -158,7 +159,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } patch := []byte(`{"spec":{"started": false}}`) - err := r.Client.Patch(context.Background(), workspaceWithConfig, client.RawPatch(types.MergePatchType, patch)) + err := r.Client.Patch(context.Background(), &workspaceWithConfig.DevWorkspace, client.RawPatch(types.MergePatchType, patch)) if err != nil { return reconcile.Result{}, err } @@ -189,7 +190,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request Message: "DevWorkspace is starting", }, } - err = r.Status().Update(ctx, workspaceWithConfig) + err = r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) if err == nil { metrics.WorkspaceStarted(&workspaceWithConfig.DevWorkspace, reqLogger) } @@ -246,12 +247,8 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request wsDefaults.ApplyDefaultTemplate(workspaceWithConfig) } - // Apply devworkspace routing annotation for external DWOC - // TODO: Cleanup - err = addExternalDWOCAnnotations(*workspaceWithConfig) - if err != nil { - reqLogger.Error(err, "Unable to apply annotations used by Devworkspace Router for external DevWorkspace-Operator configuration") - } + // Apply devworkspace routing annotation(s) to pass DWOC routing settings to DWR + solvers.AddDWOCRoutingAnnotations(*workspaceWithConfig) // Merge workspace's DWOC with an external, one if it exists // TODO: Rework @@ -472,34 +469,6 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{}, nil } -// TODO: Clean up/polish this function -func addExternalDWOCAnnotations(workspaceWithConfig common.DevWorkspaceWithConfig) error { - if !workspaceWithConfig.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { - return nil - } - - ExternalDWOCMeta := &types.NamespacedName{} - - err := workspaceWithConfig.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &ExternalDWOCMeta) - if err != nil { - return fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err) - } - - if ExternalDWOCMeta.Name == "" { - return fmt.Errorf("'name' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) - } - - if ExternalDWOCMeta.Namespace == "" { - return fmt.Errorf("'namespace' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) - } - - annotationPrefix := string(workspaceWithConfig.Spec.RoutingClass) + constants.RoutingAnnotationInfix - workspaceWithConfig.Annotations[annotationPrefix+constants.ExternalDWOCNameAnnotationSuffix] = ExternalDWOCMeta.Name - workspaceWithConfig.Annotations[annotationPrefix+constants.ExternalDWOCNamespaceAnnotationSuffix] = ExternalDWOCMeta.Namespace - return nil - -} - func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *common.DevWorkspaceWithConfig, logger logr.Logger) (reconcile.Result, error) { status := currentStatus{phase: dw.DevWorkspaceStatusStopping} if workspace.Status.Phase == devworkspacePhaseFailing || workspace.Status.Phase == dw.DevWorkspaceStatusFailed { diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index 15a7f280b..c26453d68 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -91,7 +91,7 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L // Namespace is terminating, it's redundant to clean PVC files since it's going to be removed log.Info("Namespace is terminating; clearing storage finalizer") coputil.RemoveFinalizer(workspace, constants.StorageCleanupFinalizer) - return reconcile.Result{}, r.Update(ctx, workspace) + return reconcile.Result{}, r.Update(ctx, &workspace.DevWorkspace) } storageProvisioner, err := storage.GetProvisioner(&workspace.DevWorkspace) @@ -126,7 +126,7 @@ func (r *DevWorkspaceReconciler) finalizeStorage(ctx context.Context, log logr.L } log.Info("PVC clean up successful; clearing finalizer") coputil.RemoveFinalizer(workspace, constants.StorageCleanupFinalizer) - return reconcile.Result{}, r.Update(ctx, workspace) + return reconcile.Result{}, r.Update(ctx, &workspace.DevWorkspace) } func (r *DevWorkspaceReconciler) finalizeServiceAccount(ctx context.Context, log logr.Logger, workspace *dw.DevWorkspace, finalizeStatus *currentStatus) (reconcile.Result, error) { diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 7544712ae..a05814d9d 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -77,7 +77,7 @@ func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspac workspace.Status.Message = infoMessage } - err := r.Status().Update(context.TODO(), workspace) + err := r.Status().Update(context.TODO(), &workspaceWithConfig.DevWorkspace) if err != nil { if k8sErrors.IsConflict(err) { logger.Info("Failed to update workspace status due to conflict; retrying") diff --git a/pkg/common/types.go b/pkg/common/types.go index 852e9fd02..3c679ada7 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -14,9 +14,8 @@ package common import ( - controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" ) //TODO: Rename? diff --git a/pkg/constants/metadata.go b/pkg/constants/metadata.go index 11ea1989d..fa64ec0b3 100644 --- a/pkg/constants/metadata.go +++ b/pkg/constants/metadata.go @@ -117,6 +117,11 @@ const ( // The full annotation name is supposed to be ".routing.controller.devfile.io/" RoutingAnnotationInfix = ".routing.controller.devfile.io/" + // ClusterHostSuffixAnnotationSuffix is an annotation suffix used in conjunction with RoutingAnnotationInfix. + // It is applied as an annotation to DevWorkspaceRouting objects to pass them the cluster host suffix from the DevWorkspace-Operator configuration. + // The suffix is intended to be used in the following manner: .routing.controller.devfile.io/cluster-host-suffix = + ClusterHostSuffixAnnotationSuffix = "cluster-host-suffix" + // DevWorkspaceEndpointNameAnnotation is the annotation key for storing an endpoint's name from the devfile representation DevWorkspaceEndpointNameAnnotation = "controller.devfile.io/endpoint_name" @@ -140,8 +145,4 @@ const ( // NamespaceNodeSelectorAnnotation is an annotation applied to a namespace to configure the node selector for all workspaces // in that namespace. Value should be json-encoded map[string]string NamespaceNodeSelectorAnnotation = "controller.devfile.io/node-selector" - - // TODO: Document these - ExternalDWOCNameAnnotationSuffix = "external-config-name" - ExternalDWOCNamespaceAnnotationSuffix = "external-config-namespace" ) diff --git a/pkg/provision/workspace/routing.go b/pkg/provision/workspace/routing.go index 212f7906f..5e3077f54 100644 --- a/pkg/provision/workspace/routing.go +++ b/pkg/provision/workspace/routing.go @@ -108,19 +108,22 @@ func getSpecRouting( } annotations = maputils.Append(annotations, constants.DevWorkspaceStartedStatusAnnotation, "true") + // TODO: Remove this comment + // I had to move this before the annotations get applied, as otherwise, if the routing class wasn't set in the spec, the annotation would start with a '.' which isin't allowed + routingClass := workspace.Spec.RoutingClass + if routingClass == "" { + // TODO: Just set workspace.Spec.RoutingClass = config.Routing.DefaultRoutingClass ? + routingClass = config.Routing.DefaultRoutingClass + } + // copy the annotations for the specific routingClass from the workspace object to the routing - expectedAnnotationPrefix := workspace.Spec.RoutingClass + constants.RoutingAnnotationInfix + expectedAnnotationPrefix := routingClass + constants.RoutingAnnotationInfix for k, v := range workspace.GetAnnotations() { if strings.HasPrefix(k, expectedAnnotationPrefix) { annotations = maputils.Append(annotations, k, v) } } - routingClass := workspace.Spec.RoutingClass - if routingClass == "" { - routingClass = config.Routing.DefaultRoutingClass - } - routing := &v1alpha1.DevWorkspaceRouting{ ObjectMeta: metav1.ObjectMeta{ Name: common.DevWorkspaceRoutingName(workspace.Status.DevWorkspaceId), @@ -139,7 +142,7 @@ func getSpecRouting( }, }, } - err := controllerutil.SetControllerReference(workspace, routing, scheme) + err := controllerutil.SetControllerReference(&workspaceWithConfig.DevWorkspace, routing, scheme) if err != nil { return nil, err } From 7eedccf2cae68d78d7a33d2959bdf004ee7020b7 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Thu, 25 Aug 2022 12:35:04 -0400 Subject: [PATCH 04/14] Remove some uses of global config routing options Signed-off-by: Andrew Obuchowicz --- .../workspace/devworkspace_controller.go | 20 ++++++----- controllers/workspace/finalize.go | 2 +- controllers/workspace/metrics/update.go | 36 +++++++++---------- controllers/workspace/status.go | 24 ++++++------- pkg/library/env/workspaceenv.go | 5 ++- pkg/provision/workspace/routing.go | 28 +++++++-------- 6 files changed, 58 insertions(+), 57 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 721cfbc98..2c86331dc 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -192,7 +192,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } err = r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) if err == nil { - metrics.WorkspaceStarted(&workspaceWithConfig.DevWorkspace, reqLogger) + metrics.WorkspaceStarted(workspaceWithConfig, reqLogger) } return reconcile.Result{}, err } @@ -201,25 +201,29 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting} reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting") clusterWorkspace := workspaceWithConfig.DevWorkspace.DeepCopy() + clusterWorkspaceWithConfig := &common.DevWorkspaceWithConfig{ + DevWorkspace: *workspaceWithConfig.DevWorkspace.DeepCopy(), + Config: *workspaceWithConfig.Config.DeepCopy(), + } timingInfo := map[string]string{} timing.SetTime(timingInfo, timing.DevWorkspaceStarted) defer func() (reconcile.Result, error) { - r.syncTimingToCluster(ctx, clusterWorkspace, timingInfo, reqLogger) + r.syncTimingToCluster(ctx, &clusterWorkspaceWithConfig.DevWorkspace, timingInfo, reqLogger) // Don't accidentally suppress errors by overwriting here; only check for timeout when no error // encountered in main reconcile loop. if err == nil { - if timeoutErr := checkForStartTimeout(clusterWorkspace, workspaceWithConfig.Config); timeoutErr != nil { + if timeoutErr := checkForStartTimeout(&clusterWorkspaceWithConfig.DevWorkspace, workspaceWithConfig.Config); timeoutErr != nil { reconcileResult, err = r.failWorkspace(&workspaceWithConfig.DevWorkspace, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } } if reconcileStatus.phase == dw.DevWorkspaceStatusRunning { - metrics.WorkspaceRunning(&workspaceWithConfig.DevWorkspace, reqLogger) - r.syncStartedAtToCluster(ctx, clusterWorkspace, reqLogger) + metrics.WorkspaceRunning(workspaceWithConfig, reqLogger) + r.syncStartedAtToCluster(ctx, &clusterWorkspaceWithConfig.DevWorkspace, reqLogger) } - return r.updateWorkspaceStatus(clusterWorkspace, reqLogger, &reconcileStatus, reconcileResult, err) + return r.updateWorkspaceStatus(clusterWorkspaceWithConfig, reqLogger, &reconcileStatus, reconcileResult, err) }() if workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { @@ -347,7 +351,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Step two: Create routing, and wait for routing to be ready timing.SetTime(timingInfo, timing.RoutingCreated) - routingStatus := wsprovision.SyncRoutingToCluster(&workspaceWithConfig.DevWorkspace, clusterAPI) + routingStatus := wsprovision.SyncRoutingToCluster(workspaceWithConfig, clusterAPI) if !routingStatus.Continue { if routingStatus.FailStartup { return r.failWorkspace(&workspaceWithConfig.DevWorkspace, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) @@ -497,7 +501,7 @@ func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *c if stoppedBy, ok := workspace.Annotations[constants.DevWorkspaceStopReasonAnnotation]; ok { logger.Info("Workspace stopped with reason", "stopped-by", stoppedBy) } - return r.updateWorkspaceStatus(&workspace.DevWorkspace, logger, &status, reconcile.Result{}, nil) + return r.updateWorkspaceStatus(workspace, logger, &status, reconcile.Result{}, nil) } func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig *common.DevWorkspaceWithConfig, logger logr.Logger) (stopped bool, err error) { diff --git a/controllers/workspace/finalize.go b/controllers/workspace/finalize.go index c26453d68..6b027580b 100644 --- a/controllers/workspace/finalize.go +++ b/controllers/workspace/finalize.go @@ -60,7 +60,7 @@ func (r *DevWorkspaceReconciler) finalize(ctx context.Context, log logr.Logger, // client.Update() call that removes the last finalizer. return finalizeResult, finalizeErr } - return r.updateWorkspaceStatus(&workspace.DevWorkspace, log, finalizeStatus, finalizeResult, finalizeErr) + return r.updateWorkspaceStatus(workspace, log, finalizeStatus, finalizeResult, finalizeErr) }() for _, finalizer := range workspace.Finalizers { diff --git a/controllers/workspace/metrics/update.go b/controllers/workspace/metrics/update.go index 40c3f6af0..abf045c8c 100644 --- a/controllers/workspace/metrics/update.go +++ b/controllers/workspace/metrics/update.go @@ -20,28 +20,28 @@ import ( "github.com/go-logr/logr" "github.com/prometheus/client_golang/prometheus" + "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/conditions" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" ) // WorkspaceStarted updates metrics for workspaces entering the 'Starting' phase, given a workspace. If an error is // encountered, the provided logger is used to log the error. -func WorkspaceStarted(wksp *dw.DevWorkspace, log logr.Logger) { - _, ok := wksp.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] +func WorkspaceStarted(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { + _, ok := workspaceWithConfig.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] if !ok { - incrementMetricForWorkspace(workspaceTotal, wksp, log) + incrementMetricForWorkspace(workspaceTotal, workspaceWithConfig, log) } } // WorkspaceRunning updates metrics for workspaces entering the 'Running' phase, given a workspace. If an error is // encountered, the provided logger is used to log the error. This function assumes the provided workspace has // fully-synced conditions (i.e. the WorkspaceReady condition is present). -func WorkspaceRunning(wksp *dw.DevWorkspace, log logr.Logger) { - _, ok := wksp.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] +func WorkspaceRunning(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { + _, ok := workspaceWithConfig.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] if !ok { - incrementMetricForWorkspace(workspaceStarts, wksp, log) - incrementStartTimeBucketForWorkspace(wksp, log) + incrementMetricForWorkspace(workspaceStarts, workspaceWithConfig, log) + incrementStartTimeBucketForWorkspace(workspaceWithConfig, log) } } @@ -51,14 +51,14 @@ func WorkspaceFailed(wksp *dw.DevWorkspace, log logr.Logger) { incrementMetricForWorkspaceFailure(workspaceFailures, wksp, log) } -func incrementMetricForWorkspace(metric *prometheus.CounterVec, wksp *dw.DevWorkspace, log logr.Logger) { - sourceLabel := wksp.Labels[workspaceSourceLabel] +func incrementMetricForWorkspace(metric *prometheus.CounterVec, workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { + sourceLabel := workspaceWithConfig.Labels[workspaceSourceLabel] if sourceLabel == "" { sourceLabel = "unknown" } - routingClass := wksp.Spec.RoutingClass + routingClass := workspaceWithConfig.Spec.RoutingClass if routingClass == "" { - routingClass = config.Routing.DefaultRoutingClass + routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass } ctr, err := metric.GetMetricWith(map[string]string{metricSourceLabel: sourceLabel, metricsRoutingClassLabel: routingClass}) if err != nil { @@ -80,24 +80,24 @@ func incrementMetricForWorkspaceFailure(metric *prometheus.CounterVec, wksp *dw. ctr.Inc() } -func incrementStartTimeBucketForWorkspace(wksp *dw.DevWorkspace, log logr.Logger) { - sourceLabel := wksp.Labels[workspaceSourceLabel] +func incrementStartTimeBucketForWorkspace(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { + sourceLabel := workspaceWithConfig.Labels[workspaceSourceLabel] if sourceLabel == "" { sourceLabel = "unknown" } - routingClass := wksp.Spec.RoutingClass + routingClass := workspaceWithConfig.Spec.RoutingClass if routingClass == "" { - routingClass = config.Routing.DefaultRoutingClass + routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass } hist, err := workspaceStartupTimesHist.GetMetricWith(map[string]string{metricSourceLabel: sourceLabel, metricsRoutingClassLabel: routingClass}) if err != nil { log.Error(err, "Failed to update metric") } - readyCondition := conditions.GetConditionByType(wksp.Status.Conditions, dw.DevWorkspaceReady) + readyCondition := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, dw.DevWorkspaceReady) if readyCondition == nil { return } - startedCondition := conditions.GetConditionByType(wksp.Status.Conditions, conditions.Started) + startedCondition := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, conditions.Started) if startedCondition == nil { return } diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index a05814d9d..433a7d87a 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -64,17 +64,17 @@ var clock kubeclock.Clock = &kubeclock.RealClock{} // updateWorkspaceStatus updates the current workspace's status field with conditions and phase from the passed in status. // Parameters for result and error are returned unmodified, unless error is nil and another error is encountered while // updating the status. -func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspace, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) { - syncConditions(&workspace.Status, status) - oldPhase := workspace.Status.Phase - workspace.Status.Phase = status.phase +func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspaceWithConfig *common.DevWorkspaceWithConfig, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) { + syncConditions(&workspaceWithConfig.Status, status) + oldPhase := workspaceWithConfig.Status.Phase + workspaceWithConfig.Status.Phase = status.phase - infoMessage := getInfoMessage(workspace, status) - if warn := conditions.GetConditionByType(workspace.Status.Conditions, conditions.DevWorkspaceWarning); warn != nil && warn.Status == corev1.ConditionTrue { + infoMessage := getInfoMessage(&workspaceWithConfig.DevWorkspace, status) + if warn := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, conditions.DevWorkspaceWarning); warn != nil && warn.Status == corev1.ConditionTrue { infoMessage = fmt.Sprintf("%s %s", warningPresentInfoMessage, infoMessage) } - if workspace.Status.Message != infoMessage { - workspace.Status.Message = infoMessage + if workspaceWithConfig.Status.Message != infoMessage { + workspaceWithConfig.Status.Message = infoMessage } err := r.Status().Update(context.TODO(), &workspaceWithConfig.DevWorkspace) @@ -88,7 +88,7 @@ func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *dw.DevWorkspac } } } else { - updateMetricsForPhase(workspace, oldPhase, status.phase, logger) + updateMetricsForPhase(workspaceWithConfig, oldPhase, status.phase, logger) } return reconcileResult, reconcileError @@ -225,15 +225,15 @@ func getInfoMessage(workspace *dw.DevWorkspace, status *currentStatus) string { // updateMetricsForPhase increments DevWorkspace startup metrics based on phase transitions in a DevWorkspace. It avoids // incrementing the underlying metrics where possible (e.g. reconciling an already running workspace) by only incrementing // counters when the new phase is different from the current on in the DevWorkspace. -func updateMetricsForPhase(workspace *dw.DevWorkspace, oldPhase, newPhase dw.DevWorkspacePhase, logger logr.Logger) { +func updateMetricsForPhase(workspaceWithConfig *common.DevWorkspaceWithConfig, oldPhase, newPhase dw.DevWorkspacePhase, logger logr.Logger) { if oldPhase == newPhase { return } switch newPhase { case dw.DevWorkspaceStatusRunning: - metrics.WorkspaceRunning(workspace, logger) + metrics.WorkspaceRunning(workspaceWithConfig, logger) case dw.DevWorkspaceStatusFailed: - metrics.WorkspaceFailed(workspace, logger) + metrics.WorkspaceFailed(&workspaceWithConfig.DevWorkspace, logger) } } diff --git a/pkg/library/env/workspaceenv.go b/pkg/library/env/workspaceenv.go index 84fa7aa20..ed92a74d0 100644 --- a/pkg/library/env/workspaceenv.go +++ b/pkg/library/env/workspaceenv.go @@ -25,7 +25,6 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" ) @@ -72,12 +71,12 @@ func commonEnvironmentVariables(workspaceName, workspaceId, namespace, creator s }, } - envvars = append(envvars, getProxyEnvVars()...) + envvars = append(envvars, getProxyEnvVars(config)...) return envvars } -func getProxyEnvVars() []corev1.EnvVar { +func getProxyEnvVars(config controllerv1alpha1.OperatorConfiguration) []corev1.EnvVar { if config.Routing.ProxyConfig == nil { return nil } diff --git a/pkg/provision/workspace/routing.go b/pkg/provision/workspace/routing.go index 5e3077f54..8a424cf58 100644 --- a/pkg/provision/workspace/routing.go +++ b/pkg/provision/workspace/routing.go @@ -18,14 +18,12 @@ package workspace import ( "strings" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/conversion" "github.com/devfile/devworkspace-operator/pkg/provision/sync" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" maputils "github.com/devfile/devworkspace-operator/internal/map" "github.com/devfile/devworkspace-operator/pkg/common" - "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -40,10 +38,10 @@ type RoutingProvisioningStatus struct { } func SyncRoutingToCluster( - workspace *dw.DevWorkspace, + workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) RoutingProvisioningStatus { - specRouting, err := getSpecRouting(workspace, clusterAPI.Scheme) + specRouting, err := getSpecRouting(workspaceWithConfig, clusterAPI.Scheme) if err != nil { return RoutingProvisioningStatus{ ProvisioningStatus: ProvisioningStatus{Err: err}, @@ -88,11 +86,11 @@ func SyncRoutingToCluster( } func getSpecRouting( - workspace *dw.DevWorkspace, + workspaceWithConfig *common.DevWorkspaceWithConfig, scheme *runtime.Scheme) (*v1alpha1.DevWorkspaceRouting, error) { endpoints := map[string]v1alpha1.EndpointList{} - for _, component := range workspace.Spec.Template.Components { + for _, component := range workspaceWithConfig.Spec.Template.Components { if component.Container == nil { continue } @@ -103,22 +101,22 @@ func getSpecRouting( } var annotations map[string]string - if val, ok := workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; ok { + if val, ok := workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; ok { annotations = maputils.Append(annotations, constants.DevWorkspaceRestrictedAccessAnnotation, val) } annotations = maputils.Append(annotations, constants.DevWorkspaceStartedStatusAnnotation, "true") // TODO: Remove this comment // I had to move this before the annotations get applied, as otherwise, if the routing class wasn't set in the spec, the annotation would start with a '.' which isin't allowed - routingClass := workspace.Spec.RoutingClass + routingClass := workspaceWithConfig.Spec.RoutingClass if routingClass == "" { // TODO: Just set workspace.Spec.RoutingClass = config.Routing.DefaultRoutingClass ? - routingClass = config.Routing.DefaultRoutingClass + routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass } // copy the annotations for the specific routingClass from the workspace object to the routing expectedAnnotationPrefix := routingClass + constants.RoutingAnnotationInfix - for k, v := range workspace.GetAnnotations() { + for k, v := range workspaceWithConfig.GetAnnotations() { if strings.HasPrefix(k, expectedAnnotationPrefix) { annotations = maputils.Append(annotations, k, v) } @@ -126,19 +124,19 @@ func getSpecRouting( routing := &v1alpha1.DevWorkspaceRouting{ ObjectMeta: metav1.ObjectMeta{ - Name: common.DevWorkspaceRoutingName(workspace.Status.DevWorkspaceId), - Namespace: workspace.Namespace, + Name: common.DevWorkspaceRoutingName(workspaceWithConfig.Status.DevWorkspaceId), + Namespace: workspaceWithConfig.Namespace, Labels: map[string]string{ - constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, + constants.DevWorkspaceIDLabel: workspaceWithConfig.Status.DevWorkspaceId, }, Annotations: annotations, }, Spec: v1alpha1.DevWorkspaceRoutingSpec{ - DevWorkspaceId: workspace.Status.DevWorkspaceId, + DevWorkspaceId: workspaceWithConfig.Status.DevWorkspaceId, RoutingClass: v1alpha1.DevWorkspaceRoutingClass(routingClass), Endpoints: endpoints, PodSelector: map[string]string{ - constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, + constants.DevWorkspaceIDLabel: workspaceWithConfig.Status.DevWorkspaceId, }, }, } From 58b30dc6b9cee6cca87afbd0f0d07fc61d5b5c14 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Fri, 26 Aug 2022 10:30:57 -0400 Subject: [PATCH 05/14] use new merge function to get external DWOC Signed-off-by: Andrew Obuchowicz --- .../workspace/devworkspace_controller.go | 16 +- pkg/config/common_test.go | 2 +- pkg/config/sync.go | 262 ++++-------------- pkg/config/sync_test.go | 38 ++- pkg/provision/storage/commonStorage_test.go | 6 +- .../storage/perWorkspaceStorage_test.go | 2 +- 6 files changed, 85 insertions(+), 241 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 2c86331dc..66b84b6ec 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -111,7 +111,6 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Fetch the Workspace instance workspaceWithConfig := &common.DevWorkspaceWithConfig{} - workspaceWithConfig.Config = *config.InternalConfig err = r.Get(ctx, req.NamespacedName, &workspaceWithConfig.DevWorkspace) if err != nil { if k8sErrors.IsNotFound(err) { @@ -123,6 +122,14 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Error reading the object - requeue the request. return reconcile.Result{}, err } + + // Get the config for the workspace + resolvedConfig, err := config.ResolveConfigForWorkspace(&workspaceWithConfig.DevWorkspace, clusterAPI.Client) + workspaceWithConfig.Config = *resolvedConfig + if err != nil { + reqLogger.Error(err, "Unable to apply external DevWorkspace-Operator configuration") + } + reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspaceWithConfig.Status.DevWorkspaceId) reqLogger.Info("Reconciling Workspace") @@ -254,13 +261,6 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Apply devworkspace routing annotation(s) to pass DWOC routing settings to DWR solvers.AddDWOCRoutingAnnotations(*workspaceWithConfig) - // Merge workspace's DWOC with an external, one if it exists - // TODO: Rework - err = config.ApplyExternalDWOCConfig(&workspaceWithConfig.DevWorkspace, clusterAPI.Client) - if err != nil { - reqLogger.Error(err, "Unable to apply external DevWorkspace-Operator configuration") - } - flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) if err != nil { return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) diff --git a/pkg/config/common_test.go b/pkg/config/common_test.go index ad9322a2d..3aae90e47 100644 --- a/pkg/config/common_test.go +++ b/pkg/config/common_test.go @@ -54,7 +54,7 @@ func setupForTest(t *testing.T) { configNamespace = testNamespace originalDefaultConfig := defaultConfig.DeepCopy() t.Cleanup(func() { - InternalConfig = nil + internalConfig = nil defaultConfig = originalDefaultConfig }) } diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 29c1bf26b..44a0ff129 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -18,13 +18,10 @@ package config import ( "context" "fmt" - "reflect" - "sort" "strings" "sync" dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" controller "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/config/proxy" "github.com/devfile/devworkspace-operator/pkg/constants" @@ -47,73 +44,59 @@ const ( var ( Routing *controller.RoutingConfig Workspace *controller.WorkspaceConfig - InternalConfig *controller.OperatorConfiguration // TODO: Make this unexported again + internalConfig *controller.OperatorConfiguration configMutex sync.Mutex configNamespace string log = ctrl.Log.WithName("operator-configuration") ) -// TODO: Refactor this to return the modified config, so test classes don't acces the InternalConfig -func SetConfigForTesting(config *controller.OperatorConfiguration) { +func SetConfigForTesting(config *controller.OperatorConfiguration) *controller.OperatorConfiguration { configMutex.Lock() defer configMutex.Unlock() - InternalConfig = defaultConfig.DeepCopy() - mergeConfig(config, InternalConfig) + internalConfig = defaultConfig.DeepCopy() + mergeConfig(config, internalConfig) updatePublicConfig() + return internalConfig } -func ApplyExternalDWOCConfig(workspace *dw.DevWorkspace, client crclient.Client) (err error) { +// Checks for a external DevWorkspace-Operator configuration specified by the optional workspace attribute "controller.devfile.io/devworkspace-config" +// If the external DWOC is found, it is merged with the internal/global DWOC and returned. +// If the exteranl DWOC is not found, or the "controller.devfile.io/devworkspace-config" attribute is not set, then the internal/global DWOC is returned. +func ResolveConfigForWorkspace(workspace *dw.DevWorkspace, client crclient.Client) (*controller.OperatorConfiguration, error) { + internalConfigCopy := internalConfig.DeepCopy() + if workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { + namespacedName := types.NamespacedName{} - if !workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { - return nil - } - - ExternalDWOCMeta := v1alpha1.ExternalConfig{} - - err = workspace.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &ExternalDWOCMeta) - if err != nil { - return fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err) - } - - if ExternalDWOCMeta.Name == "" { - return fmt.Errorf("'name' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) - } - - if ExternalDWOCMeta.Namespace == "" { - return fmt.Errorf("'namespace' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) - } + err := workspace.Spec.Template.Attributes.GetInto(constants.ExternalDevWorkspaceConfiguration, &namespacedName) + if err != nil { + return internalConfigCopy, fmt.Errorf("failed to read attribute %s in DevWorkspace attributes: %w", constants.ExternalDevWorkspaceConfiguration, err) + } - externalDWOC := &controller.DevWorkspaceOperatorConfig{} - namespacedName := types.NamespacedName{ - Name: ExternalDWOCMeta.Name, - Namespace: ExternalDWOCMeta.Namespace, - } + if namespacedName.Name == "" { + return internalConfigCopy, fmt.Errorf("'name' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } - err = client.Get(context.TODO(), namespacedName, externalDWOC) - if err != nil { - return fmt.Errorf("could not fetch external DWOC with name '%s' in namespace '%s': %w", ExternalDWOCMeta.Name, ExternalDWOCMeta.Namespace, err) - } + if namespacedName.Namespace == "" { + return internalConfigCopy, fmt.Errorf("'namespace' must be set for attribute %s in DevWorkspace attributes", constants.ExternalDevWorkspaceConfiguration) + } - // TODO: It might be better to just always merge rather than suffering performance hit in configsAlreadyMerged() (or maintaing configsAlreadyMerged if we don't use reflect.DeepEqual) - if !configsAlreadyMerged(externalDWOC.Config, InternalConfig) { - mergeInteralConfigWithExternal(externalDWOC.Config) + externalDWOC := &controller.DevWorkspaceOperatorConfig{} + err = client.Get(context.TODO(), namespacedName, externalDWOC) + if err != nil { + return internalConfigCopy, fmt.Errorf("could not fetch external DWOC with name '%s' in namespace '%s': %w", namespacedName.Name, namespacedName.Namespace, err) + } + // TODO: Do we need to lock the internal config? + return getMergedConfig(externalDWOC.Config, internalConfig), nil } - return nil -} - -func mergeInteralConfigWithExternal(externalConfig *controller.OperatorConfiguration) { - configMutex.Lock() - defer configMutex.Unlock() - mergeConfig(externalConfig, InternalConfig) - updatePublicConfig() + return internalConfigCopy, nil } func SetupControllerConfig(client crclient.Client) error { - if InternalConfig != nil { + if internalConfig != nil { return fmt.Errorf("internal controller configuration is already set up") } - InternalConfig = &controller.OperatorConfiguration{} + internalConfig = &controller.OperatorConfiguration{} namespace, err := infrastructure.GetNamespace() if err != nil { @@ -126,7 +109,7 @@ func SetupControllerConfig(client crclient.Client) error { return err } if config == nil { - InternalConfig = defaultConfig.DeepCopy() + internalConfig = defaultConfig.DeepCopy() } else { syncConfigFrom(config) } @@ -136,8 +119,8 @@ func SetupControllerConfig(client crclient.Client) error { return err } defaultConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix - if InternalConfig.Routing.ClusterHostSuffix == "" { - InternalConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix + if internalConfig.Routing.ClusterHostSuffix == "" { + internalConfig.Routing.ClusterHostSuffix = defaultRoutingSuffix } clusterProxy, err := proxy.GetClusterProxyConfig(client) @@ -145,21 +128,21 @@ func SetupControllerConfig(client crclient.Client) error { return err } defaultConfig.Routing.ProxyConfig = clusterProxy - InternalConfig.Routing.ProxyConfig = proxy.MergeProxyConfigs(clusterProxy, InternalConfig.Routing.ProxyConfig) + internalConfig.Routing.ProxyConfig = proxy.MergeProxyConfigs(clusterProxy, internalConfig.Routing.ProxyConfig) updatePublicConfig() return nil } func IsSetUp() bool { - return InternalConfig != nil + return internalConfig != nil } func ExperimentalFeaturesEnabled() bool { - if InternalConfig == nil || InternalConfig.EnableExperimentalFeatures == nil { + if internalConfig == nil || internalConfig.EnableExperimentalFeatures == nil { return false } - return *InternalConfig.EnableExperimentalFeatures + return *internalConfig.EnableExperimentalFeatures } func getClusterConfig(namespace string, client crclient.Client) (*controller.DevWorkspaceOperatorConfig, error) { @@ -179,21 +162,29 @@ func syncConfigFrom(newConfig *controller.DevWorkspaceOperatorConfig) { } configMutex.Lock() defer configMutex.Unlock() - InternalConfig = defaultConfig.DeepCopy() - mergeConfig(newConfig.Config, InternalConfig) + internalConfig = defaultConfig.DeepCopy() + mergeConfig(newConfig.Config, internalConfig) updatePublicConfig() } +func getMergedConfig(from, to *controller.OperatorConfiguration) *controller.OperatorConfiguration { + configMutex.Lock() + defer configMutex.Unlock() + mergedConfig := to.DeepCopy() + mergeConfig(from, mergedConfig) + return mergedConfig +} + func restoreDefaultConfig() { configMutex.Lock() defer configMutex.Unlock() - InternalConfig = defaultConfig.DeepCopy() + internalConfig = defaultConfig.DeepCopy() updatePublicConfig() } func updatePublicConfig() { - Routing = InternalConfig.Routing.DeepCopy() - Workspace = InternalConfig.Workspace.DeepCopy() + Routing = internalConfig.Routing.DeepCopy() + Workspace = internalConfig.Workspace.DeepCopy() logCurrentConfig() } @@ -239,147 +230,6 @@ func discoverRouteSuffix(client crclient.Client) (string, error) { return host, nil } -// TODO: Improve variable names? -// Returns true if 'from' has already been merged with 'to'. -// The two configs are considered merged if all fields that are set in 'from' have the same value in 'to' -func configsAlreadyMerged(from, to *controller.OperatorConfiguration) bool { - if from == nil { - return true - } - if from.EnableExperimentalFeatures != nil { - - if to.EnableExperimentalFeatures == nil { - return false - } - - if *to.EnableExperimentalFeatures != *from.EnableExperimentalFeatures { - return false - } - } - if from.Routing != nil { - if to.Routing == nil { - return false - } - if from.Routing.DefaultRoutingClass != "" && to.Routing.DefaultRoutingClass != from.Routing.DefaultRoutingClass { - return false - } - if from.Routing.ClusterHostSuffix != "" && to.Routing.ClusterHostSuffix != from.Routing.ClusterHostSuffix { - return false - } - if from.Routing.ProxyConfig != nil { - if to.Routing.ProxyConfig == nil { - return false - } - - if from.Routing.ProxyConfig.HttpProxy != "" && to.Routing.ProxyConfig.HttpProxy != from.Routing.ProxyConfig.HttpProxy { - return false - } - - if from.Routing.ProxyConfig.HttpsProxy != "" && to.Routing.ProxyConfig.HttpsProxy != from.Routing.ProxyConfig.HttpsProxy { - return false - } - - if from.Routing.ProxyConfig.NoProxy != "" && to.Routing.ProxyConfig.NoProxy != from.Routing.ProxyConfig.NoProxy { - return false - } - } - } - if from.Workspace != nil { - if to.Workspace == nil { - return false - } - if from.Workspace.StorageClassName != nil { - if to.Workspace.StorageClassName == nil { - return false - } - - if *to.Workspace.StorageClassName != *from.Workspace.StorageClassName { - return false - } - } - if from.Workspace.PVCName != "" && to.Workspace.PVCName != from.Workspace.PVCName { - return false - } - if from.Workspace.ImagePullPolicy != "" && to.Workspace.ImagePullPolicy != from.Workspace.ImagePullPolicy { - return false - } - if from.Workspace.IdleTimeout != "" && to.Workspace.IdleTimeout != from.Workspace.IdleTimeout { - return false - } - if from.Workspace.ProgressTimeout != "" && to.Workspace.ProgressTimeout != from.Workspace.ProgressTimeout { - return false - } - if from.Workspace.IgnoredUnrecoverableEvents != nil && to.Workspace.IgnoredUnrecoverableEvents != nil { - - if len(from.Workspace.IgnoredUnrecoverableEvents) != len(to.Workspace.IgnoredUnrecoverableEvents) { - return false - } - - sort.Strings(to.Workspace.IgnoredUnrecoverableEvents) - sort.Strings(from.Workspace.IgnoredUnrecoverableEvents) - - for i := range from.Workspace.IgnoredUnrecoverableEvents { - if from.Workspace.IgnoredUnrecoverableEvents[i] != to.Workspace.IgnoredUnrecoverableEvents[i] { - return false - } - } - } - if from.Workspace.CleanupOnStop != nil { - if to.Workspace.CleanupOnStop == nil { - return false - } - - if *to.Workspace.CleanupOnStop != *from.Workspace.CleanupOnStop { - return false - } - } - if from.Workspace.PodSecurityContext != nil { - if to.Workspace.PodSecurityContext == nil { - return false - } - - // TODO: Using reflect.DeepEqual could potentially really degrade performance - if !reflect.DeepEqual(from.Workspace.PodSecurityContext, to.Workspace.PodSecurityContext) { - return false - } - } - if from.Workspace.DefaultStorageSize != nil { - if to.Workspace.DefaultStorageSize == nil { - return false - } - if from.Workspace.DefaultStorageSize.Common != nil { - if to.Workspace.DefaultStorageSize.Common == nil { - return false - } - if from.Workspace.DefaultStorageSize.Common.Cmp(*to.Workspace.DefaultStorageSize.Common) != 0 { - return false - } - - } - if from.Workspace.DefaultStorageSize.PerWorkspace != nil { - if to.Workspace.DefaultStorageSize.PerWorkspace == nil { - return false - } - if from.Workspace.DefaultStorageSize.PerWorkspace.Cmp(*to.Workspace.DefaultStorageSize.PerWorkspace) != 0 { - return false - } - } - - } - if from.Workspace.DefaultTemplate != nil { - if to.Workspace.DefaultTemplate == nil { - return false - } - - // TODO: Using reflect.DeepEqual could potentially really degrade performance - if !reflect.DeepEqual(from.Workspace.DefaultTemplate, to.Workspace.DefaultTemplate) { - return false - } - } - } - return true -} - func mergeConfig(from, to *controller.OperatorConfiguration) { if to == nil { to = &controller.OperatorConfiguration{} @@ -457,7 +307,7 @@ func mergeConfig(from, to *controller.OperatorConfiguration) { // logCurrentConfig formats the current operator configuration as a plain string func logCurrentConfig() { - if InternalConfig == nil { + if internalConfig == nil { return } var config []string @@ -498,7 +348,7 @@ func logCurrentConfig() { config = append(config, fmt.Sprintf("workspace.defaultTemplate is set")) } } - if InternalConfig.EnableExperimentalFeatures != nil && *InternalConfig.EnableExperimentalFeatures { + if internalConfig.EnableExperimentalFeatures != nil && *internalConfig.EnableExperimentalFeatures { config = append(config, "enableExperimentalFeatures=true") } @@ -508,7 +358,7 @@ func logCurrentConfig() { log.Info(fmt.Sprintf("Updated config to [%s]", strings.Join(config, ","))) } - if InternalConfig.Routing.ProxyConfig != nil { - log.Info("Resolved proxy configuration", "proxy", InternalConfig.Routing.ProxyConfig) + if internalConfig.Routing.ProxyConfig != nil { + log.Info("Resolved proxy configuration", "proxy", internalConfig.Routing.ProxyConfig) } } diff --git a/pkg/config/sync_test.go b/pkg/config/sync_test.go index ea1e70753..16e3cc228 100644 --- a/pkg/config/sync_test.go +++ b/pkg/config/sync_test.go @@ -16,24 +16,18 @@ package config import ( - "context" "fmt" "testing" - attributes "github.com/devfile/api/v2/pkg/attributes" - - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/google/go-cmp/cmp" fuzz "github.com/google/gofuzz" routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" ) @@ -44,7 +38,7 @@ func TestSetupControllerConfigUsesDefault(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, defaultConfig, InternalConfig, "Config used should be the default") + assert.Equal(t, defaultConfig, internalConfig, "Config used should be the default") } func TestSetupControllerDefaultsAreExported(t *testing.T) { @@ -69,7 +63,7 @@ func TestSetupControllerConfigFailsWhenAlreadySetup(t *testing.T) { if !assert.Error(t, err, "Should return error when config is already setup") { return } - assert.Equal(t, defaultConfig, InternalConfig, "Config used should be the default") + assert.Equal(t, defaultConfig, internalConfig, "Config used should be the default") } func TestSetupControllerMergesClusterConfig(t *testing.T) { @@ -97,12 +91,12 @@ func TestSetupControllerMergesClusterConfig(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, expectedConfig, InternalConfig, fmt.Sprintf("Processed config should merge settings from cluster: %s", cmp.Diff(InternalConfig, expectedConfig))) - assert.Equal(t, InternalConfig.Routing, Routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) - assert.Equal(t, InternalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, expectedConfig, internalConfig, fmt.Sprintf("Processed config should merge settings from cluster: %s", cmp.Diff(internalConfig, expectedConfig))) + assert.Equal(t, internalConfig.Routing, Routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, internalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) } -func TestCatchesNonExistentExternalDWOC(t *testing.T) { +/* func TestCatchesNonExistentExternalDWOC(t *testing.T) { setupForTest(t) workspace := &dw.DevWorkspace{} @@ -149,7 +143,7 @@ func TestConfigUpdatedAfterMerge(t *testing.T) { EnableExperimentalFeatures: &trueBool, }) - InternalConfig = clusterConfig.Config.DeepCopy() + internalConfig = clusterConfig.Config.DeepCopy() externalConfig := buildExternalConfig(&v1alpha1.OperatorConfiguration{ Routing: &v1alpha1.RoutingConfig{ @@ -170,7 +164,7 @@ func TestConfigUpdatedAfterMerge(t *testing.T) { } // Compare the internal config and external config - if !cmp.Equal(InternalConfig, externalConfig.Config) { + if !cmp.Equal(internalConfig, externalConfig.Config) { t.Error("Internal config and external config should match after merge") } @@ -189,7 +183,7 @@ func TestConfigUpdatedAfterMerge(t *testing.T) { t.Error("Config on cluster and global config should match after merge; global config should not have been modified from merge") } } - +*/ func TestSetupControllerAlwaysSetsDefaultClusterRoutingSuffix(t *testing.T) { setupForTest(t) infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) @@ -233,12 +227,12 @@ func TestDetectsOpenShiftRouteSuffix(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, "test-host", InternalConfig.Routing.ClusterHostSuffix, "Should determine host suffix with route on OpenShift") + assert.Equal(t, "test-host", internalConfig.Routing.ClusterHostSuffix, "Should determine host suffix with route on OpenShift") } func TestSyncConfigFromIgnoresOtherConfigInOtherNamespaces(t *testing.T) { setupForTest(t) - InternalConfig = defaultConfig.DeepCopy() + internalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Workspace: &v1alpha1.WorkspaceConfig{ ImagePullPolicy: "IfNotPresent", @@ -246,12 +240,12 @@ func TestSyncConfigFromIgnoresOtherConfigInOtherNamespaces(t *testing.T) { }) config.Namespace = "not-test-namespace" syncConfigFrom(config) - assert.Equal(t, defaultConfig, InternalConfig) + assert.Equal(t, defaultConfig, internalConfig) } func TestSyncConfigFromIgnoresOtherConfigWithOtherName(t *testing.T) { setupForTest(t) - InternalConfig = defaultConfig.DeepCopy() + internalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Workspace: &v1alpha1.WorkspaceConfig{ ImagePullPolicy: "IfNotPresent", @@ -259,13 +253,13 @@ func TestSyncConfigFromIgnoresOtherConfigWithOtherName(t *testing.T) { }) config.Name = "testing-name" syncConfigFrom(config) - assert.Equal(t, defaultConfig, InternalConfig) + assert.Equal(t, defaultConfig, internalConfig) } func TestSyncConfigDoesNotChangeDefaults(t *testing.T) { setupForTest(t) oldDefaultConfig := defaultConfig.DeepCopy() - InternalConfig = defaultConfig.DeepCopy() + internalConfig = defaultConfig.DeepCopy() config := buildConfig(&v1alpha1.OperatorConfiguration{ Routing: &v1alpha1.RoutingConfig{ DefaultRoutingClass: "test-routingClass", @@ -277,7 +271,7 @@ func TestSyncConfigDoesNotChangeDefaults(t *testing.T) { EnableExperimentalFeatures: &trueBool, }) syncConfigFrom(config) - InternalConfig.Routing.DefaultRoutingClass = "Changed after the fact" + internalConfig.Routing.DefaultRoutingClass = "Changed after the fact" assert.Equal(t, defaultConfig, oldDefaultConfig) } diff --git a/pkg/provision/storage/commonStorage_test.go b/pkg/provision/storage/commonStorage_test.go index d0c4819af..21b49aa72 100644 --- a/pkg/provision/storage/commonStorage_test.go +++ b/pkg/provision/storage/commonStorage_test.go @@ -75,7 +75,7 @@ var testControllerCfg = &v1alpha1.OperatorConfiguration{ } func setupControllerCfg() { - config.SetConfigForTesting(testControllerCfg) + testControllerCfg = config.SetConfigForTesting(testControllerCfg) } func loadTestCaseOrPanic(t *testing.T, testFilepath string) testCase { @@ -141,7 +141,7 @@ func TestProvisionStorageForCommonStorageClass(t *testing.T) { // sanity check that file is read correctly. assert.NotNil(t, tt.Input.Workspace, "Input does not define workspace") workspace := &common.DevWorkspaceWithConfig{} - workspace.Config = *config.InternalConfig + workspace.Config = *testControllerCfg workspace.Spec.Template = *tt.Input.Workspace workspace.Status.DevWorkspaceId = tt.Input.DevWorkspaceID workspace.Namespace = "test-namespace" @@ -178,7 +178,7 @@ func TestTerminatingPVC(t *testing.T) { testCase := loadTestCaseOrPanic(t, "testdata/common-storage/rewrites-volumes-for-common-pvc-strategy.yaml") assert.NotNil(t, testCase.Input.Workspace, "Input does not define workspace") workspace := &common.DevWorkspaceWithConfig{} - workspace.Config = *config.InternalConfig + workspace.Config = *testControllerCfg workspace.Spec.Template = *testCase.Input.Workspace workspace.Status.DevWorkspaceId = testCase.Input.DevWorkspaceID workspace.Namespace = "test-namespace" diff --git a/pkg/provision/storage/perWorkspaceStorage_test.go b/pkg/provision/storage/perWorkspaceStorage_test.go index 18ff38ca4..e6f7e69ca 100644 --- a/pkg/provision/storage/perWorkspaceStorage_test.go +++ b/pkg/provision/storage/perWorkspaceStorage_test.go @@ -53,7 +53,7 @@ func TestRewriteContainerVolumeMountsForPerWorkspaceStorageClass(t *testing.T) { // sanity check that file is read correctly. assert.NotNil(t, tt.Input.Workspace, "Input does not define workspace") workspace := &common.DevWorkspaceWithConfig{} - workspace.Config = *config.InternalConfig + workspace.Config = *testControllerCfg workspace.Spec.Template = *tt.Input.Workspace workspace.Status.DevWorkspaceId = tt.Input.DevWorkspaceID workspace.Namespace = "test-namespace" From a0b4295340b80c05ebc06eef22e5d5e491a929a3 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Fri, 26 Aug 2022 20:40:27 -0400 Subject: [PATCH 06/14] Remove duplicate import Signed-off-by: Andrew Obuchowicz --- controllers/workspace/status.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 433a7d87a..1e23ce940 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -33,7 +33,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/controllers/workspace/metrics" "github.com/devfile/devworkspace-operator/pkg/conditions" ) @@ -241,7 +240,7 @@ func updateMetricsForPhase(workspaceWithConfig *common.DevWorkspaceWithConfig, o // startup timeout. This is determined by checking to see if the last condition transition time is more // than [timeout] duration ago. Workspaces that are not in the "Starting" phase cannot timeout. Returns // an error with message when timeout is reached. -func checkForStartTimeout(workspace *dw.DevWorkspace, config controllerv1alpha1.OperatorConfiguration) error { +func checkForStartTimeout(workspace *dw.DevWorkspace, config v1alpha1.OperatorConfiguration) error { if workspace.Status.Phase != dw.DevWorkspaceStatusStarting { return nil } From 6559ecdf461bb70e6073f50552a12bf661c1efa4 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Fri, 26 Aug 2022 20:43:24 -0400 Subject: [PATCH 07/14] Mention limitations of per-workspace DWOC Signed-off-by: Andrew Obuchowicz --- controllers/workspace/devworkspace_controller.go | 2 +- controllers/workspace/http.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 66b84b6ec..c97d2ad68 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -692,7 +692,7 @@ func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Req } } - // TODO: Address this usage of global config? + // TODO: Address this usage of global config. It'd be better to check if the PVC had a DWO-specific annotation that we can use to filter for // Otherwise, check if common PVC is deleted to make sure all DevWorkspaces see it happen if obj.GetName() != config.Workspace.PVCName || obj.GetDeletionTimestamp() == nil { // We're looking for a deleted common PVC diff --git a/controllers/workspace/http.go b/controllers/workspace/http.go index 5e032d440..acfd0424e 100644 --- a/controllers/workspace/http.go +++ b/controllers/workspace/http.go @@ -34,6 +34,8 @@ func setupHttpClients() { InsecureSkipVerify: true, } + // Note: per-workspace routing settings (that have been set with an external DWOC) + // are ignored for the HTTP Clients if config.Routing != nil && config.Routing.ProxyConfig != nil { proxyConf := httpproxy.Config{ HTTPProxy: config.Routing.ProxyConfig.HttpProxy, From 0a68b02230d5196716b6233e185ce0a1c2dfdde4 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Mon, 29 Aug 2022 23:12:52 -0400 Subject: [PATCH 08/14] Rework sync tests for external DWOC Signed-off-by: Andrew Obuchowicz --- pkg/config/sync_test.go | 97 +++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 43 deletions(-) diff --git a/pkg/config/sync_test.go b/pkg/config/sync_test.go index 16e3cc228..ecb69b1b2 100644 --- a/pkg/config/sync_test.go +++ b/pkg/config/sync_test.go @@ -16,18 +16,24 @@ package config import ( + "context" "fmt" "testing" + dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + attributes "github.com/devfile/api/v2/pkg/attributes" "github.com/google/go-cmp/cmp" fuzz "github.com/google/gofuzz" routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" ) @@ -96,81 +102,86 @@ func TestSetupControllerMergesClusterConfig(t *testing.T) { assert.Equal(t, internalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) } -/* func TestCatchesNonExistentExternalDWOC(t *testing.T) { +func TestCatchesNonExistentExternalDWOC(t *testing.T) { setupForTest(t) workspace := &dw.DevWorkspace{} attributes := attributes.Attributes{} - externalDWOCMeta := v1alpha1.ExternalConfig{} - externalDWOCMeta.Name = "external-config-name" - externalDWOCMeta.Namespace = "external-config-namespace" - - attributes.Put(constants.ExternalDevWorkspaceConfiguration, externalDWOCMeta, nil) + namespacedName := types.NamespacedName{ + Name: "external-config-name", + Namespace: "external-config-namespace", + } + attributes.Put(constants.ExternalDevWorkspaceConfiguration, namespacedName, nil) workspace.Spec.Template.DevWorkspaceTemplateSpecContent = dw.DevWorkspaceTemplateSpecContent{ Attributes: attributes, } - client := fake.NewClientBuilder().WithScheme(scheme).Build() - err := ApplyExternalDWOCConfig(workspace, client) + resolvedConfig, err := ResolveConfigForWorkspace(workspace, client) if !assert.Error(t, err, "Error should be given if external DWOC specified in workspace spec does not exist") { return } + assert.Equal(t, resolvedConfig, internalConfig, "Internal/Global DWOC should be used as fallback if external DWOC could not be resolved") } -func TestConfigUpdatedAfterMerge(t *testing.T) { +func TestMergeExternalConfig(t *testing.T) { setupForTest(t) workspace := &dw.DevWorkspace{} attributes := attributes.Attributes{} - externalDWOCMeta := v1alpha1.ExternalConfig{} - externalDWOCMeta.Name = "external-config-name" - externalDWOCMeta.Namespace = "external-config-namespace" + namespacedName := types.NamespacedName{ + Name: ExternalConfigName, + Namespace: ExternalConfigNamespace, + } - attributes.Put(constants.ExternalDevWorkspaceConfiguration, externalDWOCMeta, nil) + attributes.Put(constants.ExternalDevWorkspaceConfiguration, namespacedName, nil) workspace.Spec.Template.DevWorkspaceTemplateSpecContent = dw.DevWorkspaceTemplateSpecContent{ Attributes: attributes, } - clusterConfig := buildConfig(&v1alpha1.OperatorConfiguration{ - Routing: &v1alpha1.RoutingConfig{ - DefaultRoutingClass: "test-routingClass", - ClusterHostSuffix: "192.168.0.1.nip.io", - }, - Workspace: &v1alpha1.WorkspaceConfig{ - ImagePullPolicy: "IfNotPresent", - }, - EnableExperimentalFeatures: &trueBool, - }) - - internalConfig = clusterConfig.Config.DeepCopy() - - externalConfig := buildExternalConfig(&v1alpha1.OperatorConfiguration{ - Routing: &v1alpha1.RoutingConfig{ - DefaultRoutingClass: "test-routingClass", - ClusterHostSuffix: "192.168.0.1.nip.io", - }, - Workspace: &v1alpha1.WorkspaceConfig{ - ImagePullPolicy: "Always", - }, - EnableExperimentalFeatures: &trueBool, - }) + clusterConfig := buildConfig(defaultConfig.DeepCopy()) + originalInternalConfig := clusterConfig.Config.DeepCopy() + + // External config is based off of the default/internal config, with just a few changes made + // So when the internal config is merged with the external one, they will become identical + externalConfig := buildExternalConfig(defaultConfig.DeepCopy()) + externalConfig.Config.Workspace.ImagePullPolicy = "Always" + externalConfig.Config.Workspace.PVCName = "test-PVC-name" + storageSize := resource.MustParse("15Gi") + externalConfig.Config.Workspace.DefaultStorageSize = &v1alpha1.StorageSizes{ + Common: &storageSize, + PerWorkspace: &storageSize, + } client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(clusterConfig).WithObjects(externalConfig).Build() + err := SetupControllerConfig(client) + if !assert.NoError(t, err, "Should not return error") { + return + } + + // Sanity check + if !cmp.Equal(clusterConfig.Config, internalConfig) { + t.Error("Internal config should be same as cluster config before starting:", cmp.Diff(clusterConfig.Config, internalConfig)) + } - err := ApplyExternalDWOCConfig(workspace, client) + resolvedConfig, err := ResolveConfigForWorkspace(workspace, client) if !assert.NoError(t, err, "Should not return error") { return } - // Compare the internal config and external config - if !cmp.Equal(internalConfig, externalConfig.Config) { - t.Error("Internal config and external config should match after merge") + // Compare the resolved config and external config + if !cmp.Equal(resolvedConfig, externalConfig.Config) { + t.Error("Resolved config and external config should match after merge:", cmp.Diff(resolvedConfig, externalConfig.Config)) + } + + // Ensure the internal config was not affected by merge + if !cmp.Equal(originalInternalConfig, internalConfig) { + t.Error("Internal config should remain the same after merge") } // Get the global config off cluster and ensure it hasn't changed retrievedClusterConfig := &v1alpha1.DevWorkspaceOperatorConfig{} - namespacedName := types.NamespacedName{ + namespacedName = types.NamespacedName{ Name: OperatorConfigName, Namespace: testNamespace, } @@ -180,10 +191,10 @@ func TestConfigUpdatedAfterMerge(t *testing.T) { } if !cmp.Equal(retrievedClusterConfig.Config, clusterConfig.Config) { - t.Error("Config on cluster and global config should match after merge; global config should not have been modified from merge") + t.Error("Config on cluster and global config should match after merge; global config should not have been modified from merge:", cmp.Diff(retrievedClusterConfig, clusterConfig.Config)) } } -*/ + func TestSetupControllerAlwaysSetsDefaultClusterRoutingSuffix(t *testing.T) { setupForTest(t) infrastructure.InitializeForTesting(infrastructure.OpenShiftv4) From 8bcd04ee4af917962087c3fb9a3fdfdb0c92cf39 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 11:50:28 -0400 Subject: [PATCH 09/14] fixup! use new merge function to get external DWOC --- pkg/config/sync.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 44a0ff129..17f9d61df 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -85,7 +85,7 @@ func ResolveConfigForWorkspace(workspace *dw.DevWorkspace, client crclient.Clien if err != nil { return internalConfigCopy, fmt.Errorf("could not fetch external DWOC with name '%s' in namespace '%s': %w", namespacedName.Name, namespacedName.Namespace, err) } - // TODO: Do we need to lock the internal config? + return getMergedConfig(externalDWOC.Config, internalConfig), nil } @@ -168,10 +168,10 @@ func syncConfigFrom(newConfig *controller.DevWorkspaceOperatorConfig) { } func getMergedConfig(from, to *controller.OperatorConfiguration) *controller.OperatorConfiguration { - configMutex.Lock() - defer configMutex.Unlock() mergedConfig := to.DeepCopy() - mergeConfig(from, mergedConfig) + // TODO: Remove this comment: technically we don't need to deepCopy 'from', but we may use this function elsewhere in the future, so just incase.. + fromCopy := from.DeepCopy() + mergeConfig(fromCopy, mergedConfig) return mergedConfig } From c1c254dd5906180b58cbb9cbbb08e622610f235a Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 11:55:42 -0400 Subject: [PATCH 10/14] fixup! Add WorkspaceWithConfig struct, use it in controller --- .../workspace/devworkspace_controller.go | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index c97d2ad68..4a0372331 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -222,7 +222,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // encountered in main reconcile loop. if err == nil { if timeoutErr := checkForStartTimeout(&clusterWorkspaceWithConfig.DevWorkspace, workspaceWithConfig.Config); timeoutErr != nil { - reconcileResult, err = r.failWorkspace(&workspaceWithConfig.DevWorkspace, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + reconcileResult, err = r.failWorkspace(workspaceWithConfig, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } } if reconcileStatus.phase == dw.DevWorkspaceStatusRunning { @@ -236,7 +236,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { msg, err := r.validateCreatorLabel(clusterWorkspace) if err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) } } @@ -263,7 +263,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) if err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } if warnings != nil { reconcileStatus.setConditionTrue(conditions.DevWorkspaceWarning, flatten.FormatVariablesWarning(warnings)) @@ -278,13 +278,13 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if components != nil { eventErrors := devfilevalidation.ValidateComponents(components) if eventErrors != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } } storageProvisioner, err := storage.GetProvisioner(&workspaceWithConfig.DevWorkspace) if err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Set finalizer on DevWorkspace if necessary @@ -298,17 +298,17 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(&workspaceWithConfig.Spec.Template, workspaceWithConfig.Config) if err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add common environment variables and env vars defined via workspaceEnv attribute if err := env.AddCommonEnvironmentVariables(devfilePodAdditions, clusterWorkspace, &workspaceWithConfig.Spec.Template, workspaceWithConfig.Config); err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add init container to clone projects if projectClone, err := projects.GetProjectCloneInitContainer(workspaceWithConfig); err != nil { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } else if projectClone != nil { devfilePodAdditions.InitContainers = append(devfilePodAdditions.InitContainers, *projectClone) } @@ -318,7 +318,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request var autoMountErr *automount.AutoMountError if errors.As(err, &autoMountErr) { if autoMountErr.IsFatal { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info(autoMountErr.Error()) return reconcile.Result{Requeue: true}, nil @@ -335,7 +335,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus.setConditionFalse(conditions.StorageReady, fmt.Sprintf("Provisioning storage: %s", storageErr.Message)) return reconcile.Result{Requeue: true, RequeueAfter: storageErr.RequeueAfter}, nil case *storage.ProvisioningError: - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, storageErr } @@ -354,7 +354,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request routingStatus := wsprovision.SyncRoutingToCluster(workspaceWithConfig, clusterAPI) if !routingStatus.Continue { if routingStatus.FailStartup { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on routing to be ready") message := "Preparing networking" @@ -389,7 +389,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reqLogger.Info(provisionErr.Message) return reconcile.Result{Requeue: true, RequeueAfter: provisionErr.RequeueAfter}, nil case *metadata.ProvisioningError: - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, provisionErr } @@ -410,7 +410,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request serviceAcctStatus := wsprovision.SyncServiceAccount(&workspaceWithConfig.DevWorkspace, saAnnotations, clusterAPI) if !serviceAcctStatus.Continue { if serviceAcctStatus.FailStartup { - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting for workspace ServiceAccount") reconcileStatus.setConditionFalse(dw.DevWorkspaceServiceAccountReady, "Waiting for DevWorkspace ServiceAccount") @@ -446,7 +446,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if !deploymentStatus.Continue { if deploymentStatus.FailStartup { failureReason := metrics.DetermineProvisioningFailureReason(deploymentStatus) - return r.failWorkspace(&workspaceWithConfig.DevWorkspace, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) + return r.failWorkspace(workspaceWithConfig, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on deployment to be ready") reconcileStatus.setConditionFalse(conditions.DeploymentReady, "Waiting for workspace deployment") @@ -563,7 +563,7 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig // failWorkspace marks a workspace as failed by setting relevant fields in the status struct. // These changes are not synced to cluster immediately, and are intended to be synced to the cluster via a deferred function // in the main reconcile loop. If needed, changes can be flushed to the cluster immediately via `updateWorkspaceStatus()` -func (r *DevWorkspaceReconciler) failWorkspace(workspace *dw.DevWorkspace, msg string, reason metrics.FailureReason, logger logr.Logger, status *currentStatus) (reconcile.Result, error) { +func (r *DevWorkspaceReconciler) failWorkspace(workspace *common.DevWorkspaceWithConfig, msg string, reason metrics.FailureReason, logger logr.Logger, status *currentStatus) (reconcile.Result, error) { logger.Info("DevWorkspace failed to start: " + msg) status.phase = devworkspacePhaseFailing status.setConditionTrueWithReason(dw.DevWorkspaceFailedStart, msg, string(reason)) From bfbd54e0a27e658ca7bb1443b4af9a705ea132b8 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 12:13:13 -0400 Subject: [PATCH 11/14] add GetGlobalConfig function, unexport Routing and Workspace config Signed-off-by: Andrew Obuchowicz --- .../workspace/devworkspace_controller.go | 2 +- controllers/workspace/http.go | 10 ++-- pkg/config/sync.go | 57 ++++++++++--------- pkg/config/sync_test.go | 18 +++--- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 4a0372331..7dd375e79 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -694,7 +694,7 @@ func (r *DevWorkspaceReconciler) dwPVCHandler(obj client.Object) []reconcile.Req // TODO: Address this usage of global config. It'd be better to check if the PVC had a DWO-specific annotation that we can use to filter for // Otherwise, check if common PVC is deleted to make sure all DevWorkspaces see it happen - if obj.GetName() != config.Workspace.PVCName || obj.GetDeletionTimestamp() == nil { + if obj.GetName() != config.GetGlobalConfig().Workspace.PVCName || obj.GetDeletionTimestamp() == nil { // We're looking for a deleted common PVC return []reconcile.Request{} } diff --git a/controllers/workspace/http.go b/controllers/workspace/http.go index acfd0424e..e1bd0ad3c 100644 --- a/controllers/workspace/http.go +++ b/controllers/workspace/http.go @@ -34,13 +34,15 @@ func setupHttpClients() { InsecureSkipVerify: true, } + routingConfig := config.GetGlobalConfig().Routing + // Note: per-workspace routing settings (that have been set with an external DWOC) // are ignored for the HTTP Clients - if config.Routing != nil && config.Routing.ProxyConfig != nil { + if routingConfig != nil && routingConfig.ProxyConfig != nil { proxyConf := httpproxy.Config{ - HTTPProxy: config.Routing.ProxyConfig.HttpProxy, - HTTPSProxy: config.Routing.ProxyConfig.HttpsProxy, - NoProxy: config.Routing.ProxyConfig.NoProxy, + HTTPProxy: routingConfig.ProxyConfig.HttpProxy, + HTTPSProxy: routingConfig.ProxyConfig.HttpsProxy, + NoProxy: routingConfig.ProxyConfig.NoProxy, } proxyFunc := func(req *http.Request) (*url.URL, error) { return proxyConf.ProxyFunc()(req.URL) diff --git a/pkg/config/sync.go b/pkg/config/sync.go index 17f9d61df..b844e65ed 100644 --- a/pkg/config/sync.go +++ b/pkg/config/sync.go @@ -42,8 +42,8 @@ const ( ) var ( - Routing *controller.RoutingConfig - Workspace *controller.WorkspaceConfig + routing *controller.RoutingConfig + workspace *controller.WorkspaceConfig internalConfig *controller.OperatorConfiguration configMutex sync.Mutex configNamespace string @@ -145,6 +145,11 @@ func ExperimentalFeaturesEnabled() bool { return *internalConfig.EnableExperimentalFeatures } +func GetGlobalConfig() *controller.OperatorConfiguration { + // TODO: Could make this return a deepcopy to prevent modifying the global config when we only intend for reads + return internalConfig +} + func getClusterConfig(namespace string, client crclient.Client) (*controller.DevWorkspaceOperatorConfig, error) { clusterConfig := &controller.DevWorkspaceOperatorConfig{} if err := client.Get(context.Background(), types.NamespacedName{Name: OperatorConfigName, Namespace: namespace}, clusterConfig); err != nil { @@ -183,8 +188,8 @@ func restoreDefaultConfig() { } func updatePublicConfig() { - Routing = internalConfig.Routing.DeepCopy() - Workspace = internalConfig.Workspace.DeepCopy() + routing = internalConfig.Routing.DeepCopy() + workspace = internalConfig.Workspace.DeepCopy() logCurrentConfig() } @@ -311,40 +316,40 @@ func logCurrentConfig() { return } var config []string - if Routing != nil { - if Routing.ClusterHostSuffix != "" && Routing.ClusterHostSuffix != defaultConfig.Routing.ClusterHostSuffix { - config = append(config, fmt.Sprintf("routing.clusterHostSuffix=%s", Routing.ClusterHostSuffix)) + if routing != nil { + if routing.ClusterHostSuffix != "" && routing.ClusterHostSuffix != defaultConfig.Routing.ClusterHostSuffix { + config = append(config, fmt.Sprintf("routing.clusterHostSuffix=%s", routing.ClusterHostSuffix)) } - if Routing.DefaultRoutingClass != defaultConfig.Routing.DefaultRoutingClass { - config = append(config, fmt.Sprintf("routing.defaultRoutingClass=%s", Routing.DefaultRoutingClass)) + if routing.DefaultRoutingClass != defaultConfig.Routing.DefaultRoutingClass { + config = append(config, fmt.Sprintf("routing.defaultRoutingClass=%s", routing.DefaultRoutingClass)) } } - if Workspace != nil { - if Workspace.ImagePullPolicy != defaultConfig.Workspace.ImagePullPolicy { - config = append(config, fmt.Sprintf("workspace.imagePullPolicy=%s", Workspace.ImagePullPolicy)) + if workspace != nil { + if workspace.ImagePullPolicy != defaultConfig.Workspace.ImagePullPolicy { + config = append(config, fmt.Sprintf("workspace.imagePullPolicy=%s", workspace.ImagePullPolicy)) } - if Workspace.PVCName != defaultConfig.Workspace.PVCName { - config = append(config, fmt.Sprintf("workspace.pvcName=%s", Workspace.PVCName)) + if workspace.PVCName != defaultConfig.Workspace.PVCName { + config = append(config, fmt.Sprintf("workspace.pvcName=%s", workspace.PVCName)) } - if Workspace.StorageClassName != nil && Workspace.StorageClassName != defaultConfig.Workspace.StorageClassName { - config = append(config, fmt.Sprintf("workspace.storageClassName=%s", *Workspace.StorageClassName)) + if workspace.StorageClassName != nil && workspace.StorageClassName != defaultConfig.Workspace.StorageClassName { + config = append(config, fmt.Sprintf("workspace.storageClassName=%s", *workspace.StorageClassName)) } - if Workspace.IdleTimeout != defaultConfig.Workspace.IdleTimeout { - config = append(config, fmt.Sprintf("workspace.idleTimeout=%s", Workspace.IdleTimeout)) + if workspace.IdleTimeout != defaultConfig.Workspace.IdleTimeout { + config = append(config, fmt.Sprintf("workspace.idleTimeout=%s", workspace.IdleTimeout)) } - if Workspace.IgnoredUnrecoverableEvents != nil { + if workspace.IgnoredUnrecoverableEvents != nil { config = append(config, fmt.Sprintf("workspace.ignoredUnrecoverableEvents=%s", - strings.Join(Workspace.IgnoredUnrecoverableEvents, ";"))) + strings.Join(workspace.IgnoredUnrecoverableEvents, ";"))) } - if Workspace.DefaultStorageSize != nil { - if Workspace.DefaultStorageSize.Common != nil { - config = append(config, fmt.Sprintf("workspace.defaultStorageSize.common=%s", Workspace.DefaultStorageSize.Common.String())) + if workspace.DefaultStorageSize != nil { + if workspace.DefaultStorageSize.Common != nil { + config = append(config, fmt.Sprintf("workspace.defaultStorageSize.common=%s", workspace.DefaultStorageSize.Common.String())) } - if Workspace.DefaultStorageSize.PerWorkspace != nil { - config = append(config, fmt.Sprintf("workspace.defaultStorageSize.perWorkspace=%s", Workspace.DefaultStorageSize.PerWorkspace.String())) + if workspace.DefaultStorageSize.PerWorkspace != nil { + config = append(config, fmt.Sprintf("workspace.defaultStorageSize.perWorkspace=%s", workspace.DefaultStorageSize.PerWorkspace.String())) } } - if Workspace.DefaultTemplate != nil { + if workspace.DefaultTemplate != nil { config = append(config, fmt.Sprintf("workspace.defaultTemplate is set")) } } diff --git a/pkg/config/sync_test.go b/pkg/config/sync_test.go index ecb69b1b2..0aaf222ff 100644 --- a/pkg/config/sync_test.go +++ b/pkg/config/sync_test.go @@ -54,8 +54,8 @@ func TestSetupControllerDefaultsAreExported(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, defaultConfig.Routing, Routing, "Configuration should be exported") - assert.Equal(t, defaultConfig.Workspace, Workspace, "Configuration should be exported") + assert.Equal(t, defaultConfig.Routing, routing, "Configuration should be exported") + assert.Equal(t, defaultConfig.Workspace, workspace, "Configuration should be exported") } func TestSetupControllerConfigFailsWhenAlreadySetup(t *testing.T) { @@ -98,8 +98,8 @@ func TestSetupControllerMergesClusterConfig(t *testing.T) { return } assert.Equal(t, expectedConfig, internalConfig, fmt.Sprintf("Processed config should merge settings from cluster: %s", cmp.Diff(internalConfig, expectedConfig))) - assert.Equal(t, internalConfig.Routing, Routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) - assert.Equal(t, internalConfig.Workspace, Workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, internalConfig.Routing, routing, fmt.Sprintf("Changes to config should be propagated to exported fields")) + assert.Equal(t, internalConfig.Workspace, workspace, fmt.Sprintf("Changes to config should be propagated to exported fields")) } func TestCatchesNonExistentExternalDWOC(t *testing.T) { @@ -218,7 +218,7 @@ func TestSetupControllerAlwaysSetsDefaultClusterRoutingSuffix(t *testing.T) { return } assert.Equal(t, "test-host", defaultConfig.Routing.ClusterHostSuffix, "Should set default clusterRoutingSuffix even if config overrides it initially") - assert.Equal(t, "192.168.0.1.nip.io", Routing.ClusterHostSuffix, "Should use value from config for clusterRoutingSuffix") + assert.Equal(t, "192.168.0.1.nip.io", routing.ClusterHostSuffix, "Should use value from config for clusterRoutingSuffix") } func TestDetectsOpenShiftRouteSuffix(t *testing.T) { @@ -295,10 +295,10 @@ func TestSyncConfigRestoresClusterRoutingSuffix(t *testing.T) { }, }) syncConfigFrom(config) - assert.Equal(t, "192.168.0.1.nip.io", Routing.ClusterHostSuffix, "Should update clusterRoutingSuffix from config") + assert.Equal(t, "192.168.0.1.nip.io", routing.ClusterHostSuffix, "Should update clusterRoutingSuffix from config") config.Config.Routing.ClusterHostSuffix = "" syncConfigFrom(config) - assert.Equal(t, "default.routing.suffix", Routing.ClusterHostSuffix, "Should restore default clusterRoutingSuffix if it is available") + assert.Equal(t, "default.routing.suffix", routing.ClusterHostSuffix, "Should restore default clusterRoutingSuffix if it is available") } func TestSyncConfigDoesNotEraseClusterRoutingSuffix(t *testing.T) { @@ -318,7 +318,7 @@ func TestSyncConfigDoesNotEraseClusterRoutingSuffix(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, "test-host", Routing.ClusterHostSuffix, "Should get clusterHostSuffix from route on OpenShift") + assert.Equal(t, "test-host", routing.ClusterHostSuffix, "Should get clusterHostSuffix from route on OpenShift") syncConfigFrom(buildConfig(&v1alpha1.OperatorConfiguration{ Routing: &v1alpha1.RoutingConfig{ DefaultRoutingClass: "test-routingClass", @@ -330,7 +330,7 @@ func TestSyncConfigDoesNotEraseClusterRoutingSuffix(t *testing.T) { if !assert.NoError(t, err, "Should not return error") { return } - assert.Equal(t, "test-host", Routing.ClusterHostSuffix, "clusterHostSuffix should persist after an update") + assert.Equal(t, "test-host", routing.ClusterHostSuffix, "clusterHostSuffix should persist after an update") } func TestMergeConfigLooksAtAllFields(t *testing.T) { From 62b3424566ac8781d556fea0aff22955e27ec884 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 12:52:47 -0400 Subject: [PATCH 12/14] fixup! Don't use workspaceWithConfig struct with clusterAPI + allow basic solver to get config from annotations --- .../devworkspacerouting/solvers/basic_solver.go | 7 +------ .../controller/devworkspacerouting/solvers/common.go | 10 ---------- controllers/workspace/devworkspace_controller.go | 4 ---- pkg/constants/metadata.go | 6 ++---- pkg/provision/workspace/routing.go | 7 ++++--- 5 files changed, 7 insertions(+), 27 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 5b985eb70..c43797585 100644 --- a/controllers/controller/devworkspacerouting/solvers/basic_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/basic_solver.go @@ -56,12 +56,7 @@ func (s *BasicSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error { func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { routingObjects := RoutingObjects{} - routingAnnotationPrefix := string(routing.Spec.RoutingClass) + constants.RoutingAnnotationInfix - if _, set := routing.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix]; !set { - return routingObjects, &RoutingInvalid{"basic routing requires .config.routing.clusterHostSuffix to be set in operator config"} - } - - routingSuffix := routing.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix] + routingSuffix := routing.Annotations[constants.ClusterHostSuffixAnnotation] if routingSuffix == "" { return routingObjects, &RoutingInvalid{"basic routing requires .config.routing.clusterHostSuffix to be set in operator config"} } diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index e7c3be042..bd26dc8ea 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -138,16 +138,6 @@ func GetServiceForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList } } -func AddDWOCRoutingAnnotations(workspaceWithConfig common.DevWorkspaceWithConfig) { - routingClass := workspaceWithConfig.Spec.RoutingClass - if routingClass == "" { - routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass - } - routingAnnotationPrefix := routingClass + constants.RoutingAnnotationInfix - // TODO: We could also just pass the entire Routing config - workspaceWithConfig.Annotations[routingAnnotationPrefix+constants.ClusterHostSuffixAnnotationSuffix] = workspaceWithConfig.Config.Routing.ClusterHostSuffix -} - func getServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []corev1.Service { if len(endpoints) == 0 { return nil diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index 7dd375e79..ab71b239d 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -24,7 +24,6 @@ import ( devfilevalidation "github.com/devfile/api/v2/pkg/validation" controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/controllers/workspace/metrics" "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/conditions" @@ -258,9 +257,6 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request wsDefaults.ApplyDefaultTemplate(workspaceWithConfig) } - // Apply devworkspace routing annotation(s) to pass DWOC routing settings to DWR - solvers.AddDWOCRoutingAnnotations(*workspaceWithConfig) - flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) if err != nil { return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) diff --git a/pkg/constants/metadata.go b/pkg/constants/metadata.go index fa64ec0b3..2288942bc 100644 --- a/pkg/constants/metadata.go +++ b/pkg/constants/metadata.go @@ -117,10 +117,8 @@ const ( // The full annotation name is supposed to be ".routing.controller.devfile.io/" RoutingAnnotationInfix = ".routing.controller.devfile.io/" - // ClusterHostSuffixAnnotationSuffix is an annotation suffix used in conjunction with RoutingAnnotationInfix. - // It is applied as an annotation to DevWorkspaceRouting objects to pass them the cluster host suffix from the DevWorkspace-Operator configuration. - // The suffix is intended to be used in the following manner: .routing.controller.devfile.io/cluster-host-suffix = - ClusterHostSuffixAnnotationSuffix = "cluster-host-suffix" + // ClusterHostSuffixAnnotation is an annotation key for storing the value of the cluster host suffix to be used by the Basic DevWorkspace Routing Class. + ClusterHostSuffixAnnotation = "basic.routing.controller.devfile.io/cluster-host-suffix" // DevWorkspaceEndpointNameAnnotation is the annotation key for storing an endpoint's name from the devfile representation DevWorkspaceEndpointNameAnnotation = "controller.devfile.io/endpoint_name" diff --git a/pkg/provision/workspace/routing.go b/pkg/provision/workspace/routing.go index 8a424cf58..3f7dd8ce8 100644 --- a/pkg/provision/workspace/routing.go +++ b/pkg/provision/workspace/routing.go @@ -106,11 +106,8 @@ func getSpecRouting( } annotations = maputils.Append(annotations, constants.DevWorkspaceStartedStatusAnnotation, "true") - // TODO: Remove this comment - // I had to move this before the annotations get applied, as otherwise, if the routing class wasn't set in the spec, the annotation would start with a '.' which isin't allowed routingClass := workspaceWithConfig.Spec.RoutingClass if routingClass == "" { - // TODO: Just set workspace.Spec.RoutingClass = config.Routing.DefaultRoutingClass ? routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass } @@ -122,6 +119,10 @@ func getSpecRouting( } } + if v1alpha1.DevWorkspaceRoutingClass(routingClass) == v1alpha1.DevWorkspaceRoutingBasic { + annotations = maputils.Append(annotations, constants.ClusterHostSuffixAnnotation, workspaceWithConfig.Config.Routing.ClusterHostSuffix) + } + routing := &v1alpha1.DevWorkspaceRouting{ ObjectMeta: metav1.ObjectMeta{ Name: common.DevWorkspaceRoutingName(workspaceWithConfig.Status.DevWorkspaceId), From 24a9138274b9f93ee103be38a71d57a31e25a0d3 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 13:37:13 -0400 Subject: [PATCH 13/14] fixup! Remove some uses of global config routing options --- controllers/workspace/metrics/update.go | 34 ++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/controllers/workspace/metrics/update.go b/controllers/workspace/metrics/update.go index abf045c8c..1eebef66e 100644 --- a/controllers/workspace/metrics/update.go +++ b/controllers/workspace/metrics/update.go @@ -27,21 +27,21 @@ import ( // WorkspaceStarted updates metrics for workspaces entering the 'Starting' phase, given a workspace. If an error is // encountered, the provided logger is used to log the error. -func WorkspaceStarted(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { - _, ok := workspaceWithConfig.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] +func WorkspaceStarted(wksp *common.DevWorkspaceWithConfig, log logr.Logger) { + _, ok := wksp.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] if !ok { - incrementMetricForWorkspace(workspaceTotal, workspaceWithConfig, log) + incrementMetricForWorkspace(workspaceTotal, wksp, log) } } // WorkspaceRunning updates metrics for workspaces entering the 'Running' phase, given a workspace. If an error is // encountered, the provided logger is used to log the error. This function assumes the provided workspace has // fully-synced conditions (i.e. the WorkspaceReady condition is present). -func WorkspaceRunning(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { - _, ok := workspaceWithConfig.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] +func WorkspaceRunning(wksp *common.DevWorkspaceWithConfig, log logr.Logger) { + _, ok := wksp.GetAnnotations()[constants.DevWorkspaceStartedAtAnnotation] if !ok { - incrementMetricForWorkspace(workspaceStarts, workspaceWithConfig, log) - incrementStartTimeBucketForWorkspace(workspaceWithConfig, log) + incrementMetricForWorkspace(workspaceStarts, wksp, log) + incrementStartTimeBucketForWorkspace(wksp, log) } } @@ -51,14 +51,14 @@ func WorkspaceFailed(wksp *dw.DevWorkspace, log logr.Logger) { incrementMetricForWorkspaceFailure(workspaceFailures, wksp, log) } -func incrementMetricForWorkspace(metric *prometheus.CounterVec, workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { - sourceLabel := workspaceWithConfig.Labels[workspaceSourceLabel] +func incrementMetricForWorkspace(metric *prometheus.CounterVec, wksp *common.DevWorkspaceWithConfig, log logr.Logger) { + sourceLabel := wksp.Labels[workspaceSourceLabel] if sourceLabel == "" { sourceLabel = "unknown" } - routingClass := workspaceWithConfig.Spec.RoutingClass + routingClass := wksp.Spec.RoutingClass if routingClass == "" { - routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass + routingClass = wksp.Config.Routing.DefaultRoutingClass } ctr, err := metric.GetMetricWith(map[string]string{metricSourceLabel: sourceLabel, metricsRoutingClassLabel: routingClass}) if err != nil { @@ -80,24 +80,24 @@ func incrementMetricForWorkspaceFailure(metric *prometheus.CounterVec, wksp *dw. ctr.Inc() } -func incrementStartTimeBucketForWorkspace(workspaceWithConfig *common.DevWorkspaceWithConfig, log logr.Logger) { - sourceLabel := workspaceWithConfig.Labels[workspaceSourceLabel] +func incrementStartTimeBucketForWorkspace(wksp *common.DevWorkspaceWithConfig, log logr.Logger) { + sourceLabel := wksp.Labels[workspaceSourceLabel] if sourceLabel == "" { sourceLabel = "unknown" } - routingClass := workspaceWithConfig.Spec.RoutingClass + routingClass := wksp.Spec.RoutingClass if routingClass == "" { - routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass + routingClass = wksp.Config.Routing.DefaultRoutingClass } hist, err := workspaceStartupTimesHist.GetMetricWith(map[string]string{metricSourceLabel: sourceLabel, metricsRoutingClassLabel: routingClass}) if err != nil { log.Error(err, "Failed to update metric") } - readyCondition := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, dw.DevWorkspaceReady) + readyCondition := conditions.GetConditionByType(wksp.Status.Conditions, dw.DevWorkspaceReady) if readyCondition == nil { return } - startedCondition := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, conditions.Started) + startedCondition := conditions.GetConditionByType(wksp.Status.Conditions, conditions.Started) if startedCondition == nil { return } From 298384866471e4e8453e0aac3047559bad3e8246 Mon Sep 17 00:00:00 2001 From: Andrew Obuchowicz Date: Wed, 31 Aug 2022 13:43:28 -0400 Subject: [PATCH 14/14] fixup! Add WorkspaceWithConfig struct, use it in controller --- .../workspace/devworkspace_controller.go | 181 +++++++++--------- controllers/workspace/status.go | 34 ++-- pkg/library/defaults/helper.go | 16 +- pkg/library/projects/clone.go | 11 +- pkg/provision/storage/asyncStorage.go | 40 ++-- pkg/provision/storage/cleanup.go | 28 +-- pkg/provision/storage/commonStorage.go | 22 +-- pkg/provision/storage/ephemeralStorage.go | 6 +- pkg/provision/storage/perWorkspaceStorage.go | 20 +- pkg/provision/storage/provisioner.go | 4 +- pkg/provision/workspace/routing.go | 30 +-- 11 files changed, 195 insertions(+), 197 deletions(-) diff --git a/controllers/workspace/devworkspace_controller.go b/controllers/workspace/devworkspace_controller.go index ab71b239d..919202822 100644 --- a/controllers/workspace/devworkspace_controller.go +++ b/controllers/workspace/devworkspace_controller.go @@ -109,8 +109,8 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Fetch the Workspace instance - workspaceWithConfig := &common.DevWorkspaceWithConfig{} - err = r.Get(ctx, req.NamespacedName, &workspaceWithConfig.DevWorkspace) + workspace := &common.DevWorkspaceWithConfig{} + err = r.Get(ctx, req.NamespacedName, &workspace.DevWorkspace) if err != nil { if k8sErrors.IsNotFound(err) { // Request object not found, could have been deleted after reconcile request. @@ -123,41 +123,41 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Get the config for the workspace - resolvedConfig, err := config.ResolveConfigForWorkspace(&workspaceWithConfig.DevWorkspace, clusterAPI.Client) - workspaceWithConfig.Config = *resolvedConfig + resolvedConfig, err := config.ResolveConfigForWorkspace(&workspace.DevWorkspace, clusterAPI.Client) + workspace.Config = *resolvedConfig if err != nil { reqLogger.Error(err, "Unable to apply external DevWorkspace-Operator configuration") } - reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspaceWithConfig.Status.DevWorkspaceId) + reqLogger = reqLogger.WithValues(constants.DevWorkspaceIDLoggerKey, workspace.Status.DevWorkspaceId) reqLogger.Info("Reconciling Workspace") // Check if the DevWorkspaceRouting instance is marked to be deleted, which is // indicated by the deletion timestamp being set. - if workspaceWithConfig.GetDeletionTimestamp() != nil { + if workspace.GetDeletionTimestamp() != nil { reqLogger.Info("Finalizing DevWorkspace") - return r.finalize(ctx, reqLogger, workspaceWithConfig) + return r.finalize(ctx, reqLogger, workspace) } // Ensure workspaceID is set. - if workspaceWithConfig.Status.DevWorkspaceId == "" { - workspaceId, err := r.getWorkspaceId(ctx, &workspaceWithConfig.DevWorkspace) + if workspace.Status.DevWorkspaceId == "" { + workspaceId, err := r.getWorkspaceId(ctx, &workspace.DevWorkspace) if err != nil { - workspaceWithConfig.Status.Phase = dw.DevWorkspaceStatusFailed - workspaceWithConfig.Status.Message = fmt.Sprintf("Failed to set DevWorkspace ID: %s", err.Error()) - return reconcile.Result{}, r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) + workspace.Status.Phase = dw.DevWorkspaceStatusFailed + workspace.Status.Message = fmt.Sprintf("Failed to set DevWorkspace ID: %s", err.Error()) + return reconcile.Result{}, r.Status().Update(ctx, &workspace.DevWorkspace) } - workspaceWithConfig.Status.DevWorkspaceId = workspaceId - err = r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) + workspace.Status.DevWorkspaceId = workspaceId + err = r.Status().Update(ctx, &workspace.DevWorkspace) return reconcile.Result{Requeue: true}, err } // Stop failed workspaces - if workspaceWithConfig.Status.Phase == devworkspacePhaseFailing && workspaceWithConfig.Spec.Started { + if workspace.Status.Phase == devworkspacePhaseFailing && workspace.Spec.Started { // If debug annotation is present, leave the deployment in place to let users // view logs. - if workspaceWithConfig.Annotations[constants.DevWorkspaceDebugStartAnnotation] == "true" { - if isTimeout, err := checkForFailingTimeout(workspaceWithConfig); err != nil { + if workspace.Annotations[constants.DevWorkspaceDebugStartAnnotation] == "true" { + if isTimeout, err := checkForFailingTimeout(workspace); err != nil { return reconcile.Result{}, err } else if !isTimeout { return reconcile.Result{}, nil @@ -165,7 +165,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } patch := []byte(`{"spec":{"started": false}}`) - err := r.Client.Patch(context.Background(), &workspaceWithConfig.DevWorkspace, client.RawPatch(types.MergePatchType, patch)) + err := r.Client.Patch(context.Background(), &workspace.DevWorkspace, client.RawPatch(types.MergePatchType, patch)) if err != nil { return reconcile.Result{}, err } @@ -175,20 +175,20 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } // Handle stopped workspaces - if !workspaceWithConfig.Spec.Started { - timing.ClearAnnotations(&workspaceWithConfig.DevWorkspace) - r.removeStartedAtFromCluster(ctx, &workspaceWithConfig.DevWorkspace, reqLogger) - r.syncTimingToCluster(ctx, &workspaceWithConfig.DevWorkspace, map[string]string{}, reqLogger) - return r.stopWorkspace(ctx, workspaceWithConfig, reqLogger) + if !workspace.Spec.Started { + timing.ClearAnnotations(&workspace.DevWorkspace) + r.removeStartedAtFromCluster(ctx, &workspace.DevWorkspace, reqLogger) + r.syncTimingToCluster(ctx, &workspace.DevWorkspace, map[string]string{}, reqLogger) + return r.stopWorkspace(ctx, workspace, reqLogger) } // If this is the first reconcile for a starting workspace, mark it as starting now. This is done outside the regular // updateWorkspaceStatus function to ensure it gets set immediately - if workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusStarting && workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusRunning { + if workspace.Status.Phase != dw.DevWorkspaceStatusStarting && workspace.Status.Phase != dw.DevWorkspaceStatusRunning { // Set 'Started' condition as early as possible to get accurate timing metrics - workspaceWithConfig.Status.Phase = dw.DevWorkspaceStatusStarting - workspaceWithConfig.Status.Message = "Initializing DevWorkspace" - workspaceWithConfig.Status.Conditions = []dw.DevWorkspaceCondition{ + workspace.Status.Phase = dw.DevWorkspaceStatusStarting + workspace.Status.Message = "Initializing DevWorkspace" + workspace.Status.Conditions = []dw.DevWorkspaceCondition{ { Type: conditions.Started, Status: corev1.ConditionTrue, @@ -196,9 +196,9 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request Message: "DevWorkspace is starting", }, } - err = r.Status().Update(ctx, &workspaceWithConfig.DevWorkspace) + err = r.Status().Update(ctx, &workspace.DevWorkspace) if err == nil { - metrics.WorkspaceStarted(workspaceWithConfig, reqLogger) + metrics.WorkspaceStarted(workspace, reqLogger) } return reconcile.Result{}, err } @@ -206,115 +206,114 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Prepare handling workspace status and condition reconcileStatus := currentStatus{phase: dw.DevWorkspaceStatusStarting} reconcileStatus.setConditionTrue(conditions.Started, "DevWorkspace is starting") - clusterWorkspace := workspaceWithConfig.DevWorkspace.DeepCopy() - clusterWorkspaceWithConfig := &common.DevWorkspaceWithConfig{ - DevWorkspace: *workspaceWithConfig.DevWorkspace.DeepCopy(), - Config: *workspaceWithConfig.Config.DeepCopy(), + clusterWorkspace := &common.DevWorkspaceWithConfig{ + DevWorkspace: *workspace.DevWorkspace.DeepCopy(), + Config: *workspace.Config.DeepCopy(), } timingInfo := map[string]string{} timing.SetTime(timingInfo, timing.DevWorkspaceStarted) defer func() (reconcile.Result, error) { - r.syncTimingToCluster(ctx, &clusterWorkspaceWithConfig.DevWorkspace, timingInfo, reqLogger) + r.syncTimingToCluster(ctx, &clusterWorkspace.DevWorkspace, timingInfo, reqLogger) // Don't accidentally suppress errors by overwriting here; only check for timeout when no error // encountered in main reconcile loop. if err == nil { - if timeoutErr := checkForStartTimeout(&clusterWorkspaceWithConfig.DevWorkspace, workspaceWithConfig.Config); timeoutErr != nil { - reconcileResult, err = r.failWorkspace(workspaceWithConfig, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + if timeoutErr := checkForStartTimeout(&clusterWorkspace.DevWorkspace, clusterWorkspace.Config); timeoutErr != nil { + reconcileResult, err = r.failWorkspace(workspace, timeoutErr.Error(), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } } if reconcileStatus.phase == dw.DevWorkspaceStatusRunning { - metrics.WorkspaceRunning(workspaceWithConfig, reqLogger) - r.syncStartedAtToCluster(ctx, &clusterWorkspaceWithConfig.DevWorkspace, reqLogger) + metrics.WorkspaceRunning(clusterWorkspace, reqLogger) + r.syncStartedAtToCluster(ctx, &clusterWorkspace.DevWorkspace, reqLogger) } - return r.updateWorkspaceStatus(clusterWorkspaceWithConfig, reqLogger, &reconcileStatus, reconcileResult, err) + return r.updateWorkspaceStatus(clusterWorkspace, reqLogger, &reconcileStatus, reconcileResult, err) }() - if workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { - msg, err := r.validateCreatorLabel(clusterWorkspace) + if workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] == "true" { + msg, err := r.validateCreatorLabel(&clusterWorkspace.DevWorkspace) if err != nil { - return r.failWorkspace(workspaceWithConfig, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, msg, metrics.ReasonWorkspaceEngineFailure, reqLogger, &reconcileStatus) } } if _, ok := clusterWorkspace.Annotations[constants.DevWorkspaceStopReasonAnnotation]; ok { delete(clusterWorkspace.Annotations, constants.DevWorkspaceStopReasonAnnotation) - err = r.Update(context.TODO(), clusterWorkspace) + err = r.Update(context.TODO(), &clusterWorkspace.DevWorkspace) return reconcile.Result{Requeue: true}, err } timing.SetTime(timingInfo, timing.ComponentsCreated) flattenHelpers := flatten.ResolverTools{ - WorkspaceNamespace: workspaceWithConfig.Namespace, + WorkspaceNamespace: workspace.Namespace, Context: ctx, K8sClient: r.Client, HttpClient: httpClient, } - if wsDefaults.NeedsDefaultTemplate(workspaceWithConfig) { - wsDefaults.ApplyDefaultTemplate(workspaceWithConfig) + if wsDefaults.NeedsDefaultTemplate(workspace) { + wsDefaults.ApplyDefaultTemplate(workspace) } - flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspaceWithConfig.Spec.Template, flattenHelpers) + flattenedWorkspace, warnings, err := flatten.ResolveDevWorkspace(&workspace.Spec.Template, flattenHelpers) if err != nil { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } if warnings != nil { reconcileStatus.setConditionTrue(conditions.DevWorkspaceWarning, flatten.FormatVariablesWarning(warnings)) } else { reconcileStatus.setConditionFalse(conditions.DevWorkspaceWarning, "No warnings in processing DevWorkspace") } - workspaceWithConfig.Spec.Template = *flattenedWorkspace + workspace.Spec.Template = *flattenedWorkspace reconcileStatus.setConditionTrue(conditions.DevWorkspaceResolved, "Resolved plugins and parents from DevWorkspace") // Verify that the devworkspace components are valid after flattening - components := workspaceWithConfig.Spec.Template.Components + components := workspace.Spec.Template.Components if components != nil { eventErrors := devfilevalidation.ValidateComponents(components) if eventErrors != nil { - return r.failWorkspace(workspaceWithConfig, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, eventErrors.Error(), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } } - storageProvisioner, err := storage.GetProvisioner(&workspaceWithConfig.DevWorkspace) + storageProvisioner, err := storage.GetProvisioner(&workspace.DevWorkspace) if err != nil { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning storage: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Set finalizer on DevWorkspace if necessary // Note: we need to check the flattened workspace to see if a finalizer is needed, as plugins could require storage - if storageProvisioner.NeedsStorage(&workspaceWithConfig.Spec.Template) { - coputil.AddFinalizer(clusterWorkspace, constants.StorageCleanupFinalizer) - if err := r.Update(ctx, clusterWorkspace); err != nil { + if storageProvisioner.NeedsStorage(&workspace.Spec.Template) { + coputil.AddFinalizer(&clusterWorkspace.DevWorkspace, constants.StorageCleanupFinalizer) + if err := r.Update(ctx, &clusterWorkspace.DevWorkspace); err != nil { return reconcile.Result{}, err } } - devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(&workspaceWithConfig.Spec.Template, workspaceWithConfig.Config) + devfilePodAdditions, err := containerlib.GetKubeContainersFromDevfile(&workspace.Spec.Template, workspace.Config) if err != nil { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Error processing devfile: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add common environment variables and env vars defined via workspaceEnv attribute - if err := env.AddCommonEnvironmentVariables(devfilePodAdditions, clusterWorkspace, &workspaceWithConfig.Spec.Template, workspaceWithConfig.Config); err != nil { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + if err := env.AddCommonEnvironmentVariables(devfilePodAdditions, &clusterWorkspace.DevWorkspace, &workspace.Spec.Template, clusterWorkspace.Config); err != nil { + return r.failWorkspace(workspace, fmt.Sprintf("Failed to process workspace environment variables: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } // Add init container to clone projects - if projectClone, err := projects.GetProjectCloneInitContainer(workspaceWithConfig); err != nil { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + if projectClone, err := projects.GetProjectCloneInitContainer(workspace); err != nil { + return r.failWorkspace(workspace, fmt.Sprintf("Failed to set up project-clone init container: %s", err), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } else if projectClone != nil { devfilePodAdditions.InitContainers = append(devfilePodAdditions.InitContainers, *projectClone) } // Add automount resources into devfile containers - if err := automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspaceWithConfig.Namespace); err != nil { + if err := automount.ProvisionAutoMountResourcesInto(devfilePodAdditions, clusterAPI, workspace.Namespace); err != nil { var autoMountErr *automount.AutoMountError if errors.As(err, &autoMountErr) { if autoMountErr.IsFatal { - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Failed to process automount resources: %s", err), metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info(autoMountErr.Error()) return reconcile.Result{Requeue: true}, nil @@ -323,7 +322,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } } - err = storageProvisioner.ProvisionStorage(devfilePodAdditions, workspaceWithConfig, clusterAPI) + err = storageProvisioner.ProvisionStorage(devfilePodAdditions, workspace, clusterAPI) if err != nil { switch storageErr := err.(type) { case *storage.NotReadyError: @@ -331,7 +330,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus.setConditionFalse(conditions.StorageReady, fmt.Sprintf("Provisioning storage: %s", storageErr.Message)) return reconcile.Result{Requeue: true, RequeueAfter: storageErr.RequeueAfter}, nil case *storage.ProvisioningError: - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning storage: %s", storageErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, storageErr } @@ -340,17 +339,17 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request timing.SetTime(timingInfo, timing.ComponentsReady) - rbacStatus := wsprovision.SyncRBAC(&workspaceWithConfig.DevWorkspace, clusterAPI) + rbacStatus := wsprovision.SyncRBAC(&workspace.DevWorkspace, clusterAPI) if rbacStatus.Err != nil || !rbacStatus.Continue { return reconcile.Result{Requeue: true}, rbacStatus.Err } // Step two: Create routing, and wait for routing to be ready timing.SetTime(timingInfo, timing.RoutingCreated) - routingStatus := wsprovision.SyncRoutingToCluster(workspaceWithConfig, clusterAPI) + routingStatus := wsprovision.SyncRoutingToCluster(workspace, clusterAPI) if !routingStatus.Continue { if routingStatus.FailStartup { - return r.failWorkspace(workspaceWithConfig, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, routingStatus.Message, metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on routing to be ready") message := "Preparing networking" @@ -366,7 +365,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus.setConditionTrue(dw.DevWorkspaceRoutingReady, "Networking ready") timing.SetTime(timingInfo, timing.RoutingReady) - statusOk, err := syncWorkspaceMainURL(clusterWorkspace, routingStatus.ExposedEndpoints, clusterAPI) + statusOk, err := syncWorkspaceMainURL(&clusterWorkspace.DevWorkspace, routingStatus.ExposedEndpoints, clusterAPI) if err != nil { return reconcile.Result{}, err } @@ -375,17 +374,17 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{Requeue: true}, nil } - annotate.AddURLAttributesToEndpoints(&workspaceWithConfig.Spec.Template, routingStatus.ExposedEndpoints) + annotate.AddURLAttributesToEndpoints(&workspace.Spec.Template, routingStatus.ExposedEndpoints) // Step three: provision a configmap on the cluster to mount the flattened devfile in deployment containers - err = metadata.ProvisionWorkspaceMetadata(devfilePodAdditions, clusterWorkspace, &workspaceWithConfig.DevWorkspace, clusterAPI) + err = metadata.ProvisionWorkspaceMetadata(devfilePodAdditions, &clusterWorkspace.DevWorkspace, &workspace.DevWorkspace, clusterAPI) if err != nil { switch provisionErr := err.(type) { case *metadata.NotReadyError: reqLogger.Info(provisionErr.Message) return reconcile.Result{Requeue: true, RequeueAfter: provisionErr.RequeueAfter}, nil case *metadata.ProvisioningError: - return r.failWorkspace(workspaceWithConfig, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, fmt.Sprintf("Error provisioning metadata configmap: %s", provisionErr), metrics.ReasonInfrastructureFailure, reqLogger, &reconcileStatus) default: return reconcile.Result{}, provisionErr } @@ -403,10 +402,10 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request if routingPodAdditions != nil { saAnnotations = routingPodAdditions.ServiceAccountAnnotations } - serviceAcctStatus := wsprovision.SyncServiceAccount(&workspaceWithConfig.DevWorkspace, saAnnotations, clusterAPI) + serviceAcctStatus := wsprovision.SyncServiceAccount(&workspace.DevWorkspace, saAnnotations, clusterAPI) if !serviceAcctStatus.Continue { if serviceAcctStatus.FailStartup { - return r.failWorkspace(workspaceWithConfig, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, serviceAcctStatus.Message, metrics.ReasonBadRequest, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting for workspace ServiceAccount") reconcileStatus.setConditionFalse(dw.DevWorkspaceServiceAccountReady, "Waiting for DevWorkspace ServiceAccount") @@ -415,9 +414,9 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request } return reconcile.Result{Requeue: serviceAcctStatus.Requeue}, serviceAcctStatus.Err } - if wsprovision.NeedsServiceAccountFinalizer(&workspaceWithConfig.Spec.Template) { - coputil.AddFinalizer(clusterWorkspace, constants.ServiceAccountCleanupFinalizer) - if err := r.Update(ctx, clusterWorkspace); err != nil { + if wsprovision.NeedsServiceAccountFinalizer(&workspace.Spec.Template) { + coputil.AddFinalizer(&clusterWorkspace.DevWorkspace, constants.ServiceAccountCleanupFinalizer) + if err := r.Update(ctx, &clusterWorkspace.DevWorkspace); err != nil { return reconcile.Result{}, err } } @@ -425,7 +424,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request serviceAcctName := serviceAcctStatus.ServiceAccountName reconcileStatus.setConditionTrue(dw.DevWorkspaceServiceAccountReady, "DevWorkspace serviceaccount ready") - pullSecretStatus := wsprovision.PullSecrets(clusterAPI, serviceAcctName, workspaceWithConfig.GetNamespace()) + pullSecretStatus := wsprovision.PullSecrets(clusterAPI, serviceAcctName, workspace.GetNamespace()) if !pullSecretStatus.Continue { reconcileStatus.setConditionFalse(conditions.PullSecretsReady, "Waiting for DevWorkspace pull secrets") if !pullSecretStatus.Requeue && pullSecretStatus.Err == nil { @@ -438,11 +437,11 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request // Step six: Create deployment and wait for it to be ready timing.SetTime(timingInfo, timing.DeploymentCreated) - deploymentStatus := wsprovision.SyncDeploymentToCluster(&workspaceWithConfig.DevWorkspace, allPodAdditions, serviceAcctName, clusterAPI, &workspaceWithConfig.Config) + deploymentStatus := wsprovision.SyncDeploymentToCluster(&workspace.DevWorkspace, allPodAdditions, serviceAcctName, clusterAPI, &workspace.Config) if !deploymentStatus.Continue { if deploymentStatus.FailStartup { failureReason := metrics.DetermineProvisioningFailureReason(deploymentStatus) - return r.failWorkspace(workspaceWithConfig, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) + return r.failWorkspace(workspace, deploymentStatus.Info(), failureReason, reqLogger, &reconcileStatus) } reqLogger.Info("Waiting on deployment to be ready") reconcileStatus.setConditionFalse(conditions.DeploymentReady, "Waiting for workspace deployment") @@ -454,7 +453,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request reconcileStatus.setConditionTrue(conditions.DeploymentReady, "DevWorkspace deployment ready") timing.SetTime(timingInfo, timing.DeploymentReady) - serverReady, err := checkServerStatus(clusterWorkspace) + serverReady, err := checkServerStatus(&clusterWorkspace.DevWorkspace) if err != nil { return reconcile.Result{}, err } @@ -463,7 +462,7 @@ func (r *DevWorkspaceReconciler) Reconcile(ctx context.Context, req ctrl.Request return reconcile.Result{RequeueAfter: 1 * time.Second}, nil } timing.SetTime(timingInfo, timing.DevWorkspaceReady) - timing.SummarizeStartup(clusterWorkspace) + timing.SummarizeStartup(&clusterWorkspace.DevWorkspace) reconcileStatus.setConditionTrue(dw.DevWorkspaceReady, "") reconcileStatus.phase = dw.DevWorkspaceStatusRunning return reconcile.Result{}, nil @@ -500,11 +499,11 @@ func (r *DevWorkspaceReconciler) stopWorkspace(ctx context.Context, workspace *c return r.updateWorkspaceStatus(workspace, logger, &status, reconcile.Result{}, nil) } -func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig *common.DevWorkspaceWithConfig, logger logr.Logger) (stopped bool, err error) { +func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspace *common.DevWorkspaceWithConfig, logger logr.Logger) (stopped bool, err error) { workspaceDeployment := &appsv1.Deployment{} namespaceName := types.NamespacedName{ - Name: common.DeploymentName(workspaceWithConfig.Status.DevWorkspaceId), - Namespace: workspaceWithConfig.Namespace, + Name: common.DeploymentName(workspace.Status.DevWorkspaceId), + Namespace: workspace.Namespace, } err = r.Get(ctx, namespaceName, workspaceDeployment) if err != nil { @@ -517,8 +516,8 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig // Update DevWorkspaceRouting to have `devworkspace-started` annotation "false" routing := &controllerv1alpha1.DevWorkspaceRouting{} routingRef := types.NamespacedName{ - Name: common.DevWorkspaceRoutingName(workspaceWithConfig.Status.DevWorkspaceId), - Namespace: workspaceWithConfig.Namespace, + Name: common.DevWorkspaceRoutingName(workspace.Status.DevWorkspaceId), + Namespace: workspace.Namespace, } err = r.Get(ctx, routingRef, routing) if err != nil { @@ -537,11 +536,11 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig } // CleanupOnStop should never be nil as a default is always set - if workspaceWithConfig.Config.Workspace.CleanupOnStop == nil || !*workspaceWithConfig.Config.Workspace.CleanupOnStop { + if workspace.Config.Workspace.CleanupOnStop == nil || !*workspace.Config.Workspace.CleanupOnStop { replicas := workspaceDeployment.Spec.Replicas if replicas == nil || *replicas > 0 { logger.Info("Stopping workspace") - err = wsprovision.ScaleDeploymentToZero(ctx, &workspaceWithConfig.DevWorkspace, r.Client) + err = wsprovision.ScaleDeploymentToZero(ctx, &workspace.DevWorkspace, r.Client) if err != nil && !k8sErrors.IsConflict(err) { return false, err } @@ -551,7 +550,7 @@ func (r *DevWorkspaceReconciler) doStop(ctx context.Context, workspaceWithConfig return workspaceDeployment.Status.Replicas == 0, nil } else { logger.Info("Cleaning up workspace-owned objects") - requeue, err := r.deleteWorkspaceOwnedObjects(ctx, &workspaceWithConfig.DevWorkspace) + requeue, err := r.deleteWorkspaceOwnedObjects(ctx, &workspace.DevWorkspace) return !requeue, err } } diff --git a/controllers/workspace/status.go b/controllers/workspace/status.go index 1e23ce940..ba4cd8473 100644 --- a/controllers/workspace/status.go +++ b/controllers/workspace/status.go @@ -63,20 +63,20 @@ var clock kubeclock.Clock = &kubeclock.RealClock{} // updateWorkspaceStatus updates the current workspace's status field with conditions and phase from the passed in status. // Parameters for result and error are returned unmodified, unless error is nil and another error is encountered while // updating the status. -func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspaceWithConfig *common.DevWorkspaceWithConfig, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) { - syncConditions(&workspaceWithConfig.Status, status) - oldPhase := workspaceWithConfig.Status.Phase - workspaceWithConfig.Status.Phase = status.phase +func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspace *common.DevWorkspaceWithConfig, logger logr.Logger, status *currentStatus, reconcileResult reconcile.Result, reconcileError error) (reconcile.Result, error) { + syncConditions(&workspace.Status, status) + oldPhase := workspace.Status.Phase + workspace.Status.Phase = status.phase - infoMessage := getInfoMessage(&workspaceWithConfig.DevWorkspace, status) - if warn := conditions.GetConditionByType(workspaceWithConfig.Status.Conditions, conditions.DevWorkspaceWarning); warn != nil && warn.Status == corev1.ConditionTrue { + infoMessage := getInfoMessage(&workspace.DevWorkspace, status) + if warn := conditions.GetConditionByType(workspace.Status.Conditions, conditions.DevWorkspaceWarning); warn != nil && warn.Status == corev1.ConditionTrue { infoMessage = fmt.Sprintf("%s %s", warningPresentInfoMessage, infoMessage) } - if workspaceWithConfig.Status.Message != infoMessage { - workspaceWithConfig.Status.Message = infoMessage + if workspace.Status.Message != infoMessage { + workspace.Status.Message = infoMessage } - err := r.Status().Update(context.TODO(), &workspaceWithConfig.DevWorkspace) + err := r.Status().Update(context.TODO(), &workspace.DevWorkspace) if err != nil { if k8sErrors.IsConflict(err) { logger.Info("Failed to update workspace status due to conflict; retrying") @@ -87,7 +87,7 @@ func (r *DevWorkspaceReconciler) updateWorkspaceStatus(workspaceWithConfig *comm } } } else { - updateMetricsForPhase(workspaceWithConfig, oldPhase, status.phase, logger) + updateMetricsForPhase(workspace, oldPhase, status.phase, logger) } return reconcileResult, reconcileError @@ -224,15 +224,15 @@ func getInfoMessage(workspace *dw.DevWorkspace, status *currentStatus) string { // updateMetricsForPhase increments DevWorkspace startup metrics based on phase transitions in a DevWorkspace. It avoids // incrementing the underlying metrics where possible (e.g. reconciling an already running workspace) by only incrementing // counters when the new phase is different from the current on in the DevWorkspace. -func updateMetricsForPhase(workspaceWithConfig *common.DevWorkspaceWithConfig, oldPhase, newPhase dw.DevWorkspacePhase, logger logr.Logger) { +func updateMetricsForPhase(workspace *common.DevWorkspaceWithConfig, oldPhase, newPhase dw.DevWorkspacePhase, logger logr.Logger) { if oldPhase == newPhase { return } switch newPhase { case dw.DevWorkspaceStatusRunning: - metrics.WorkspaceRunning(workspaceWithConfig, logger) + metrics.WorkspaceRunning(workspace, logger) case dw.DevWorkspaceStatusFailed: - metrics.WorkspaceFailed(&workspaceWithConfig.DevWorkspace, logger) + metrics.WorkspaceFailed(&workspace.DevWorkspace, logger) } } @@ -266,17 +266,17 @@ func checkForStartTimeout(workspace *dw.DevWorkspace, config v1alpha1.OperatorCo // configured progress timeout. If the workspace is not in the Failing state or does not have a DevWorkspaceFailed // condition set, returns false. Otherwise, returns true if the workspace has timed out. Returns an error if // timeout is configured with an unparsable duration. -func checkForFailingTimeout(workspaceWithConfig *common.DevWorkspaceWithConfig) (isTimedOut bool, err error) { - if workspaceWithConfig.Status.Phase != devworkspacePhaseFailing { +func checkForFailingTimeout(workspace *common.DevWorkspaceWithConfig) (isTimedOut bool, err error) { + if workspace.Status.Phase != devworkspacePhaseFailing { return false, nil } - timeout, err := time.ParseDuration(workspaceWithConfig.Config.Workspace.ProgressTimeout) + timeout, err := time.ParseDuration(workspace.Config.Workspace.ProgressTimeout) if err != nil { return false, fmt.Errorf("invalid duration specified for timeout: %w", err) } currTime := clock.Now() failedTime := time.Time{} - for _, condition := range workspaceWithConfig.Status.Conditions { + for _, condition := range workspace.Status.Conditions { if condition.Type == dw.DevWorkspaceFailedStart { failedTime = condition.LastTransitionTime.Time } diff --git a/pkg/library/defaults/helper.go b/pkg/library/defaults/helper.go index 32e66561b..52bf9e51a 100644 --- a/pkg/library/defaults/helper.go +++ b/pkg/library/defaults/helper.go @@ -22,16 +22,16 @@ import ( // Overwrites the content of the workspace's Template Spec with the workspace config's Template Spec, // with the exception of the workspace's projects. // If the workspace's Template Spec defined any projects, they are preserved. -func ApplyDefaultTemplate(workspaceWithConfig *common.DevWorkspaceWithConfig) { - if workspaceWithConfig.Config.Workspace.DefaultTemplate == nil { +func ApplyDefaultTemplate(workspace *common.DevWorkspaceWithConfig) { + if workspace.Config.Workspace.DefaultTemplate == nil { return } - defaultCopy := workspaceWithConfig.Config.Workspace.DefaultTemplate.DeepCopy() - originalProjects := workspaceWithConfig.Spec.Template.Projects - workspaceWithConfig.Spec.Template.DevWorkspaceTemplateSpecContent = *defaultCopy - workspaceWithConfig.Spec.Template.Projects = append(workspaceWithConfig.Spec.Template.Projects, originalProjects...) + defaultCopy := workspace.Config.Workspace.DefaultTemplate.DeepCopy() + originalProjects := workspace.Spec.Template.Projects + workspace.Spec.Template.DevWorkspaceTemplateSpecContent = *defaultCopy + workspace.Spec.Template.Projects = append(workspace.Spec.Template.Projects, originalProjects...) } -func NeedsDefaultTemplate(workspaceWithConfig *common.DevWorkspaceWithConfig) bool { - return len(workspaceWithConfig.Spec.Template.Components) == 0 && workspaceWithConfig.Config.Workspace.DefaultTemplate != nil +func NeedsDefaultTemplate(workspace *common.DevWorkspaceWithConfig) bool { + return len(workspace.Spec.Template.Components) == 0 && workspace.Config.Workspace.DefaultTemplate != nil } diff --git a/pkg/library/projects/clone.go b/pkg/library/projects/clone.go index 04971bf4a..79a312146 100644 --- a/pkg/library/projects/clone.go +++ b/pkg/library/projects/clone.go @@ -32,16 +32,15 @@ const ( projectClonerContainerName = "project-clone" ) -func GetProjectCloneInitContainer(workspaceWithConfig *common.DevWorkspaceWithConfig) (*corev1.Container, error) { +func GetProjectCloneInitContainer(workspace *common.DevWorkspaceWithConfig) (*corev1.Container, error) { - workspace := workspaceWithConfig.Spec.Template - if len(workspace.Projects) == 0 { + if len(workspace.Spec.Template.Projects) == 0 { return nil, nil } - if workspace.Attributes.GetString(constants.ProjectCloneAttribute, nil) == constants.ProjectCloneDisable { + if workspace.Spec.Template.Attributes.GetString(constants.ProjectCloneAttribute, nil) == constants.ProjectCloneDisable { return nil, nil } - if !hasContainerComponents(&workspace) { + if !hasContainerComponents(&workspace.Spec.Template) { // Avoid adding project-clone init container when DevWorkspace does not define any containers return nil, nil } @@ -94,7 +93,7 @@ func GetProjectCloneInitContainer(workspaceWithConfig *common.DevWorkspaceWithCo MountPath: constants.DefaultProjectsSourcesRoot, }, }, - ImagePullPolicy: corev1.PullPolicy(workspaceWithConfig.Config.Workspace.ImagePullPolicy), + ImagePullPolicy: corev1.PullPolicy(workspace.Config.Workspace.ImagePullPolicy), }, nil } diff --git a/pkg/provision/storage/asyncStorage.go b/pkg/provision/storage/asyncStorage.go index 50c4f04d5..ed101c68a 100644 --- a/pkg/provision/storage/asyncStorage.go +++ b/pkg/provision/storage/asyncStorage.go @@ -46,39 +46,39 @@ func (*AsyncStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplateS return needsStorage(workspace) } -func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { +func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { if err := checkConfigured(); err != nil { return &ProvisioningError{ Message: fmt.Sprintf("%s. Contact an administrator to resolve this issue.", err.Error()), } } - numWorkspaces, _, err := p.getAsyncWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) + numWorkspaces, _, err := p.getAsyncWorkspaceCount(workspace.Namespace, clusterAPI) if err != nil { return err } // If there is more than one started workspace using async storage, then we fail starting additional ones // Note we need to check phase so as to not accidentally fail an already-running workspace when a second one // is created. - if numWorkspaces > 1 && workspaceWithConfig.Status.Phase != dw.DevWorkspaceStatusRunning { + if numWorkspaces > 1 && workspace.Status.Phase != dw.DevWorkspaceStatusRunning { return &ProvisioningError{ - Message: fmt.Sprintf("cannot provision storage for workspace %s", workspaceWithConfig.Name), + Message: fmt.Sprintf("cannot provision storage for workspace %s", workspace.Name), Err: fmt.Errorf("at most one workspace using async storage can be running in a namespace"), } } // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspace.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !p.NeedsStorage(&workspaceWithConfig.Spec.Template) { + if !p.NeedsStorage(&workspace.Spec.Template) { return nil } // Sync SSH keypair to cluster - secret, configmap, err := asyncstorage.GetOrCreateSSHConfig(&workspaceWithConfig.DevWorkspace, clusterAPI) + secret, configmap, err := asyncstorage.GetOrCreateSSHConfig(&workspace.DevWorkspace, clusterAPI) if err != nil { if errors.Is(err, asyncstorage.NotReadyError) { return &NotReadyError{ @@ -89,12 +89,12 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd return err } - pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) if err != nil { return err } - pvcTerminating, err := checkPVCTerminating(pvcName, workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) + pvcTerminating, err := checkPVCTerminating(pvcName, workspace.Namespace, clusterAPI, workspace.Config) if err != nil { return err } else if pvcTerminating { @@ -106,7 +106,7 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd if pvcName != "" { // Create common PVC if needed - clusterPVC, err := syncCommonPVC(workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) + clusterPVC, err := syncCommonPVC(workspace.Namespace, clusterAPI, workspace.Config) if err != nil { return err } @@ -114,7 +114,7 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd } // Create async server deployment - deploy, err := asyncstorage.SyncWorkspaceSyncDeploymentToCluster(workspaceWithConfig.Namespace, configmap, pvcName, clusterAPI, workspaceWithConfig.Config) + deploy, err := asyncstorage.SyncWorkspaceSyncDeploymentToCluster(workspace.Namespace, configmap, pvcName, clusterAPI, workspace.Config) if err != nil { if errors.Is(err, asyncstorage.NotReadyError) { return &NotReadyError{ @@ -157,32 +157,32 @@ func (p *AsyncStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdd return err } - volumes, err := p.addVolumesForAsyncStorage(podAdditions, &workspaceWithConfig.DevWorkspace) + volumes, err := p.addVolumesForAsyncStorage(podAdditions, &workspace.DevWorkspace) if err != nil { return err } sshSecretVolume := asyncstorage.GetVolumeFromSecret(secret) - asyncSidecar := asyncstorage.GetAsyncSidecar(workspaceWithConfig.Status.DevWorkspaceId, sshSecretVolume.Name, volumes) + asyncSidecar := asyncstorage.GetAsyncSidecar(workspace.Status.DevWorkspaceId, sshSecretVolume.Name, volumes) podAdditions.Containers = append(podAdditions.Containers, *asyncSidecar) podAdditions.Volumes = append(podAdditions.Volumes, *sshSecretVolume) return nil } -func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { +func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // TODO: This approach relies on there being a maximum of one workspace running per namespace. - asyncDeploy, err := asyncstorage.GetWorkspaceSyncDeploymentCluster(workspaceWithConfig.Namespace, clusterAPI) + asyncDeploy, err := asyncstorage.GetWorkspaceSyncDeploymentCluster(workspace.Namespace, clusterAPI) if err != nil { if k8sErrors.IsNotFound(err) { - return runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) + return runCommonPVCCleanupJob(workspace, clusterAPI) } else { return err } } // Check if another workspace is currently using the async server - numWorkspaces, totalWorkspaces, err := p.getAsyncWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) + numWorkspaces, totalWorkspaces, err := p.getAsyncWorkspaceCount(workspace.Namespace, clusterAPI) if err != nil { return err } @@ -190,7 +190,7 @@ func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *c case 0: // no problem case 1: - if workspaceWithConfig.Spec.Started { + if workspace.Spec.Started { // This is the only workspace using the async server, we can safely stop it break } @@ -219,12 +219,12 @@ func (p *AsyncStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *c } // Clean up PVC using usual job - err = runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) + err = runCommonPVCCleanupJob(workspace, clusterAPI) if err != nil { return err } - retry, err := asyncstorage.RemoveAuthorizedKeyFromConfigMap(&workspaceWithConfig.DevWorkspace, clusterAPI) + retry, err := asyncstorage.RemoveAuthorizedKeyFromConfigMap(&workspace.DevWorkspace, clusterAPI) if err != nil { return &ProvisioningError{ Message: "Failed to remove authorized key from async storage configmap", diff --git a/pkg/provision/storage/cleanup.go b/pkg/provision/storage/cleanup.go index 87bb5b233..61f8377cf 100644 --- a/pkg/provision/storage/cleanup.go +++ b/pkg/provision/storage/cleanup.go @@ -113,30 +113,30 @@ func runCommonPVCCleanupJob(workspace *common.DevWorkspaceWithConfig, clusterAPI } } -func getSpecCommonPVCCleanupJob(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*batchv1.Job, error) { - workspaceId := workspaceWithConfig.Status.DevWorkspaceId +func getSpecCommonPVCCleanupJob(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*batchv1.Job, error) { + workspaceId := workspace.Status.DevWorkspaceId - pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) if err != nil { return nil, err } if pvcName == "" { - pvcName = workspaceWithConfig.Config.Workspace.PVCName + pvcName = workspace.Config.Workspace.PVCName } jobLabels := map[string]string{ constants.DevWorkspaceIDLabel: workspaceId, - constants.DevWorkspaceNameLabel: workspaceWithConfig.Name, - constants.DevWorkspaceCreatorLabel: workspaceWithConfig.Labels[constants.DevWorkspaceCreatorLabel], + constants.DevWorkspaceNameLabel: workspace.Name, + constants.DevWorkspaceCreatorLabel: workspace.Labels[constants.DevWorkspaceCreatorLabel], } - if restrictedAccess, needsRestrictedAccess := workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; needsRestrictedAccess { + if restrictedAccess, needsRestrictedAccess := workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; needsRestrictedAccess { jobLabels[constants.DevWorkspaceRestrictedAccessAnnotation] = restrictedAccess } job := &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: common.PVCCleanupJobName(workspaceId), - Namespace: workspaceWithConfig.Namespace, + Namespace: workspace.Namespace, Labels: jobLabels, }, Spec: batchv1.JobSpec{ @@ -148,7 +148,7 @@ func getSpecCommonPVCCleanupJob(workspaceWithConfig *common.DevWorkspaceWithConf }, Spec: corev1.PodSpec{ RestartPolicy: "Never", - SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(workspaceWithConfig.Config), + SecurityContext: wsprovision.GetDevWorkspaceSecurityContext(workspace.Config), Volumes: []corev1.Volume{ { Name: pvcName, @@ -191,7 +191,7 @@ func getSpecCommonPVCCleanupJob(workspaceWithConfig *common.DevWorkspaceWithConf }, } - podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(workspaceWithConfig.Namespace, clusterAPI) + podTolerations, nodeSelector, err := nsconfig.GetNamespacePodTolerationsAndNodeSelector(workspace.Namespace, clusterAPI) if err != nil { return nil, err } @@ -202,16 +202,16 @@ func getSpecCommonPVCCleanupJob(workspaceWithConfig *common.DevWorkspaceWithConf job.Spec.Template.Spec.NodeSelector = nodeSelector } - if err := controllerutil.SetControllerReference(workspaceWithConfig, job, clusterAPI.Scheme); err != nil { + if err := controllerutil.SetControllerReference(workspace, job, clusterAPI.Scheme); err != nil { return nil, err } return job, nil } -func commonPVCExists(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (bool, error) { +func commonPVCExists(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (bool, error) { namespacedName := types.NamespacedName{ - Name: workspaceWithConfig.Config.Workspace.PVCName, - Namespace: workspaceWithConfig.Namespace, + Name: workspace.Config.Workspace.PVCName, + Namespace: workspace.Namespace, } err := clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, &corev1.PersistentVolumeClaim{}) if err != nil { diff --git a/pkg/provision/storage/commonStorage.go b/pkg/provision/storage/commonStorage.go index b87ef226f..7dddb4068 100644 --- a/pkg/provision/storage/commonStorage.go +++ b/pkg/provision/storage/commonStorage.go @@ -42,23 +42,23 @@ func (*CommonStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTemplate return needsStorage(workspace) } -func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { +func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspace.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !p.NeedsStorage(&workspaceWithConfig.Spec.Template) { + if !p.NeedsStorage(&workspace.Spec.Template) { return nil } - pvcName, err := checkForExistingCommonPVC(workspaceWithConfig.Namespace, clusterAPI) + pvcName, err := checkForExistingCommonPVC(workspace.Namespace, clusterAPI) if err != nil { return err } - pvcTerminating, err := checkPVCTerminating(pvcName, workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) + pvcTerminating, err := checkPVCTerminating(pvcName, workspace.Namespace, clusterAPI, workspace.Config) if err != nil { return err } else if pvcTerminating { @@ -69,14 +69,14 @@ func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAd } if pvcName == "" { - commonPVC, err := syncCommonPVC(workspaceWithConfig.Namespace, clusterAPI, workspaceWithConfig.Config) + commonPVC, err := syncCommonPVC(workspace.Namespace, clusterAPI, workspace.Config) if err != nil { return err } pvcName = commonPVC.Name } - if err := p.rewriteContainerVolumeMounts(workspaceWithConfig.Status.DevWorkspaceId, pvcName, podAdditions, &workspaceWithConfig.Spec.Template); err != nil { + if err := p.rewriteContainerVolumeMounts(workspace.Status.DevWorkspaceId, pvcName, podAdditions, &workspace.Spec.Template); err != nil { return &ProvisioningError{ Err: err, Message: "Could not rewrite container volume mounts", @@ -86,8 +86,8 @@ func (p *CommonStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAd return nil } -func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { - totalWorkspaces, err := getSharedPVCWorkspaceCount(workspaceWithConfig.Namespace, clusterAPI) +func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { + totalWorkspaces, err := getSharedPVCWorkspaceCount(workspace.Namespace, clusterAPI) if err != nil { return err } @@ -95,10 +95,10 @@ func (p *CommonStorageProvisioner) CleanupWorkspaceStorage(workspaceWithConfig * // If the number of common + async workspaces that exist (started or stopped) is zero, // delete common PVC instead of running cleanup job if totalWorkspaces > 0 { - return runCommonPVCCleanupJob(workspaceWithConfig, clusterAPI) + return runCommonPVCCleanupJob(workspace, clusterAPI) } else { sharedPVC := &corev1.PersistentVolumeClaim{} - namespacedName := types.NamespacedName{Name: workspaceWithConfig.Config.Workspace.PVCName, Namespace: workspaceWithConfig.Namespace} + namespacedName := types.NamespacedName{Name: workspace.Config.Workspace.PVCName, Namespace: workspace.Namespace} err := clusterAPI.Client.Get(clusterAPI.Ctx, namespacedName, sharedPVC) if err != nil { diff --git a/pkg/provision/storage/ephemeralStorage.go b/pkg/provision/storage/ephemeralStorage.go index 89ecbcd93..abed5880b 100644 --- a/pkg/provision/storage/ephemeralStorage.go +++ b/pkg/provision/storage/ephemeralStorage.go @@ -35,8 +35,8 @@ func (e EphemeralStorageProvisioner) NeedsStorage(_ *dw.DevWorkspaceTemplateSpec return false } -func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, _ sync.ClusterAPI) error { - persistent, ephemeral, projects := getWorkspaceVolumes(&workspaceWithConfig.DevWorkspace) +func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, _ sync.ClusterAPI) error { + persistent, ephemeral, projects := getWorkspaceVolumes(&workspace.DevWorkspace) if _, err := addEphemeralVolumesToPodAdditions(podAdditions, persistent); err != nil { return err } @@ -48,7 +48,7 @@ func (e EphemeralStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.Pod return err } } else { - if container.AnyMountSources(workspaceWithConfig.Spec.Template.Components) { + if container.AnyMountSources(workspace.Spec.Template.Components) { projectsComponent := dw.Component{Name: "projects"} projectsComponent.Volume = &dw.VolumeComponent{} if _, err := addEphemeralVolumesToPodAdditions(podAdditions, []dw.Component{projectsComponent}); err != nil { diff --git a/pkg/provision/storage/perWorkspaceStorage.go b/pkg/provision/storage/perWorkspaceStorage.go index 8a6ab88eb..79fac25ad 100644 --- a/pkg/provision/storage/perWorkspaceStorage.go +++ b/pkg/provision/storage/perWorkspaceStorage.go @@ -44,19 +44,19 @@ func (*PerWorkspaceStorageProvisioner) NeedsStorage(workspace *dw.DevWorkspaceTe return false } -func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { +func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error { // Add ephemeral volumes - if err := addEphemeralVolumesFromWorkspace(&workspaceWithConfig.DevWorkspace, podAdditions); err != nil { + if err := addEphemeralVolumesFromWorkspace(&workspace.DevWorkspace, podAdditions); err != nil { return err } // If persistent storage is not needed, we're done - if !needsStorage(&workspaceWithConfig.Spec.Template) { + if !needsStorage(&workspace.Spec.Template) { return nil } // Get perWorkspace PVC spec and sync it with cluster - perWorkspacePVC, err := syncPerWorkspacePVC(workspaceWithConfig, clusterAPI) + perWorkspacePVC, err := syncPerWorkspacePVC(workspace, clusterAPI) if err != nil { return err } @@ -70,7 +70,7 @@ func (p *PerWorkspaceStorageProvisioner) ProvisionStorage(podAdditions *v1alpha1 } // Rewrite container volume mounts - if err := p.rewriteContainerVolumeMounts(workspaceWithConfig.Status.DevWorkspaceId, pvcName, podAdditions, &workspaceWithConfig.Spec.Template); err != nil { + if err := p.rewriteContainerVolumeMounts(workspace.Status.DevWorkspaceId, pvcName, podAdditions, &workspace.Spec.Template); err != nil { return &ProvisioningError{ Err: err, Message: "Could not rewrite container volume mounts", @@ -155,14 +155,14 @@ func (p *PerWorkspaceStorageProvisioner) rewriteContainerVolumeMounts(workspaceI return nil } -func syncPerWorkspacePVC(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) { - namespacedConfig, err := nsconfig.ReadNamespacedConfig(workspaceWithConfig.Namespace, clusterAPI) +func syncPerWorkspacePVC(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) (*corev1.PersistentVolumeClaim, error) { + namespacedConfig, err := nsconfig.ReadNamespacedConfig(workspace.Namespace, clusterAPI) if err != nil { return nil, fmt.Errorf("failed to read namespace-specific configuration: %w", err) } // TODO: Determine the storage size that is needed by iterating through workspace volumes, // adding the sizes specified and figuring out overrides/defaults - pvcSize := *workspaceWithConfig.Config.Workspace.DefaultStorageSize.PerWorkspace + pvcSize := *workspace.Config.Workspace.DefaultStorageSize.PerWorkspace if namespacedConfig != nil && namespacedConfig.PerWorkspacePVCSize != "" { pvcSize, err = resource.ParseQuantity(namespacedConfig.PerWorkspacePVCSize) if err != nil { @@ -170,12 +170,12 @@ func syncPerWorkspacePVC(workspaceWithConfig *common.DevWorkspaceWithConfig, clu } } - pvc, err := getPVCSpec(common.PerWorkspacePVCName(workspaceWithConfig.Status.DevWorkspaceId), workspaceWithConfig.Namespace, pvcSize, workspaceWithConfig.Config) + pvc, err := getPVCSpec(common.PerWorkspacePVCName(workspace.Status.DevWorkspaceId), workspace.Namespace, pvcSize, workspace.Config) if err != nil { return nil, err } - if err := controllerutil.SetControllerReference(&workspaceWithConfig.DevWorkspace, pvc, clusterAPI.Scheme); err != nil { + if err := controllerutil.SetControllerReference(&workspace.DevWorkspace, pvc, clusterAPI.Scheme); err != nil { return nil, err } diff --git a/pkg/provision/storage/provisioner.go b/pkg/provision/storage/provisioner.go index 8126fec19..8ca5165ab 100644 --- a/pkg/provision/storage/provisioner.go +++ b/pkg/provision/storage/provisioner.go @@ -29,14 +29,14 @@ type Provisioner interface { // out-of-pod required objects to the cluster. // Returns NotReadyError to signify that storage is not ready, ProvisioningError when a fatal issue is encountered, // and other error if there is an unexpected problem. - ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error + ProvisionStorage(podAdditions *v1alpha1.PodAdditions, workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error // NeedsStorage returns whether the current workspace needs a PVC to be provisioned, given this storage strategy. NeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool // CleanupWorkspaceStorage removes any objects provisioned by in the ProvisionStorage step that aren't automatically removed when a // DevWorkspace is deleted (e.g. delete subfolders in a common PVC assigned to the workspace) // Returns nil on success (DevWorkspace can be deleted), NotReadyError if additional reconciles are necessary, ProvisioningError when // a fatal issue is encountered, and any other error if an unexpected problem arises. - CleanupWorkspaceStorage(workspaceWithConfig *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error + CleanupWorkspaceStorage(workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) error } // GetProvisioner returns the storage provisioner that should be used for the current workspace diff --git a/pkg/provision/workspace/routing.go b/pkg/provision/workspace/routing.go index 3f7dd8ce8..fed240d06 100644 --- a/pkg/provision/workspace/routing.go +++ b/pkg/provision/workspace/routing.go @@ -38,10 +38,10 @@ type RoutingProvisioningStatus struct { } func SyncRoutingToCluster( - workspaceWithConfig *common.DevWorkspaceWithConfig, + workspace *common.DevWorkspaceWithConfig, clusterAPI sync.ClusterAPI) RoutingProvisioningStatus { - specRouting, err := getSpecRouting(workspaceWithConfig, clusterAPI.Scheme) + specRouting, err := getSpecRouting(workspace, clusterAPI.Scheme) if err != nil { return RoutingProvisioningStatus{ ProvisioningStatus: ProvisioningStatus{Err: err}, @@ -86,11 +86,11 @@ func SyncRoutingToCluster( } func getSpecRouting( - workspaceWithConfig *common.DevWorkspaceWithConfig, + workspace *common.DevWorkspaceWithConfig, scheme *runtime.Scheme) (*v1alpha1.DevWorkspaceRouting, error) { endpoints := map[string]v1alpha1.EndpointList{} - for _, component := range workspaceWithConfig.Spec.Template.Components { + for _, component := range workspace.Spec.Template.Components { if component.Container == nil { continue } @@ -101,47 +101,47 @@ func getSpecRouting( } var annotations map[string]string - if val, ok := workspaceWithConfig.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; ok { + if val, ok := workspace.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]; ok { annotations = maputils.Append(annotations, constants.DevWorkspaceRestrictedAccessAnnotation, val) } annotations = maputils.Append(annotations, constants.DevWorkspaceStartedStatusAnnotation, "true") - routingClass := workspaceWithConfig.Spec.RoutingClass + routingClass := workspace.Spec.RoutingClass if routingClass == "" { - routingClass = workspaceWithConfig.Config.Routing.DefaultRoutingClass + routingClass = workspace.Config.Routing.DefaultRoutingClass } // copy the annotations for the specific routingClass from the workspace object to the routing expectedAnnotationPrefix := routingClass + constants.RoutingAnnotationInfix - for k, v := range workspaceWithConfig.GetAnnotations() { + for k, v := range workspace.GetAnnotations() { if strings.HasPrefix(k, expectedAnnotationPrefix) { annotations = maputils.Append(annotations, k, v) } } if v1alpha1.DevWorkspaceRoutingClass(routingClass) == v1alpha1.DevWorkspaceRoutingBasic { - annotations = maputils.Append(annotations, constants.ClusterHostSuffixAnnotation, workspaceWithConfig.Config.Routing.ClusterHostSuffix) + annotations = maputils.Append(annotations, constants.ClusterHostSuffixAnnotation, workspace.Config.Routing.ClusterHostSuffix) } routing := &v1alpha1.DevWorkspaceRouting{ ObjectMeta: metav1.ObjectMeta{ - Name: common.DevWorkspaceRoutingName(workspaceWithConfig.Status.DevWorkspaceId), - Namespace: workspaceWithConfig.Namespace, + Name: common.DevWorkspaceRoutingName(workspace.Status.DevWorkspaceId), + Namespace: workspace.Namespace, Labels: map[string]string{ - constants.DevWorkspaceIDLabel: workspaceWithConfig.Status.DevWorkspaceId, + constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, }, Annotations: annotations, }, Spec: v1alpha1.DevWorkspaceRoutingSpec{ - DevWorkspaceId: workspaceWithConfig.Status.DevWorkspaceId, + DevWorkspaceId: workspace.Status.DevWorkspaceId, RoutingClass: v1alpha1.DevWorkspaceRoutingClass(routingClass), Endpoints: endpoints, PodSelector: map[string]string{ - constants.DevWorkspaceIDLabel: workspaceWithConfig.Status.DevWorkspaceId, + constants.DevWorkspaceIDLabel: workspace.Status.DevWorkspaceId, }, }, } - err := controllerutil.SetControllerReference(&workspaceWithConfig.DevWorkspace, routing, scheme) + err := controllerutil.SetControllerReference(&workspace.DevWorkspace, routing, scheme) if err != nil { return nil, err }