diff --git a/config/config-defaults.yaml b/config/config-defaults.yaml index 4e1bda1ea884..4960b0073c82 100644 --- a/config/config-defaults.yaml +++ b/config/config-defaults.yaml @@ -61,3 +61,10 @@ data: # revisions to by default. If omitted, no value is specified # and the system default is used. revision-memory-limit: "200M" # 200 megabytes of memory + + # container-name-template contains a template for the default + # container name, if none is specified. This field supports + # Go templating and is supplied with the ObjectMeta of the + # enclosing Service or Configuration, so values such as + # {{.Name}} are also valid. + container-name-template: "user-container" diff --git a/docs/spec/spec.md b/docs/spec/spec.md index a9be6fcb61dc..9b64befce009 100644 --- a/docs/spec/spec.md +++ b/docs/spec/spec.md @@ -377,6 +377,7 @@ otherwise noted. ```yaml container: # v1.Container + name: ... # Optional args: [...] # Optional command: [...] # Optional env: ... # Optional diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index 232475e33966..0f442948c511 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -17,15 +17,30 @@ limitations under the License. package config import ( + "bytes" + "context" + "fmt" + "io/ioutil" "strconv" + "text/template" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/knative/pkg/apis" ) const ( // DefaultsConfigName is the name of config map for the defaults. DefaultsConfigName = "config-defaults" + + // DefaultRevisionTimeoutSeconds will be set if timeoutSeconds not specified. + DefaultRevisionTimeoutSeconds = 5 * 60 + + // DefaultUserContainerName is the default name we give to the container + // specified by the user, if `name:` is omitted. + DefaultUserContainerName = "user-container" ) // NewDefaultsConfigFromMap creates a Defaults from the supplied Map @@ -78,6 +93,22 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) { } } + if raw, ok := data["container-name-template"]; !ok { + nc.UserContainerNameTemplate = DefaultUserContainerName + } else { + tmpl, err := template.New("user-container").Parse(raw) + if err != nil { + return nil, err + } + // Check that the template properly applies to ObjectMeta. + if err := tmpl.Execute(ioutil.Discard, metav1.ObjectMeta{}); err != nil { + return nil, fmt.Errorf("error executing template: %v", err) + } + // We store the raw template because we run deepcopy-gen on the + // config and that doesn't copy nicely. + nc.UserContainerNameTemplate = raw + } + return nc, nil } @@ -86,17 +117,25 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) return NewDefaultsConfigFromMap(config.Data) } -const ( - // DefaultRevisionTimeoutSeconds will be set if timeoutSeconds not specified. - DefaultRevisionTimeoutSeconds = 5 * 60 -) - // Defaults includes the default values to be populated by the webhook. type Defaults struct { RevisionTimeoutSeconds int64 + UserContainerNameTemplate string + RevisionCPURequest *resource.Quantity RevisionCPULimit *resource.Quantity RevisionMemoryRequest *resource.Quantity RevisionMemoryLimit *resource.Quantity } + +// UserContainerName returns the name of the user container based on the context. +func (d *Defaults) UserContainerName(ctx context.Context) string { + tmpl := template.Must( + template.New("user-container").Parse(d.UserContainerNameTemplate)) + buf := &bytes.Buffer{} + if err := tmpl.Execute(buf, apis.ParentMeta(ctx)); err != nil { + return "" + } + return buf.String() +} diff --git a/pkg/apis/config/defaults_test.go b/pkg/apis/config/defaults_test.go index 51d5a4a8419e..f323f59996d3 100644 --- a/pkg/apis/config/defaults_test.go +++ b/pkg/apis/config/defaults_test.go @@ -17,9 +17,11 @@ limitations under the License. package config import ( + "context" "testing" "github.com/google/go-cmp/cmp" + "github.com/knative/pkg/apis" "github.com/knative/pkg/system" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -43,6 +45,7 @@ func TestDefaultsConfigurationFromFile(t *testing.T) { func TestDefaultsConfiguration(t *testing.T) { oneTwoThree := resource.MustParse("123m") + configTests := []struct { name string wantErr bool @@ -52,7 +55,8 @@ func TestDefaultsConfiguration(t *testing.T) { name: "defaults configuration", wantErr: false, wantDefaults: &Defaults{ - RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds, + RevisionTimeoutSeconds: DefaultRevisionTimeoutSeconds, + UserContainerNameTemplate: DefaultUserContainerName, }, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -65,8 +69,9 @@ func TestDefaultsConfiguration(t *testing.T) { name: "specified values", wantErr: false, wantDefaults: &Defaults{ - RevisionTimeoutSeconds: 123, - RevisionCPURequest: &oneTwoThree, + RevisionTimeoutSeconds: 123, + RevisionCPURequest: &oneTwoThree, + UserContainerNameTemplate: "{{.Name}}", }, config: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -76,6 +81,7 @@ func TestDefaultsConfiguration(t *testing.T) { Data: map[string]string{ "revision-timeout-seconds": "123", "revision-cpu-request": "123m", + "container-name-template": "{{.Name}}", }, }, }, { @@ -91,6 +97,20 @@ func TestDefaultsConfiguration(t *testing.T) { "revision-timeout-seconds": "asdf", }, }, + }, { + name: "bad name template", + wantErr: true, + wantDefaults: (*Defaults)(nil), + config: &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + // This is an intentional typo. + "container-name-template": "{{.NAme}}", + }, + }, }, { name: "bad resource", wantErr: true, @@ -107,14 +127,59 @@ func TestDefaultsConfiguration(t *testing.T) { }} for _, tt := range configTests { - actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) + t.Run(tt.name, func(t *testing.T) { + actualDefaults, err := NewDefaultsConfigFromConfigMap(tt.config) + + if (err != nil) != tt.wantErr { + t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) + } + + if diff := cmp.Diff(actualDefaults, tt.wantDefaults, ignoreStuff); diff != "" { + t.Fatalf("Test: %q; want %v, but got %v", tt.name, tt.wantDefaults, actualDefaults) + } + }) + } +} + +func TestTemplating(t *testing.T) { + tests := []struct { + name string + template string + want string + }{{ + name: "groot", + template: "{{.Name}}", + want: "i-am-groot", + }, { + name: "complex", + template: "{{.Namespace}}-of-the-galaxy", + want: "guardians-of-the-galaxy", + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + def, err := NewDefaultsConfigFromConfigMap(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: DefaultsConfigName, + }, + Data: map[string]string{ + "container-name-template": test.template, + }, + }) + if err != nil { + t.Errorf("Error parsing defaults: %v", err) + } - if (err != nil) != tt.wantErr { - t.Fatalf("Test: %q; NewDefaultsConfigFromConfigMap() error = %v, WantErr %v", tt.name, err, tt.wantErr) - } + ctx := apis.WithinParent(context.Background(), metav1.ObjectMeta{ + Name: "i-am-groot", + Namespace: "guardians", + }) - if diff := cmp.Diff(actualDefaults, tt.wantDefaults, ignoreResourceQuantity); diff != "" { - t.Fatalf("Test: %q; want %v, but got %v", tt.name, tt.wantDefaults, actualDefaults) - } + got := def.UserContainerName(ctx) + if test.want != got { + t.Errorf("UserContainerName() = %v, wanted %v", got, test.want) + } + }) } } diff --git a/pkg/apis/config/store_test.go b/pkg/apis/config/store_test.go index 25cbd362d1b3..037915774350 100644 --- a/pkg/apis/config/store_test.go +++ b/pkg/apis/config/store_test.go @@ -28,7 +28,9 @@ import ( . "github.com/knative/pkg/configmap/testing" ) -var ignoreResourceQuantity = cmpopts.IgnoreUnexported(resource.Quantity{}) +var ignoreStuff = cmp.Options{ + cmpopts.IgnoreUnexported(resource.Quantity{}), +} func TestStoreLoadWithContext(t *testing.T) { defer logtesting.ClearAll() @@ -42,7 +44,7 @@ func TestStoreLoadWithContext(t *testing.T) { t.Run("defaults", func(t *testing.T) { expected, _ := NewDefaultsConfigFromConfigMap(defaultsConfig) - if diff := cmp.Diff(expected, config.Defaults, ignoreResourceQuantity); diff != "" { + if diff := cmp.Diff(expected, config.Defaults, ignoreStuff...); diff != "" { t.Errorf("Unexpected defaults config (-want, +got): %v", diff) } }) @@ -56,7 +58,7 @@ func TestStoreLoadWithContextOrDefaults(t *testing.T) { t.Run("defaults", func(t *testing.T) { expected, _ := NewDefaultsConfigFromConfigMap(defaultsConfig) - if diff := cmp.Diff(expected, config.Defaults, ignoreResourceQuantity); diff != "" { + if diff := cmp.Diff(expected, config.Defaults, ignoreStuff...); diff != "" { t.Errorf("Unexpected defaults config (-want, +got): %v", diff) } }) diff --git a/pkg/apis/serving/fieldmask.go b/pkg/apis/serving/fieldmask.go index 4a3b61e9e6a1..ba3f59c6ae7f 100644 --- a/pkg/apis/serving/fieldmask.go +++ b/pkg/apis/serving/fieldmask.go @@ -114,6 +114,7 @@ func ContainerMask(in *corev1.Container) *corev1.Container { out := new(corev1.Container) // Allowed fields + out.Name = in.Name out.Args = in.Args out.Command = in.Command out.Env = in.Env @@ -133,7 +134,6 @@ func ContainerMask(in *corev1.Container) *corev1.Container { // Disallowed fields // This list is unnecessary, but added here for clarity out.Lifecycle = nil - out.Name = "" out.Stdin = false out.StdinOnce = false out.TTY = false diff --git a/pkg/apis/serving/fieldmask_test.go b/pkg/apis/serving/fieldmask_test.go index a96db92a07b3..5d499aeecc15 100644 --- a/pkg/apis/serving/fieldmask_test.go +++ b/pkg/apis/serving/fieldmask_test.go @@ -130,6 +130,7 @@ func TestPodSpecMask(t *testing.T) { func TestContainerMask(t *testing.T) { want := &corev1.Container{ + Name: "foo", Args: []string{"hello"}, Command: []string{"world"}, Env: []corev1.EnvVar{{}}, diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 0eddc2122846..6c6ecd2ae697 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -46,6 +46,10 @@ var ( "/var/log", ) + reservedContainerNames = sets.NewString( + "queue-proxy", + ) + reservedEnvVars = sets.NewString( "PORT", "K_SERVICE", @@ -186,6 +190,13 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi errs := apis.CheckDisallowedFields(container, *ContainerMask(&container)) + if reservedContainerNames.Has(container.Name) { + errs = errs.Also(&apis.FieldError{ + Message: fmt.Sprintf("%q is a reserved container name", container.Name), + Paths: []string{"name"}, + }) + } + // Env errs = errs.Also(validateEnv(container.Env).ViaField("env")) // EnvFrom diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index 3d1528aa2606..755287060194 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -130,11 +130,12 @@ func TestPodSpecValidation(t *testing.T) { name: "bad pod spec", ps: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "steve", - Image: "helloworld", + Name: "steve", + Image: "helloworld", + Lifecycle: &corev1.Lifecycle{}, }}, }, - want: apis.ErrDisallowedFields("containers[0].name"), + want: apis.ErrDisallowedFields("containers[0].lifecycle"), }, { name: "missing all", ps: corev1.PodSpec{ @@ -211,12 +212,13 @@ func TestContainerValidation(t *testing.T) { Details: "image: \"foo:bar:baz\", error: could not parse reference", }, }, { - name: "has a name", + name: "has a lifecycle", c: corev1.Container{ - Name: "foo", - Image: "foo", + Name: "foo", + Image: "foo", + Lifecycle: &corev1.Lifecycle{}, }, - want: apis.ErrDisallowedFields("name"), + want: apis.ErrDisallowedFields("lifecycle"), }, { name: "has resources", c: corev1.Container{ @@ -664,7 +666,6 @@ func TestContainerValidation(t *testing.T) { }}, }, want: apis.ErrDisallowedFields("lifecycle").Also( - apis.ErrDisallowedFields("name")).Also( apis.ErrDisallowedFields("stdin")).Also( apis.ErrDisallowedFields("stdinOnce")).Also( apis.ErrDisallowedFields("tty")).Also( @@ -672,10 +673,9 @@ func TestContainerValidation(t *testing.T) { }, { name: "has numerous problems", c: corev1.Container{ - Name: "foo", Lifecycle: &corev1.Lifecycle{}, }, - want: apis.ErrDisallowedFields("name", "lifecycle").Also( + want: apis.ErrDisallowedFields("lifecycle").Also( apis.ErrMissingField("image")), }} diff --git a/pkg/apis/serving/v1alpha1/configuration_defaults.go b/pkg/apis/serving/v1alpha1/configuration_defaults.go index 02dba97f4bca..9f8706bc3015 100644 --- a/pkg/apis/serving/v1alpha1/configuration_defaults.go +++ b/pkg/apis/serving/v1alpha1/configuration_defaults.go @@ -25,6 +25,7 @@ import ( ) func (c *Configuration) SetDefaults(ctx context.Context) { + ctx = apis.WithinParent(ctx, c.ObjectMeta) c.Spec.SetDefaults(apis.WithinSpec(ctx)) } diff --git a/pkg/apis/serving/v1alpha1/configuration_defaults_test.go b/pkg/apis/serving/v1alpha1/configuration_defaults_test.go index 998c50dd8d8e..0b3c935f43d1 100644 --- a/pkg/apis/serving/v1alpha1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1alpha1/configuration_defaults_test.go @@ -67,6 +67,7 @@ func TestConfigurationDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -95,6 +96,7 @@ func TestConfigurationDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -127,6 +129,7 @@ func TestConfigurationDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -161,6 +164,7 @@ func TestConfigurationDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, diff --git a/pkg/apis/serving/v1alpha1/configuration_validation_test.go b/pkg/apis/serving/v1alpha1/configuration_validation_test.go index 0adab4b0857a..0b4829091cd2 100644 --- a/pkg/apis/serving/v1alpha1/configuration_validation_test.go +++ b/pkg/apis/serving/v1alpha1/configuration_validation_test.go @@ -69,13 +69,14 @@ func TestConfigurationSpecValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "stuart", - Image: "hellworld", + Name: "stuart", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, }, - want: apis.ErrDisallowedFields("revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("revisionTemplate.spec.container.lifecycle"), }, { name: "build is not allowed", c: &ConfigurationSpec{ @@ -188,14 +189,15 @@ func TestConfigurationValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "stuart", - Image: "hellworld", + Name: "stuart", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, }, }, - want: apis.ErrDisallowedFields("spec.revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("spec.revisionTemplate.spec.container.lifecycle"), }, { name: "propagate revision failures (template)", c: &Configuration{ diff --git a/pkg/apis/serving/v1alpha1/revision_defaults_test.go b/pkg/apis/serving/v1alpha1/revision_defaults_test.go index f7d30bd45973..b86822c5f355 100644 --- a/pkg/apis/serving/v1alpha1/revision_defaults_test.go +++ b/pkg/apis/serving/v1alpha1/revision_defaults_test.go @@ -46,6 +46,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -64,6 +65,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -94,6 +96,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(123), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -117,6 +120,7 @@ func TestRevisionDefaulting(t *testing.T) { want: &Revision{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Image: "foo", VolumeMounts: []corev1.VolumeMount{{ Name: "bar", @@ -152,6 +156,7 @@ func TestRevisionDefaulting(t *testing.T) { RevisionSpec: v1beta1.RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "foo", VolumeMounts: []corev1.VolumeMount{{ Name: "bar", @@ -195,6 +200,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "foo", Resources: defaultResources, }}, @@ -220,6 +226,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -241,6 +248,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -264,6 +272,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 20bd585dc66b..2d3dc3c741ab 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -197,11 +197,12 @@ func TestRevisionSpecValidation(t *testing.T) { name: "bad container spec", rs: &RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "steve", - Image: "helloworld", + Name: "steve", + Image: "helloworld", + Lifecycle: &corev1.Lifecycle{}, }, }, - want: apis.ErrDisallowedFields("container.name"), + want: apis.ErrDisallowedFields("container.lifecycle"), }, { name: "exceed max timeout", rs: &RevisionSpec{ @@ -279,12 +280,13 @@ func TestRevisionTemplateSpecValidation(t *testing.T) { rts: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "kevin", - Image: "helloworld", + Name: "kevin", + Image: "helloworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, - want: apis.ErrDisallowedFields("spec.container.name"), + want: apis.ErrDisallowedFields("spec.container.lifecycle"), }, { name: "has revision template name", rts: &RevisionTemplateSpec{ @@ -385,12 +387,13 @@ func TestRevisionValidation(t *testing.T) { }, Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "kevin", - Image: "helloworld", + Name: "kevin", + Image: "helloworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, - want: apis.ErrDisallowedFields("spec.container.name"), + want: apis.ErrDisallowedFields("spec.container.lifecycle"), }, { name: "invalid name - dots and too long", r: &Revision{ diff --git a/pkg/apis/serving/v1alpha1/service_defaults.go b/pkg/apis/serving/v1alpha1/service_defaults.go index cfd6eca3cd33..d42493fbbf63 100644 --- a/pkg/apis/serving/v1alpha1/service_defaults.go +++ b/pkg/apis/serving/v1alpha1/service_defaults.go @@ -27,6 +27,7 @@ import ( ) func (s *Service) SetDefaults(ctx context.Context) { + ctx = apis.WithinParent(ctx, s.ObjectMeta) s.Spec.SetDefaults(apis.WithinSpec(ctx)) if ui := apis.GetUserInfo(ctx); ui != nil { diff --git a/pkg/apis/serving/v1alpha1/service_defaults_test.go b/pkg/apis/serving/v1alpha1/service_defaults_test.go index 402367cb3504..c216063e40b4 100644 --- a/pkg/apis/serving/v1alpha1/service_defaults_test.go +++ b/pkg/apis/serving/v1alpha1/service_defaults_test.go @@ -88,6 +88,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -123,6 +124,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -171,6 +173,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -204,6 +207,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -240,6 +244,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -289,6 +294,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -326,6 +332,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -361,6 +368,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -420,6 +428,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -477,6 +486,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -532,6 +542,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -569,6 +580,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "blah", Resources: defaultResources, }}, @@ -606,6 +618,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), }, DeprecatedContainer: &corev1.Container{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }, }, @@ -653,6 +666,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "blah", Resources: defaultResources, }}, @@ -706,6 +720,7 @@ func TestServiceDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "blah", Resources: defaultResources, }}, diff --git a/pkg/apis/serving/v1alpha1/service_validation_test.go b/pkg/apis/serving/v1alpha1/service_validation_test.go index 492de47a8ab4..bc5b8bc3311d 100644 --- a/pkg/apis/serving/v1alpha1/service_validation_test.go +++ b/pkg/apis/serving/v1alpha1/service_validation_test.go @@ -257,8 +257,9 @@ func TestServiceValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "foo", - Image: "hellworld", + Name: "foo", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, @@ -266,7 +267,7 @@ func TestServiceValidation(t *testing.T) { }, }, }, - want: apis.ErrDisallowedFields("spec.runLatest.configuration.revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("spec.runLatest.configuration.revisionTemplate.spec.container.lifecycle"), }, { name: "invalid pinned", s: &Service{ @@ -280,8 +281,9 @@ func TestServiceValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "foo", - Image: "hellworld", + Name: "foo", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, @@ -289,7 +291,7 @@ func TestServiceValidation(t *testing.T) { }, }, }, - want: apis.ErrDisallowedFields("spec.pinned.configuration.revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("spec.pinned.configuration.revisionTemplate.spec.container.lifecycle"), }, { name: "invalid release -- too few revisions; nil", s: &Service{ @@ -737,14 +739,15 @@ func TestRunLatestTypeValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "stuart", - Image: "hellworld", + Name: "stuart", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, }, }, - want: apis.ErrDisallowedFields("configuration.revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("configuration.revisionTemplate.spec.container.lifecycle"), }} for _, test := range tests { @@ -799,14 +802,15 @@ func TestPinnedTypeValidation(t *testing.T) { DeprecatedRevisionTemplate: &RevisionTemplateSpec{ Spec: RevisionSpec{ DeprecatedContainer: &corev1.Container{ - Name: "stuart", - Image: "hellworld", + Name: "stuart", + Image: "hellworld", + Lifecycle: &corev1.Lifecycle{}, }, }, }, }, }, - want: apis.ErrDisallowedFields("configuration.revisionTemplate.spec.container.name"), + want: apis.ErrDisallowedFields("configuration.revisionTemplate.spec.container.lifecycle"), }} for _, test := range tests { diff --git a/pkg/apis/serving/v1beta1/configuration_defaults_test.go b/pkg/apis/serving/v1beta1/configuration_defaults_test.go index 7c14115548a5..64c73fbdd482 100644 --- a/pkg/apis/serving/v1beta1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1beta1/configuration_defaults_test.go @@ -41,6 +41,7 @@ func TestConfigurationDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -70,6 +71,7 @@ func TestConfigurationDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -101,6 +103,7 @@ func TestConfigurationDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, diff --git a/pkg/apis/serving/v1beta1/revision_defaults.go b/pkg/apis/serving/v1beta1/revision_defaults.go index 3f2ba93f29d1..44ebbc7722e5 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults.go +++ b/pkg/apis/serving/v1beta1/revision_defaults.go @@ -52,6 +52,10 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { rs.PodSpec.Containers = []corev1.Container{container} }() + if container.Name == "" { + container.Name = cfg.Defaults.UserContainerName(ctx) + } + if container.Resources.Requests == nil { container.Resources.Requests = corev1.ResourceList{} } diff --git a/pkg/apis/serving/v1beta1/revision_defaults_test.go b/pkg/apis/serving/v1beta1/revision_defaults_test.go index e1f474085682..4965ed998250 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults_test.go +++ b/pkg/apis/serving/v1beta1/revision_defaults_test.go @@ -53,6 +53,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -80,6 +81,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(123), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -105,6 +107,7 @@ func TestRevisionDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "foo", VolumeMounts: []corev1.VolumeMount{{ Name: "bar", @@ -123,6 +126,11 @@ func TestRevisionDefaulting(t *testing.T) { Spec: RevisionSpec{ ContainerConcurrency: 1, TimeoutSeconds: ptr.Int64(99), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "foo", + }}, + }, }, }, want: &Revision{ @@ -131,6 +139,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(99), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: "foo", Resources: defaultResources, }}, }, @@ -146,6 +155,7 @@ func TestRevisionDefaulting(t *testing.T) { TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index bb3f02f4f50d..76b31d9b17f7 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -198,12 +198,13 @@ func TestRevisionSpecValidation(t *testing.T) { rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "steve", - Image: "helloworld", + Name: "steve", + Image: "helloworld", + Lifecycle: &corev1.Lifecycle{}, }}, }, }, - want: apis.ErrDisallowedFields("containers[0].name"), + want: apis.ErrDisallowedFields("containers[0].lifecycle"), }, { name: "missing container", rs: &RevisionSpec{ diff --git a/pkg/apis/serving/v1beta1/service_defaults.go b/pkg/apis/serving/v1beta1/service_defaults.go index 6be301174fa8..182640062957 100644 --- a/pkg/apis/serving/v1beta1/service_defaults.go +++ b/pkg/apis/serving/v1beta1/service_defaults.go @@ -19,7 +19,10 @@ package v1beta1 import ( "context" + "k8s.io/apimachinery/pkg/api/equality" + "github.com/knative/pkg/apis" + "github.com/knative/serving/pkg/apis/serving" ) // SetDefaults implements apis.Defaultable @@ -27,7 +30,24 @@ func (s *Service) SetDefaults(ctx context.Context) { ctx = apis.WithinParent(ctx, s.ObjectMeta) s.Spec.SetDefaults(apis.WithinSpec(ctx)) - // TODO(mattmoor): UserInfo stuff. + if ui := apis.GetUserInfo(ctx); ui != nil { + ans := s.GetAnnotations() + if ans == nil { + ans = map[string]string{} + defer s.SetAnnotations(ans) + } + + if apis.IsInUpdate(ctx) { + old := apis.GetBaseline(ctx).(*Service) + if equality.Semantic.DeepEqual(old.Spec, s.Spec) { + return + } + ans[serving.UpdaterAnnotation] = ui.Username + } else { + ans[serving.CreatorAnnotation] = ui.Username + ans[serving.UpdaterAnnotation] = ui.Username + } + } } // SetDefaults implements apis.Defaultable diff --git a/pkg/apis/serving/v1beta1/service_defaults_test.go b/pkg/apis/serving/v1beta1/service_defaults_test.go index ad58d22146de..68b3c75ee852 100644 --- a/pkg/apis/serving/v1beta1/service_defaults_test.go +++ b/pkg/apis/serving/v1beta1/service_defaults_test.go @@ -21,10 +21,13 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/knative/pkg/ptr" + authv1 "k8s.io/api/authentication/v1" corev1 "k8s.io/api/core/v1" + "github.com/knative/pkg/apis" + "github.com/knative/pkg/ptr" "github.com/knative/serving/pkg/apis/config" + "github.com/knative/serving/pkg/apis/serving" ) func TestServiceDefaulting(t *testing.T) { @@ -42,6 +45,7 @@ func TestServiceDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Resources: defaultResources, }}, }, @@ -81,6 +85,7 @@ func TestServiceDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -122,6 +127,7 @@ func TestServiceDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -175,6 +181,7 @@ func TestServiceDefaulting(t *testing.T) { Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, Image: "busybox", Resources: defaultResources, }}, @@ -214,3 +221,131 @@ func TestServiceDefaulting(t *testing.T) { }) } } + +func TestAnnotateUserInfo(t *testing.T) { + const ( + u1 = "oveja@knative.dev" + u2 = "cabra@knative.dev" + u3 = "vaca@knative.dev" + ) + + withUserAnns := func(u1, u2 string, s *Service) *Service { + a := s.GetAnnotations() + if a == nil { + a = map[string]string{} + defer s.SetAnnotations(a) + } + a[serving.CreatorAnnotation] = u1 + a[serving.UpdaterAnnotation] = u2 + return s + } + + tests := []struct { + name string + user string + this *Service + prev *Service + wantAnns map[string]string + }{{ + name: "create-new", + user: u1, + this: &Service{}, + prev: nil, + wantAnns: map[string]string{ + serving.CreatorAnnotation: u1, + serving.UpdaterAnnotation: u1, + }, + }, { + // Old objects don't have the annotation, and unless there's a change in + // data they won't get it. + name: "update-no-diff-old-object", + user: u1, + this: &Service{}, + prev: &Service{}, + wantAnns: map[string]string{}, + }, { + name: "update-no-diff-new-object", + user: u2, + this: withUserAnns(u1, u1, &Service{}), + prev: withUserAnns(u1, u1, &Service{}), + wantAnns: map[string]string{ + serving.CreatorAnnotation: u1, + serving.UpdaterAnnotation: u1, + }, + }, { + name: "update-diff-old-object", + user: u2, + this: &Service{ + Spec: ServiceSpec{ + ConfigurationSpec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ContainerConcurrency: 1, + }, + }, + }, + }, + }, + prev: &Service{ + Spec: ServiceSpec{ + ConfigurationSpec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ContainerConcurrency: 2, + }, + }, + }, + }, + }, + wantAnns: map[string]string{ + serving.UpdaterAnnotation: u2, + }, + }, { + name: "update-diff-new-object", + user: u3, + this: withUserAnns(u1, u2, &Service{ + Spec: ServiceSpec{ + ConfigurationSpec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ContainerConcurrency: 1, + }, + }, + }, + }, + }), + prev: withUserAnns(u1, u2, &Service{ + Spec: ServiceSpec{ + ConfigurationSpec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + ContainerConcurrency: 2, + }, + }, + }, + }, + }), + wantAnns: map[string]string{ + serving.CreatorAnnotation: u1, + serving.UpdaterAnnotation: u3, + }, + }} + + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + ctx := apis.WithUserInfo(context.Background(), &authv1.UserInfo{ + Username: test.user, + }) + if test.prev != nil { + ctx = apis.WithinUpdate(ctx, test.prev) + test.prev.SetDefaults(ctx) + } + test.this.SetDefaults(ctx) + if got, want := test.this.GetAnnotations(), test.wantAnns; !cmp.Equal(got, want) { + t.Errorf("Annotations = %v, want: %v, diff (-got, +want): %s", got, want, cmp.Diff(got, want)) + } + }) + } +} diff --git a/pkg/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index 9b6fc196340f..4d763fee240c 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -17,6 +17,7 @@ limitations under the License. package revision import ( + "context" "testing" "time" @@ -70,7 +71,7 @@ const ( ) func testRevision() *v1alpha1.Revision { - return &v1alpha1.Revision{ + rev := &v1alpha1.Revision{ ObjectMeta: metav1.ObjectMeta{ SelfLink: "/apis/serving/v1alpha1/namespaces/test/revisions/test-rev", Name: "test-rev", @@ -118,6 +119,8 @@ func testRevision() *v1alpha1.Revision { }, }, } + rev.SetDefaults(context.Background()) + return rev } func getTestDeploymentConfig() *deployment.Config { diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 6a2968ceb582..f82fea2668d9 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -85,7 +85,7 @@ func (c *Reconciler) reconcileDeployment(ctx context.Context, rev *v1alpha1.Revi } for _, status := range pod.Status.ContainerStatuses { - if status.Name == resources.UserContainerName { + if status.Name == rev.Spec.GetContainer().Name { if t := status.LastTerminationState.Terminated; t != nil { logger.Infof("%s marking exiting with: %d/%s", rev.Name, t.ExitCode, t.Message) rev.Status.MarkContainerExiting(t.ExitCode, t.Message) diff --git a/pkg/reconciler/revision/resources/constants.go b/pkg/reconciler/revision/resources/constants.go index bc6ead976429..d471c0b4c13e 100644 --- a/pkg/reconciler/revision/resources/constants.go +++ b/pkg/reconciler/revision/resources/constants.go @@ -21,8 +21,6 @@ import ( ) const ( - // UserContainerName is the name of the user-container in the PodSpec - UserContainerName = "user-container" // QueueContainerName is the name of the queue proxy side car QueueContainerName = "queue-proxy" diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 2eeaaafcc250..d483e6ea442e 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -110,8 +110,7 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) { func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observabilityConfig *metrics.ObservabilityConfig, autoscalerConfig *autoscaler.Config, deploymentConfig *deployment.Config) *corev1.PodSpec { userContainer := rev.Spec.GetContainer().DeepCopy() // Adding or removing an overwritten corev1.Container field here? Don't forget to - // update the validations in pkg/webhook.validateContainer. - userContainer.Name = UserContainerName + // update the fieldmasks / validations in pkg/apis/serving userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) userContainer.Lifecycle = userLifecycle diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 0e6d5a1f66de..7c0fcbc3b769 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -43,8 +43,9 @@ import ( ) var ( + containerName = "my-container-name" defaultUserContainer = &corev1.Container{ - Name: UserContainerName, + Name: containerName, Image: "busybox", Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, @@ -126,7 +127,7 @@ var ( Value: pkgmetrics.Domain(), }, { Name: "USER_CONTAINER_NAME", - Value: "user-container", + Value: containerName, }, { Name: "ENABLE_VAR_LOG_COLLECTION", Value: "false", @@ -199,6 +200,7 @@ var ( }, Spec: v1alpha1.RevisionSpec{ DeprecatedContainer: &corev1.Container{ + Name: containerName, Image: "busybox", }, RevisionSpec: v1beta1.RevisionSpec{ diff --git a/pkg/reconciler/revision/resources/queue.go b/pkg/reconciler/revision/resources/queue.go index 02a4756e706d..ab8e097084f0 100644 --- a/pkg/reconciler/revision/resources/queue.go +++ b/pkg/reconciler/revision/resources/queue.go @@ -256,7 +256,7 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, o Value: pkgmetrics.Domain(), }, { Name: "USER_CONTAINER_NAME", - Value: UserContainerName, + Value: rev.Spec.GetContainer().Name, }, { Name: "ENABLE_VAR_LOG_COLLECTION", Value: strconv.FormatBool(observabilityConfig.EnableVarLogCollection), diff --git a/pkg/reconciler/revision/resources/queue_test.go b/pkg/reconciler/revision/resources/queue_test.go index 92190cd97e89..1499072b6dde 100644 --- a/pkg/reconciler/revision/resources/queue_test.go +++ b/pkg/reconciler/revision/resources/queue_test.go @@ -95,6 +95,7 @@ func TestMakeQueueContainer(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ + Name: containerName, Ports: []corev1.ContainerPort{{ ContainerPort: 1955, Name: string(networking.ProtocolH2C), @@ -335,6 +336,14 @@ func TestMakeQueueContainer(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { + if len(test.rev.Spec.PodSpec.Containers) == 0 { + test.rev.Spec.PodSpec = corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: containerName, + }}, + } + } + got := makeQueueContainer(test.rev, test.lc, test.oc, test.ac, test.cc) sortEnv(got.Env) if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { @@ -373,7 +382,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "bar", + Name: containerName, Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ corev1.ResourceName("memory"): resource.MustParse("2Gi"), @@ -430,7 +439,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "bar", + Name: containerName, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("cpu"): resource.MustParse("50m"), @@ -484,7 +493,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "bar", + Name: containerName, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("cpu"): resource.MustParse("50m"), @@ -537,7 +546,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: "bar", + Name: containerName, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceName("memory"): resource.MustParse("900000Pi"), @@ -611,7 +620,7 @@ var defaultEnv = map[string]string{ "SYSTEM_NAMESPACE": system.Namespace(), "METRICS_DOMAIN": pkgmetrics.Domain(), "QUEUE_SERVING_PORT": "8012", - "USER_CONTAINER_NAME": "user-container", + "USER_CONTAINER_NAME": containerName, "ENABLE_VAR_LOG_COLLECTION": "false", "VAR_LOG_VOLUME_NAME": varLogVolumeName, "INTERNAL_VOLUME_PATH": internalVolumePath, @@ -628,20 +637,17 @@ func env(overrides map[string]string) []corev1.EnvVar { }) } - env = append(env, []corev1.EnvVar{ - { - Name: "SERVING_POD", - ValueFrom: &corev1.EnvVarSource{ - FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, - }, + env = append(env, []corev1.EnvVar{{ + Name: "SERVING_POD", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, }, - { - Name: "SERVING_POD_IP", - ValueFrom: &corev1.EnvVarSource{ - FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.podIP"}, - }, + }, { + Name: "SERVING_POD_IP", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "status.podIP"}, }, - }...) + }}...) sortEnv(env) return env diff --git a/test/crd.go b/test/crd.go index d8a2d5204954..b7ef5e32d0a4 100644 --- a/test/crd.go +++ b/test/crd.go @@ -207,7 +207,10 @@ func LatestServiceLegacy(names ResourceNames, options *Options, fopt ...v1alpha1 a := append([]v1alpha1testing.ServiceOption{ v1alpha1testing.WithRunLatestConfigSpec(*LegacyConfigurationSpec(ptest.ImagePath(names.Image), options)), }, fopt...) - return v1alpha1testing.ServiceWithoutNamespace(names.Service, a...) + svc := v1alpha1testing.ServiceWithoutNamespace(names.Service, a...) + // Clear the name, which is put there by defaulting. + svc.Spec.DeprecatedRunLatest.Configuration.GetTemplate().Spec.GetContainer().Name = "" + return svc } // AppendRandomString will generate a random string that begins with prefix. This is useful diff --git a/test/service.go b/test/service.go index 93d58215c381..8d6522e7e367 100644 --- a/test/service.go +++ b/test/service.go @@ -137,6 +137,48 @@ func CreateRunLatestServiceReady(t *testing.T, clients *Clients, names *Resource return resources, err } +// CreateRunLatestServiceLegacyReady creates a new Service in state 'Ready'. This function expects Service and Image name passed in through 'names'. +// Names is updated with the Route and Configuration created by the Service and ResourceObjects is returned with the Service, Route, and Configuration objects. +// Returns error if the service does not come up correctly. +func CreateRunLatestServiceLegacyReady(t *testing.T, clients *Clients, names *ResourceNames, options *Options, fopt ...rtesting.ServiceOption) (*ResourceObjects, error) { + if names.Image == "" { + return nil, fmt.Errorf("expected non-empty Image name; got Image=%v", names.Image) + } + + t.Logf("Creating a new Service %s.", names.Service) + svc, err := CreateLatestServiceLegacy(t, clients, *names, options, fopt...) + if err != nil { + return nil, err + } + + // Populate Route and Configuration Objects with name + names.Route = serviceresourcenames.Route(svc) + names.Config = serviceresourcenames.Configuration(svc) + + // If the Service name was not specified, populate it + if names.Service == "" { + names.Service = svc.Name + } + + t.Logf("Waiting for Service %q to transition to Ready.", names.Service) + if err := WaitForServiceState(clients.ServingClient, names.Service, IsServiceReady, "ServiceIsReady"); err != nil { + return nil, err + } + + t.Log("Checking to ensure Service Status is populated for Ready service", names.Service) + err = validateCreatedServiceStatus(clients, names) + if err != nil { + return nil, err + } + + t.Log("Getting latest objects Created by Service", names.Service) + resources, err := GetResourceObjects(clients, *names) + if err == nil { + t.Log("Successfully created Service", names.Service) + } + return resources, err +} + // CreateLatestService creates a service in namespace with the name names.Service and names.Image func CreateLatestService(t *testing.T, clients *Clients, names ResourceNames, options *Options, fopt ...rtesting.ServiceOption) (*v1alpha1.Service, error) { service := LatestService(names, options, fopt...) diff --git a/test/upgrade/probe_test.go b/test/upgrade/probe_test.go index a0cd87d270d0..8a049ee68c62 100644 --- a/test/upgrade/probe_test.go +++ b/test/upgrade/probe_test.go @@ -50,7 +50,7 @@ func TestProbe(t *testing.T) { } defer test.TearDown(clients, names) - objects, err := test.CreateRunLatestServiceReady(t, clients, &names, &test.Options{}) + objects, err := test.CreateRunLatestServiceLegacyReady(t, clients, &names, &test.Options{}) if err != nil { t.Fatalf("Failed to create Service: %v", err) } diff --git a/test/upgrade/service_preupgrade_test.go b/test/upgrade/service_preupgrade_test.go index a6c5667a8f1a..a21e01079afa 100644 --- a/test/upgrade/service_preupgrade_test.go +++ b/test/upgrade/service_preupgrade_test.go @@ -35,7 +35,7 @@ func TestRunLatestServicePreUpgrade(t *testing.T) { names.Service = serviceName names.Image = test.PizzaPlanet1 - resources, err := test.CreateRunLatestServiceReady(t, clients, &names, &test.Options{}) + resources, err := test.CreateRunLatestServiceLegacyReady(t, clients, &names, &test.Options{}) if err != nil { t.Fatalf("Failed to create Service: %v", err) } @@ -51,7 +51,7 @@ func TestRunLatestServicePreUpgradeAndScaleToZero(t *testing.T) { names.Service = scaleToZeroServiceName names.Image = test.PizzaPlanet1 - resources, err := test.CreateRunLatestServiceReady(t, clients, &names, &test.Options{}) + resources, err := test.CreateRunLatestServiceLegacyReady(t, clients, &names, &test.Options{}) if err != nil { t.Fatalf("Failed to create Service: %v", err) }