Allow users to specify their container name.

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"

docs/spec/spec.md

@@ -377,6 +377,7 @@ otherwise noted.
 
 ```yaml
 container: # v1.Container
+  name: ... # Optional
   args: [...] # Optional
   command: [...] # Optional
   env: ... # Optional

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()
+}

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)
+			}
+		})
 	}
 }

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)
 		}
 	})

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

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{{}},

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

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,18 +666,16 @@ 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(
 			apis.ErrDisallowedFields("volumeDevices")),
 	}, {
 		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")),
 	}}
 

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))
 }