From bb982139c63c762d99a35e5be1150db9fb309ede Mon Sep 17 00:00:00 2001 From: Danielle Lancashire Date: Tue, 20 Aug 2024 14:46:40 +0200 Subject: [PATCH] spinapp: Relax executor validation I've been using the Spin Operator with Flux deployments, and requring executors to exist before creating a Spin App leads to some... rather un-ergonomic requirements, by nature of how resource installations need to be tiered when orchestrating tools like Flux: Specifically: 1) Install dependencies (cert-manager) 2) Install Spin Operator 3) Install executor 4) finally install application This seems reasonable enough at first, but because executors and applications are in the same domain (namespace), it's more awkward than one would otherwise expect, because every application suddenly becomes _two_ sets of packages rather than one. By relaxing this requirement, we still provide feedback to users (events), but allow those to be collapsed back into a single installation step - which is a great improvement for the simplest-possible case. --- internal/controller/spinapp_controller.go | 5 +- internal/webhook/spinapp_validating.go | 31 ----------- internal/webhook/spinapp_validating_test.go | 59 --------------------- 3 files changed, 4 insertions(+), 91 deletions(-) diff --git a/internal/controller/spinapp_controller.go b/internal/controller/spinapp_controller.go index 4983c18..77cda52 100644 --- a/internal/controller/spinapp_controller.go +++ b/internal/controller/spinapp_controller.go @@ -21,6 +21,7 @@ import ( "fmt" "hash/adler32" "maps" + "time" "github.com/pelletier/go-toml/v2" appsv1 "k8s.io/api/apps/v1" @@ -110,7 +111,9 @@ func (r *SpinAppReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct log.Error(err, "unable to fetch executor") r.Recorder.Event(&spinApp, "Warning", "MissingExecutor", fmt.Sprintf("Could not find SpinAppExecutor %s/%s", req.NamespacedName.Namespace, spinApp.Spec.Executor)) - return ctrl.Result{}, err + return ctrl.Result{ + RequeueAfter: 15 * time.Second, + }, err } // Update the status of the SpinApp diff --git a/internal/webhook/spinapp_validating.go b/internal/webhook/spinapp_validating.go index a02bb83..fa232ba 100644 --- a/internal/webhook/spinapp_validating.go +++ b/internal/webhook/spinapp_validating.go @@ -54,16 +54,9 @@ func (v *SpinAppValidator) ValidateDelete(ctx context.Context, obj runtime.Objec func (v *SpinAppValidator) validateSpinApp(ctx context.Context, spinApp *spinv1alpha1.SpinApp) error { var allErrs field.ErrorList - executor, err := validateExecutor(spinApp.Spec, v.fetchExecutor(ctx, spinApp.Namespace)) - if err != nil { - allErrs = append(allErrs, err) - } if err := validateReplicas(spinApp.Spec); err != nil { allErrs = append(allErrs, err) } - if err := validateAnnotations(spinApp.Spec, executor); err != nil { - allErrs = append(allErrs, err) - } if len(allErrs) == 0 { return nil } @@ -113,27 +106,3 @@ func validateReplicas(spec spinv1alpha1.SpinAppSpec) *field.Error { return nil } - -func validateAnnotations(spec spinv1alpha1.SpinAppSpec, executor *spinv1alpha1.SpinAppExecutor) *field.Error { - // We can't do any validation if the executor isn't available, but validation - // will fail because of earlier errors. - if executor == nil { - return nil - } - - if executor.Spec.CreateDeployment { - return nil - } - // TODO: Make these validations opt in for executors? - Some runtimes may want these regardless. - if len(spec.DeploymentAnnotations) != 0 { - return field.Invalid( - field.NewPath("spec").Child("deploymentAnnotations"), - spec.DeploymentAnnotations, - "deploymentAnnotations can't be set when the executor does not use operator deployments") - } - if len(spec.PodAnnotations) != 0 { - return field.Invalid(field.NewPath("spec").Child("podAnnotations"), spec.PodAnnotations, "podAnnotations can't be set when the executor does not use operator deployments") - } - - return nil -} diff --git a/internal/webhook/spinapp_validating_test.go b/internal/webhook/spinapp_validating_test.go index 7a7e50a..78e8f84 100644 --- a/internal/webhook/spinapp_validating_test.go +++ b/internal/webhook/spinapp_validating_test.go @@ -1,29 +1,12 @@ package webhook import ( - "errors" "testing" spinv1alpha1 "github.com/spinkube/spin-operator/api/v1alpha1" - "github.com/spinkube/spin-operator/internal/constants" "github.com/stretchr/testify/require" ) -func TestValidateExecutor(t *testing.T) { - t.Parallel() - - _, fldErr := validateExecutor(spinv1alpha1.SpinAppSpec{}, func(string) (*spinv1alpha1.SpinAppExecutor, error) { return nil, nil }) - require.EqualError(t, fldErr, "spec.executor: Invalid value: \"\": executor must be set, likely no default executor was set because you have no executors installed") - - _, fldErr = validateExecutor( - spinv1alpha1.SpinAppSpec{Executor: constants.CyclotronExecutor}, - func(string) (*spinv1alpha1.SpinAppExecutor, error) { return nil, errors.New("executor not found?") }) - require.EqualError(t, fldErr, "spec.executor: Invalid value: \"cyclotron\": executor does not exist in namespace") - - _, fldErr = validateExecutor(spinv1alpha1.SpinAppSpec{Executor: constants.ContainerDShimSpinExecutor}, func(string) (*spinv1alpha1.SpinAppExecutor, error) { return nil, nil }) - require.Nil(t, fldErr) -} - func TestValidateReplicas(t *testing.T) { t.Parallel() @@ -33,45 +16,3 @@ func TestValidateReplicas(t *testing.T) { fldErr = validateReplicas(spinv1alpha1.SpinAppSpec{Replicas: 1}) require.Nil(t, fldErr) } - -func TestValidateAnnotations(t *testing.T) { - t.Parallel() - - deploymentlessExecutor := &spinv1alpha1.SpinAppExecutor{ - Spec: spinv1alpha1.SpinAppExecutorSpec{ - CreateDeployment: false, - }, - } - deploymentfullExecutor := &spinv1alpha1.SpinAppExecutor{ - Spec: spinv1alpha1.SpinAppExecutorSpec{ - CreateDeployment: true, - }, - } - - fldErr := validateAnnotations(spinv1alpha1.SpinAppSpec{ - Executor: "an-executor", - DeploymentAnnotations: map[string]string{"key": "asdf"}, - }, deploymentlessExecutor) - require.EqualError(t, fldErr, - `spec.deploymentAnnotations: Invalid value: map[string]string{"key":"asdf"}: `+ - `deploymentAnnotations can't be set when the executor does not use operator deployments`) - - fldErr = validateAnnotations(spinv1alpha1.SpinAppSpec{ - Executor: "an-executor", - PodAnnotations: map[string]string{"key": "asdf"}, - }, deploymentlessExecutor) - require.EqualError(t, fldErr, - `spec.podAnnotations: Invalid value: map[string]string{"key":"asdf"}: `+ - `podAnnotations can't be set when the executor does not use operator deployments`) - - fldErr = validateAnnotations(spinv1alpha1.SpinAppSpec{ - Executor: "an-executor", - DeploymentAnnotations: map[string]string{"key": "asdf"}, - }, deploymentfullExecutor) - require.Nil(t, fldErr) - - fldErr = validateAnnotations(spinv1alpha1.SpinAppSpec{ - Executor: "an-executor", - }, deploymentlessExecutor) - require.Nil(t, fldErr) -}