From b6a8fa9213e40c1059d39928ca09a016dffc2eec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roland=20Hu=C3=9F?= Date: Mon, 3 Jun 2019 22:30:35 +0200 Subject: [PATCH] fix(service update): Only update fields which have been sent by server. (#155) * fix(service update): Only update fields which have been sent by server. This reflects the lemonade process step1. Tests have been adapted to verify this behaviours. The only situation when we update field coming from the server is for "kn service update" for envs, image and requests/limits. All other operation are either create (here, we always have to send the old fields), or read (get/describe). Fixes #144. * chore: typo fix * refactor(service update/create): Moved from Configuration to RevisionTemplateSpec In order to proper handling the v1alpha1 -> v1beta1 migration methods has been updated to get rid fo Configuration within the service as this is completely inlined in v1beta1. The helper methods have been also updated accordingly. I think we are good now. --- pkg/kn/commands/configuration_edit_flags.go | 16 ++- pkg/kn/commands/service_create.go | 7 +- pkg/kn/commands/service_create_test.go | 74 ++++++------- pkg/kn/commands/service_update.go | 8 +- pkg/kn/commands/service_update_test.go | 59 +++++----- pkg/serving/config_changes.go | 99 ++++++++++++----- pkg/serving/config_changes_test.go | 116 +++++++++++++++----- pkg/serving/service.go | 22 +++- 8 files changed, 261 insertions(+), 140 deletions(-) diff --git a/pkg/kn/commands/configuration_edit_flags.go b/pkg/kn/commands/configuration_edit_flags.go index 8f24e158f..184e04684 100644 --- a/pkg/kn/commands/configuration_edit_flags.go +++ b/pkg/kn/commands/configuration_edit_flags.go @@ -54,7 +54,13 @@ func (p *ConfigurationEditFlags) AddCreateFlags(command *cobra.Command) { command.MarkFlagRequired("image") } -func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec, cmd *cobra.Command) error { +func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *cobra.Command) error { + + template, err := servinglib.GetRevisionTemplate(service) + if err != nil { + return err + } + envMap := map[string]string{} for _, pairStr := range p.Env { pairSlice := strings.SplitN(pairStr, "=", 2) @@ -65,12 +71,12 @@ func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec } envMap[pairSlice[0]] = pairSlice[1] } - err := servinglib.UpdateEnvVars(config, envMap) - if err != nil { + if err := servinglib.UpdateEnvVars(template, envMap); err != nil { return err } + if cmd.Flags().Changed("image") { - err = servinglib.UpdateImage(config, p.Image) + err = servinglib.UpdateImage(template, p.Image) if err != nil { return err } @@ -83,7 +89,7 @@ func (p *ConfigurationEditFlags) Apply(config *servingv1alpha1.ConfigurationSpec if err != nil { return err } - err = servinglib.UpdateResources(config, requestsResources, limitsResources) + err = servinglib.UpdateResources(template, requestsResources, limitsResources) if err != nil { return err } diff --git a/pkg/kn/commands/service_create.go b/pkg/kn/commands/service_create.go index 50a81bb39..dcda99095 100644 --- a/pkg/kn/commands/service_create.go +++ b/pkg/kn/commands/service_create.go @@ -18,7 +18,6 @@ import ( "errors" "fmt" - serving_lib "github.com/knative/client/pkg/serving" servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/spf13/cobra" corev1 "k8s.io/api/core/v1" @@ -81,11 +80,7 @@ func NewServiceCreateCommand(p *KnParams) *cobra.Command { }, } - config, err := serving_lib.GetConfiguration(&service) - if err != nil { - return err - } - err = editFlags.Apply(config, cmd) + err = editFlags.Apply(&service, cmd) if err != nil { return err } diff --git a/pkg/kn/commands/service_create_test.go b/pkg/kn/commands/service_create_test.go index c08e1a7ac..7cfc014dd 100644 --- a/pkg/kn/commands/service_create_test.go +++ b/pkg/kn/commands/service_create_test.go @@ -74,11 +74,11 @@ func TestServiceCreateImage(t *testing.T) { } else if !action.Matches("create", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "created") || !strings.Contains(output, "default") { t.Fatalf("wrong stdout message: %v", output) @@ -99,20 +99,20 @@ func TestServiceCreateEnv(t *testing.T) { "A": "DOGS", "B": "WOLVES"} - conf, err := servinglib.GetConfiguration(created) - actualEnvVars, err := servinglib.EnvToMap(conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + template, err := servinglib.GetRevisionTemplate(created) + actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env) if err != nil { t.Fatal(err) } if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !reflect.DeepEqual( actualEnvVars, expectedEnvVars) { - t.Fatalf("wrong env vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + t.Fatalf("wrong env vars %v", template.Spec.DeprecatedContainer.Env) } } @@ -131,14 +131,14 @@ func TestServiceCreateWithRequests(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "64Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } } @@ -157,14 +157,14 @@ func TestServiceCreateWithLimits(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } @@ -186,21 +186,21 @@ func TestServiceCreateRequestsLimitsCPU(t *testing.T) { corev1.ResourceCPU: parseQuantity(t, "1000m"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -223,21 +223,21 @@ func TestServiceCreateRequestsLimitsMemory(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -264,21 +264,21 @@ func TestServiceCreateRequestsLimitsCPUMemory(t *testing.T) { corev1.ResourceMemory: parseQuantity(t, "1024Mi"), } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + template.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", template.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + template.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", template.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -304,11 +304,11 @@ func TestServiceCreateImageForce(t *testing.T) { } else if !action.Matches("create", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(created) + template, err := servinglib.GetRevisionTemplate(created) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "default") { t.Fatalf("wrong output: %s", output) } @@ -333,19 +333,19 @@ func TestServiceCreateEnvForce(t *testing.T) { "A": "CATS", "B": "LIONS"} - conf, err := servinglib.GetConfiguration(created) - actualEnvVars, err := servinglib.EnvToMap(conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + template, err := servinglib.GetRevisionTemplate(created) + actualEnvVars, err := servinglib.EnvToMap(template.Spec.DeprecatedContainer.Env) if err != nil { t.Fatal(err) } if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:v2" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } else if !reflect.DeepEqual( actualEnvVars, expectedEnvVars) { - t.Fatalf("wrong env vars:%v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + t.Fatalf("wrong env vars:%v", template.Spec.DeprecatedContainer.Env) } else if !strings.Contains(output, "foo") || !strings.Contains(output, "default") { t.Fatalf("wrong output: %s", output) } diff --git a/pkg/kn/commands/service_update.go b/pkg/kn/commands/service_update.go index 0d80e5b9a..1fad551b8 100644 --- a/pkg/kn/commands/service_update.go +++ b/pkg/kn/commands/service_update.go @@ -17,7 +17,6 @@ package commands import ( "errors" - serving_lib "github.com/knative/client/pkg/serving" "github.com/spf13/cobra" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -53,12 +52,9 @@ func NewServiceUpdateCommand(p *KnParams) *cobra.Command { if err != nil { return err } + service = service.DeepCopy() - config, err := serving_lib.GetConfiguration(service) - if err != nil { - return err - } - err = editFlags.Apply(config, cmd) + err = editFlags.Apply(service, cmd) if err != nil { return err } diff --git a/pkg/kn/commands/service_update_test.go b/pkg/kn/commands/service_update_test.go index 6e3869fc0..6bbe450ed 100644 --- a/pkg/kn/commands/service_update_test.go +++ b/pkg/kn/commands/service_update_test.go @@ -93,12 +93,12 @@ func TestServiceUpdateImage(t *testing.T) { }, } - config, err := servinglib.GetConfiguration(orig) + template, err := servinglib.GetRevisionTemplate(orig) if err != nil { t.Fatal(err) } - servinglib.UpdateImage(config, "gcr.io/foo/bar:baz") + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ "service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy"}) @@ -108,11 +108,12 @@ func TestServiceUpdateImage(t *testing.T) { } else if !action.Matches("update", "services") { t.Fatalf("Bad action %v", action) } - conf, err := servinglib.GetConfiguration(updated) + + template, err = servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/quux:xyzzy" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) } } @@ -139,12 +140,12 @@ func TestServiceUpdateEnv(t *testing.T) { }, } - config, err := servinglib.GetConfiguration(orig) + template, err := servinglib.GetRevisionTemplate(orig) if err != nil { t.Fatal(err) } - servinglib.UpdateImage(config, "gcr.io/foo/bar:baz") + servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ "service", "update", "foo", "-e", "TARGET=Awesome"}) @@ -159,13 +160,13 @@ func TestServiceUpdateEnv(t *testing.T) { Value: "Awesome", } - conf, err := servinglib.GetConfiguration(updated) + template, err = servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { - t.Fatalf("wrong image set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image) - } else if conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env[0] != expectedEnvVar { - t.Fatalf("wrong env set: %v", conf.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + } else if template.Spec.DeprecatedContainer.Image != "gcr.io/foo/bar:baz" { + t.Fatalf("wrong image set: %v", template.Spec.DeprecatedContainer.Image) + } else if template.Spec.DeprecatedContainer.Env[0] != expectedEnvVar { + t.Fatalf("wrong env set: %v", template.Spec.DeprecatedContainer.Env) } } @@ -189,20 +190,20 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) { corev1.ResourceMemory: resource.MustParse("1024Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -227,20 +228,20 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) { corev1.ResourceMemory: resource.MustParse("2048Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -267,20 +268,20 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) { corev1.ResourceMemory: resource.MustParse("2048Mi"), } - newConfig, err := servinglib.GetConfiguration(updated) + newTemplate, err := servinglib.GetRevisionTemplate(updated) if err != nil { t.Fatal(err) } else { if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests, + newTemplate.Spec.DeprecatedContainer.Resources.Requests, expectedRequestsVars) { - t.Fatalf("wrong requests vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests) + t.Fatalf("wrong requests vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Requests) } if !reflect.DeepEqual( - newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits, + newTemplate.Spec.DeprecatedContainer.Resources.Limits, expectedLimitsVars) { - t.Fatalf("wrong limits vars %v", newConfig.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits) + t.Fatalf("wrong limits vars %v", newTemplate.Spec.DeprecatedContainer.Resources.Limits) } } } @@ -308,12 +309,12 @@ func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, lim }, } - config, err := servinglib.GetConfiguration(service) + template, err := servinglib.GetRevisionTemplate(service) if err != nil { t.Fatal(err) } - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources = corev1.ResourceRequirements{ + template.Spec.DeprecatedContainer.Resources = corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse(requestCPU), corev1.ResourceMemory: resource.MustParse(requestMemory), diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 56430cc79..f9ee71bb3 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -24,28 +24,13 @@ import ( // Give the configuration all the env var values listed in the given map of // vars. Does not touch any environment variables not mentioned, but it can add // new env vars and change the values of existing ones. -func UpdateEnvVars(config *servingv1alpha1.ConfigurationSpec, vars map[string]string) error { - set := make(map[string]bool) - for i, _ := range config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env { - envVar := &config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env[i] - value, present := vars[envVar.Name] - if present { - envVar.Value = value - set[envVar.Name] = true - } - } - for name, value := range vars { - if !set[name] { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = append( - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env, - corev1.EnvVar{ - Name: name, - Value: value, - }) - } +func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error { + container, err := extractContainer(template) + if err != nil { + return err } + container.Env = updateEnvVarsFromMap(container.Env, vars) return nil - } // Utility function to translate between the API list form of env vars, and the @@ -55,34 +40,88 @@ func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { for _, envVar := range vars { _, present := result[envVar.Name] if present { - return nil, fmt.Errorf("Env var name present more than once: %v", envVar.Name) + return nil, fmt.Errorf("env var name present more than once: %v", envVar.Name) } result[envVar.Name] = envVar.Value } return result, nil } -func UpdateImage(config *servingv1alpha1.ConfigurationSpec, image string) error { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Image = image +// Update a given image +func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) error { + container, err := extractContainer(template) + if err != nil { + return err + } + container.Image = image return nil } -func UpdateResources(config *servingv1alpha1.ConfigurationSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { - if config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests == nil { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests = corev1.ResourceList{} +func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { + container, err := extractContainer(template) + if err != nil { + return err + } + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} } for k, v := range requestsResourceList { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Requests[k] = v + container.Resources.Requests[k] = v } - if config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits == nil { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits = corev1.ResourceList{} + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} } for k, v := range limitsResourceList { - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Resources.Limits[k] = v + container.Resources.Limits[k] = v } return nil } + +// ======================================================================================= + +func usesOldV1alpha1ContainerField(revision *servingv1alpha1.RevisionTemplateSpec) bool { + return revision.Spec.DeprecatedContainer != nil +} + +func extractContainer(template *servingv1alpha1.RevisionTemplateSpec) (*corev1.Container, error) { + if usesOldV1alpha1ContainerField(template) { + return template.Spec.DeprecatedContainer, nil + } + containers := template.Spec.Containers + if len(containers) == 0 { + return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers") + } + if len(containers) > 1 { + return nil, fmt.Errorf("internal: can't extract container for updating environment"+ + " variables as the configuration contains "+ + "more than one container (i.e. %d containers)", len(containers)) + } + return &containers[0], nil +} + +func updateEnvVarsFromMap(env []corev1.EnvVar, vars map[string]string) []corev1.EnvVar { + set := make(map[string]bool) + for i, _ := range env { + envVar := &env[i] + value, present := vars[envVar.Name] + if present { + envVar.Value = value + set[envVar.Name] = true + } + } + for name, value := range vars { + if !set[name] { + env = append( + env, + corev1.EnvVar{ + Name: name, + Value: value, + }) + } + } + return env +} diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index d4dbe2d0b..35c8d4c13 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -15,6 +15,7 @@ package serving import ( + "github.com/knative/serving/pkg/apis/serving/v1beta1" "reflect" "testing" @@ -23,16 +24,25 @@ import ( ) func TestUpdateEnvVarsNew(t *testing.T) { - config := getEmptyConfigurationSpec() + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsNew(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsNew(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { env := map[string]string{ "a": "foo", "b": "bar", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } @@ -41,14 +51,24 @@ func TestUpdateEnvVarsNew(t *testing.T) { } } -func TestUpdateEnvVarsAppend(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ - corev1.EnvVar{Name: "a", Value: "foo"}} +func TestUpdateEnvVarsAppendOld(t *testing.T) { + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsAppendOld(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsAppendOld(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + } env := map[string]string{ "b": "bar", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } @@ -58,23 +78,32 @@ func TestUpdateEnvVarsAppend(t *testing.T) { "b": "bar", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } func TestUpdateEnvVarsModify(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsModify(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsModify(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ corev1.EnvVar{Name: "a", Value: "foo"}} env := map[string]string{ "a": "fancy", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(revision, env) if err != nil { t.Fatal(err) } @@ -83,25 +112,34 @@ func TestUpdateEnvVarsModify(t *testing.T) { "a": "fancy", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } func TestUpdateEnvVarsBoth(t *testing.T) { - config := getEmptyConfigurationSpec() - config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env = []corev1.EnvVar{ + template, container := getV1alpha1RevisionTemplateWithOldFields() + testUpdateEnvVarsBoth(t, template, container) + assertNoV1alpha1(t, template) + + template, container = getV1alpha1Config() + testUpdateEnvVarsBoth(t, template, container) + assertNoV1alpha1Old(t, template) +} + +func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, container *corev1.Container) { + container.Env = []corev1.EnvVar{ corev1.EnvVar{Name: "a", Value: "foo"}, corev1.EnvVar{Name: "c", Value: "caroline"}} env := map[string]string{ "a": "fancy", "b": "boo", } - err := UpdateEnvVars(&config, env) + err := UpdateEnvVars(template, env) if err != nil { t.Fatal(err) } @@ -112,21 +150,49 @@ func TestUpdateEnvVarsBoth(t *testing.T) { "c": "caroline", } - found, err := EnvToMap(config.DeprecatedRevisionTemplate.Spec.DeprecatedContainer.Env) + found, err := EnvToMap(container.Env) if err != nil { t.Fatal(err) } if !reflect.DeepEqual(expected, found) { - t.Fatalf("Env did not match expected %v found %v", env, found) + t.Fatalf("Env did not match expected %v, found %v", env, found) } } -func getEmptyConfigurationSpec() servingv1alpha1.ConfigurationSpec { - return servingv1alpha1.ConfigurationSpec{ - DeprecatedRevisionTemplate: &servingv1alpha1.RevisionTemplateSpec{ - Spec: servingv1alpha1.RevisionSpec{ - DeprecatedContainer: &corev1.Container{}, +// ========================================================================================================= + +func getV1alpha1RevisionTemplateWithOldFields() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { + container := &corev1.Container{} + template := &servingv1alpha1.RevisionTemplateSpec{ + Spec: servingv1alpha1.RevisionSpec{ + DeprecatedContainer: container, + }, + } + return template, container +} + +func getV1alpha1Config() (*servingv1alpha1.RevisionTemplateSpec, *corev1.Container) { + containers := []corev1.Container{{}} + template := &servingv1alpha1.RevisionTemplateSpec{ + Spec: servingv1alpha1.RevisionSpec{ + RevisionSpec: v1beta1.RevisionSpec{ + PodSpec: v1beta1.PodSpec{ + Containers: containers, + }, }, }, } + return template, &containers[0] +} + +func assertNoV1alpha1Old(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { + if template.Spec.DeprecatedContainer != nil { + t.Error("Assuming only new v1alpha1 fields but found spec.container") + } +} + +func assertNoV1alpha1(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec) { + if template.Spec.Containers != nil { + t.Error("Assuming only old v1alpha1 fields but found spec.template") + } } diff --git a/pkg/serving/service.go b/pkg/serving/service.go index f146956a7..22a3b2c6e 100644 --- a/pkg/serving/service.go +++ b/pkg/serving/service.go @@ -20,7 +20,25 @@ import ( servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1" ) -func GetConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { +// Get the revision template associated with a service. +// Depending on the structure returned either the new v1beta1 fields or the +// 'old' v1alpha1 fields are looked up. +// The returned revision template can be updated in place. +// An error is returned if no revision template could be extracted +func GetRevisionTemplate(service *servingv1alpha1.Service) (*servingv1alpha1.RevisionTemplateSpec, error) { + // Try v1beta1 field first + if service.Spec.Template != nil { + return service.Spec.Template, nil + } + + config, err := getConfiguration(service) + if err != nil { + return nil, err + } + return config.DeprecatedRevisionTemplate, nil +} + +func getConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.ConfigurationSpec, error) { if service.Spec.DeprecatedRunLatest != nil { return &service.Spec.DeprecatedRunLatest.Configuration, nil } else if service.Spec.DeprecatedRelease != nil { @@ -28,6 +46,6 @@ func GetConfiguration(service *servingv1alpha1.Service) (*servingv1alpha1.Config } else if service.Spec.DeprecatedPinned != nil { return &service.Spec.DeprecatedPinned.Configuration, nil } else { - return nil, errors.New("Service does not specify a Configuration") + return nil, errors.New("service does not specify a Configuration") } }