From 202fbe7d50a5991cca862724e839365d644dd3b1 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Tue, 11 Jun 2024 22:42:19 +0800 Subject: [PATCH 1/9] Fix activation creation issues --- api/pkg/apis/v1alpha1/vendors/stage-vendor.go | 51 +++++-- k8s/apis/workflow/v1/activation_webhook.go | 138 ++++++++++++++++++ k8s/config/oss/webhook/manifests.yaml | 40 +++++ k8s/main.go | 4 + .../templates/symphony-core/symphonyk8s.yaml | 40 +++++ 5 files changed, 257 insertions(+), 16 deletions(-) create mode 100644 k8s/apis/workflow/v1/activation_webhook.go diff --git a/api/pkg/apis/v1alpha1/vendors/stage-vendor.go b/api/pkg/apis/v1alpha1/vendors/stage-vendor.go index a0860f2e8..10fba49c5 100644 --- a/api/pkg/apis/v1alpha1/vendors/stage-vendor.go +++ b/api/pkg/apis/v1alpha1/vendors/stage-vendor.go @@ -83,19 +83,29 @@ func (s *StageVendor) Init(config vendors.VendorConfig, factories []managers.IMa return v1alpha2.NewCOAError(nil, "event body is not an activation job", v1alpha2.BadRequest) } campaignName := api_utils.ReplaceSeperator(actData.Campaign) + campaign, err := s.CampaignsManager.GetState(context.TODO(), campaignName, actData.Namespace) if err != nil { log.Error("V (Stage): unable to find campaign: %+v", err) + err = s.reportActivationStatusWithBadRequest(actData.Activation, actData.Namespace, err) + // If report status succeeded, return an empty err so the subscribe function will not be retried + // The actual error will be stored in Activation cr return err } activation, err := s.ActivationsManager.GetState(context.TODO(), actData.Activation, actData.Namespace) if err != nil { log.Error("V (Stage): unable to find activation: %+v", err) + err = s.reportActivationStatusWithBadRequest(actData.Activation, actData.Namespace, err) + // If report status succeeded, return an empty err so the subscribe function will not be retried + // The actual error will be stored in Activation cr return err } evt, err := s.StageManager.HandleActivationEvent(context.TODO(), actData, *campaign.Spec, activation) if err != nil { + err = s.reportActivationStatusWithBadRequest(actData.Activation, actData.Namespace, err) + // If report status succeeded, return an empty err so the subscribe function will not be retried + // The actual error will be stored in Activation cr return err } @@ -122,30 +132,22 @@ func (s *StageVendor) Init(config vendors.VendorConfig, factories []managers.IMa err := json.Unmarshal(jData, &triggerData) if err != nil { err = v1alpha2.NewCOAError(nil, "event body is not an activation job", v1alpha2.BadRequest) - status.Status = v1alpha2.BadRequest - status.StatusMessage = v1alpha2.BadRequest.String() - status.ErrorMessage = err.Error() - status.IsActive = false sLog.Errorf("V (Stage): failed to deserialize activation data: %v", err) - err = s.ActivationsManager.ReportStatus(context.TODO(), triggerData.Activation, triggerData.Namespace, status) - if err != nil { - sLog.Errorf("V (Stage): failed to report error status: %v (%v)", status.ErrorMessage, err) - } + err = s.reportActivationStatusWithBadRequest(triggerData.Activation, triggerData.Namespace, err) + // If report status succeeded, return an empty err so the subscribe function will not be retried + // The actual error will be stored in Activation cr + return err } status.Outputs["__namespace"] = triggerData.Namespace campaignName := api_utils.ReplaceSeperator(triggerData.Campaign) campaign, err := s.CampaignsManager.GetState(context.TODO(), campaignName, triggerData.Namespace) if err != nil { - status.Status = v1alpha2.BadRequest - status.StatusMessage = v1alpha2.BadRequest.String() - status.ErrorMessage = err.Error() - status.IsActive = false sLog.Errorf("V (Stage): failed to get campaign spec: %v", err) - err = s.ActivationsManager.ReportStatus(context.TODO(), triggerData.Activation, triggerData.Namespace, status) - if err != nil { - sLog.Errorf("V (Stage): failed to report error status: %v (%v)", status.ErrorMessage, err) - } + err = s.reportActivationStatusWithBadRequest(triggerData.Activation, triggerData.Namespace, err) + // If report status succeeded, return an empty err so the subscribe function will not be retried + // The actual error will be stored in Activation cr + return err } status.Stage = triggerData.Stage status.ActivationGeneration = triggerData.ActivationGeneration @@ -306,3 +308,20 @@ func (s *StageVendor) Init(config vendors.VendorConfig, factories []managers.IMa }) return nil } + +func (s *StageVendor) reportActivationStatusWithBadRequest(activation string, namespace string, err error) error { + status := model.ActivationStatus{ + Stage: "", + NextStage: "", + Outputs: map[string]interface{}{}, + Status: v1alpha2.BadRequest, + StatusMessage: v1alpha2.BadRequest.String(), + ErrorMessage: err.Error(), + IsActive: true, + } + err = s.ActivationsManager.ReportStatus(context.TODO(), activation, namespace, status) + if err != nil { + sLog.Errorf("V (Stage): failed to report error status on activtion %s/%s: %v (%v)", namespace, activation, status.ErrorMessage, err) + } + return err +} diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go new file mode 100644 index 000000000..2b13baf4f --- /dev/null +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -0,0 +1,138 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + * SPDX-License-Identifier: MIT + */ + +package v1 + +import ( + "context" + "gopls-workspace/apis/metrics/v1" + "strings" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation/field" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +// log is for logging in this package. +var activationlog = logf.Log.WithName("activation-resource") +var myActivationClient client.Client +var catalogWebhookValidationMetrics *metrics.Metrics + +func (r *Activation) SetupWebhookWithManager(mgr ctrl.Manager) error { + myActivationClient = mgr.GetClient() + mgr.GetFieldIndexer().IndexField(context.Background(), &Activation{}, ".metadata.name", func(rawObj client.Object) []string { + activation := rawObj.(*Activation) + return []string{activation.Name} + }) + + // initialize the controller operation metrics + if catalogWebhookValidationMetrics == nil { + metrics, err := metrics.New() + if err != nil { + return err + } + catalogWebhookValidationMetrics = metrics + } + + return ctrl.NewWebhookManagedBy(mgr). + For(r). + Complete() +} + +// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! + +//+kubebuilder:webhook:path=/mutate-workflow-symphony-v1-activation,mutating=true,failurePolicy=fail,sideEffects=None,groups=workflow.symphony,resources=activations,verbs=create;update,versions=v1,name=mactivation.kb.io,admissionReviewVersions=v1 + +var _ webhook.Defaulter = &Activation{} + +// Default implements webhook.Defaulter so a webhook will be registered for the type +func (r *Activation) Default() { + activationlog.Info("default", "name", r.Name) +} + +// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. + +//+kubebuilder:webhook:path=/validate-workflow-symphony-v1-activation,mutating=false,failurePolicy=fail,sideEffects=None,groups=workflow.symphony,resources=activations,verbs=create;update,versions=v1,name=mactivation.kb.io,admissionReviewVersions=v1 + +var _ webhook.Validator = &Activation{} + +// ValidateCreate implements webhook.Validator so a webhook will be registered for the type +func (r *Activation) ValidateCreate() (admission.Warnings, error) { + activationlog.Info("validate create", "name", r.Name) + + validateCreateTime := time.Now() + validationError := r.validateCreateActivation() + if validationError != nil { + catalogWebhookValidationMetrics.ControllerValidationLatency( + validateCreateTime, + metrics.CreateOperationType, + metrics.InvalidResource, + metrics.CatalogResourceType) + } else { + catalogWebhookValidationMetrics.ControllerValidationLatency( + validateCreateTime, + metrics.CreateOperationType, + metrics.ValidResource, + metrics.CatalogResourceType) + } + + return nil, validationError +} + +// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type +func (r *Activation) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { + activationlog.Info("validate update", "name", r.Name) + + return nil, nil +} + +// ValidateDelete implements webhook.Validator so a webhook will be registered for the type +func (r *Activation) ValidateDelete() (admission.Warnings, error) { + activationlog.Info("validate delete", "name", r.Name) + + return nil, nil +} + +func (r *Activation) validateCreateActivation() error { + var allErrs field.ErrorList + + if err := r.validateNameOnCreate(); err != nil { + allErrs = append(allErrs, err) + } + if err := r.validateCampaignOnCreate(); err != nil { + allErrs = append(allErrs, err) + } + if len(allErrs) == 0 { + return nil + } + + return apierrors.NewInvalid(schema.GroupKind{Group: "workflow.symphony", Kind: "Activation"}, r.Name, allErrs) +} + +func (r *Activation) validateNameOnCreate() *field.Error { + if strings.Contains(r.ObjectMeta.Name, "-") { + return field.Invalid(field.NewPath("metadata").Child("name"), r.ObjectMeta.Name, "name must not contain '-'") + } + return nil +} +func (r *Activation) validateCampaignOnCreate() *field.Error { + if r.Spec.Campaign == "" { + return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "campaign must not be empty") + } + var campaign Campaign + err := myActivationClient.Get(context.Background(), client.ObjectKey{Name: r.Spec.Campaign, Namespace: r.Namespace}, &campaign) + if err != nil { + return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "campaign doesn't exist") + } + return nil +} diff --git a/k8s/config/oss/webhook/manifests.yaml b/k8s/config/oss/webhook/manifests.yaml index 89adaa190..6de9bf1c3 100644 --- a/k8s/config/oss/webhook/manifests.yaml +++ b/k8s/config/oss/webhook/manifests.yaml @@ -104,6 +104,26 @@ webhooks: resources: - solutions sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-workflow-symphony-v1-activation + failurePolicy: Fail + name: mactivation.kb.io + rules: + - apiGroups: + - workflow.symphony + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - activations + sideEffects: None - admissionReviewVersions: - v1 clientConfig: @@ -312,6 +332,26 @@ webhooks: resources: - solutions sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-workflow-symphony-v1-activation + failurePolicy: Fail + name: mactivation.kb.io + rules: + - apiGroups: + - workflow.symphony + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - activations + sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/k8s/main.go b/k8s/main.go index bfc615a05..206d4253d 100644 --- a/k8s/main.go +++ b/k8s/main.go @@ -294,6 +294,10 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "Skill") os.Exit(1) } + if err = (&workflowv1.Activation{}).SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Skill") + os.Exit(1) + } if err = (&federationv1.Catalog{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Catalog") os.Exit(1) diff --git a/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml b/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml index cb85fcc57..3f38e57b3 100644 --- a/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml +++ b/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml @@ -2745,6 +2745,26 @@ webhooks: resources: - solutions sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: '{{ include "symphony.fullname" . }}-webhook-service' + namespace: '{{ .Release.Namespace }}' + path: /mutate-workflow-symphony-v1-activation + failurePolicy: Fail + name: mactivation.kb.io + rules: + - apiGroups: + - workflow.symphony + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - activations + sideEffects: None - admissionReviewVersions: - v1 clientConfig: @@ -2956,6 +2976,26 @@ webhooks: resources: - solutions sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: '{{ include "symphony.fullname" . }}-webhook-service' + namespace: '{{ .Release.Namespace }}' + path: /validate-workflow-symphony-v1-activation + failurePolicy: Fail + name: mactivation.kb.io + rules: + - apiGroups: + - workflow.symphony + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - activations + sideEffects: None - admissionReviewVersions: - v1 clientConfig: From ddd3d49dbbe710be4cc0f1bad26208862e76d3c9 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Tue, 11 Jun 2024 23:50:56 +0800 Subject: [PATCH 2/9] convert campaign name --- k8s/apis/workflow/v1/activation_webhook.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 2b13baf4f..7cb1cdd2c 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -129,8 +129,9 @@ func (r *Activation) validateCampaignOnCreate() *field.Error { if r.Spec.Campaign == "" { return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "campaign must not be empty") } + campaignName := strings.Replace(r.Spec.Campaign, ":", "-", -1) var campaign Campaign - err := myActivationClient.Get(context.Background(), client.ObjectKey{Name: r.Spec.Campaign, Namespace: r.Namespace}, &campaign) + err := myActivationClient.Get(context.Background(), client.ObjectKey{Name: campaignName, Namespace: r.Namespace}, &campaign) if err != nil { return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "campaign doesn't exist") } From c8bac858e1b314a612dbcdc5d922371cb56472e8 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Wed, 12 Jun 2024 00:08:44 +0800 Subject: [PATCH 3/9] Fix naming typo --- k8s/apis/workflow/v1/activation_webhook.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 7cb1cdd2c..311b14c01 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -26,7 +26,7 @@ import ( // log is for logging in this package. var activationlog = logf.Log.WithName("activation-resource") var myActivationClient client.Client -var catalogWebhookValidationMetrics *metrics.Metrics +var activationWebhookValidationMetrics *metrics.Metrics func (r *Activation) SetupWebhookWithManager(mgr ctrl.Manager) error { myActivationClient = mgr.GetClient() @@ -36,12 +36,12 @@ func (r *Activation) SetupWebhookWithManager(mgr ctrl.Manager) error { }) // initialize the controller operation metrics - if catalogWebhookValidationMetrics == nil { + if activationWebhookValidationMetrics == nil { metrics, err := metrics.New() if err != nil { return err } - catalogWebhookValidationMetrics = metrics + activationWebhookValidationMetrics = metrics } return ctrl.NewWebhookManagedBy(mgr). @@ -73,13 +73,13 @@ func (r *Activation) ValidateCreate() (admission.Warnings, error) { validateCreateTime := time.Now() validationError := r.validateCreateActivation() if validationError != nil { - catalogWebhookValidationMetrics.ControllerValidationLatency( + activationWebhookValidationMetrics.ControllerValidationLatency( validateCreateTime, metrics.CreateOperationType, metrics.InvalidResource, metrics.CatalogResourceType) } else { - catalogWebhookValidationMetrics.ControllerValidationLatency( + activationWebhookValidationMetrics.ControllerValidationLatency( validateCreateTime, metrics.CreateOperationType, metrics.ValidResource, From 6e5318dac9cdfdd149c893fb322d94c12e94091b Mon Sep 17 00:00:00 2001 From: Xingdong Date: Wed, 12 Jun 2024 14:10:22 +0800 Subject: [PATCH 4/9] wekbook block changing activation spec --- .../activations/activations-manager.go | 1 + .../apis/v1alpha1/providers/states/k8s/k8s.go | 14 ++++++- k8s/apis/workflow/v1/activation_webhook.go | 42 ++++++++++++++++++- 3 files changed, 54 insertions(+), 3 deletions(-) diff --git a/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go b/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go index 4a289d99e..58b6be563 100644 --- a/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go +++ b/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go @@ -216,6 +216,7 @@ func (t *ActivationsManager) ReportStatus(ctx context.Context, name string, name } current.UpdateTime = time.Now().Format(time.RFC3339) // TODO: is this correct? Shouldn't it be reported? + activationState.Spec = nil activationState.Status = ¤t entry.Body = activationState diff --git a/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go b/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go index 3500bbcce..da21c31cc 100644 --- a/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go +++ b/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go @@ -197,7 +197,8 @@ func (s *K8sStateProvider) Upsert(ctx context.Context, entry states.UpsertReques } j, _ := json.Marshal(entry.Value.Body) - item, err := s.DynamicClient.Resource(resourceId).Namespace(namespace).Get(ctx, entry.Value.ID, metav1.GetOptions{}) + var item *unstructured.Unstructured + item, err = s.DynamicClient.Resource(resourceId).Namespace(namespace).Get(ctx, entry.Value.ID, metav1.GetOptions{}) if err != nil { template := fmt.Sprintf(`{"apiVersion":"%s/v1", "kind": "%s", "metadata": {}}`, group, kind) var unc *unstructured.Unstructured @@ -252,6 +253,7 @@ func (s *K8sStateProvider) Upsert(ctx context.Context, entry states.UpsertReques item.SetLabels(metadata.Labels) item.SetAnnotations(metadata.Annotations) } + getResourceVersion := false if v, ok := dict["spec"]; ok { item.Object["spec"] = v @@ -260,8 +262,18 @@ func (s *K8sStateProvider) Upsert(ctx context.Context, entry states.UpsertReques sLog.Errorf(" P (K8s State): failed to update object: %v", err) return "", err } + getResourceVersion = true } if v, ok := dict["status"]; ok { + if getResourceVersion { + // Get latest resource version in case the the object spec is also updated + item, err = s.DynamicClient.Resource(resourceId).Namespace(namespace).Get(ctx, entry.Value.ID, metav1.GetOptions{}) + if err != nil { + sLog.Errorf(" P (K8s State): failed to get object when trying to update status: %v", err) + return "", err + } + } + if vMap, ok := v.(map[string]interface{}); ok { status := &unstructured.Unstructured{ Object: map[string]interface{}{ diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 311b14c01..060dbe451 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -8,6 +8,7 @@ package v1 import ( "context" + "fmt" "gopls-workspace/apis/metrics/v1" "strings" "time" @@ -92,8 +93,29 @@ func (r *Activation) ValidateCreate() (admission.Warnings, error) { // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type func (r *Activation) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { activationlog.Info("validate update", "name", r.Name) - - return nil, nil + validateUpdateTime := time.Now() + oldActivation, ok := old.(*Activation) + if !ok { + return nil, fmt.Errorf("expected an Activation object") + } + // Compare the Spec of the current and old Activation objects + validationError := oldActivation.validateSpecOnUpdate(oldActivation) + if validationError != nil { + activationWebhookValidationMetrics.ControllerValidationLatency( + validateUpdateTime, + metrics.UpdateOperationType, + metrics.InvalidResource, + metrics.InstanceResourceType, + ) + } else { + activationWebhookValidationMetrics.ControllerValidationLatency( + validateUpdateTime, + metrics.UpdateOperationType, + metrics.ValidResource, + metrics.InstanceResourceType, + ) + } + return nil, validationError } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type @@ -137,3 +159,19 @@ func (r *Activation) validateCampaignOnCreate() *field.Error { } return nil } + +func (r *Activation) validateSpecOnUpdate(oldActivation *Activation) *field.Error { + if r.Spec.Campaign != oldActivation.Spec.Campaign { + return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "updates to activation spec.Campaign are not allowed") + } + if r.Spec.Stage != oldActivation.Spec.Stage { + return field.Invalid(field.NewPath("spec").Child("stage"), r.Spec.Stage, "updates to activation spec.Stage are not allowed") + } + if r.Spec.Inputs.String() != oldActivation.Spec.Inputs.String() { + return field.Invalid(field.NewPath("spec").Child("inputs"), r.Spec.Inputs, "updates to activation spec.Inputs are not allowed") + } + if r.Spec.Generation != oldActivation.Spec.Generation { + return field.Invalid(field.NewPath("spec").Child("generation"), r.Spec.Generation, "updates to activation spec.Generation are not allowed") + } + return nil +} From 3bdaa8c4b3c3133112fcc3d59f80149cf58c909f Mon Sep 17 00:00:00 2001 From: Xingdong Date: Wed, 12 Jun 2024 20:42:43 +0800 Subject: [PATCH 5/9] Add integration tests --- k8s/apis/workflow/v1/activation_webhook.go | 2 +- .../scenarios/04.workflow/magefile.go | 27 +++++++++++++++++++ .../manifest/activation-campaignnotexist.yaml | 7 +++++ .../manifest/activation-stage.yaml | 8 ++++++ 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 test/integration/scenarios/04.workflow/manifest/activation-campaignnotexist.yaml create mode 100644 test/integration/scenarios/04.workflow/manifest/activation-stage.yaml diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 060dbe451..6b2170dd4 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -99,7 +99,7 @@ func (r *Activation) ValidateUpdate(old runtime.Object) (admission.Warnings, err return nil, fmt.Errorf("expected an Activation object") } // Compare the Spec of the current and old Activation objects - validationError := oldActivation.validateSpecOnUpdate(oldActivation) + validationError := r.validateSpecOnUpdate(oldActivation) if validationError != nil { activationWebhookValidationMetrics.ControllerValidationLatency( validateUpdateTime, diff --git a/test/integration/scenarios/04.workflow/magefile.go b/test/integration/scenarios/04.workflow/magefile.go index e5f9925a7..0b38e0519 100644 --- a/test/integration/scenarios/04.workflow/magefile.go +++ b/test/integration/scenarios/04.workflow/magefile.go @@ -59,6 +59,10 @@ var ( testVerify = []string{ "./verify/...", } + + CampaignNotExistActivation = "test/integration/scenarios/04.workflow/manifest/activation-campaignnotexist.yaml" + + WithStageActivation = "test/integration/scenarios/04.workflow/manifest/activation-stage.yaml" ) // Entry point for running the tests @@ -80,6 +84,10 @@ func Test() error { if err != nil { return err } + err = FaultTest(namespace) + if err != nil { + return err + } } return nil @@ -181,6 +189,25 @@ func Verify() error { return nil } +func FaultTest(namespace string) error { + repoPath := os.Getenv("REPO_PATH") + if repoPath == "" { + repoPath = "../../../../" + } + var err error + CampaignNotExistActivationAbs := filepath.Join(repoPath, CampaignNotExistActivation) + err = shellcmd.Command(fmt.Sprintf("kubectl apply -f %s -n %s", CampaignNotExistActivationAbs, namespace)).Run() + if err == nil { + return fmt.Errorf("fault test failed for non-existing campaign") + } + WithStageActivationAbs := filepath.Join(repoPath, WithStageActivation) + err = shellcmd.Command(fmt.Sprintf("kubectl apply -f %s -n %s", WithStageActivationAbs, namespace)).Run() + if err == nil { + return fmt.Errorf("fault test failed for non-existing campaign") + } + return nil +} + // Clean up func Cleanup() { localenvCmd(fmt.Sprintf("dumpSymphonyLogsForTest '%s'", TEST_NAME), "") diff --git a/test/integration/scenarios/04.workflow/manifest/activation-campaignnotexist.yaml b/test/integration/scenarios/04.workflow/manifest/activation-campaignnotexist.yaml new file mode 100644 index 000000000..c0219095b --- /dev/null +++ b/test/integration/scenarios/04.workflow/manifest/activation-campaignnotexist.yaml @@ -0,0 +1,7 @@ +apiVersion: workflow.symphony/v1 +kind: Activation +metadata: + name: 04workflow2 +spec: + campaign: "04campaign:v2" + \ No newline at end of file diff --git a/test/integration/scenarios/04.workflow/manifest/activation-stage.yaml b/test/integration/scenarios/04.workflow/manifest/activation-stage.yaml new file mode 100644 index 000000000..7afe89c36 --- /dev/null +++ b/test/integration/scenarios/04.workflow/manifest/activation-stage.yaml @@ -0,0 +1,8 @@ +apiVersion: workflow.symphony/v1 +kind: Activation +metadata: + name: 04workflow +spec: + campaign: "04campaign:v1" + stage: "deploy" + \ No newline at end of file From 27eee2527f4dc908d52cb103ffd8af23228ee6c1 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Wed, 12 Jun 2024 20:44:35 +0800 Subject: [PATCH 6/9] resolve conflict --- k8s/config/oss/webhook/manifests.yaml | 34 +++++++++---------- .../templates/symphony-core/symphonyk8s.yaml | 34 +++++++++---------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/k8s/config/oss/webhook/manifests.yaml b/k8s/config/oss/webhook/manifests.yaml index 6de9bf1c3..a356ea748 100644 --- a/k8s/config/oss/webhook/manifests.yaml +++ b/k8s/config/oss/webhook/manifests.yaml @@ -110,19 +110,19 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-workflow-symphony-v1-activation + path: /mutate-solution-symphony-v1-solutioncontainer failurePolicy: Fail - name: mactivation.kb.io + name: msolutioncontainer.kb.io rules: - apiGroups: - - workflow.symphony + - solution.symphony apiVersions: - v1 operations: - CREATE - UPDATE resources: - - activations + - solutioncontainers sideEffects: None - admissionReviewVersions: - v1 @@ -130,19 +130,19 @@ webhooks: service: name: webhook-service namespace: system - path: /mutate-solution-symphony-v1-solutioncontainer + path: /mutate-workflow-symphony-v1-activation failurePolicy: Fail - name: msolutioncontainer.kb.io + name: mactivation.kb.io rules: - apiGroups: - - solution.symphony + - workflow.symphony apiVersions: - v1 operations: - CREATE - UPDATE resources: - - solutioncontainers + - activations sideEffects: None - admissionReviewVersions: - v1 @@ -338,19 +338,20 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-workflow-symphony-v1-activation + path: /validate-solution-symphony-v1-solutioncontainer failurePolicy: Fail - name: mactivation.kb.io + name: vsolutioncontainer.kb.io rules: - apiGroups: - - workflow.symphony + - solution.symphony apiVersions: - v1 operations: - CREATE - UPDATE + - DELETE resources: - - activations + - solutioncontainers sideEffects: None - admissionReviewVersions: - v1 @@ -358,20 +359,19 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-solution-symphony-v1-solutioncontainer + path: /validate-workflow-symphony-v1-activation failurePolicy: Fail - name: vsolutioncontainer.kb.io + name: mactivation.kb.io rules: - apiGroups: - - solution.symphony + - workflow.symphony apiVersions: - v1 operations: - CREATE - UPDATE - - DELETE resources: - - solutioncontainers + - activations sideEffects: None - admissionReviewVersions: - v1 diff --git a/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml b/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml index 3f38e57b3..42004edd4 100644 --- a/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml +++ b/packages/helm/symphony/templates/symphony-core/symphonyk8s.yaml @@ -2751,19 +2751,19 @@ webhooks: service: name: '{{ include "symphony.fullname" . }}-webhook-service' namespace: '{{ .Release.Namespace }}' - path: /mutate-workflow-symphony-v1-activation + path: /mutate-solution-symphony-v1-solutioncontainer failurePolicy: Fail - name: mactivation.kb.io + name: msolutioncontainer.kb.io rules: - apiGroups: - - workflow.symphony + - solution.symphony apiVersions: - v1 operations: - CREATE - UPDATE resources: - - activations + - solutioncontainers sideEffects: None - admissionReviewVersions: - v1 @@ -2771,19 +2771,19 @@ webhooks: service: name: '{{ include "symphony.fullname" . }}-webhook-service' namespace: '{{ .Release.Namespace }}' - path: /mutate-solution-symphony-v1-solutioncontainer + path: /mutate-workflow-symphony-v1-activation failurePolicy: Fail - name: msolutioncontainer.kb.io + name: mactivation.kb.io rules: - apiGroups: - - solution.symphony + - workflow.symphony apiVersions: - v1 operations: - CREATE - UPDATE resources: - - solutioncontainers + - activations sideEffects: None - admissionReviewVersions: - v1 @@ -2982,19 +2982,20 @@ webhooks: service: name: '{{ include "symphony.fullname" . }}-webhook-service' namespace: '{{ .Release.Namespace }}' - path: /validate-workflow-symphony-v1-activation + path: /validate-solution-symphony-v1-solutioncontainer failurePolicy: Fail - name: mactivation.kb.io + name: vsolutioncontainer.kb.io rules: - apiGroups: - - workflow.symphony + - solution.symphony apiVersions: - v1 operations: - CREATE - UPDATE + - DELETE resources: - - activations + - solutioncontainers sideEffects: None - admissionReviewVersions: - v1 @@ -3002,20 +3003,19 @@ webhooks: service: name: '{{ include "symphony.fullname" . }}-webhook-service' namespace: '{{ .Release.Namespace }}' - path: /validate-solution-symphony-v1-solutioncontainer + path: /validate-workflow-symphony-v1-activation failurePolicy: Fail - name: vsolutioncontainer.kb.io + name: mactivation.kb.io rules: - apiGroups: - - solution.symphony + - workflow.symphony apiVersions: - v1 operations: - CREATE - UPDATE - - DELETE resources: - - solutioncontainers + - activations sideEffects: None - admissionReviewVersions: - v1 From 1c627832340a03d6c954601949634befb706b085 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Fri, 14 Jun 2024 10:51:00 +0800 Subject: [PATCH 7/9] Keep activationState.Spec to work for other state provider --- .../apis/v1alpha1/managers/activations/activations-manager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go b/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go index 58b6be563..4a289d99e 100644 --- a/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go +++ b/api/pkg/apis/v1alpha1/managers/activations/activations-manager.go @@ -216,7 +216,6 @@ func (t *ActivationsManager) ReportStatus(ctx context.Context, name string, name } current.UpdateTime = time.Now().Format(time.RFC3339) // TODO: is this correct? Shouldn't it be reported? - activationState.Spec = nil activationState.Status = ¤t entry.Body = activationState From 4c4c62de67d5a528b368a4b0d108b293306ad1a5 Mon Sep 17 00:00:00 2001 From: Xingdong Date: Fri, 14 Jun 2024 17:24:51 +0800 Subject: [PATCH 8/9] Fix validateUpdate error type --- .../apis/v1alpha1/providers/states/k8s/k8s.go | 14 ++++++-------- k8s/apis/workflow/v1/activation_webhook.go | 17 +++++++++++------ 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go b/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go index da21c31cc..cff8c9697 100644 --- a/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go +++ b/api/pkg/apis/v1alpha1/providers/states/k8s/k8s.go @@ -173,8 +173,6 @@ func (s *K8sStateProvider) Upsert(ctx context.Context, entry states.UpsertReques var err error = nil defer observ_utils.CloseSpanWithError(span, &err) - sLog.Info(" P (K8s State): upsert state") - namespace := model.ReadPropertyCompat(entry.Metadata, "namespace", nil) group := model.ReadPropertyCompat(entry.Metadata, "group", nil) version := model.ReadPropertyCompat(entry.Metadata, "version", nil) @@ -184,6 +182,7 @@ func (s *K8sStateProvider) Upsert(ctx context.Context, entry states.UpsertReques if namespace == "" { namespace = "default" } + sLog.Info(" P (K8s State): upsert state %s in namespace %s, traceId: %s", entry.Value.ID, namespace, span.SpanContext().TraceID().String()) resourceId := schema.GroupVersionResource{ Group: group, @@ -322,13 +321,13 @@ func (s *K8sStateProvider) List(ctx context.Context, request states.ListRequest) var err error = nil defer observ_utils.CloseSpanWithError(span, &err) - sLog.Info(" P (K8s State): list state") - namespace := model.ReadPropertyCompat(request.Metadata, "namespace", nil) group := model.ReadPropertyCompat(request.Metadata, "group", nil) version := model.ReadPropertyCompat(request.Metadata, "version", nil) resource := model.ReadPropertyCompat(request.Metadata, "resource", nil) + sLog.Infof(" P (K8s State): list state for %s.%s in namespace %s, traceId: %s", resource, group, namespace, span.SpanContext().TraceID().String()) + var namespaces []string if namespace == "" { ret, err := s.ListAllNamespaces(ctx, version) @@ -434,8 +433,6 @@ func (s *K8sStateProvider) Delete(ctx context.Context, request states.DeleteRequ var err error = nil defer observ_utils.CloseSpanWithError(span, &err) - sLog.Info(" P (K8s State): delete state") - namespace := model.ReadPropertyCompat(request.Metadata, "namespace", nil) group := model.ReadPropertyCompat(request.Metadata, "group", nil) version := model.ReadPropertyCompat(request.Metadata, "version", nil) @@ -449,6 +446,7 @@ func (s *K8sStateProvider) Delete(ctx context.Context, request states.DeleteRequ if namespace == "" { namespace = "default" } + sLog.Infof(" P (K8s State): delete state %s in namespace %s, traceId: %s", request.ID, namespace, span.SpanContext().TraceID().String()) if request.ID == "" { err := v1alpha2.NewCOAError(nil, "found invalid request ID", v1alpha2.BadRequest) @@ -470,8 +468,6 @@ func (s *K8sStateProvider) Get(ctx context.Context, request states.GetRequest) ( var err error = nil defer observ_utils.CloseSpanWithError(span, &err) - sLog.Info(" P (K8s State): get state") - namespace := model.ReadPropertyCompat(request.Metadata, "namespace", nil) group := model.ReadPropertyCompat(request.Metadata, "group", nil) version := model.ReadPropertyCompat(request.Metadata, "version", nil) @@ -481,6 +477,8 @@ func (s *K8sStateProvider) Get(ctx context.Context, request states.GetRequest) ( namespace = "default" } + sLog.Infof(" P (K8s State): get state %s in namespace %s, traceId: %s", request.ID, namespace, span.SpanContext().TraceID().String()) + resourceId := schema.GroupVersionResource{ Group: group, Version: version, diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 6b2170dd4..03124beea 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -160,18 +160,23 @@ func (r *Activation) validateCampaignOnCreate() *field.Error { return nil } -func (r *Activation) validateSpecOnUpdate(oldActivation *Activation) *field.Error { +func (r *Activation) validateSpecOnUpdate(oldActivation *Activation) error { + var allErrs field.ErrorList if r.Spec.Campaign != oldActivation.Spec.Campaign { - return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "updates to activation spec.Campaign are not allowed") + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "updates to activation spec.Campaign are not allowed")) } if r.Spec.Stage != oldActivation.Spec.Stage { - return field.Invalid(field.NewPath("spec").Child("stage"), r.Spec.Stage, "updates to activation spec.Stage are not allowed") + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("stage"), r.Spec.Stage, "updates to activation spec.Stage are not allowed")) } if r.Spec.Inputs.String() != oldActivation.Spec.Inputs.String() { - return field.Invalid(field.NewPath("spec").Child("inputs"), r.Spec.Inputs, "updates to activation spec.Inputs are not allowed") + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("inputs"), r.Spec.Inputs, "updates to activation spec.Inputs are not allowed")) } if r.Spec.Generation != oldActivation.Spec.Generation { - return field.Invalid(field.NewPath("spec").Child("generation"), r.Spec.Generation, "updates to activation spec.Generation are not allowed") + allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("generation"), r.Spec.Generation, "updates to activation spec.Generation are not allowed")) } - return nil + if len(allErrs) == 0 { + return nil + } + + return apierrors.NewInvalid(schema.GroupKind{Group: "workflow.symphony", Kind: "Activation"}, r.Name, allErrs) } From d95661c013395f55b1aae6fb875fd0796244e41b Mon Sep 17 00:00:00 2001 From: Xingdong Date: Fri, 14 Jun 2024 17:27:43 +0800 Subject: [PATCH 9/9] Use utils helper function --- k8s/apis/workflow/v1/activation_webhook.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/k8s/apis/workflow/v1/activation_webhook.go b/k8s/apis/workflow/v1/activation_webhook.go index 03124beea..14242646d 100644 --- a/k8s/apis/workflow/v1/activation_webhook.go +++ b/k8s/apis/workflow/v1/activation_webhook.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/eclipse-symphony/symphony/k8s/utils" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -151,7 +152,7 @@ func (r *Activation) validateCampaignOnCreate() *field.Error { if r.Spec.Campaign == "" { return field.Invalid(field.NewPath("spec").Child("campaign"), r.Spec.Campaign, "campaign must not be empty") } - campaignName := strings.Replace(r.Spec.Campaign, ":", "-", -1) + campaignName := utils.ReplaceLastSeperator(r.Spec.Campaign, ":", "-") var campaign Campaign err := myActivationClient.Get(context.Background(), client.ObjectKey{Name: campaignName, Namespace: r.Namespace}, &campaign) if err != nil {