From 4f643b5e178787db5a228ce71c819c3bdb0b658d Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Tue, 22 Sep 2020 19:33:39 +0800 Subject: [PATCH 1/5] moving utilities of handling podspec to podspec_helper --- .../service/configuration_edit_flags.go | 68 +- pkg/kn/commands/service/create_mock_test.go | 8 +- pkg/kn/commands/service/create_test.go | 11 +- .../service/service_update_mock_test.go | 48 +- pkg/kn/commands/service/update_test.go | 19 +- pkg/kn/flags/podspec.go | 169 +++++ pkg/kn/flags/podspec_helper.go | 611 +++++++++++++++++ pkg/kn/flags/podspec_helper_test.go | 346 ++++++++++ pkg/kn/flags/podspec_test.go | 93 +++ pkg/serving/config_changes.go | 622 +----------------- pkg/serving/config_changes_test.go | 390 +---------- pkg/util/corev1_helper.go | 87 +++ pkg/util/corev1_helper_test.go | 54 ++ 13 files changed, 1431 insertions(+), 1095 deletions(-) create mode 100644 pkg/kn/flags/podspec_helper.go create mode 100644 pkg/kn/flags/podspec_helper_test.go create mode 100644 pkg/util/corev1_helper.go create mode 100644 pkg/util/corev1_helper_test.go diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index d14f72fb60..647a5040c8 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -198,53 +198,10 @@ func (p *ConfigurationEditFlags) Apply( cmd *cobra.Command) error { template := &service.Spec.Template - if cmd.Flags().Changed("env") { - envMap, err := util.MapFromArrayAllowingSingles(p.PodSpecFlags.Env, "=") - if err != nil { - return fmt.Errorf("Invalid --env: %w", err) - } - - envToRemove := util.ParseMinusSuffix(envMap) - err = servinglib.UpdateEnvVars(template, envMap, envToRemove) - if err != nil { - return err - } - } - - if cmd.Flags().Changed("env-from") { - envFromSourceToUpdate := []string{} - envFromSourceToRemove := []string{} - for _, name := range p.PodSpecFlags.EnvFrom { - if name == "-" { - return fmt.Errorf("\"-\" is not a valid value for \"--env-from\"") - } else if strings.HasSuffix(name, "-") { - envFromSourceToRemove = append(envFromSourceToRemove, name[:len(name)-1]) - } else { - envFromSourceToUpdate = append(envFromSourceToUpdate, name) - } - } - - err := servinglib.UpdateEnvFrom(template, envFromSourceToUpdate, envFromSourceToRemove) - if err != nil { - return err - } - } - - if cmd.Flags().Changed("mount") || cmd.Flags().Changed("volume") { - mountsToUpdate, mountsToRemove, err := util.OrderedMapAndRemovalListFromArray(p.PodSpecFlags.Mount, "=") - if err != nil { - return fmt.Errorf("Invalid --mount: %w", err) - } - - volumesToUpdate, volumesToRemove, err := util.OrderedMapAndRemovalListFromArray(p.PodSpecFlags.Volume, "=") - if err != nil { - return fmt.Errorf("Invalid --volume: %w", err) - } - err = servinglib.UpdateVolumeMountsAndVolumes(template, mountsToUpdate, mountsToRemove, volumesToUpdate, volumesToRemove) - if err != nil { - return err - } + err := p.PodSpecFlags.ResolvePodSpec(&template.Spec.PodSpec, cmd) + if err != nil { + return err } name, err := servinglib.GenerateRevisionName(p.RevisionName, service) @@ -258,10 +215,6 @@ func (p *ConfigurationEditFlags) Apply( imageSet := false if cmd.Flags().Changed("image") { - err = servinglib.UpdateImage(template, p.PodSpecFlags.Image.String()) - if err != nil { - return err - } imageSet = true } _, userImagePresent := template.Annotations[servinglib.UserImageAnnotationKey] @@ -472,21 +425,6 @@ func (p *ConfigurationEditFlags) Apply( } - if cmd.Flags().Changed("service-account") { - err = servinglib.UpdateServiceAccountName(template, p.PodSpecFlags.ServiceAccountName) - if err != nil { - return err - } - } - - if cmd.Flags().Changed("pull-secret") { - servinglib.UpdateImagePullSecrets(template, p.PodSpecFlags.ImagePullSecrets) - } - - if cmd.Flags().Changed("user") { - servinglib.UpdateUser(template, p.PodSpecFlags.User) - } - if cmd.Flags().Changed("scale-init") { containsAnnotation := func(annotationList []string, annotation string) bool { for _, element := range annotationList { diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index d62a38a565..277e563e9b 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -279,7 +279,7 @@ func TestServiceCreateWithMountConfigMap(t *testing.T) { template := &service.Spec.Template template.Spec.Volumes = []corev1.Volume{ { - Name: servinglib.GenerateVolumeName("/mount/path"), + Name: util.GenerateVolumeName("/mount/path"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -292,7 +292,7 @@ func TestServiceCreateWithMountConfigMap(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: servinglib.GenerateVolumeName("/mount/path"), + Name: util.GenerateVolumeName("/mount/path"), MountPath: "/mount/path", ReadOnly: true, }, @@ -359,7 +359,7 @@ func TestServiceCreateWithMountSecret(t *testing.T) { template := &service.Spec.Template template.Spec.Volumes = []corev1.Volume{ { - Name: servinglib.GenerateVolumeName("/mount/path"), + Name: util.GenerateVolumeName("/mount/path"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name", @@ -370,7 +370,7 @@ func TestServiceCreateWithMountSecret(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: servinglib.GenerateVolumeName("/mount/path"), + Name: util.GenerateVolumeName("/mount/path"), MountPath: "/mount/path", ReadOnly: true, }, diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go index b6dea01794..6ebd0a8946 100644 --- a/pkg/kn/commands/service/create_test.go +++ b/pkg/kn/commands/service/create_test.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/watch" "knative.dev/client/pkg/kn/commands" - servinglib "knative.dev/client/pkg/serving" "knative.dev/client/pkg/util" "knative.dev/client/pkg/wait" network "knative.dev/networking/pkg" @@ -225,7 +224,7 @@ func TestServiceCreateEnv(t *testing.T) { if err != nil { t.Fatal(err) } - actualEnvVars, err := servinglib.EnvToMap(template.Spec.Containers[0].Env) + actualEnvVars, err := util.EnvToMap(template.Spec.Containers[0].Env) if err != nil { t.Fatal(err) } @@ -726,7 +725,7 @@ func TestServiceCreateEnvForce(t *testing.T) { if err != nil { t.Fatal(err) } - actualEnvVars, err := servinglib.EnvToMap(template.Spec.Containers[0].Env) + actualEnvVars, err := util.EnvToMap(template.Spec.Containers[0].Env) if err != nil { t.Fatal(err) } else if template.Spec.Containers[0].Image != "gcr.io/foo/bar:v2" { @@ -993,7 +992,7 @@ func TestServiceCreateFromYAMLWithOverride(t *testing.T) { assert.Assert(t, action.Matches("create", "services")) assert.Equal(t, created.Name, "foo") - actualEnvVar, err := servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + actualEnvVar, err := util.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) assert.NilError(t, err) assert.DeepEqual(t, actualEnvVar, expectedEnvVars) @@ -1007,7 +1006,7 @@ func TestServiceCreateFromYAMLWithOverride(t *testing.T) { assert.Assert(t, action.Matches("create", "services")) assert.Equal(t, created.Name, "foo") - actualEnvVar, err = servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + actualEnvVar, err = util.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) assert.NilError(t, err) assert.DeepEqual(t, actualEnvVar, expectedEnvVars) @@ -1020,7 +1019,7 @@ func TestServiceCreateFromYAMLWithOverride(t *testing.T) { assert.Assert(t, action.Matches("create", "services")) assert.Equal(t, created.Name, "foo") - actualEnvVar, err = servinglib.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) + actualEnvVar, err = util.EnvToMap(created.Spec.Template.Spec.GetContainer().Env) assert.NilError(t, err) assert.DeepEqual(t, actualEnvVar, expectedEnvVars) diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index 885647f6c4..d60059b801 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -1069,7 +1069,7 @@ func TestServiceUpdateWithAddingMount(t *testing.T) { template.Spec.Volumes = []corev1.Volume{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1079,7 +1079,7 @@ func TestServiceUpdateWithAddingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name", @@ -1090,12 +1090,12 @@ func TestServiceUpdateWithAddingMount(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), MountPath: "/mount/config-map-path", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), MountPath: "/mount/secret-path", ReadOnly: true, }, @@ -1134,7 +1134,7 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { template.Spec.Volumes = []corev1.Volume{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1144,7 +1144,7 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name-1", @@ -1155,12 +1155,12 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), MountPath: "/mount/config-map-path", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), MountPath: "/mount/secret-path", ReadOnly: true, }, @@ -1174,7 +1174,7 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { template.Spec.Volumes = []corev1.Volume{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1184,7 +1184,7 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name-2", @@ -1195,12 +1195,12 @@ func TestServiceUpdateWithUpdatingMount(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path"), + Name: util.GenerateVolumeName("/mount/config-map-path"), MountPath: "/mount/config-map-path", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path"), + Name: util.GenerateVolumeName("/mount/secret-path"), MountPath: "/mount/secret-path", ReadOnly: true, }, @@ -1243,7 +1243,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { template.Spec.Volumes = []corev1.Volume{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-1"), + Name: util.GenerateVolumeName("/mount/config-map-path-1"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1253,7 +1253,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-1"), + Name: util.GenerateVolumeName("/mount/secret-path-1"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name-1", @@ -1261,7 +1261,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-2"), + Name: util.GenerateVolumeName("/mount/config-map-path-2"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1271,7 +1271,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-2"), + Name: util.GenerateVolumeName("/mount/secret-path-2"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name-2", @@ -1292,22 +1292,22 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-1"), + Name: util.GenerateVolumeName("/mount/config-map-path-1"), MountPath: "/mount/config-map-path-1", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-1"), + Name: util.GenerateVolumeName("/mount/secret-path-1"), MountPath: "/mount/secret-path-1", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-2"), + Name: util.GenerateVolumeName("/mount/config-map-path-2"), MountPath: "/mount/config-map-path-2", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-2"), + Name: util.GenerateVolumeName("/mount/secret-path-2"), MountPath: "/mount/secret-path-2", ReadOnly: true, }, @@ -1326,7 +1326,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { template.Spec.Volumes = []corev1.Volume{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-1"), + Name: util.GenerateVolumeName("/mount/config-map-path-1"), VolumeSource: corev1.VolumeSource{ ConfigMap: &corev1.ConfigMapVolumeSource{ LocalObjectReference: corev1.LocalObjectReference{ @@ -1336,7 +1336,7 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { }, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-2"), + Name: util.GenerateVolumeName("/mount/secret-path-2"), VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{ SecretName: "secret-name-2", @@ -1357,12 +1357,12 @@ func TestServiceUpdateWithRemovingMount(t *testing.T) { template.Spec.Containers[0].VolumeMounts = []corev1.VolumeMount{ { - Name: clientserving.GenerateVolumeName("/mount/config-map-path-1"), + Name: util.GenerateVolumeName("/mount/config-map-path-1"), MountPath: "/mount/config-map-path-1", ReadOnly: true, }, { - Name: clientserving.GenerateVolumeName("/mount/secret-path-2"), + Name: util.GenerateVolumeName("/mount/secret-path-2"), MountPath: "/mount/secret-path-2", ReadOnly: true, }, diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go index d38de86b1e..259c6e985e 100644 --- a/pkg/kn/commands/service/update_test.go +++ b/pkg/kn/commands/service/update_test.go @@ -37,6 +37,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/watch" clienttesting "k8s.io/client-go/testing" + "knative.dev/client/pkg/kn/flags" "knative.dev/serving/pkg/apis/serving" servingv1 "knative.dev/serving/pkg/apis/serving/v1" ) @@ -142,7 +143,7 @@ func TestServiceUpdateImageSync(t *testing.T) { orig := newEmptyService() template := &orig.Spec.Template - err := servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") if err != nil { t.Fatal(err) } @@ -164,7 +165,7 @@ func TestServiceUpdateImage(t *testing.T) { orig := newEmptyService() template := &orig.Spec.Template - err := servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") if err != nil { t.Fatal(err) } @@ -215,7 +216,7 @@ func TestServiceUpdateCommand(t *testing.T) { origTemplate := &orig.Spec.Template - err := servinglib.UpdateContainerCommand(origTemplate, "./start") + err := flags.UpdateContainerCommand(&origTemplate.Spec.PodSpec, "./start") assert.NilError(t, err) action, updated, _, err := fakeServiceUpdate(orig, []string{ @@ -232,7 +233,7 @@ func TestServiceUpdateArg(t *testing.T) { origTemplate := orig.Spec.Template - err := servinglib.UpdateContainerArg(&origTemplate, []string{"myArg0"}) + err := flags.UpdateContainerArg(&origTemplate.Spec.PodSpec, []string{"myArg0"}) assert.NilError(t, err) action, updated, _, err := fakeServiceUpdate(orig, []string{ @@ -456,7 +457,7 @@ func TestServiceUpdateEnv(t *testing.T) { {Name: "OTHEREXISTING"}, } - servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") action, updated, _, err := fakeServiceUpdate(orig, []string{ "service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--env=OTHEREXISTING-=whatever", "--no-wait"}) @@ -482,7 +483,7 @@ func TestServiceUpdatePinsToDigestWhenAsked(t *testing.T) { template := &orig.Spec.Template delete(template.Annotations, servinglib.UserImageAnnotationKey) - err := servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") if err != nil { t.Fatal(err) } @@ -505,7 +506,7 @@ func TestServiceUpdatePinsToDigestWhenPreviouslyDidSo(t *testing.T) { orig := newEmptyService() template := &orig.Spec.Template - err := servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") if err != nil { t.Fatal(err) } @@ -528,7 +529,7 @@ func TestServiceUpdateDoesntPinToDigestWhenUnAsked(t *testing.T) { orig := newEmptyService() template := orig.Spec.Template - err := servinglib.UpdateImage(&template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") assert.NilError(t, err) action, updated, _, err := fakeServiceUpdate(orig, []string{ @@ -552,7 +553,7 @@ func TestServiceUpdateDoesntPinToDigestWhenPreviouslyDidnt(t *testing.T) { template := &orig.Spec.Template delete(template.Annotations, servinglib.UserImageAnnotationKey) - err := servinglib.UpdateImage(template, "gcr.io/foo/bar:baz") + err := flags.UpdateImage(&template.Spec.PodSpec, "gcr.io/foo/bar:baz") if err != nil { t.Fatal(err) } diff --git a/pkg/kn/flags/podspec.go b/pkg/kn/flags/podspec.go index e0c71717fb..b3ee4af613 100644 --- a/pkg/kn/flags/podspec.go +++ b/pkg/kn/flags/podspec.go @@ -16,7 +16,14 @@ package flags import ( "errors" + "fmt" + "strings" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "knative.dev/client/pkg/util" + + "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -163,3 +170,165 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string { flagNames = append(flagNames, "user") return flagNames } + +func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Command) error { + //var podSpec = &corev1.PodSpec{Containers: []corev1.Container{{}}} + var err error + + if cmd.Flags().Changed("env") { + envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=") + if err != nil { + return fmt.Errorf("Invalid --env: %w", err) + } + + envToRemove := util.ParseMinusSuffix(envMap) + err = UpdateEnvVars(podSpec, envMap, envToRemove) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("env-from") { + envFromSourceToUpdate := []string{} + envFromSourceToRemove := []string{} + for _, name := range p.EnvFrom { + if name == "-" { + return fmt.Errorf("\"-\" is not a valid value for \"--env-from\"") + } else if strings.HasSuffix(name, "-") { + envFromSourceToRemove = append(envFromSourceToRemove, name[:len(name)-1]) + } else { + envFromSourceToUpdate = append(envFromSourceToUpdate, name) + } + } + + err := UpdateEnvFrom(podSpec, envFromSourceToUpdate, envFromSourceToRemove) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("mount") || cmd.Flags().Changed("volume") { + mountsToUpdate, mountsToRemove, err := util.OrderedMapAndRemovalListFromArray(p.Mount, "=") + if err != nil { + return fmt.Errorf("Invalid --mount: %w", err) + } + + volumesToUpdate, volumesToRemove, err := util.OrderedMapAndRemovalListFromArray(p.Volume, "=") + if err != nil { + return fmt.Errorf("Invalid --volume: %w", err) + } + + err = UpdateVolumeMountsAndVolumes(podSpec, mountsToUpdate, mountsToRemove, volumesToUpdate, volumesToRemove) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("image") { + err = UpdateImage(podSpec, p.Image.String()) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("limits-cpu") || cmd.Flags().Changed("limits-memory") { + if cmd.Flags().Changed("limit") { + return fmt.Errorf("only one of (DEPRECATED) --limits-cpu / --limits-memory and --limit can be specified") + } + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --limits-cpu / --limits-memory are deprecated and going to be removed in future release, please use --limit instead.\n\n") + } + + if cmd.Flags().Changed("requests-cpu") || cmd.Flags().Changed("requests-memory") { + if cmd.Flags().Changed("request") { + return fmt.Errorf("only one of (DEPRECATED) --requests-cpu / --requests-memory and --request can be specified") + } + fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --requests-cpu / --requests-memory are deprecated and going to be removed in future release, please use --request instead.\n\n") + } + + limitsResources, err := p.computeResources(p.LimitsFlags) + if err != nil { + return err + } + requestsResources, err := p.computeResources(p.RequestsFlags) + if err != nil { + return err + } + err = UpdateResourcesDeprecated(podSpec, requestsResources, limitsResources) + if err != nil { + return err + } + + requestsToRemove, limitsToRemove, err := p.Resources.Validate() + if err != nil { + return err + } + + err = UpdateResources(podSpec, p.Resources.ResourceRequirements, requestsToRemove, limitsToRemove) + if err != nil { + return err + } + + if cmd.Flags().Changed("cmd") { + err = UpdateContainerCommand(podSpec, p.Command) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("arg") { + err = UpdateContainerArg(podSpec, p.Arg) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("port") { + err = UpdateContainerPort(podSpec, p.Port) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("service-account") { + err = UpdateServiceAccountName(podSpec, p.ServiceAccountName) + if err != nil { + return err + } + } + + if cmd.Flags().Changed("pull-secret") { + UpdateImagePullSecrets(podSpec, p.ImagePullSecrets) + } + + if cmd.Flags().Changed("user") { + UpdateUser(podSpec, p.User) + } + + return nil +} + +func (p *PodSpecFlags) computeResources(resourceFlags ResourceFlags) (corev1.ResourceList, error) { + resourceList := corev1.ResourceList{} + + if resourceFlags.CPU != "" { + cpuQuantity, err := resource.ParseQuantity(resourceFlags.CPU) + if err != nil { + return corev1.ResourceList{}, + fmt.Errorf("Error parsing %q: %w", resourceFlags.CPU, err) + } + + resourceList[corev1.ResourceCPU] = cpuQuantity + } + + if resourceFlags.Memory != "" { + memoryQuantity, err := resource.ParseQuantity(resourceFlags.Memory) + if err != nil { + return corev1.ResourceList{}, + fmt.Errorf("Error parsing %q: %w", resourceFlags.Memory, err) + } + + resourceList[corev1.ResourceMemory] = memoryQuantity + } + + return resourceList, nil +} diff --git a/pkg/kn/flags/podspec_helper.go b/pkg/kn/flags/podspec_helper.go new file mode 100644 index 0000000000..05895a460e --- /dev/null +++ b/pkg/kn/flags/podspec_helper.go @@ -0,0 +1,611 @@ +// Copyright © 2020 The Knative Authors +// +// 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 flags + +import ( + "fmt" + "sort" + "strconv" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" + "knative.dev/client/pkg/util" +) + +// VolumeSourceType is a type standing for enumeration of ConfigMap and Secret +type VolumeSourceType int + +// Enumeration of volume source types: ConfigMap or Secret +const ( + ConfigMapVolumeSourceType VolumeSourceType = iota + SecretVolumeSourceType + PortFormatErr = "the port specification '%s' is not valid. Please provide in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'." +) + +var ( + UserImageAnnotationKey = "client.knative.dev/user-image" +) + +func (vt VolumeSourceType) String() string { + names := [...]string{"config-map", "secret"} + if vt < ConfigMapVolumeSourceType || vt > SecretVolumeSourceType { + return "unknown" + } + return names[vt] +} + +func containerOfPodSpec(spec *corev1.PodSpec) (*corev1.Container, error) { + if len(spec.Containers) == 0 { + newContainer := corev1.Container{} + spec.Containers = append(spec.Containers, newContainer) + } + return &spec.Containers[0], nil +} + +// UpdateEnvVars gives 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, then sort by env key name. +func UpdateEnvVars(spec *corev1.PodSpec, toUpdate map[string]string, toRemove []string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + updated := updateEnvVarsFromMap(container.Env, toUpdate) + updated = removeEnvVars(updated, toRemove) + // Sort by env key name + sort.SliceStable(updated, func(i, j int) bool { + return updated[i].Name < updated[j].Name + }) + container.Env = updated + + return nil +} + +// UpdateEnvFrom updates envFrom +func UpdateEnvFrom(spec *corev1.PodSpec, toUpdate []string, toRemove []string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + envFrom, err := updateEnvFrom(container.EnvFrom, toUpdate) + if err != nil { + return err + } + container.EnvFrom, err = removeEnvFrom(envFrom, toRemove) + return err +} + +// UpdateVolumeMountsAndVolumes updates the configuration for volume mounts and volumes. +func UpdateVolumeMountsAndVolumes(spec *corev1.PodSpec, + mountsToUpdate *util.OrderedMap, mountsToRemove []string, volumesToUpdate *util.OrderedMap, volumesToRemove []string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + + volumeSourceInfoByName, mountsToUpdate, err := reviseVolumeInfoAndMountsToUpdate(spec.Volumes, mountsToUpdate, volumesToUpdate) + if err != nil { + return err + } + + volumes, err := updateVolumesFromMap(spec.Volumes, volumeSourceInfoByName) + if err != nil { + return err + } + + volumeMounts, err := updateVolumeMountsFromMap(container.VolumeMounts, mountsToUpdate, volumes) + if err != nil { + return err + } + + volumesToRemove = reviseVolumesToRemove(container.VolumeMounts, volumesToRemove, mountsToRemove) + + container.VolumeMounts = removeVolumeMounts(volumeMounts, mountsToRemove) + spec.Volumes, err = removeVolumes(volumes, volumesToRemove, container.VolumeMounts) + + return err +} + +// UpdateImage a given image +func UpdateImage(spec *corev1.PodSpec, image string) error { + // When not setting the image to a digest, add the user image annotation. + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + container.Image = image + return nil +} + +// UpdateContainerCommand updates container with a given argument +func UpdateContainerCommand(spec *corev1.PodSpec, command string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + container.Command = []string{command} + return nil +} + +// UpdateContainerArg updates container with a given argument +func UpdateContainerArg(spec *corev1.PodSpec, arg []string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + container.Args = arg + return nil +} + +// UpdateContainerPort updates container with a given name:port +func UpdateContainerPort(spec *corev1.PodSpec, port string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + + var containerPort int64 + var name string + + elements := strings.SplitN(port, ":", 2) + if len(elements) == 2 { + name = elements[0] + containerPort, err = strconv.ParseInt(elements[1], 10, 32) + if err != nil { + return fmt.Errorf(PortFormatErr, port) + } + } else { + name = "" + containerPort, err = strconv.ParseInt(elements[0], 10, 32) + if err != nil { + return fmt.Errorf(PortFormatErr, port) + } + } + + container.Ports = []corev1.ContainerPort{{ + ContainerPort: int32(containerPort), + Name: name, + }} + return nil +} + +// UpdateUser updates container with a given user id +func UpdateUser(spec *corev1.PodSpec, user int64) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + container.SecurityContext = &corev1.SecurityContext{ + RunAsUser: &user, + } + return nil +} + +// UpdateResources updates container resources for given revision spec +func UpdateResources(spec *corev1.PodSpec, resources corev1.ResourceRequirements, requestsToRemove, limitsToRemove []string) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} + } + + for k, v := range resources.Requests { + container.Resources.Requests[k] = v + } + + for _, reqToRemove := range requestsToRemove { + delete(container.Resources.Requests, corev1.ResourceName(reqToRemove)) + } + + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + + for k, v := range resources.Limits { + container.Resources.Limits[k] = v + } + + for _, limToRemove := range limitsToRemove { + delete(container.Resources.Limits, corev1.ResourceName(limToRemove)) + } + + return nil +} + +// UpdateResourcesDeprecated updates resources as requested +func UpdateResourcesDeprecated(spec *corev1.PodSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { + container, err := containerOfPodSpec(spec) + if err != nil { + return err + } + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} + } + + for k, v := range requestsResourceList { + container.Resources.Requests[k] = v + } + + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + + for k, v := range limitsResourceList { + container.Resources.Limits[k] = v + } + + return nil +} + +// UpdateServiceAccountName updates the service account name used for the corresponding knative service +func UpdateServiceAccountName(spec *corev1.PodSpec, serviceAccountName string) error { + serviceAccountName = strings.TrimSpace(serviceAccountName) + spec.ServiceAccountName = serviceAccountName + return nil +} + +// UpdateImagePullSecrets updates the image pull secrets used for the corresponding knative service +func UpdateImagePullSecrets(spec *corev1.PodSpec, pullsecrets string) { + pullsecrets = strings.TrimSpace(pullsecrets) + if pullsecrets == "" { + spec.ImagePullSecrets = nil + } else { + spec.ImagePullSecrets = []corev1.LocalObjectReference{{ + Name: pullsecrets, + }} + } +} + +// ======================================================================================= +func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { + set := sets.NewString() + for i := range env { + envVar := &env[i] + if val, ok := toUpdate[envVar.Name]; ok { + envVar.Value = val + set.Insert(envVar.Name) + } + } + for name, val := range toUpdate { + if !set.Has(name) { + env = append(env, corev1.EnvVar{Name: name, Value: val}) + } + } + return env +} + +func removeEnvVars(env []corev1.EnvVar, toRemove []string) []corev1.EnvVar { + for _, name := range toRemove { + for i, envVar := range env { + if envVar.Name == name { + env = append(env[:i], env[i+1:]...) + break + } + } + } + return env +} + +func updateEnvFrom(envFromSources []corev1.EnvFromSource, toUpdate []string) ([]corev1.EnvFromSource, error) { + existingNameSet := make(map[string]bool) + + for _, envSrc := range envFromSources { + if canonicalName, err := getCanonicalNameFromEnvFromSource(&envSrc); err == nil { + existingNameSet[canonicalName] = true + } + } + + for _, s := range toUpdate { + info, err := newVolumeSourceInfoWithSpecString(s) + if err != nil { + return nil, err + } + + if _, ok := existingNameSet[info.getCanonicalName()]; !ok { + envFromSources = append(envFromSources, *info.createEnvFromSource()) + } + } + + return envFromSources, nil +} + +func removeEnvFrom(envFromSources []corev1.EnvFromSource, toRemove []string) ([]corev1.EnvFromSource, error) { + for _, name := range toRemove { + info, err := newVolumeSourceInfoWithSpecString(name) + if err != nil { + return nil, err + } + for i, envSrc := range envFromSources { + if (info.volumeSourceType == ConfigMapVolumeSourceType && envSrc.ConfigMapRef != nil && info.volumeSourceName == envSrc.ConfigMapRef.Name) || + (info.volumeSourceType == SecretVolumeSourceType && envSrc.SecretRef != nil && info.volumeSourceName == envSrc.SecretRef.Name) { + envFromSources = append(envFromSources[:i], envFromSources[i+1:]...) + break + } + } + } + + if len(envFromSources) == 0 { + envFromSources = nil + } + + return envFromSources, nil +} + +func updateVolume(volume *corev1.Volume, info *volumeSourceInfo) error { + switch info.volumeSourceType { + case ConfigMapVolumeSourceType: + volume.Secret = nil + volume.ConfigMap = &corev1.ConfigMapVolumeSource{LocalObjectReference: corev1.LocalObjectReference{Name: info.volumeSourceName}} + case SecretVolumeSourceType: + volume.ConfigMap = nil + volume.Secret = &corev1.SecretVolumeSource{SecretName: info.volumeSourceName} + default: + return fmt.Errorf("Invalid VolumeSourceType") + } + return nil +} + +// updateVolumeMountsFromMap updates or adds volume mounts. If a given name of a volume is not existing, it returns an error +func updateVolumeMountsFromMap(volumeMounts []corev1.VolumeMount, toUpdate *util.OrderedMap, volumes []corev1.Volume) ([]corev1.VolumeMount, error) { + set := make(map[string]bool) + + for i := range volumeMounts { + volumeMount := &volumeMounts[i] + name, present := toUpdate.GetString(volumeMount.MountPath) + + if present { + if !existsVolumeNameInVolumes(name, volumes) { + return nil, fmt.Errorf("There is no volume matched with %q", name) + } + + volumeMount.ReadOnly = true + volumeMount.Name = name + set[volumeMount.MountPath] = true + } + } + + it := toUpdate.Iterator() + for mountPath, name, ok := it.NextString(); ok; mountPath, name, ok = it.NextString() { + if !set[mountPath] { + volumeMounts = append(volumeMounts, corev1.VolumeMount{ + Name: name, + ReadOnly: true, + MountPath: mountPath, + }) + } + } + + return volumeMounts, nil +} + +func removeVolumeMounts(volumeMounts []corev1.VolumeMount, toRemove []string) []corev1.VolumeMount { + for _, mountPath := range toRemove { + for i, volumeMount := range volumeMounts { + if volumeMount.MountPath == mountPath { + volumeMounts = append(volumeMounts[:i], volumeMounts[i+1:]...) + break + } + } + } + + if len(volumeMounts) == 0 { + return nil + } + + return volumeMounts +} + +// updateVolumesFromMap updates or adds volumes regardless whether the volume is used or not +func updateVolumesFromMap(volumes []corev1.Volume, toUpdate *util.OrderedMap) ([]corev1.Volume, error) { + set := make(map[string]bool) + + for i := range volumes { + volume := &volumes[i] + info, present := toUpdate.Get(volume.Name) + if present { + err := updateVolume(volume, info.(*volumeSourceInfo)) + if err != nil { + return nil, err + } + set[volume.Name] = true + } + } + + it := toUpdate.Iterator() + for name, info, ok := it.Next(); ok; name, info, ok = it.Next() { + if !set[name] { + volumes = append(volumes, corev1.Volume{Name: name}) + updateVolume(&volumes[len(volumes)-1], info.(*volumeSourceInfo)) + } + } + + return volumes, nil +} + +// removeVolumes removes volumes. If there is a volume mount referencing the volume, it causes an error +func removeVolumes(volumes []corev1.Volume, toRemove []string, volumeMounts []corev1.VolumeMount) ([]corev1.Volume, error) { + for _, name := range toRemove { + for i, volume := range volumes { + if volume.Name == name { + if existsVolumeNameInVolumeMounts(name, volumeMounts) { + return nil, fmt.Errorf("The volume %q cannot be removed because it is mounted", name) + } + volumes = append(volumes[:i], volumes[i+1:]...) + break + } + } + } + + if len(volumes) == 0 { + return nil, nil + } + + return volumes, nil +} + +// ======================================================================================= + +type volumeSourceInfo struct { + volumeSourceType VolumeSourceType + volumeSourceName string +} + +func newVolumeSourceInfoWithSpecString(spec string) (*volumeSourceInfo, error) { + slices := strings.SplitN(spec, ":", 2) + if len(slices) != 2 { + return nil, fmt.Errorf("argument requires a value that contains the : character; got %q", spec) + } + + var volumeSourceType VolumeSourceType + + typeString := strings.TrimSpace(slices[0]) + volumeSourceName := strings.TrimSpace(slices[1]) + + switch typeString { + case "config-map", "cm": + volumeSourceType = ConfigMapVolumeSourceType + case "secret", "sc": + volumeSourceType = SecretVolumeSourceType + default: + return nil, fmt.Errorf("unsupported volume source type \"%q\"; supported volume source types are \"config-map\" and \"secret\"", slices[0]) + } + + if len(volumeSourceName) == 0 { + return nil, fmt.Errorf("the name of %s cannot be an empty string", volumeSourceType) + } + + return &volumeSourceInfo{ + volumeSourceType: volumeSourceType, + volumeSourceName: volumeSourceName, + }, nil +} + +func (vol *volumeSourceInfo) getCanonicalName() string { + return fmt.Sprintf("%s:%s", vol.volumeSourceType, vol.volumeSourceName) +} + +func getCanonicalNameFromEnvFromSource(envSrc *corev1.EnvFromSource) (string, error) { + if envSrc.ConfigMapRef != nil { + return fmt.Sprintf("%s:%s", ConfigMapVolumeSourceType, envSrc.ConfigMapRef.Name), nil + } + if envSrc.SecretRef != nil { + return fmt.Sprintf("%s:%s", SecretVolumeSourceType, envSrc.SecretRef.Name), nil + } + + return "", fmt.Errorf("there is no ConfigMapRef or SecretRef in a EnvFromSource") +} + +func (vol *volumeSourceInfo) createEnvFromSource() *corev1.EnvFromSource { + switch vol.volumeSourceType { + case ConfigMapVolumeSourceType: + return &corev1.EnvFromSource{ + ConfigMapRef: &corev1.ConfigMapEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: vol.volumeSourceName, + }}} + case SecretVolumeSourceType: + return &corev1.EnvFromSource{ + SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: vol.volumeSourceName, + }}} + } + + return nil +} + +// ======================================================================================= + +func existsVolumeNameInVolumes(volumeName string, volumes []corev1.Volume) bool { + for _, volume := range volumes { + if volume.Name == volumeName { + return true + } + } + return false +} + +func existsVolumeNameInVolumeMounts(volumeName string, volumeMounts []corev1.VolumeMount) bool { + for _, volumeMount := range volumeMounts { + if volumeMount.Name == volumeName { + return true + } + } + return false +} + +// ======================================================================================= + +func reviseVolumeInfoAndMountsToUpdate(volumes []corev1.Volume, mountsToUpdate *util.OrderedMap, + volumesToUpdate *util.OrderedMap) (*util.OrderedMap, *util.OrderedMap, error) { + volumeSourceInfoByName := util.NewOrderedMap() //make(map[string]*volumeSourceInfo) + mountsToUpdateRevised := util.NewOrderedMap() //make(map[string]string) + + it := mountsToUpdate.Iterator() + for path, value, ok := it.NextString(); ok; path, value, ok = it.NextString() { + // slices[0] -> config-map, cm, secret, sc, volume, or vo + // slices[1] -> secret, config-map, or volume name + slices := strings.SplitN(value, ":", 2) + if len(slices) == 1 { + mountsToUpdateRevised.Set(path, slices[0]) + } else { + switch volumeType := slices[0]; volumeType { + case "config-map", "cm": + generatedName := util.GenerateVolumeName(path) + volumeSourceInfoByName.Set(generatedName, &volumeSourceInfo{ + volumeSourceType: ConfigMapVolumeSourceType, + volumeSourceName: slices[1], + }) + mountsToUpdateRevised.Set(path, generatedName) + case "secret", "sc": + generatedName := util.GenerateVolumeName(path) + volumeSourceInfoByName.Set(generatedName, &volumeSourceInfo{ + volumeSourceType: SecretVolumeSourceType, + volumeSourceName: slices[1], + }) + mountsToUpdateRevised.Set(path, generatedName) + + default: + return nil, nil, fmt.Errorf("unsupported volume type \"%q\"; supported volume types are \"config-map or cm\", \"secret or sc\", and \"volume or vo\"", slices[0]) + } + } + } + + it = volumesToUpdate.Iterator() + for name, value, ok := it.NextString(); ok; name, value, ok = it.NextString() { + info, err := newVolumeSourceInfoWithSpecString(value) + if err != nil { + return nil, nil, err + } + volumeSourceInfoByName.Set(name, info) + } + + return volumeSourceInfoByName, mountsToUpdateRevised, nil +} + +func reviseVolumesToRemove(volumeMounts []corev1.VolumeMount, volumesToRemove []string, mountsToRemove []string) []string { + for _, pathToRemove := range mountsToRemove { + for _, volumeMount := range volumeMounts { + if volumeMount.MountPath == pathToRemove && volumeMount.Name == util.GenerateVolumeName(pathToRemove) { + volumesToRemove = append(volumesToRemove, volumeMount.Name) + } + } + } + return volumesToRemove +} diff --git a/pkg/kn/flags/podspec_helper_test.go b/pkg/kn/flags/podspec_helper_test.go new file mode 100644 index 0000000000..cb119894e4 --- /dev/null +++ b/pkg/kn/flags/podspec_helper_test.go @@ -0,0 +1,346 @@ +/* +Copyright 2020 The Knative Authors + +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 flags + +import ( + "fmt" + "testing" + + "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" + "knative.dev/client/pkg/util" + "knative.dev/pkg/ptr" +) + +func getPodSpec() (*corev1.PodSpec, *corev1.Container) { + spec := &corev1.PodSpec{ + Containers: []corev1.Container{{}}, + } + return spec, &spec.Containers[0] +} + +func TestUpdateEnvVarsNew(t *testing.T) { + spec, _ := getPodSpec() + env := []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + {Name: "b", Value: "bar"}, + } + found, err := util.EnvToMap(env) + assert.NilError(t, err) + err = UpdateEnvVars(spec, found, []string{}) + assert.NilError(t, err) + assert.DeepEqual(t, env, spec.Containers[0].Env) +} + +func TestUpdateEnvFrom(t *testing.T) { + spec, container := getPodSpec() + container.EnvFrom = append(container.EnvFrom, + corev1.EnvFromSource{ + ConfigMapRef: &corev1.ConfigMapEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "config-map-existing-name", + }}}, + corev1.EnvFromSource{ + SecretRef: &corev1.SecretEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "secret-existing-name", + }}}, + ) + UpdateEnvFrom(spec, + []string{"config-map:config-map-new-name-1", "secret:secret-new-name-1"}, + []string{"config-map:config-map-existing-name", "secret:secret-existing-name"}) + assert.Equal(t, len(container.EnvFrom), 2) + assert.Equal(t, container.EnvFrom[0].ConfigMapRef.Name, "config-map-new-name-1") + assert.Equal(t, container.EnvFrom[1].SecretRef.Name, "secret-new-name-1") +} + +func TestUpdateVolumeMountsAndVolumes(t *testing.T) { + spec, container := getPodSpec() + spec.Volumes = append(spec.Volumes, + corev1.Volume{ + Name: "existing-config-map-volume-name-1", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "existing-config-map-1", + }}}}, + corev1.Volume{ + Name: "existing-config-map-volume-name-2", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "existing-config-map-2", + }}}}, + corev1.Volume{ + Name: "existing-secret-volume-name-1", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: "existing-secret-1", + }}}, + corev1.Volume{ + Name: "existing-secret-volume-name-2", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: "existing-secret-2", + }}}) + + container.VolumeMounts = append(container.VolumeMounts, + corev1.VolumeMount{ + Name: "existing-config-map-volume-name-1", + ReadOnly: true, + MountPath: "/existing-config-map-1/mount/path", + }, + corev1.VolumeMount{ + Name: "existing-config-map-volume-name-2", + ReadOnly: true, + MountPath: "/existing-config-map-2/mount/path", + }, + corev1.VolumeMount{ + Name: "existing-secret-volume-name-1", + ReadOnly: true, + MountPath: "/existing-secret-1/mount/path", + }, + corev1.VolumeMount{ + Name: "existing-secret-volume-name-2", + ReadOnly: true, + MountPath: "/existing-secret-2/mount/path", + }, + ) + + err := UpdateVolumeMountsAndVolumes(spec, + util.NewOrderedMapWithKVStrings([][]string{{"/new-config-map/mount/path", "new-config-map-volume-name"}}), + []string{}, + util.NewOrderedMapWithKVStrings([][]string{{"new-config-map-volume-name", "config-map:new-config-map"}}), + []string{}) + assert.NilError(t, err) + + err = UpdateVolumeMountsAndVolumes(spec, + util.NewOrderedMapWithKVStrings([][]string{{"/updated-config-map/mount/path", "existing-config-map-volume-name-2"}}), + []string{}, + util.NewOrderedMapWithKVStrings([][]string{{"existing-config-map-volume-name-2", "config-map:updated-config-map"}}), + []string{}) + assert.NilError(t, err) + + err = UpdateVolumeMountsAndVolumes(spec, + util.NewOrderedMapWithKVStrings([][]string{{"/new-secret/mount/path", "new-secret-volume-name"}}), + []string{}, + util.NewOrderedMapWithKVStrings([][]string{{"new-secret-volume-name", "secret:new-secret"}}), + []string{}) + assert.NilError(t, err) + + err = UpdateVolumeMountsAndVolumes(spec, + util.NewOrderedMapWithKVStrings([][]string{{"/updated-secret/mount/path", "existing-secret-volume-name-2"}}), + []string{"/existing-config-map-1/mount/path", + "/existing-secret-1/mount/path"}, + util.NewOrderedMapWithKVStrings([][]string{{"existing-secret-volume-name-2", "secret:updated-secret"}}), + []string{"existing-config-map-volume-name-1", + "existing-secret-volume-name-1"}) + assert.NilError(t, err) + + assert.Equal(t, len(spec.Volumes), 4) + assert.Equal(t, len(container.VolumeMounts), 6) + assert.Equal(t, spec.Volumes[0].Name, "existing-config-map-volume-name-2") + assert.Equal(t, spec.Volumes[0].ConfigMap.Name, "updated-config-map") + assert.Equal(t, spec.Volumes[1].Name, "existing-secret-volume-name-2") + assert.Equal(t, spec.Volumes[1].Secret.SecretName, "updated-secret") + assert.Equal(t, spec.Volumes[2].Name, "new-config-map-volume-name") + assert.Equal(t, spec.Volumes[2].ConfigMap.Name, "new-config-map") + assert.Equal(t, spec.Volumes[3].Name, "new-secret-volume-name") + assert.Equal(t, spec.Volumes[3].Secret.SecretName, "new-secret") + + assert.Equal(t, container.VolumeMounts[0].Name, "existing-config-map-volume-name-2") + assert.Equal(t, container.VolumeMounts[0].MountPath, "/existing-config-map-2/mount/path") + assert.Equal(t, container.VolumeMounts[1].Name, "existing-secret-volume-name-2") + assert.Equal(t, container.VolumeMounts[1].MountPath, "/existing-secret-2/mount/path") + assert.Equal(t, container.VolumeMounts[2].Name, "new-config-map-volume-name") + assert.Equal(t, container.VolumeMounts[2].MountPath, "/new-config-map/mount/path") + assert.Equal(t, container.VolumeMounts[3].Name, "existing-config-map-volume-name-2") + assert.Equal(t, container.VolumeMounts[3].MountPath, "/updated-config-map/mount/path") + assert.Equal(t, container.VolumeMounts[4].Name, "new-secret-volume-name") + assert.Equal(t, container.VolumeMounts[4].MountPath, "/new-secret/mount/path") + assert.Equal(t, container.VolumeMounts[5].Name, "existing-secret-volume-name-2") + assert.Equal(t, container.VolumeMounts[5].MountPath, "/updated-secret/mount/path") +} + +func TestUpdateContainerImage(t *testing.T) { + spec, _ := getPodSpec() + err := UpdateImage(spec, "gcr.io/foo/bar:baz") + assert.NilError(t, err) + // Verify update is successful or not + checkContainerImage(t, spec, "gcr.io/foo/bar:baz") + // Update spec with container image info + spec.Containers[0].Image = "docker.io/foo/bar:baz" + err = UpdateImage(spec, "query.io/foo/bar:baz") + assert.NilError(t, err) + // Verify that given image overrides the existing container image + checkContainerImage(t, spec, "query.io/foo/bar:baz") +} + +func checkContainerImage(t *testing.T, spec *corev1.PodSpec, image string) { + if got, want := spec.Containers[0].Image, image; got != want { + t.Errorf("Failed to update the container image: got=%s, want=%s", got, want) + } +} + +func TestUpdateContainerCommand(t *testing.T) { + spec, _ := getPodSpec() + err := UpdateContainerCommand(spec, "/app/start") + assert.NilError(t, err) + assert.DeepEqual(t, spec.Containers[0].Command, []string{"/app/start"}) + + err = UpdateContainerCommand(spec, "/app/latest") + assert.NilError(t, err) + assert.DeepEqual(t, spec.Containers[0].Command, []string{"/app/latest"}) +} + +func TestUpdateContainerArg(t *testing.T) { + spec, _ := getPodSpec() + err := UpdateContainerArg(spec, []string{"--myArg"}) + assert.NilError(t, err) + assert.DeepEqual(t, spec.Containers[0].Args, []string{"--myArg"}) + + err = UpdateContainerArg(spec, []string{"myArg1", "--myArg2"}) + assert.NilError(t, err) + assert.DeepEqual(t, spec.Containers[0].Args, []string{"myArg1", "--myArg2"}) +} + +func TestUpdateContainerPort(t *testing.T) { + spec, _ := getPodSpec() + for _, tc := range []struct { + name string + input string + isErr bool + expPort int32 + expName string + }{{ + name: "only port 8888", + input: "8888", + expPort: int32(8888), + }, { + name: "name and port h2c:8080", + input: "h2c:8080", + expPort: int32(8080), + expName: "h2c", + }, { + name: "error case - not correct format", + input: "h2c:800000000000000000", + isErr: true, + }, { + name: "error case - empty port", + input: "h2c:", + isErr: true, + }, { + name: "error case - wrong format", + input: "8080:h2c", + isErr: true, + }, { + name: "error case - multiple :", + input: "h2c:8080:proto", + isErr: true, + }, { + name: "empty name no error", + input: ":8888", + expPort: int32(8888), + }} { + t.Run(tc.name, func(t *testing.T) { + err := UpdateContainerPort(spec, tc.input) + if tc.isErr { + assert.Error(t, err, fmt.Sprintf(PortFormatErr, tc.input)) + } else { + assert.NilError(t, err) + assert.Equal(t, spec.Containers[0].Ports[0].ContainerPort, tc.expPort) + assert.Equal(t, spec.Containers[0].Ports[0].Name, tc.expName) + } + }) + } +} + +func TestUpdateUser(t *testing.T) { + spec, _ := getPodSpec() + err := UpdateUser(spec, int64(1001)) + assert.NilError(t, err) + + checkUserUpdate(t, spec, ptr.Int64(int64(1001))) + + spec.Containers[0].SecurityContext.RunAsUser = ptr.Int64(int64(1002)) + err = UpdateUser(spec, int64(1002)) + assert.NilError(t, err) + + checkUserUpdate(t, spec, ptr.Int64(int64(1002))) +} + +func checkUserUpdate(t *testing.T, spec *corev1.PodSpec, user *int64) { + assert.DeepEqual(t, spec.Containers[0].SecurityContext.RunAsUser, user) +} + +func TestUpdateServiceAccountName(t *testing.T) { + spec, _ := getPodSpec() + spec.ServiceAccountName = "" + + UpdateServiceAccountName(spec, "foo-bar") + assert.Equal(t, spec.ServiceAccountName, "foo-bar") + + UpdateServiceAccountName(spec, "") + assert.Equal(t, spec.ServiceAccountName, "") +} + +func TestUpdateImagePullSecrets(t *testing.T) { + spec, _ := getPodSpec() + spec.ImagePullSecrets = nil + + UpdateImagePullSecrets(spec, "quay") + assert.Equal(t, spec.ImagePullSecrets[0].Name, "quay") + + UpdateImagePullSecrets(spec, " ") + assert.Check(t, spec.ImagePullSecrets == nil) +} + +func TestUpdateEnvVarsModify(t *testing.T) { + spec, container := getPodSpec() + container.Env = []corev1.EnvVar{ + {Name: "a", Value: "foo"}} + env := map[string]string{ + "a": "fancy", + } + err := UpdateEnvVars(spec, env, []string{}) + assert.NilError(t, err) + + expected := map[string]string{ + "a": "fancy", + } + + found, err := util.EnvToMap(container.Env) + assert.NilError(t, err) + assert.DeepEqual(t, expected, found) +} + +func TestUpdateEnvVarsRemove(t *testing.T) { + spec, container := getPodSpec() + container.Env = []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + {Name: "b", Value: "bar"}, + } + remove := []string{"b"} + err := UpdateEnvVars(spec, map[string]string{}, remove) + assert.NilError(t, err) + + expected := []corev1.EnvVar{ + {Name: "a", Value: "foo"}, + } + + assert.DeepEqual(t, expected, container.Env) +} diff --git a/pkg/kn/flags/podspec_test.go b/pkg/kn/flags/podspec_test.go index 2248c3613d..f889114bd1 100644 --- a/pkg/kn/flags/podspec_test.go +++ b/pkg/kn/flags/podspec_test.go @@ -15,10 +15,14 @@ package flags import ( + "bytes" "testing" "github.com/spf13/cobra" "gotest.tools/assert" + corev1 "k8s.io/api/core/v1" + "knative.dev/client/pkg/util" + "knative.dev/pkg/ptr" ) func TestPodSpecFlags(t *testing.T) { @@ -49,3 +53,92 @@ func TestUniqueStringArg(t *testing.T) { assert.Equal(t, "test", a.String()) assert.Equal(t, "string", a.Type()) } + +func TestPodSpecResolve(t *testing.T) { + args := []string{"--image", "repo/user/imageID:tag", "--env", "b=c", + "--port", "8080", "--limit", "cpu=1000m", "--limit", "memory=1024Mi", + "--cmd", "/app/start", "--arg", "myArg1", "--service-account", "foo-bar-account", + "--mount", "/mount/path=volume-name", "--volume", "volume-name=cm:config-map-name", + "--env-from", "config-map:config-map-name", "--user", "1001"} + expectedPodSpec := corev1.PodSpec{ + Containers: []corev1.Container{ + { + Image: "repo/user/imageID:tag", + Command: []string{"/app/start"}, + Args: []string{"myArg1"}, + Ports: []corev1.ContainerPort{ + { + ContainerPort: 8080, + }, + }, + Env: []corev1.EnvVar{{Name: "b", Value: "c"}}, + EnvFrom: []corev1.EnvFromSource{ + { + ConfigMapRef: &corev1.ConfigMapEnvSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "config-map-name", + }, + }, + }, + }, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: parseQuantity("1000m"), + corev1.ResourceMemory: parseQuantity("1024Mi"), + }, + Requests: corev1.ResourceList{}, + }, + VolumeMounts: []corev1.VolumeMount{{Name: "volume-name", ReadOnly: true, MountPath: "/mount/path"}}, + SecurityContext: &corev1.SecurityContext{ + RunAsUser: ptr.Int64(int64(1001)), + }, + }, + }, + ServiceAccountName: "foo-bar-account", + Volumes: []corev1.Volume{ + { + Name: "volume-name", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "config-map-name", + }, + }, + }, + }, + }, + } + flags := &PodSpecFlags{} + testCmd := &cobra.Command{ + Use: "test", + Run: func(cmd *cobra.Command, args []string) { + podSpec := &corev1.PodSpec{Containers: []corev1.Container{{}}} + err := flags.ResolvePodSpec(podSpec, cmd) + assert.NilError(t, err, "PodSpec cannot be resolved.") + assert.DeepEqual(t, expectedPodSpec, *podSpec) + }, + } + testCmd.SetArgs(args) + flags.AddFlags(testCmd.Flags()) + testCmd.Execute() +} + +func TestPodSpecResolveReturnError(t *testing.T) { + outBuf := bytes.Buffer{} + flags := &PodSpecFlags{} + testCmd := &cobra.Command{ + Use: "test", + Run: func(cmd *cobra.Command, args []string) { + podSpec := &corev1.PodSpec{Containers: []corev1.Container{{}}} + flags.ResolvePodSpec(podSpec, cmd) + }, + } + testCmd.SetOut(&outBuf) + + args := []string{"--requests-cpu", "1000m"} + testCmd.SetArgs(args) + flags.AddFlags(testCmd.Flags()) + testCmd.Execute() + out := outBuf.String() + assert.Assert(t, util.ContainsAll(out, "WARNING", "deprecated")) +} diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 74fcb32b3f..04a36ed24d 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -15,22 +15,17 @@ package serving import ( - "crypto/sha1" "errors" "fmt" - "sort" "strconv" "strings" "time" - "unicode" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" "knative.dev/pkg/ptr" "knative.dev/serving/pkg/apis/autoscaling" servingv1 "knative.dev/serving/pkg/apis/serving/v1" - "knative.dev/client/pkg/util" + "knative.dev/client/pkg/kn/flags" ) // VolumeSourceType is a type standing for enumeration of ConfigMap and Secret @@ -56,128 +51,6 @@ func (vt VolumeSourceType) String() string { return names[vt] } -// UpdateEnvVars gives 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, then sort by env key name. -func UpdateEnvVars(template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - updated := updateEnvVarsFromMap(container.Env, toUpdate) - updated = removeEnvVars(updated, toRemove) - // Sort by env key name - sort.SliceStable(updated, func(i, j int) bool { - return updated[i].Name < updated[j].Name - }) - container.Env = updated - - return nil -} - -// UpdateEnvFrom updates envFrom -func UpdateEnvFrom(template *servingv1.RevisionTemplateSpec, toUpdate []string, toRemove []string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - envFrom, err := updateEnvFrom(container.EnvFrom, toUpdate) - if err != nil { - return err - } - container.EnvFrom, err = removeEnvFrom(envFrom, toRemove) - return err -} - -func reviseVolumeInfoAndMountsToUpdate(volumes []corev1.Volume, mountsToUpdate *util.OrderedMap, - volumesToUpdate *util.OrderedMap) (*util.OrderedMap, *util.OrderedMap, error) { - volumeSourceInfoByName := util.NewOrderedMap() //make(map[string]*volumeSourceInfo) - mountsToUpdateRevised := util.NewOrderedMap() //make(map[string]string) - - it := mountsToUpdate.Iterator() - for path, value, ok := it.NextString(); ok; path, value, ok = it.NextString() { - // slices[0] -> config-map, cm, secret, sc, volume, or vo - // slices[1] -> secret, config-map, or volume name - slices := strings.SplitN(value, ":", 2) - if len(slices) == 1 { - mountsToUpdateRevised.Set(path, slices[0]) - } else { - switch volumeType := slices[0]; volumeType { - case "config-map", "cm": - generatedName := GenerateVolumeName(path) - volumeSourceInfoByName.Set(generatedName, &volumeSourceInfo{ - volumeSourceType: ConfigMapVolumeSourceType, - volumeSourceName: slices[1], - }) - mountsToUpdateRevised.Set(path, generatedName) - case "secret", "sc": - generatedName := GenerateVolumeName(path) - volumeSourceInfoByName.Set(generatedName, &volumeSourceInfo{ - volumeSourceType: SecretVolumeSourceType, - volumeSourceName: slices[1], - }) - mountsToUpdateRevised.Set(path, generatedName) - - default: - return nil, nil, fmt.Errorf("unsupported volume type \"%q\"; supported volume types are \"config-map or cm\", \"secret or sc\", and \"volume or vo\"", slices[0]) - } - } - } - - it = volumesToUpdate.Iterator() - for name, value, ok := it.NextString(); ok; name, value, ok = it.NextString() { - info, err := newVolumeSourceInfoWithSpecString(value) - if err != nil { - return nil, nil, err - } - volumeSourceInfoByName.Set(name, info) - } - - return volumeSourceInfoByName, mountsToUpdateRevised, nil -} - -func reviseVolumesToRemove(volumeMounts []corev1.VolumeMount, volumesToRemove []string, mountsToRemove []string) []string { - for _, pathToRemove := range mountsToRemove { - for _, volumeMount := range volumeMounts { - if volumeMount.MountPath == pathToRemove && volumeMount.Name == GenerateVolumeName(pathToRemove) { - volumesToRemove = append(volumesToRemove, volumeMount.Name) - } - } - } - return volumesToRemove -} - -// UpdateVolumeMountsAndVolumes updates the configuration for volume mounts and volumes. -func UpdateVolumeMountsAndVolumes(template *servingv1.RevisionTemplateSpec, - mountsToUpdate *util.OrderedMap, mountsToRemove []string, volumesToUpdate *util.OrderedMap, volumesToRemove []string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - - volumeSourceInfoByName, mountsToUpdate, err := reviseVolumeInfoAndMountsToUpdate(template.Spec.Volumes, mountsToUpdate, volumesToUpdate) - if err != nil { - return err - } - - volumes, err := updateVolumesFromMap(template.Spec.Volumes, volumeSourceInfoByName) - if err != nil { - return err - } - - volumeMounts, err := updateVolumeMountsFromMap(container.VolumeMounts, mountsToUpdate, volumes) - if err != nil { - return err - } - - volumesToRemove = reviseVolumesToRemove(container.VolumeMounts, volumesToRemove, mountsToRemove) - - container.VolumeMounts = removeVolumeMounts(volumeMounts, mountsToRemove) - template.Spec.Volumes, err = removeVolumes(volumes, volumesToRemove, container.VolumeMounts) - - return err -} - // UpdateMinScale updates min scale annotation func UpdateMinScale(template *servingv1.RevisionTemplateSpec, min int) error { return UpdateRevisionTemplateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min)) @@ -216,31 +89,6 @@ func UpdateConcurrencyLimit(template *servingv1.RevisionTemplateSpec, limit int6 return nil } -// EnvToMap is an utility function to translate between the API list form of env vars, and the -// more convenient map form. -func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { - result := map[string]string{} - for _, envVar := range vars { - _, present := result[envVar.Name] - if present { - return nil, fmt.Errorf("env var name present more than once: %v", envVar.Name) - } - result[envVar.Name] = envVar.Value - } - return result, nil -} - -// UpdateImage a given image -func UpdateImage(template *servingv1.RevisionTemplateSpec, image string) error { - // When not setting the image to a digest, add the user image annotation. - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - container.Image = image - return nil -} - // UnsetUserImageAnnot removes the user image annotation func UnsetUserImageAnnot(template *servingv1.RevisionTemplateSpec) { delete(template.Annotations, UserImageAnnotationKey) @@ -284,134 +132,11 @@ func FreezeImageToDigest(template *servingv1.RevisionTemplateSpec, baseRevision } if baseRevision.Status.DeprecatedImageDigest != "" { - return UpdateImage(template, baseRevision.Status.DeprecatedImageDigest) + return flags.UpdateImage(&template.Spec.PodSpec, baseRevision.Status.DeprecatedImageDigest) } return nil } -// UpdateContainerCommand updates container with a given argument -func UpdateContainerCommand(template *servingv1.RevisionTemplateSpec, command string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - container.Command = []string{command} - return nil -} - -// UpdateContainerArg updates container with a given argument -func UpdateContainerArg(template *servingv1.RevisionTemplateSpec, arg []string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - container.Args = arg - return nil -} - -// UpdateContainerPort updates container with a given name:port -func UpdateContainerPort(template *servingv1.RevisionTemplateSpec, port string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - - var containerPort int64 - var name string - - elements := strings.SplitN(port, ":", 2) - if len(elements) == 2 { - name = elements[0] - containerPort, err = strconv.ParseInt(elements[1], 10, 32) - if err != nil { - return fmt.Errorf(PortFormatErr, port) - } - } else { - name = "" - containerPort, err = strconv.ParseInt(elements[0], 10, 32) - if err != nil { - return fmt.Errorf(PortFormatErr, port) - } - } - - container.Ports = []corev1.ContainerPort{{ - ContainerPort: int32(containerPort), - Name: name, - }} - return nil -} - -// UpdateRunAsUser updates container with a given user id -func UpdateUser(template *servingv1.RevisionTemplateSpec, user int64) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - container.SecurityContext = &corev1.SecurityContext{ - RunAsUser: &user, - } - return nil -} - -// UpdateResources updates container resources for given revision template -func UpdateResources(template *servingv1.RevisionTemplateSpec, resources corev1.ResourceRequirements, requestsToRemove, limitsToRemove []string) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - - if container.Resources.Requests == nil { - container.Resources.Requests = corev1.ResourceList{} - } - - for k, v := range resources.Requests { - container.Resources.Requests[k] = v - } - - for _, reqToRemove := range requestsToRemove { - delete(container.Resources.Requests, corev1.ResourceName(reqToRemove)) - } - - if container.Resources.Limits == nil { - container.Resources.Limits = corev1.ResourceList{} - } - - for k, v := range resources.Limits { - container.Resources.Limits[k] = v - } - - for _, limToRemove := range limitsToRemove { - delete(container.Resources.Limits, corev1.ResourceName(limToRemove)) - } - - return nil -} - -// UpdateResourcesDeprecated updates resources as requested -func UpdateResourcesDeprecated(template *servingv1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error { - container, err := ContainerOfRevisionTemplate(template) - if err != nil { - return err - } - if container.Resources.Requests == nil { - container.Resources.Requests = corev1.ResourceList{} - } - - for k, v := range requestsResourceList { - container.Resources.Requests[k] = v - } - - if container.Resources.Limits == nil { - container.Resources.Limits = corev1.ResourceList{} - } - - for k, v := range limitsResourceList { - container.Resources.Limits[k] = v - } - - return nil -} - // UpdateLabels updates the labels by adding items from `add` then removing any items from `remove` func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []string) map[string]string { if labelsMap == nil { @@ -454,66 +179,6 @@ func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, return UpdateRevisionTemplateAnnotations(template, map[string]string{annotation: value}, []string{}) } -// UpdateServiceAccountName updates the service account name used for the corresponding knative service -func UpdateServiceAccountName(template *servingv1.RevisionTemplateSpec, serviceAccountName string) error { - serviceAccountName = strings.TrimSpace(serviceAccountName) - template.Spec.ServiceAccountName = serviceAccountName - return nil -} - -// UpdateImagePullSecrets updates the image pull secrets used for the corresponding knative service -func UpdateImagePullSecrets(template *servingv1.RevisionTemplateSpec, pullsecrets string) { - pullsecrets = strings.TrimSpace(pullsecrets) - if pullsecrets == "" { - template.Spec.ImagePullSecrets = nil - } else { - template.Spec.ImagePullSecrets = []corev1.LocalObjectReference{{ - Name: pullsecrets, - }} - } -} - -// GenerateVolumeName generates a volume name with respect to a given path string. -// Current implementation basically sanitizes the path string by replacing "/" with "-" -// To reduce any chance of duplication, a checksum part generated from the path string is appended to the sanitized string. -// The volume name must follow the DNS label standard as defined in RFC 1123. This means the name must: -// - contain at most 63 characters -// - contain only lowercase alphanumeric characters or '-' -// - start with an alphanumeric character -// - end with an alphanumeric character -func GenerateVolumeName(path string) string { - builder := &strings.Builder{} - for idx, r := range path { - switch { - case unicode.IsLower(r) || unicode.IsDigit(r) || r == '-': - builder.WriteRune(r) - case unicode.IsUpper(r): - builder.WriteRune(unicode.ToLower(r)) - case r == '/': - if idx != 0 { - builder.WriteRune('-') - } - default: - builder.WriteRune('-') - } - } - - vname := appendCheckSum(builder.String(), path) - - // the name must start with an alphanumeric character - if !unicode.IsLetter(rune(vname[0])) && !unicode.IsNumber(rune(vname[0])) { - vname = fmt.Sprintf("k-%s", vname) - } - - // contain at most 63 characters - if len(vname) > 63 { - // must end with an alphanumeric character - vname = fmt.Sprintf("%s-n", vname[0:61]) - } - - return vname -} - // ======================================================================================= func updateAnnotations(annotations map[string]string, toUpdate map[string]string, toRemove []string) error { @@ -525,286 +190,3 @@ func updateAnnotations(annotations map[string]string, toUpdate map[string]string } return nil } - -func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { - set := sets.NewString() - for i := range env { - envVar := &env[i] - if val, ok := toUpdate[envVar.Name]; ok { - envVar.Value = val - set.Insert(envVar.Name) - } - } - for name, val := range toUpdate { - if !set.Has(name) { - env = append(env, corev1.EnvVar{Name: name, Value: val}) - } - } - return env -} - -func removeEnvVars(env []corev1.EnvVar, toRemove []string) []corev1.EnvVar { - for _, name := range toRemove { - for i, envVar := range env { - if envVar.Name == name { - env = append(env[:i], env[i+1:]...) - break - } - } - } - return env -} - -func updateEnvFrom(envFromSources []corev1.EnvFromSource, toUpdate []string) ([]corev1.EnvFromSource, error) { - existingNameSet := make(map[string]bool) - - for _, envSrc := range envFromSources { - if canonicalName, err := getCanonicalNameFromEnvFromSource(&envSrc); err == nil { - existingNameSet[canonicalName] = true - } - } - - for _, s := range toUpdate { - info, err := newVolumeSourceInfoWithSpecString(s) - if err != nil { - return nil, err - } - - if _, ok := existingNameSet[info.getCanonicalName()]; !ok { - envFromSources = append(envFromSources, *info.createEnvFromSource()) - } - } - - return envFromSources, nil -} - -func removeEnvFrom(envFromSources []corev1.EnvFromSource, toRemove []string) ([]corev1.EnvFromSource, error) { - for _, name := range toRemove { - info, err := newVolumeSourceInfoWithSpecString(name) - if err != nil { - return nil, err - } - for i, envSrc := range envFromSources { - if (info.volumeSourceType == ConfigMapVolumeSourceType && envSrc.ConfigMapRef != nil && info.volumeSourceName == envSrc.ConfigMapRef.Name) || - (info.volumeSourceType == SecretVolumeSourceType && envSrc.SecretRef != nil && info.volumeSourceName == envSrc.SecretRef.Name) { - envFromSources = append(envFromSources[:i], envFromSources[i+1:]...) - break - } - } - } - - if len(envFromSources) == 0 { - envFromSources = nil - } - - return envFromSources, nil -} - -func updateVolume(volume *corev1.Volume, info *volumeSourceInfo) error { - switch info.volumeSourceType { - case ConfigMapVolumeSourceType: - volume.Secret = nil - volume.ConfigMap = &corev1.ConfigMapVolumeSource{LocalObjectReference: corev1.LocalObjectReference{Name: info.volumeSourceName}} - case SecretVolumeSourceType: - volume.ConfigMap = nil - volume.Secret = &corev1.SecretVolumeSource{SecretName: info.volumeSourceName} - default: - return fmt.Errorf("Invalid VolumeSourceType") - } - return nil -} - -// updateVolumeMountsFromMap updates or adds volume mounts. If a given name of a volume is not existing, it returns an error -func updateVolumeMountsFromMap(volumeMounts []corev1.VolumeMount, toUpdate *util.OrderedMap, volumes []corev1.Volume) ([]corev1.VolumeMount, error) { - set := make(map[string]bool) - - for i := range volumeMounts { - volumeMount := &volumeMounts[i] - name, present := toUpdate.GetString(volumeMount.MountPath) - - if present { - if !existsVolumeNameInVolumes(name, volumes) { - return nil, fmt.Errorf("There is no volume matched with %q", name) - } - - volumeMount.ReadOnly = true - volumeMount.Name = name - set[volumeMount.MountPath] = true - } - } - - it := toUpdate.Iterator() - for mountPath, name, ok := it.NextString(); ok; mountPath, name, ok = it.NextString() { - if !set[mountPath] { - volumeMounts = append(volumeMounts, corev1.VolumeMount{ - Name: name, - ReadOnly: true, - MountPath: mountPath, - }) - } - } - - return volumeMounts, nil -} - -func removeVolumeMounts(volumeMounts []corev1.VolumeMount, toRemove []string) []corev1.VolumeMount { - for _, mountPath := range toRemove { - for i, volumeMount := range volumeMounts { - if volumeMount.MountPath == mountPath { - volumeMounts = append(volumeMounts[:i], volumeMounts[i+1:]...) - break - } - } - } - - if len(volumeMounts) == 0 { - return nil - } - - return volumeMounts -} - -// updateVolumesFromMap updates or adds volumes regardless whether the volume is used or not -func updateVolumesFromMap(volumes []corev1.Volume, toUpdate *util.OrderedMap) ([]corev1.Volume, error) { - set := make(map[string]bool) - - for i := range volumes { - volume := &volumes[i] - info, present := toUpdate.Get(volume.Name) - if present { - err := updateVolume(volume, info.(*volumeSourceInfo)) - if err != nil { - return nil, err - } - set[volume.Name] = true - } - } - - it := toUpdate.Iterator() - for name, info, ok := it.Next(); ok; name, info, ok = it.Next() { - if !set[name] { - volumes = append(volumes, corev1.Volume{Name: name}) - updateVolume(&volumes[len(volumes)-1], info.(*volumeSourceInfo)) - } - } - - return volumes, nil -} - -// removeVolumes removes volumes. If there is a volume mount referencing the volume, it causes an error -func removeVolumes(volumes []corev1.Volume, toRemove []string, volumeMounts []corev1.VolumeMount) ([]corev1.Volume, error) { - for _, name := range toRemove { - for i, volume := range volumes { - if volume.Name == name { - if existsVolumeNameInVolumeMounts(name, volumeMounts) { - return nil, fmt.Errorf("The volume %q cannot be removed because it is mounted", name) - } - volumes = append(volumes[:i], volumes[i+1:]...) - break - } - } - } - - if len(volumes) == 0 { - return nil, nil - } - - return volumes, nil -} - -// ======================================================================================= - -type volumeSourceInfo struct { - volumeSourceType VolumeSourceType - volumeSourceName string -} - -func newVolumeSourceInfoWithSpecString(spec string) (*volumeSourceInfo, error) { - slices := strings.SplitN(spec, ":", 2) - if len(slices) != 2 { - return nil, fmt.Errorf("argument requires a value that contains the : character; got %q", spec) - } - - var volumeSourceType VolumeSourceType - - typeString := strings.TrimSpace(slices[0]) - volumeSourceName := strings.TrimSpace(slices[1]) - - switch typeString { - case "config-map", "cm": - volumeSourceType = ConfigMapVolumeSourceType - case "secret", "sc": - volumeSourceType = SecretVolumeSourceType - default: - return nil, fmt.Errorf("unsupported volume source type \"%q\"; supported volume source types are \"config-map\" and \"secret\"", slices[0]) - } - - if len(volumeSourceName) == 0 { - return nil, fmt.Errorf("the name of %s cannot be an empty string", volumeSourceType) - } - - return &volumeSourceInfo{ - volumeSourceType: volumeSourceType, - volumeSourceName: volumeSourceName, - }, nil -} - -func (vol *volumeSourceInfo) getCanonicalName() string { - return fmt.Sprintf("%s:%s", vol.volumeSourceType, vol.volumeSourceName) -} - -func getCanonicalNameFromEnvFromSource(envSrc *corev1.EnvFromSource) (string, error) { - if envSrc.ConfigMapRef != nil { - return fmt.Sprintf("%s:%s", ConfigMapVolumeSourceType, envSrc.ConfigMapRef.Name), nil - } - if envSrc.SecretRef != nil { - return fmt.Sprintf("%s:%s", SecretVolumeSourceType, envSrc.SecretRef.Name), nil - } - - return "", fmt.Errorf("there is no ConfigMapRef or SecretRef in a EnvFromSource") -} - -func (vol *volumeSourceInfo) createEnvFromSource() *corev1.EnvFromSource { - switch vol.volumeSourceType { - case ConfigMapVolumeSourceType: - return &corev1.EnvFromSource{ - ConfigMapRef: &corev1.ConfigMapEnvSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: vol.volumeSourceName, - }}} - case SecretVolumeSourceType: - return &corev1.EnvFromSource{ - SecretRef: &corev1.SecretEnvSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: vol.volumeSourceName, - }}} - } - - return nil -} - -// ======================================================================================= - -func existsVolumeNameInVolumes(volumeName string, volumes []corev1.Volume) bool { - for _, volume := range volumes { - if volume.Name == volumeName { - return true - } - } - return false -} - -func existsVolumeNameInVolumeMounts(volumeName string, volumeMounts []corev1.VolumeMount) bool { - for _, volumeMount := range volumeMounts { - if volumeMount.Name == volumeName { - return true - } - } - return false -} - -func appendCheckSum(sanitizedString, path string) string { - checkSum := sha1.Sum([]byte(path)) - shortCheckSum := checkSum[0:4] - return fmt.Sprintf("%s-%x", sanitizedString, shortCheckSum) -} diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index eed1b75cc0..18fe53b8eb 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -15,7 +15,6 @@ package serving import ( - "fmt" "reflect" "strconv" "testing" @@ -73,19 +72,6 @@ func TestUpdateInvalidAutoscalingAnnotations(t *testing.T) { } } -func TestUpdateEnvVarsNew(t *testing.T) { - template, container := getRevisionTemplate() - env := []corev1.EnvVar{ - {Name: "a", Value: "foo"}, - {Name: "b", Value: "bar"}, - } - found, err := EnvToMap(env) - assert.NilError(t, err) - err = UpdateEnvVars(template, found, []string{}) - assert.NilError(t, err) - assert.DeepEqual(t, env, container.Env) -} - type userImageAnnotCase struct { image string annot string @@ -132,42 +118,6 @@ func TestFreezeImageToDigest(t *testing.T) { assert.Equal(t, container.Image, "gcr.io/foo/bar@sha256:deadbeef") } -func TestUpdateEnvVarsModify(t *testing.T) { - revision, container := getRevisionTemplate() - container.Env = []corev1.EnvVar{ - {Name: "a", Value: "foo"}} - env := map[string]string{ - "a": "fancy", - } - err := UpdateEnvVars(revision, env, []string{}) - assert.NilError(t, err) - - expected := map[string]string{ - "a": "fancy", - } - - found, err := EnvToMap(container.Env) - assert.NilError(t, err) - assert.DeepEqual(t, expected, found) -} - -func TestUpdateEnvVarsRemove(t *testing.T) { - revision, container := getRevisionTemplate() - container.Env = []corev1.EnvVar{ - {Name: "a", Value: "foo"}, - {Name: "b", Value: "bar"}, - } - remove := []string{"b"} - err := UpdateEnvVars(revision, map[string]string{}, remove) - assert.NilError(t, err) - - expected := []corev1.EnvVar{ - {Name: "a", Value: "foo"}, - } - - assert.DeepEqual(t, expected, container.Env) -} - func TestUpdateMinScale(t *testing.T) { template, _ := getRevisionTemplate() err := UpdateMinScale(template, 10) @@ -223,126 +173,29 @@ func TestUpdateConcurrencyLimit(t *testing.T) { assert.ErrorContains(t, err, "invalid") } -func TestUpdateContainerImage(t *testing.T) { - template, _ := getRevisionTemplate() - err := UpdateImage(template, "gcr.io/foo/bar:baz") - assert.NilError(t, err) - // Verify update is successful or not - checkContainerImage(t, template, "gcr.io/foo/bar:baz") - // Update template with container image info - template.Spec.Containers[0].Image = "docker.io/foo/bar:baz" - err = UpdateImage(template, "query.io/foo/bar:baz") - assert.NilError(t, err) - // Verify that given image overrides the existing container image - checkContainerImage(t, template, "query.io/foo/bar:baz") -} - -func checkContainerImage(t *testing.T, template *servingv1.RevisionTemplateSpec, image string) { - if got, want := template.Spec.Containers[0].Image, image; got != want { - t.Errorf("Failed to update the container image: got=%s, want=%s", got, want) - } -} - -func TestUpdateContainerCommand(t *testing.T) { - template, _ := getRevisionTemplate() - err := UpdateContainerCommand(template, "/app/start") - assert.NilError(t, err) - assert.DeepEqual(t, template.Spec.Containers[0].Command, []string{"/app/start"}) - - err = UpdateContainerCommand(template, "/app/latest") - assert.NilError(t, err) - assert.DeepEqual(t, template.Spec.Containers[0].Command, []string{"/app/latest"}) -} - -func TestUpdateContainerArg(t *testing.T) { - template, _ := getRevisionTemplate() - err := UpdateContainerArg(template, []string{"--myArg"}) - assert.NilError(t, err) - assert.DeepEqual(t, template.Spec.Containers[0].Args, []string{"--myArg"}) - - err = UpdateContainerArg(template, []string{"myArg1", "--myArg2"}) - assert.NilError(t, err) - assert.DeepEqual(t, template.Spec.Containers[0].Args, []string{"myArg1", "--myArg2"}) -} - -func TestUpdateContainerPort(t *testing.T) { - template, _ := getRevisionTemplate() - for _, tc := range []struct { - name string - input string - isErr bool - expPort int32 - expName string - }{{ - name: "only port 8888", - input: "8888", - expPort: int32(8888), - }, { - name: "name and port h2c:8080", - input: "h2c:8080", - expPort: int32(8080), - expName: "h2c", - }, { - name: "error case - not correct format", - input: "h2c:800000000000000000", - isErr: true, - }, { - name: "error case - empty port", - input: "h2c:", - isErr: true, - }, { - name: "error case - wrong format", - input: "8080:h2c", - isErr: true, - }, { - name: "error case - multiple :", - input: "h2c:8080:proto", - isErr: true, - }, { - name: "empty name no error", - input: ":8888", - expPort: int32(8888), - }} { - t.Run(tc.name, func(t *testing.T) { - err := UpdateContainerPort(template, tc.input) - if tc.isErr { - assert.Error(t, err, fmt.Sprintf(PortFormatErr, tc.input)) - } else { - assert.NilError(t, err) - assert.Equal(t, template.Spec.Containers[0].Ports[0].ContainerPort, tc.expPort) - assert.Equal(t, template.Spec.Containers[0].Ports[0].Name, tc.expName) - } - }) - } -} - -func checkUserUpdate(t *testing.T, template *servingv1.RevisionTemplateSpec, user *int64) { - assert.DeepEqual(t, template.Spec.Containers[0].SecurityContext.RunAsUser, user) -} - -func TestUpdateEnvVarsBoth(t *testing.T) { - template, container := getRevisionTemplate() - container.Env = []corev1.EnvVar{ - {Name: "a", Value: "foo"}, - {Name: "c", Value: "caroline"}, - {Name: "d", Value: "byebye"}, - } - env := map[string]string{ - "a": "fancy", - "b": "boo", - } - remove := []string{"d"} - err := UpdateEnvVars(template, env, remove) - assert.NilError(t, err) - - expected := []corev1.EnvVar{ - {Name: "a", Value: "fancy"}, - {Name: "b", Value: "boo"}, - {Name: "c", Value: "caroline"}, - } - - assert.DeepEqual(t, expected, container.Env) -} +// func TestUpdateEnvVarsBoth(t *testing.T) { +// template, container := getRevisionTemplate() +// container.Env = []corev1.EnvVar{ +// {Name: "a", Value: "foo"}, +// {Name: "c", Value: "caroline"}, +// {Name: "d", Value: "byebye"}, +// } +// env := map[string]string{ +// "a": "fancy", +// "b": "boo", +// } +// remove := []string{"d"} +// err := UpdateEnvVars(template, env, remove) +// assert.NilError(t, err) + +// expected := []corev1.EnvVar{ +// {Name: "a", Value: "fancy"}, +// {Name: "b", Value: "boo"}, +// {Name: "c", Value: "caroline"}, +// } + +// assert.DeepEqual(t, expected, container.Env) +// } func TestUpdateLabelsNew(t *testing.T) { service, template, _ := getService() @@ -427,158 +280,6 @@ func TestUpdateLabelsRemoveExisting(t *testing.T) { assert.DeepEqual(t, expected, actual) } -func TestUpdateEnvFrom(t *testing.T) { - template, container := getRevisionTemplate() - container.EnvFrom = append(container.EnvFrom, - corev1.EnvFromSource{ - ConfigMapRef: &corev1.ConfigMapEnvSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "config-map-existing-name", - }}}, - corev1.EnvFromSource{ - SecretRef: &corev1.SecretEnvSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "secret-existing-name", - }}}, - ) - UpdateEnvFrom(template, - []string{"config-map:config-map-new-name-1", "secret:secret-new-name-1"}, - []string{"config-map:config-map-existing-name", "secret:secret-existing-name"}) - assert.Equal(t, len(container.EnvFrom), 2) - assert.Equal(t, container.EnvFrom[0].ConfigMapRef.Name, "config-map-new-name-1") - assert.Equal(t, container.EnvFrom[1].SecretRef.Name, "secret-new-name-1") -} - -func TestUpdateVolumeMountsAndVolumes(t *testing.T) { - template, container := getRevisionTemplate() - template.Spec.Volumes = append(template.Spec.Volumes, - corev1.Volume{ - Name: "existing-config-map-volume-name-1", - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "existing-config-map-1", - }}}}, - corev1.Volume{ - Name: "existing-config-map-volume-name-2", - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: "existing-config-map-2", - }}}}, - corev1.Volume{ - Name: "existing-secret-volume-name-1", - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: "existing-secret-1", - }}}, - corev1.Volume{ - Name: "existing-secret-volume-name-2", - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: "existing-secret-2", - }}}) - - container.VolumeMounts = append(container.VolumeMounts, - corev1.VolumeMount{ - Name: "existing-config-map-volume-name-1", - ReadOnly: true, - MountPath: "/existing-config-map-1/mount/path", - }, - corev1.VolumeMount{ - Name: "existing-config-map-volume-name-2", - ReadOnly: true, - MountPath: "/existing-config-map-2/mount/path", - }, - corev1.VolumeMount{ - Name: "existing-secret-volume-name-1", - ReadOnly: true, - MountPath: "/existing-secret-1/mount/path", - }, - corev1.VolumeMount{ - Name: "existing-secret-volume-name-2", - ReadOnly: true, - MountPath: "/existing-secret-2/mount/path", - }, - ) - - err := UpdateVolumeMountsAndVolumes(template, - util.NewOrderedMapWithKVStrings([][]string{{"/new-config-map/mount/path", "new-config-map-volume-name"}}), - []string{}, - util.NewOrderedMapWithKVStrings([][]string{{"new-config-map-volume-name", "config-map:new-config-map"}}), - []string{}) - assert.NilError(t, err) - - err = UpdateVolumeMountsAndVolumes(template, - util.NewOrderedMapWithKVStrings([][]string{{"/updated-config-map/mount/path", "existing-config-map-volume-name-2"}}), - []string{}, - util.NewOrderedMapWithKVStrings([][]string{{"existing-config-map-volume-name-2", "config-map:updated-config-map"}}), - []string{}) - assert.NilError(t, err) - - err = UpdateVolumeMountsAndVolumes(template, - util.NewOrderedMapWithKVStrings([][]string{{"/new-secret/mount/path", "new-secret-volume-name"}}), - []string{}, - util.NewOrderedMapWithKVStrings([][]string{{"new-secret-volume-name", "secret:new-secret"}}), - []string{}) - assert.NilError(t, err) - - err = UpdateVolumeMountsAndVolumes(template, - util.NewOrderedMapWithKVStrings([][]string{{"/updated-secret/mount/path", "existing-secret-volume-name-2"}}), - []string{"/existing-config-map-1/mount/path", - "/existing-secret-1/mount/path"}, - util.NewOrderedMapWithKVStrings([][]string{{"existing-secret-volume-name-2", "secret:updated-secret"}}), - []string{"existing-config-map-volume-name-1", - "existing-secret-volume-name-1"}) - assert.NilError(t, err) - - assert.Equal(t, len(template.Spec.Volumes), 4) - assert.Equal(t, len(container.VolumeMounts), 6) - assert.Equal(t, template.Spec.Volumes[0].Name, "existing-config-map-volume-name-2") - assert.Equal(t, template.Spec.Volumes[0].ConfigMap.Name, "updated-config-map") - assert.Equal(t, template.Spec.Volumes[1].Name, "existing-secret-volume-name-2") - assert.Equal(t, template.Spec.Volumes[1].Secret.SecretName, "updated-secret") - assert.Equal(t, template.Spec.Volumes[2].Name, "new-config-map-volume-name") - assert.Equal(t, template.Spec.Volumes[2].ConfigMap.Name, "new-config-map") - assert.Equal(t, template.Spec.Volumes[3].Name, "new-secret-volume-name") - assert.Equal(t, template.Spec.Volumes[3].Secret.SecretName, "new-secret") - - assert.Equal(t, container.VolumeMounts[0].Name, "existing-config-map-volume-name-2") - assert.Equal(t, container.VolumeMounts[0].MountPath, "/existing-config-map-2/mount/path") - assert.Equal(t, container.VolumeMounts[1].Name, "existing-secret-volume-name-2") - assert.Equal(t, container.VolumeMounts[1].MountPath, "/existing-secret-2/mount/path") - assert.Equal(t, container.VolumeMounts[2].Name, "new-config-map-volume-name") - assert.Equal(t, container.VolumeMounts[2].MountPath, "/new-config-map/mount/path") - assert.Equal(t, container.VolumeMounts[3].Name, "existing-config-map-volume-name-2") - assert.Equal(t, container.VolumeMounts[3].MountPath, "/updated-config-map/mount/path") - assert.Equal(t, container.VolumeMounts[4].Name, "new-secret-volume-name") - assert.Equal(t, container.VolumeMounts[4].MountPath, "/new-secret/mount/path") - assert.Equal(t, container.VolumeMounts[5].Name, "existing-secret-volume-name-2") - assert.Equal(t, container.VolumeMounts[5].MountPath, "/updated-secret/mount/path") -} - -func TestUpdateServiceAccountName(t *testing.T) { - template, _ := getRevisionTemplate() - template.Spec.ServiceAccountName = "" - - UpdateServiceAccountName(template, "foo-bar") - assert.Equal(t, template.Spec.ServiceAccountName, "foo-bar") - - UpdateServiceAccountName(template, "") - assert.Equal(t, template.Spec.ServiceAccountName, "") -} - -func TestUpdateImagePullSecrets(t *testing.T) { - template, _ := getRevisionTemplate() - template.Spec.ImagePullSecrets = nil - - UpdateImagePullSecrets(template, "quay") - assert.Equal(t, template.Spec.ImagePullSecrets[0].Name, "quay") - - UpdateImagePullSecrets(template, " ") - assert.Check(t, template.Spec.ImagePullSecrets == nil) -} - func TestUpdateRevisionTemplateAnnotationsNew(t *testing.T) { _, template, _ := getService() @@ -681,51 +382,6 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) { assert.DeepEqual(t, expected, actual) } -func TestGenerateVolumeName(t *testing.T) { - actual := []string{ - "Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ", - "/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/", - "", - "/", - "/path.mypath/", - "/.path.mypath", - } - - expected := []string{ - "ab12---------------------------------xz", - "ab12---------------------------------xz-", - "k-", - "k-", - "path-mypath-", - "k--path-mypath", - } - - for i := range actual { - actualName := GenerateVolumeName(actual[i]) - expectedName := appendCheckSum(expected[i], actual[i]) - assert.Equal(t, actualName, expectedName) - } - - // 63 char limit case, no need to append the checksum in expected string - expName_63 := "k---ab12---------------------------------xz-ab12--------------n" - assert.Equal(t, len(expName_63), 63) - assert.Equal(t, GenerateVolumeName("/./Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/"), expName_63) -} - -func TestUpdateUser(t *testing.T) { - template, _ := getRevisionTemplate() - err := UpdateUser(template, int64(1001)) - assert.NilError(t, err) - - checkUserUpdate(t, template, ptr.Int64(int64(1001))) - - template.Spec.Containers[0].SecurityContext.RunAsUser = ptr.Int64(int64(1002)) - err = UpdateUser(template, int64(1002)) - assert.NilError(t, err) - - checkUserUpdate(t, template, ptr.Int64(int64(1002))) -} - // // ========================================================================================================= diff --git a/pkg/util/corev1_helper.go b/pkg/util/corev1_helper.go new file mode 100644 index 0000000000..9c4cca36d7 --- /dev/null +++ b/pkg/util/corev1_helper.go @@ -0,0 +1,87 @@ +/* +Copyright 2020 The Knative Authors + +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 util + +import ( + "crypto/sha1" + "fmt" + "strings" + "unicode" + + corev1 "k8s.io/api/core/v1" +) + +// EnvToMap is an utility function to translate between the API list form of env vars, and the +// more convenient map form. +func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) { + result := map[string]string{} + for _, envVar := range vars { + _, present := result[envVar.Name] + if present { + return nil, fmt.Errorf("env var name present more than once: %v", envVar.Name) + } + result[envVar.Name] = envVar.Value + } + return result, nil +} + +// GenerateVolumeName generates a volume name with respect to a given path string. +// Current implementation basically sanitizes the path string by replacing "/" with "-" +// To reduce any chance of duplication, a checksum part generated from the path string is appended to the sanitized string. +// The volume name must follow the DNS label standard as defined in RFC 1123. This means the name must: +// - contain at most 63 characters +// - contain only lowercase alphanumeric characters or '-' +// - start with an alphanumeric character +// - end with an alphanumeric character +func GenerateVolumeName(path string) string { + builder := &strings.Builder{} + for idx, r := range path { + switch { + case unicode.IsLower(r) || unicode.IsDigit(r) || r == '-': + builder.WriteRune(r) + case unicode.IsUpper(r): + builder.WriteRune(unicode.ToLower(r)) + case r == '/': + if idx != 0 { + builder.WriteRune('-') + } + default: + builder.WriteRune('-') + } + } + + vname := appendCheckSum(builder.String(), path) + + // the name must start with an alphanumeric character + if !unicode.IsLetter(rune(vname[0])) && !unicode.IsNumber(rune(vname[0])) { + vname = fmt.Sprintf("k-%s", vname) + } + + // contain at most 63 characters + if len(vname) > 63 { + // must end with an alphanumeric character + vname = fmt.Sprintf("%s-n", vname[0:61]) + } + + return vname +} + +func appendCheckSum(sanitizedString, path string) string { + checkSum := sha1.Sum([]byte(path)) + shortCheckSum := checkSum[0:4] + return fmt.Sprintf("%s-%x", sanitizedString, shortCheckSum) +} diff --git a/pkg/util/corev1_helper_test.go b/pkg/util/corev1_helper_test.go new file mode 100644 index 0000000000..c7a8d71690 --- /dev/null +++ b/pkg/util/corev1_helper_test.go @@ -0,0 +1,54 @@ +/* +Copyright 2020 The Knative Authors + +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 util + +import ( + "testing" + + "gotest.tools/assert" +) + +func TestGenerateVolumeName(t *testing.T) { + actual := []string{ + "Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ", + "/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/", + "", + "/", + "/path.mypath/", + "/.path.mypath", + } + + expected := []string{ + "ab12---------------------------------xz", + "ab12---------------------------------xz-", + "k-", + "k-", + "path-mypath-", + "k--path-mypath", + } + + for i := range actual { + actualName := GenerateVolumeName(actual[i]) + expectedName := appendCheckSum(expected[i], actual[i]) + assert.Equal(t, actualName, expectedName) + } + + // 63 char limit case, no need to append the checksum in expected string + expName_63 := "k---ab12---------------------------------xz-ab12--------------n" + assert.Equal(t, len(expName_63), 63) + assert.Equal(t, GenerateVolumeName("/./Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/Ab12~`!@#$%^&*()-=_+[]{}|/\\<>,./?:;\"'xZ/"), expName_63) +} From 47de9588e6436d0437ffecd7453be002b57cebc7 Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Tue, 6 Oct 2020 18:34:07 +0800 Subject: [PATCH 2/5] address comments --- pkg/kn/flags/podspec.go | 10 +++++----- pkg/kn/flags/podspec_helper.go | 7 +------ 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/kn/flags/podspec.go b/pkg/kn/flags/podspec.go index b3ee4af613..083e3d0fd0 100644 --- a/pkg/kn/flags/podspec.go +++ b/pkg/kn/flags/podspec.go @@ -290,10 +290,7 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman } if cmd.Flags().Changed("service-account") { - err = UpdateServiceAccountName(podSpec, p.ServiceAccountName) - if err != nil { - return err - } + UpdateServiceAccountName(podSpec, p.ServiceAccountName) } if cmd.Flags().Changed("pull-secret") { @@ -301,7 +298,10 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman } if cmd.Flags().Changed("user") { - UpdateUser(podSpec, p.User) + err = UpdateUser(podSpec, p.User) + if err != nil { + return err + } } return nil diff --git a/pkg/kn/flags/podspec_helper.go b/pkg/kn/flags/podspec_helper.go index 05895a460e..757b488f95 100644 --- a/pkg/kn/flags/podspec_helper.go +++ b/pkg/kn/flags/podspec_helper.go @@ -35,10 +35,6 @@ const ( PortFormatErr = "the port specification '%s' is not valid. Please provide in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'." ) -var ( - UserImageAnnotationKey = "client.knative.dev/user-image" -) - func (vt VolumeSourceType) String() string { names := [...]string{"config-map", "secret"} if vt < ConfigMapVolumeSourceType || vt > SecretVolumeSourceType { @@ -254,10 +250,9 @@ func UpdateResourcesDeprecated(spec *corev1.PodSpec, requestsResourceList corev1 } // UpdateServiceAccountName updates the service account name used for the corresponding knative service -func UpdateServiceAccountName(spec *corev1.PodSpec, serviceAccountName string) error { +func UpdateServiceAccountName(spec *corev1.PodSpec, serviceAccountName string) { serviceAccountName = strings.TrimSpace(serviceAccountName) spec.ServiceAccountName = serviceAccountName - return nil } // UpdateImagePullSecrets updates the image pull secrets used for the corresponding knative service From fb97dcf928d0cb336979a325d326697fa6358349 Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Wed, 14 Oct 2020 17:14:29 +0800 Subject: [PATCH 3/5] refactor code base --- .../service/configuration_edit_flags.go | 72 ------------------- pkg/util/corev1_helper.go | 2 +- 2 files changed, 1 insertion(+), 73 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 647a5040c8..82f4a8578a 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -21,8 +21,6 @@ import ( "github.com/spf13/cobra" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" knflags "knative.dev/client/pkg/kn/flags" @@ -245,50 +243,6 @@ func (p *ConfigurationEditFlags) Apply( fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --requests-cpu / --requests-memory are deprecated and going to be removed in future release, please use --request instead.\n\n") } - limitsResources, err := p.computeResources(p.PodSpecFlags.LimitsFlags) - if err != nil { - return err - } - requestsResources, err := p.computeResources(p.PodSpecFlags.RequestsFlags) - if err != nil { - return err - } - err = servinglib.UpdateResourcesDeprecated(template, requestsResources, limitsResources) - if err != nil { - return err - } - - requestsToRemove, limitsToRemove, err := p.PodSpecFlags.Resources.Validate() - if err != nil { - return err - } - - err = servinglib.UpdateResources(template, p.PodSpecFlags.Resources.ResourceRequirements, requestsToRemove, limitsToRemove) - if err != nil { - return err - } - - if cmd.Flags().Changed("cmd") { - err = servinglib.UpdateContainerCommand(template, p.PodSpecFlags.Command) - if err != nil { - return err - } - } - - if cmd.Flags().Changed("arg") { - err = servinglib.UpdateContainerArg(template, p.PodSpecFlags.Arg) - if err != nil { - return err - } - } - - if cmd.Flags().Changed("port") { - err = servinglib.UpdateContainerPort(template, p.PodSpecFlags.Port) - if err != nil { - return err - } - } - // Deprecated "min-scale" in 0.19, updated to "scale-min" if cmd.Flags().Changed("scale-min") || cmd.Flags().Changed("min-scale") { err = servinglib.UpdateMinScale(template, p.MinScale) @@ -462,32 +416,6 @@ func (p *ConfigurationEditFlags) updateLabels(obj *metav1.ObjectMeta, flagLabels return nil } -func (p *ConfigurationEditFlags) computeResources(resourceFlags knflags.ResourceFlags) (corev1.ResourceList, error) { - resourceList := corev1.ResourceList{} - - if resourceFlags.CPU != "" { - cpuQuantity, err := resource.ParseQuantity(resourceFlags.CPU) - if err != nil { - return corev1.ResourceList{}, - fmt.Errorf("Error parsing %q: %w", resourceFlags.CPU, err) - } - - resourceList[corev1.ResourceCPU] = cpuQuantity - } - - if resourceFlags.Memory != "" { - memoryQuantity, err := resource.ParseQuantity(resourceFlags.Memory) - if err != nil { - return corev1.ResourceList{}, - fmt.Errorf("Error parsing %q: %w", resourceFlags.Memory, err) - } - - resourceList[corev1.ResourceMemory] = memoryQuantity - } - - return resourceList, nil -} - // AnyMutation returns true if there are any revision template mutations in the // command. func (p *ConfigurationEditFlags) AnyMutation(cmd *cobra.Command) bool { diff --git a/pkg/util/corev1_helper.go b/pkg/util/corev1_helper.go index 9c4cca36d7..22f23f2df2 100644 --- a/pkg/util/corev1_helper.go +++ b/pkg/util/corev1_helper.go @@ -68,7 +68,7 @@ func GenerateVolumeName(path string) string { // the name must start with an alphanumeric character if !unicode.IsLetter(rune(vname[0])) && !unicode.IsNumber(rune(vname[0])) { - vname = fmt.Sprintf("k-%s", vname) + vname = "k-" + vname } // contain at most 63 characters From da55b4001af6824397f982ae56e98cc0b77ed5f0 Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Mon, 26 Oct 2020 16:21:39 +0800 Subject: [PATCH 4/5] change the input parameter from ccmd to flagset --- .../service/configuration_edit_flags.go | 2 +- pkg/kn/flags/podspec.go | 39 ++++++------------- pkg/kn/flags/podspec_test.go | 10 +++-- 3 files changed, 19 insertions(+), 32 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 82f4a8578a..d8cbcc0c67 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -197,7 +197,7 @@ func (p *ConfigurationEditFlags) Apply( template := &service.Spec.Template - err := p.PodSpecFlags.ResolvePodSpec(&template.Spec.PodSpec, cmd) + err := p.PodSpecFlags.ResolvePodSpec(&template.Spec.PodSpec, cmd.Flags()) if err != nil { return err } diff --git a/pkg/kn/flags/podspec.go b/pkg/kn/flags/podspec.go index 083e3d0fd0..7fe1bbd89d 100644 --- a/pkg/kn/flags/podspec.go +++ b/pkg/kn/flags/podspec.go @@ -23,7 +23,6 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "knative.dev/client/pkg/util" - "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -171,11 +170,11 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string { return flagNames } -func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Command) error { - //var podSpec = &corev1.PodSpec{Containers: []corev1.Container{{}}} +// ResolvePodSpec will create corev1.PodSpec based on the flag inputs +func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec /*cmd *cobra.Command*/, flags *pflag.FlagSet) error { var err error - if cmd.Flags().Changed("env") { + if flags.Changed("env") { envMap, err := util.MapFromArrayAllowingSingles(p.Env, "=") if err != nil { return fmt.Errorf("Invalid --env: %w", err) @@ -188,7 +187,7 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman } } - if cmd.Flags().Changed("env-from") { + if flags.Changed("env-from") { envFromSourceToUpdate := []string{} envFromSourceToRemove := []string{} for _, name := range p.EnvFrom { @@ -207,7 +206,7 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman } } - if cmd.Flags().Changed("mount") || cmd.Flags().Changed("volume") { + if flags.Changed("mount") || flags.Changed("volume") { mountsToUpdate, mountsToRemove, err := util.OrderedMapAndRemovalListFromArray(p.Mount, "=") if err != nil { return fmt.Errorf("Invalid --mount: %w", err) @@ -224,27 +223,13 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman } } - if cmd.Flags().Changed("image") { + if flags.Changed("image") { err = UpdateImage(podSpec, p.Image.String()) if err != nil { return err } } - if cmd.Flags().Changed("limits-cpu") || cmd.Flags().Changed("limits-memory") { - if cmd.Flags().Changed("limit") { - return fmt.Errorf("only one of (DEPRECATED) --limits-cpu / --limits-memory and --limit can be specified") - } - fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --limits-cpu / --limits-memory are deprecated and going to be removed in future release, please use --limit instead.\n\n") - } - - if cmd.Flags().Changed("requests-cpu") || cmd.Flags().Changed("requests-memory") { - if cmd.Flags().Changed("request") { - return fmt.Errorf("only one of (DEPRECATED) --requests-cpu / --requests-memory and --request can be specified") - } - fmt.Fprintf(cmd.OutOrStdout(), "\nWARNING: flags --requests-cpu / --requests-memory are deprecated and going to be removed in future release, please use --request instead.\n\n") - } - limitsResources, err := p.computeResources(p.LimitsFlags) if err != nil { return err @@ -268,36 +253,36 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Comman return err } - if cmd.Flags().Changed("cmd") { + if flags.Changed("cmd") { err = UpdateContainerCommand(podSpec, p.Command) if err != nil { return err } } - if cmd.Flags().Changed("arg") { + if flags.Changed("arg") { err = UpdateContainerArg(podSpec, p.Arg) if err != nil { return err } } - if cmd.Flags().Changed("port") { + if flags.Changed("port") { err = UpdateContainerPort(podSpec, p.Port) if err != nil { return err } } - if cmd.Flags().Changed("service-account") { + if flags.Changed("service-account") { UpdateServiceAccountName(podSpec, p.ServiceAccountName) } - if cmd.Flags().Changed("pull-secret") { + if flags.Changed("pull-secret") { UpdateImagePullSecrets(podSpec, p.ImagePullSecrets) } - if cmd.Flags().Changed("user") { + if flags.Changed("user") { err = UpdateUser(podSpec, p.User) if err != nil { return err diff --git a/pkg/kn/flags/podspec_test.go b/pkg/kn/flags/podspec_test.go index f889114bd1..5e4e8af3ba 100644 --- a/pkg/kn/flags/podspec_test.go +++ b/pkg/kn/flags/podspec_test.go @@ -16,6 +16,7 @@ package flags import ( "bytes" + "fmt" "testing" "github.com/spf13/cobra" @@ -113,7 +114,7 @@ func TestPodSpecResolve(t *testing.T) { Use: "test", Run: func(cmd *cobra.Command, args []string) { podSpec := &corev1.PodSpec{Containers: []corev1.Container{{}}} - err := flags.ResolvePodSpec(podSpec, cmd) + err := flags.ResolvePodSpec(podSpec, cmd.Flags()) assert.NilError(t, err, "PodSpec cannot be resolved.") assert.DeepEqual(t, expectedPodSpec, *podSpec) }, @@ -130,15 +131,16 @@ func TestPodSpecResolveReturnError(t *testing.T) { Use: "test", Run: func(cmd *cobra.Command, args []string) { podSpec := &corev1.PodSpec{Containers: []corev1.Container{{}}} - flags.ResolvePodSpec(podSpec, cmd) + err := flags.ResolvePodSpec(podSpec, cmd.Flags()) + fmt.Fprint(cmd.OutOrStdout(), "Return error: ", err) }, } testCmd.SetOut(&outBuf) - args := []string{"--requests-cpu", "1000m"} + args := []string{"--mount", "123456"} testCmd.SetArgs(args) flags.AddFlags(testCmd.Flags()) testCmd.Execute() out := outBuf.String() - assert.Assert(t, util.ContainsAll(out, "WARNING", "deprecated")) + assert.Assert(t, util.ContainsAll(out, "Invalid", "mount")) } From 493e264fba9b8dd7412ee92b2237b7d058a0c28a Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Tue, 27 Oct 2020 14:41:53 +0800 Subject: [PATCH 5/5] remove comments and add CHANGELOG --- CHANGELOG.adoc | 4 ++++ pkg/kn/flags/podspec.go | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index f4f10a7bb0..2a35c00ffb 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -17,6 +17,10 @@ |=== | | Description | PR +| 🐣 +| Add function ResolvePodSpec and move the utilities to podspec_helper +| https://github.com/knative/client/pull/1024[#1024] + | 🐛 | Fix various misspellings and linter items | https://github.com/knative/client/pull/1057[#1057] diff --git a/pkg/kn/flags/podspec.go b/pkg/kn/flags/podspec.go index 7fe1bbd89d..049b743fa6 100644 --- a/pkg/kn/flags/podspec.go +++ b/pkg/kn/flags/podspec.go @@ -171,7 +171,7 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string { } // ResolvePodSpec will create corev1.PodSpec based on the flag inputs -func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec /*cmd *cobra.Command*/, flags *pflag.FlagSet) error { +func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, flags *pflag.FlagSet) error { var err error if flags.Changed("env") {