From 70805a67db57e5902613562cb893d343d23e5c83 Mon Sep 17 00:00:00 2001 From: Anshul Sharma Date: Wed, 20 Dec 2023 13:17:37 +0000 Subject: [PATCH] Add support for Profiles (#1903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Added test case for Profiles * changed config profiles to be a map, added an accessor method for getting a certain configuration profile * Added test cases for profile check * added profile feature * Fix generated files * Fix go.mod deps * Added support for labels and added test cases --------- Co-authored-by: Roland Huß Co-authored-by: David Simansky --- docs/cmd/kn_service_apply.md | 1 + docs/cmd/kn_service_create.md | 4 + docs/cmd/kn_service_update.md | 1 + .../service/configuration_edit_flags.go | 64 ++++++++ .../service/configuration_edit_flags_test.go | 140 ++++++++++++++++++ pkg/kn/commands/service/create.go | 5 +- pkg/kn/config/config.go | 56 +++++++ pkg/kn/config/config_test.go | 49 ++++++ pkg/kn/config/testing.go | 2 + pkg/kn/config/types.go | 21 +++ 10 files changed, 342 insertions(+), 1 deletion(-) diff --git a/docs/cmd/kn_service_apply.md b/docs/cmd/kn_service_apply.md index 29d10d9f2..a3d441644 100644 --- a/docs/cmd/kn_service_apply.md +++ b/docs/cmd/kn_service_apply.md @@ -60,6 +60,7 @@ kn service apply s0 --filename my-svc.yml --probe-liveness-opts string Add common options to liveness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= --probe-readiness string Add readiness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path, exec:cmd[,cmd,...], tcp:host:port. --probe-readiness-opts string Add common options to readiness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= + --profile string The profile name must be defined in config.yaml or part of the built-in profile, e.g. Istio. Related annotations and labels will be added to the service.To unset, specify the profile name followed by a "-" (e.g., name-). --pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. diff --git a/docs/cmd/kn_service_create.md b/docs/cmd/kn_service_create.md index 2a5d030a5..3f676830d 100644 --- a/docs/cmd/kn_service_create.md +++ b/docs/cmd/kn_service_create.md @@ -49,6 +49,9 @@ kn service create NAME --image IMAGE kn service create gitopstest -n test-ns --image knativesamples/helloworld --target=/user/knfiles kn service create gitopstest --image knativesamples/helloworld --target=/user/knfiles/test.yaml kn service create gitopstest --image knativesamples/helloworld --target=/user/knfiles/test.json + + # Create a service with profile + kn service create profiletest --image knativesamples/helloworld --profile istio ``` ### Options @@ -85,6 +88,7 @@ kn service create NAME --image IMAGE --probe-liveness-opts string Add common options to liveness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= --probe-readiness string Add readiness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path, exec:cmd[,cmd,...], tcp:host:port. --probe-readiness-opts string Add common options to readiness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= + --profile string The profile name must be defined in config.yaml or part of the built-in profile, e.g. Istio. Related annotations and labels will be added to the service.To unset, specify the profile name followed by a "-" (e.g., name-). --pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. diff --git a/docs/cmd/kn_service_update.md b/docs/cmd/kn_service_update.md index 1142e6b19..ce7fbbf06 100644 --- a/docs/cmd/kn_service_update.md +++ b/docs/cmd/kn_service_update.md @@ -72,6 +72,7 @@ kn service update NAME --probe-liveness-opts string Add common options to liveness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= --probe-readiness string Add readiness probe to Service deployment. Supported probe types are HTTGet, Exec and TCPSocket. Format: [http,https]:host:port:path, exec:cmd[,cmd,...], tcp:host:port. --probe-readiness-opts string Add common options to readiness probe. Common opts (comma separated, case insensitive): InitialDelaySeconds=, FailureThreshold=, SuccessThreshold=, PeriodSeconds=, TimeoutSeconds= + --profile string The profile name must be defined in config.yaml or part of the built-in profile, e.g. Istio. Related annotations and labels will be added to the service.To unset, specify the profile name followed by a "-" (e.g., name-). --pull-policy string Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent --pull-secret string Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace. --request strings The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'. diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index 17c9798af..d4d7a45d6 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -24,6 +24,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/serving/pkg/apis/config" + knconfig "knative.dev/client/pkg/kn/config" knflags "knative.dev/client/pkg/kn/flags" servinglib "knative.dev/client/pkg/serving" "knative.dev/client/pkg/util" @@ -57,6 +58,7 @@ type ConfigurationEditFlags struct { ClusterLocal bool ScaleInit int TimeoutSeconds int64 + Profile string // Preferences about how to do the action. LockToDigest bool @@ -187,6 +189,11 @@ func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) { "Duration in seconds that the request routing layer will wait for a request delivered to a "+""+ "container to begin replying") p.markFlagMakesRevision("timeout") + + command.Flags().StringVar(&p.Profile, "profile", "", + "The profile name must be defined in config.yaml or part of the built-in profile, e.g. Istio. Related annotations and labels will be added to the service."+ + "To unset, specify the profile name followed by a \"-\" (e.g., name-).") + p.markFlagMakesRevision("profile") } // AddUpdateFlags adds the flags specific to update. @@ -479,6 +486,63 @@ func (p *ConfigurationEditFlags) Apply( service.Spec.Template.Spec.TimeoutSeconds = &p.TimeoutSeconds } + if cmd.Flags().Changed("profile") { + profileName := "" + deleteProfile := false + if strings.HasSuffix(p.Profile, "-") { + profileName = p.Profile[:len(p.Profile)-1] + deleteProfile = true + } else { + profileName = p.Profile + deleteProfile = false + } + + if len(knconfig.GlobalConfig.Profile(profileName).Annotations) > 0 || len(knconfig.GlobalConfig.Profile(profileName).Labels) > 0 { + annotations := knconfig.GlobalConfig.Profile(profileName).Annotations + labels := knconfig.GlobalConfig.Profile(profileName).Labels + + profileAnnotations := make(util.StringMap) + for _, value := range annotations { + profileAnnotations[value.Name] = value.Value + } + + profileLabels := make(util.StringMap) + for _, value := range labels { + profileLabels[value.Name] = value.Value + } + + if deleteProfile { + var annotationsToRemove []string + for _, value := range annotations { + annotationsToRemove = append(annotationsToRemove, value.Name) + } + + var labelsToRemove []string + for _, value := range labels { + labelsToRemove = append(labelsToRemove, value.Name) + } + + if err = servinglib.UpdateRevisionTemplateAnnotations(template, map[string]string{}, annotationsToRemove); err != nil { + return err + } + updatedLabels := servinglib.UpdateLabels(service.ObjectMeta.Labels, map[string]string{}, labelsToRemove) + service.ObjectMeta.Labels = updatedLabels // In case service.ObjectMeta.Labels was nil + } else { + if err = servinglib.UpdateRevisionTemplateAnnotations(template, profileAnnotations, []string{}); err != nil { + return err + } + updatedLabels := servinglib.UpdateLabels(service.ObjectMeta.Labels, profileLabels, []string{}) + service.ObjectMeta.Labels = updatedLabels // In case service.ObjectMeta.Labels was nil + } + + if err != nil { + return err + } + } else { + return fmt.Errorf("profile %s doesn't exist", profileName) + } + } + return nil } diff --git a/pkg/kn/commands/service/configuration_edit_flags_test.go b/pkg/kn/commands/service/configuration_edit_flags_test.go index 853407e86..4955fcc94 100644 --- a/pkg/kn/commands/service/configuration_edit_flags_test.go +++ b/pkg/kn/commands/service/configuration_edit_flags_test.go @@ -15,10 +15,15 @@ package service import ( + "os" + "path/filepath" "testing" + "github.com/spf13/viper" "gotest.tools/v3/assert" + "knative.dev/client/pkg/kn/commands" + "knative.dev/client/pkg/kn/config" "knative.dev/client/pkg/util" "knative.dev/serving/pkg/apis/autoscaling" ) @@ -75,3 +80,138 @@ func TestScaleActivation(t *testing.T) { assert.NilError(t, err) assert.Equal(t, svc.Spec.Template.Annotations[autoscaling.ActivationScaleKey], "2") } + +func TestApplyDefaultProfileFlag(t *testing.T) { + var editFlags ConfigurationEditFlags + knParams := &commands.KnParams{} + cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams) + + editFlags.AddCreateFlags(cmd) + + err := config.BootstrapConfig() + assert.NilError(t, err) + + svc := createTestService("test-svc", []string{"test-svc-00001"}, goodConditions()) + cmd.SetArgs([]string{"--profile", "istio"}) + cmd.Execute() + editFlags.Apply(&svc, nil, cmd) + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.istio.io/inject"], "true") + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.istio.io/rewriteAppHTTPProbers"], "true") + assert.Equal(t, svc.Spec.Template.Annotations["serving.knative.openshift.io/enablePassthrough"], "true") +} + +func TestApplyProfileFlag(t *testing.T) { + var editFlags ConfigurationEditFlags + knParams := &commands.KnParams{} + cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams) + configYaml := ` +profiles: + testprofile: + labels: + - name: environment + value: "test" + annotations: + - name: sidecar.testprofile.io/inject + value: "true" + - name: sidecar.testprofile.io/rewriteAppHTTPProbers + value: "true" +` + _, cleanup := setupConfig(t, configYaml) + defer cleanup() + + editFlags.AddCreateFlags(cmd) + + err := config.BootstrapConfig() + assert.NilError(t, err) + + svc := createTestService("test-svc", []string{"test-svc-00001"}, goodConditions()) + cmd.SetArgs([]string{"--profile", "testprofile"}) + cmd.Execute() + editFlags.Apply(&svc, nil, cmd) + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.testprofile.io/inject"], "true") + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.testprofile.io/rewriteAppHTTPProbers"], "true") + assert.Equal(t, svc.ObjectMeta.Labels["environment"], "test") +} + +func TestDeleteProfileFlag(t *testing.T) { + var editFlags ConfigurationEditFlags + knParams := &commands.KnParams{} + cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams) + configYaml := ` +profiles: + testprofile: + labels: + - name: environment + value: "test" + annotations: + - name: sidecar.testprofile.io/inject + value: "true" + - name: sidecar.testprofile.io/rewriteAppHTTPProbers + value: "true" +` + _, cleanup := setupConfig(t, configYaml) + defer cleanup() + + editFlags.AddCreateFlags(cmd) + + err := config.BootstrapConfig() + assert.NilError(t, err) + + svc := createTestService("test-svc", []string{"test-svc-00001"}, goodConditions()) + cmd.SetArgs([]string{"--profile", "testprofile"}) + cmd.Execute() + editFlags.Apply(&svc, nil, cmd) + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.testprofile.io/inject"], "true") + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.testprofile.io/rewriteAppHTTPProbers"], "true") + assert.Equal(t, svc.ObjectMeta.Labels["environment"], "test") + + cmd.SetArgs([]string{"--profile", "testprofile-"}) + cmd.Execute() + editFlags.Apply(&svc, nil, cmd) + + assert.Equal(t, svc.Spec.Template.Annotations["sidecar.istio.io/inject"], "") + assert.Equal(t, len(svc.Spec.Template.Annotations), 1) + + assert.Equal(t, svc.ObjectMeta.Labels["environment"], "") +} + +func TestApplyProfileFlagError(t *testing.T) { + var editFlags ConfigurationEditFlags + knParams := &commands.KnParams{} + cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams) + editFlags.AddCreateFlags(cmd) + err := config.BootstrapConfig() + assert.NilError(t, err) + + svc := createTestService("test-svc", []string{"test-svc-00001"}, goodConditions()) + cmd.SetArgs([]string{"--profile", "invalidprofile"}) + cmd.Execute() + err = editFlags.Apply(&svc, nil, cmd) + assert.Assert(t, util.ContainsAll(err.Error(), "profile", "invalidprofile")) +} + +func setupConfig(t *testing.T, configContent string) (string, func()) { + tmpDir := t.TempDir() + + // Avoid to be fooled by the things in the the real homedir + oldHome := os.Getenv("HOME") + os.Setenv("HOME", tmpDir) + + // Save old args + backupArgs := os.Args + + // WriteCache out a temporary configContent file + var cfgFile string + if configContent != "" { + cfgFile = filepath.Join(tmpDir, "config.yaml") + os.Args = []string{"kn", "--config", cfgFile} + err := os.WriteFile(cfgFile, []byte(configContent), 0644) + assert.NilError(t, err) + } + return cfgFile, func() { + // Cleanup everything + os.Setenv("HOME", oldHome) + os.Args = backupArgs + viper.Reset() + } +} diff --git a/pkg/kn/commands/service/create.go b/pkg/kn/commands/service/create.go index d3d60561c..24bf476ff 100644 --- a/pkg/kn/commands/service/create.go +++ b/pkg/kn/commands/service/create.go @@ -80,7 +80,10 @@ var create_example = ` # Create the service in offline mode instead of kubernetes cluster (Beta) kn service create gitopstest -n test-ns --image knativesamples/helloworld --target=/user/knfiles kn service create gitopstest --image knativesamples/helloworld --target=/user/knfiles/test.yaml - kn service create gitopstest --image knativesamples/helloworld --target=/user/knfiles/test.json` + kn service create gitopstest --image knativesamples/helloworld --target=/user/knfiles/test.json + + # Create a service with profile + kn service create profiletest --image knativesamples/helloworld --profile istio` func NewServiceCreateCommand(p *commands.KnParams) *cobra.Command { var editFlags ConfigurationEditFlags diff --git a/pkg/kn/config/config.go b/pkg/kn/config/config.go index 27689a713..75e5dc49c 100644 --- a/pkg/kn/config/config.go +++ b/pkg/kn/config/config.go @@ -64,6 +64,9 @@ type config struct { // channelTypeMappings is a list of channel type mapping channelTypeMappings []ChannelTypeMapping + + // profiles is a map of profiles from the config file and built-in profiles + profiles map[string]Profile } func (c *config) ContextSharing() bool { @@ -100,6 +103,10 @@ func (c *config) SinkMappings() []SinkMapping { return c.sinkMappings } +func (c *config) Profile(profile string) Profile { + return c.profiles[profile] +} + func (c *config) ChannelTypeMappings() []ChannelTypeMapping { return c.channelTypeMappings } @@ -167,6 +174,12 @@ func BootstrapConfig() error { return err } + // Deserialize profiles if configured + err = parseProfiles() + if err != nil { + return err + } + // Deserialize channel type mappings if configured err = parseChannelTypeMappings() return err @@ -267,6 +280,49 @@ func parseSinkMappings() error { return nil } +// parse profiles and store them in the global configuration +func parseProfiles() error { + if viper.IsSet(profiles) { + err := viper.UnmarshalKey(profiles, &globalConfig.profiles) + if err != nil { + return fmt.Errorf("error while parsing profiles in configuration file %s: %w", + viper.ConfigFileUsed(), err) + } + } + globalConfig.profiles = mergeProfilesWithBuiltInProfiles(globalConfig.profiles) + + return nil +} + +// defaultProfiles returns the built-in profiles +func builtInProfiles() map[string]Profile { + return map[string]Profile{ + istio: { + Annotations: []NamedValue{ + {Name: "sidecar.istio.io/inject", Value: "true"}, + {Name: "sidecar.istio.io/rewriteAppHTTPProbers", Value: "true"}, + {Name: "serving.knative.openshift.io/enablePassthrough", Value: "true"}, + }, + }, + } +} + +// mergeProfilesWithDefaultProfiles merges the given profiles with the built-in profiles +func mergeProfilesWithBuiltInProfiles(profiles map[string]Profile) map[string]Profile { + builtInProfiles := builtInProfiles() + mergedProfiles := make(map[string]Profile, len(builtInProfiles)+len(profiles)) + + for key, value := range builtInProfiles { + mergedProfiles[key] = value + } + + for key, value := range profiles { + mergedProfiles[key] = value + } + + return mergedProfiles +} + // parse channel type mappings and store them in the global configuration func parseChannelTypeMappings() error { if viper.IsSet(keyChannelTypeMappings) { diff --git a/pkg/kn/config/config_test.go b/pkg/kn/config/config_test.go index bc91795d6..5f7005504 100644 --- a/pkg/kn/config/config_test.go +++ b/pkg/kn/config/config_test.go @@ -24,10 +24,27 @@ import ( "gotest.tools/v3/assert" ) +// TestDefaultProfile tests that the default profile is correctly loaded +func TestDefaultProfile(t *testing.T) { + profile := builtInProfiles() + assert.Equal(t, len(profile[istio].Labels), 0) + assert.Equal(t, len(profile[istio].Annotations), 3) +} + func TestBootstrapConfig(t *testing.T) { configYaml := ` plugins: directory: /tmp +profiles: + knative: + labels: + - name: environment + value: "knative" + annotations: + - name: sidecar.knative.io/inject + value: "true" + - name: sidecar.knative.io/rewriteAppHTTPProbers + value: "true" eventing: sink-mappings: - prefix: service @@ -51,6 +68,38 @@ eventing: assert.Equal(t, GlobalConfig.PluginsDir(), "/tmp") assert.Equal(t, GlobalConfig.LookupPluginsInPath(), true) assert.Equal(t, len(GlobalConfig.SinkMappings()), 1) + assert.Equal(t, len(GlobalConfig.Profile("istio").Annotations), 3) + assert.DeepEqual(t, GlobalConfig.Profile("istio").Annotations, []NamedValue{ + { + Name: "sidecar.istio.io/inject", + Value: "true", + }, + { + Name: "sidecar.istio.io/rewriteAppHTTPProbers", + Value: "true", + }, + { + Name: "serving.knative.openshift.io/enablePassthrough", + Value: "true", + }, + }) + assert.DeepEqual(t, GlobalConfig.Profile("knative").Annotations, []NamedValue{ + { + Name: "sidecar.knative.io/inject", + Value: "true", + }, + { + Name: "sidecar.knative.io/rewriteAppHTTPProbers", + Value: "true", + }, + }) + assert.Equal(t, len(GlobalConfig.Profile("knative").Labels), 1) + assert.DeepEqual(t, GlobalConfig.Profile("knative").Labels, []NamedValue{ + { + Name: "environment", + Value: "knative", + }, + }) assert.DeepEqual(t, (GlobalConfig.SinkMappings())[0], SinkMapping{ Prefix: "service", Resource: "services", diff --git a/pkg/kn/config/testing.go b/pkg/kn/config/testing.go index 7a935a33f..b1c64ac21 100644 --- a/pkg/kn/config/testing.go +++ b/pkg/kn/config/testing.go @@ -24,6 +24,7 @@ type TestConfig struct { TestLookupPluginsInPath bool TestSinkMappings []SinkMapping TestChannelTypeMappings []ChannelTypeMapping + TestProfiles map[string]Profile } // Ensure that TestConfig implements the configuration interface @@ -35,3 +36,4 @@ func (t TestConfig) ConfigFile() string { return t.TestCo func (t TestConfig) LookupPluginsInPath() bool { return t.TestLookupPluginsInPath } func (t TestConfig) SinkMappings() []SinkMapping { return t.TestSinkMappings } func (t TestConfig) ChannelTypeMappings() []ChannelTypeMapping { return t.TestChannelTypeMappings } +func (t TestConfig) Profile(profile string) Profile { return t.TestProfiles[profile] } diff --git a/pkg/kn/config/types.go b/pkg/kn/config/types.go index 9af5f2bed..7997da0c9 100644 --- a/pkg/kn/config/types.go +++ b/pkg/kn/config/types.go @@ -37,6 +37,9 @@ type Config interface { // ChannelTypeMappings returns additional mappings for channel type aliases ChannelTypeMappings() []ChannelTypeMapping + + // Profile returns a configured profile with this name or nil of no such profile is configured + Profile(profile string) Profile } // SinkMappings is the struct of sink prefix config in kn config @@ -71,12 +74,25 @@ type ChannelTypeMapping struct { Version string } +// NamedValue is the struct of name and values in the Profile struct +type NamedValue struct { + Name string `yaml:"name"` + Value string `yaml:"value"` +} + +// Profile is the struct of profile config in kn config +type Profile struct { + Annotations []NamedValue `yaml:"annotations"` + Labels []NamedValue `yaml:"labels"` +} + // config Keys for looking up in viper const ( keyFeaturesContextSharing = "features.context-sharing" keyPluginsDirectory = "plugins.directory" keySinkMappings = "eventing.sink-mappings" keyChannelTypeMappings = "eventing.channel-type-mappings" + profiles = "profiles" ) // legacy config keys, deprecated @@ -90,3 +106,8 @@ const ( const ( flagPluginsDir = "plugins-dir" ) + +// default profiles +const ( + istio = "istio" +)