From 726b566f83223de15c49db7edac59e7a4957d4da Mon Sep 17 00:00:00 2001 From: Zbynek Roubalik Date: Tue, 21 Feb 2023 16:24:26 +0100 Subject: [PATCH] fix: deploy annotations & labels correctly (#1574) * fix: deploy annotations & labels correctly Signed-off-by: Zbynek Roubalik * increase timeout Signed-off-by: Zbynek Roubalik --------- Signed-off-by: Zbynek Roubalik --- Makefile | 2 +- pkg/functions/client_int_test.go | 113 ++++++++++++++++++++++++++++++- pkg/knative/deployer.go | 68 ++++++++++++------- 3 files changed, 155 insertions(+), 28 deletions(-) diff --git a/Makefile b/Makefile index 569173cb..d8287d56 100644 --- a/Makefile +++ b/Makefile @@ -142,7 +142,7 @@ test-typescript: ## Test Typescript templates ################### test-integration: ## Run integration tests using an available cluster. - go test -tags integration --coverprofile=coverage.txt ./... -v + go test -tags integration -timeout 30m --coverprofile=coverage.txt ./... -v test-e2e: ## Run end-to-end tests using an available cluster. ./test/e2e_lifecycle_tests.sh node diff --git a/pkg/functions/client_int_test.go b/pkg/functions/client_int_test.go index 39503e39..1260a584 100644 --- a/pkg/functions/client_int_test.go +++ b/pkg/functions/client_int_test.go @@ -22,7 +22,7 @@ import ( "knative.dev/pkg/ptr" ) -// # Integration Tsets +// # Integration Tests // // go test -tags integration ./... // @@ -159,6 +159,115 @@ func TestDeployWithOptions(t *testing.T) { } } +func TestUpdateWithAnnotationsAndLabels(t *testing.T) { + functionName := "updateannlab" + defer Within(t, "testdata/example.com/"+functionName)() + verbose := true + + servingClient, err := knative.NewServingClient(DefaultNamespace) + + // Deploy a function without any annotations or labels + client := newClient(verbose) + + if _, err := client.New(context.Background(), fn.Function{Name: functionName, Root: ".", Runtime: "go"}); err != nil { + t.Fatal(err) + } + defer del(t, client, functionName) + + if err := client.Deploy(context.Background(), "."); err != nil { + t.Fatal(err) + } + + // Update function with a new set of annotations and labels + // deploy and check that deployed kcsv contains correct annotations and labels + f, err := fn.NewFunction(".") + if err != nil { + t.Fatal(err) + } + + annotations := map[string]string{"ann1": "val1", "ann2": "val2"} + labels := []fn.Label{ + {Key: ptr.String("lab1"), Value: ptr.String("v1")}, + {Key: ptr.String("lab2"), Value: ptr.String("v2")}, + } + f.Deploy.Annotations = annotations + f.Deploy.Labels = labels + if err := f.Write(); err != nil { + t.Fatal(err) + } + + if err := client.Deploy(context.Background(), ".", fn.WithDeploySkipBuildCheck(true)); err != nil { + t.Fatal(err) + } + + ksvc, err := servingClient.GetService(context.Background(), functionName) + if err != nil { + t.Fatal(err) + } + + for k, v := range annotations { + if val, ok := ksvc.Annotations[k]; ok { + if v != val { + t.Fatal(fmt.Errorf("expected annotation %q to have value %q, but the annotation in the deployed service has value %q", k, v, val)) + } + } else { + t.Fatal(fmt.Errorf("annotation %q not found in the deployed service", k)) + } + } + for _, l := range labels { + if val, ok := ksvc.Labels[*l.Key]; ok { + if *l.Value != val { + t.Fatal(fmt.Errorf("expected label %q to have value %q, but the label in the deployed service has value %q", *l.Key, *l.Value, val)) + } + } else { + t.Fatal(fmt.Errorf("label %q not found in the deployed service", *l.Key)) + } + } + + // Remove some annotations and labels + // deploy and check that deployed kcsv contains correct annotations and labels + f, err = fn.NewFunction(".") + if err != nil { + t.Fatal(err) + } + + annotations = map[string]string{"ann1": "val1"} + labels = []fn.Label{{Key: ptr.String("lab1"), Value: ptr.String("v1")}} + f.Deploy.Annotations = annotations + f.Deploy.Labels = labels + if err := f.Write(); err != nil { + t.Fatal(err) + } + + if err := client.Deploy(context.Background(), ".", fn.WithDeploySkipBuildCheck(true)); err != nil { + t.Fatal(err) + } + + ksvc, err = servingClient.GetService(context.Background(), functionName) + if err != nil { + t.Fatal(err) + } + + for k, v := range annotations { + if val, ok := ksvc.Annotations[k]; ok { + if v != val { + t.Fatal(fmt.Errorf("expected annotation %q to have value %q, but the annotation in the deployed service has value %q", k, v, val)) + } + } else { + t.Fatal(fmt.Errorf("annotation %q not found in the deployed service", k)) + } + } + for _, l := range labels { + if val, ok := ksvc.Labels[*l.Key]; ok { + if *l.Value != val { + t.Fatal(fmt.Errorf("expected label %q to have value %q, but the label in the deployed service has value %q", *l.Key, *l.Value, val)) + } + } else { + t.Fatal(fmt.Errorf("label %q not found in the deployed service", *l.Key)) + } + } +} + // TestRemove deletes func TestRemove(t *testing.T) { defer Within(t, "testdata/example.com/remove")() @@ -435,7 +544,7 @@ func del(t *testing.T, c *fn.Client, name string) { // waitFor the named function to become available in List output. // TODO: the API should be synchronous, but that depends first on // Create returning the derived name such that we can bake polling in. -// Ideally the Boson provider's Creaet would be made syncrhonous. +// Ideally the provider's Create would be made syncrhonous. func waitFor(t *testing.T, c *fn.Client, name string) { t.Helper() var pollInterval = 2 * time.Second diff --git a/pkg/knative/deployer.go b/pkg/knative/deployer.go index 15eae24f..83f618bc 100644 --- a/pkg/knative/deployer.go +++ b/pkg/knative/deployer.go @@ -140,7 +140,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu _ = GetKServiceLogs(ctx, d.Namespace, f.Name, f.ImageWithDigest(), &since, out) }() - _, err = client.GetService(ctx, f.Name) + previousService, err := client.GetService(ctx, f.Name) if err != nil { if errors.IsNotFound(err) { @@ -255,7 +255,7 @@ func (d *Deployer) Deploy(ctx context.Context, f fn.Function) (fn.DeploymentResu return fn.DeploymentResult{}, err } - _, err = client.UpdateServiceWithRetry(ctx, f.Name, updateService(f, newEnv, newEnvFrom, newVolumes, newVolumeMounts, d.decorator), 3) + _, err = client.UpdateServiceWithRetry(ctx, f.Name, updateService(f, previousService, newEnv, newEnvFrom, newVolumes, newVolumeMounts, d.decorator), 3) if err != nil { err = fmt.Errorf("knative deployer failed to update the Knative Service: %v", err) return fn.DeploymentResult{}, err @@ -334,15 +334,12 @@ func generateNewService(f fn.Function, decorator DeployDecorator) (*v1.Service, } container.VolumeMounts = newVolumeMounts - labels, err := f.LabelsMap() + labels, err := generateServiceLabels(f, decorator) if err != nil { return nil, err } - if decorator != nil { - labels = decorator.UpdateLabels(f, labels) - } - annotations := newServiceAnnotations(f, decorator) + annotations := generateServiceAnnotations(f, decorator, nil) // we need to create a separate map for Annotations specified in a Revision, // in case we will need to specify autoscaling annotations -> these could be only in a Revision not in a Service @@ -385,10 +382,28 @@ func generateNewService(f fn.Function, decorator DeployDecorator) (*v1.Service, return service, nil } -// newServiceAnnotations creates a final map of service annotations based +// generateServiceLabels creates a final map of service labels based +// on the function's defined labels plus the +// application of any provided label decorator. +func generateServiceLabels(f fn.Function, d DeployDecorator) (ll map[string]string, err error) { + ll, err = f.LabelsMap() + if err != nil { + return + } + + if d != nil { + ll = d.UpdateLabels(f, ll) + } + + return +} + +// generateServiceAnnotations creates a final map of service annotations based // on static defaults plus the function's defined annotations plus the // application of any provided annotation decorator. -func newServiceAnnotations(f fn.Function, d DeployDecorator) (aa map[string]string) { +// Also sets `serving.knative.dev/creator` to a value specified in annotations in the service reference in the previousService parameter, +// this is beneficial when we are updating a service to pass validation on Knative side - the annotation is immutable. +func generateServiceAnnotations(f fn.Function, d DeployDecorator, previousService *v1.Service) (aa map[string]string) { aa = make(map[string]string) // Enables Dapr support. @@ -407,6 +422,14 @@ func newServiceAnnotations(f fn.Function, d DeployDecorator) (aa map[string]stri aa = d.UpdateAnnotations(f, aa) } + // Set correct creator if we are updating a function + if previousService != nil { + knativeCreatorAnnotation := "serving.knative.dev/creator" + if val, ok := previousService.Annotations[knativeCreatorAnnotation]; ok { + aa[knativeCreatorAnnotation] = val + } + } + return } @@ -423,26 +446,24 @@ func daprAnnotations(appid string) map[string]string { return aa } -func updateService(f fn.Function, newEnv []corev1.EnvVar, newEnvFrom []corev1.EnvFromSource, newVolumes []corev1.Volume, newVolumeMounts []corev1.VolumeMount, decorator DeployDecorator) func(service *v1.Service) (*v1.Service, error) { +func updateService(f fn.Function, previousService *v1.Service, newEnv []corev1.EnvVar, newEnvFrom []corev1.EnvFromSource, newVolumes []corev1.Volume, newVolumeMounts []corev1.VolumeMount, decorator DeployDecorator) func(service *v1.Service) (*v1.Service, error) { return func(service *v1.Service) (*v1.Service, error) { // Removing the name so the k8s server can fill it in with generated name, // this prevents conflicts in Revision name when updating the KService from multiple places. service.Spec.Template.Name = "" - if service.Spec.Template.ObjectMeta.Annotations == nil { - service.Spec.Template.ObjectMeta.Annotations = make(map[string]string) + annotations := generateServiceAnnotations(f, decorator, previousService) + + // we need to create a separate map for Annotations specified in a Revision, + // in case we will need to specify autoscaling annotations -> these could be only in a Revision not in a Service + revisionAnnotations := make(map[string]string) + for k, v := range annotations { + revisionAnnotations[k] = v } - // Don't bother being as clever as we are with env variables - // Just set the annotations and labels to be whatever we find in func.yaml - if decorator != nil { - service.ObjectMeta.Annotations = decorator.UpdateAnnotations(f, service.ObjectMeta.Annotations) - } + service.ObjectMeta.Annotations = annotations + service.Spec.Template.ObjectMeta.Annotations = revisionAnnotations - for k, v := range f.Deploy.Annotations { - service.ObjectMeta.Annotations[k] = v - service.Spec.Template.ObjectMeta.Annotations[k] = v - } // I hate that we have to do this. Users should not see these values. // It is an implementation detail. These health endpoints should not be // a part of func.yaml since the user can only mess things up by changing @@ -463,13 +484,10 @@ func updateService(f fn.Function, newEnv []corev1.EnvVar, newEnvFrom []corev1.En return service, err } - labels, err := f.LabelsMap() + labels, err := generateServiceLabels(f, decorator) if err != nil { return nil, err } - if decorator != nil { - labels = decorator.UpdateLabels(f, labels) - } service.ObjectMeta.Labels = labels service.Spec.Template.ObjectMeta.Labels = labels