diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 09be210985cc..3590d58e0cda 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -257,7 +257,17 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { errs = errs.Also(ValidateContainer(ps.Containers[0], volumes). ViaFieldIndex("containers", 0)) default: - errs = errs.Also(apis.ErrMultipleOneOf("containers")) + errs = errs.Also(ValidateMultiContainerPorts(ps.Containers)).ViaField("containers") + for i := range ps.Containers { + // probes are not allowed other than serving container + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZU + if len(ps.Containers[i].Ports) == 0 { + errs = errs.Also(ValidateSidecarContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) + } else { + errs = errs.Also(ValidateContainer(ps.Containers[i], volumes).ViaFieldIndex("containers", i)) + } + } + } if ps.ServiceAccountName != "" { for range validation.IsDNS1123Subdomain(ps.ServiceAccountName) { @@ -267,7 +277,58 @@ func ValidatePodSpec(ps corev1.PodSpec) *apis.FieldError { return errs } +// ValidateMultiContainerPorts validates port when specified multiple containers +func ValidateMultiContainerPorts(containers []corev1.Container) *apis.FieldError { + var ( + count int + errs *apis.FieldError + ) + for i := range containers { + count += len(containers[i].Ports) + } + if count == 0 { + errs = errs.Also(apis.ErrMissingField(apis.CurrentField)) + } + return errs.Also(portValidation(count)).ViaField("ports") +} + +// ValidateSidecarContainer validate fields for non serving containers +func ValidateSidecarContainer(container corev1.Container, volumes sets.String) *apis.FieldError { + var errs *apis.FieldError + if container.LivenessProbe != nil { + errs = errs.Also(apis.CheckDisallowedFields(*container.LivenessProbe, + *ProbeMask(&corev1.Probe{})).ViaField("livenessProbe")) + } + if container.ReadinessProbe != nil { + errs = errs.Also(apis.CheckDisallowedFields(*container.ReadinessProbe, + *ProbeMask(&corev1.Probe{})).ViaField("readinessProbe")) + } + return errs.Also(validate(container, volumes)) +} + +// ValidateContainer validate fields for serving containers func ValidateContainer(container corev1.Container, volumes sets.String) *apis.FieldError { + var errs *apis.FieldError + errs = errs.Also(portValidation(len(container.Ports))).ViaField("ports") + // Liveness Probes + errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) + // Readiness Probes + errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe")) + return errs.Also(validate(container, volumes)) +} + +func portValidation(count int) *apis.FieldError { + if count > 1 { + return &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{apis.CurrentField}, + Details: "Only a single port is allowed", + } + } + return nil +} + +func validate(container corev1.Container, volumes sets.String) *apis.FieldError { if equality.Semantic.DeepEqual(container, corev1.Container{}) { return apis.ErrMissingField(apis.CurrentField) } @@ -296,12 +357,8 @@ func ValidateContainer(container corev1.Container, volumes sets.String) *apis.Fi } errs = errs.Also(fe) } - // Liveness Probes - errs = errs.Also(validateProbe(container.LivenessProbe).ViaField("livenessProbe")) // Ports errs = errs.Also(validateContainerPorts(container.Ports).ViaField("ports")) - // Readiness Probes - errs = errs.Also(validateReadinessProbe(container.ReadinessProbe).ViaField("readinessProbe")) // Resources errs = errs.Also(validateResources(&container.Resources).ViaField("resources")) // SecurityContext @@ -391,20 +448,11 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError { if len(ports) == 0 { return nil } - var errs *apis.FieldError // user can set container port which names "user-port" to define application's port. // Queue-proxy will use it to send requests to application // if user didn't set any port, it will set default port user-port=8080. - if len(ports) > 1 { - errs = errs.Also(&apis.FieldError{ - Message: "More than one container port is set", - Paths: []string{apis.CurrentField}, - Details: "Only a single port is allowed", - }) - } - userPort := ports[0] errs = errs.Also(apis.CheckDisallowedFields(userPort, *ContainerPortMask(&userPort))) diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index ba1c7a991bd7..e8fe05ca03cc 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -150,15 +150,92 @@ func TestPodSpecValidation(t *testing.T) { }, want: apis.ErrMissingField("containers"), }, { - name: "too many containers", + name: "more than one container", ps: corev1.PodSpec{ Containers: []corev1.Container{{ Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + }}, + }, + want: nil, + }, { + name: "probes are not allowed for non serving containers", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + LivenessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + ReadinessProbe: &corev1.Probe{ + TimeoutSeconds: 1, + }, + }}, + }, + want: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, + }, + }, { + name: "too many containers with no port", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }, { + Image: "helloworld", + }}, + }, + want: apis.ErrMissingField("containers.ports"), + }, { + name: "too many containers with too many port", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, + }, { + name: "too many containers with too many port for a single container", + ps: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }, { + ContainerPort: 90, + }}, }, { Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }}, }, - want: apis.ErrMultipleOneOf("containers"), + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports, containers[0].ports"}, + Details: "Only a single port is allowed", + }, }, { name: "extra field", ps: corev1.PodSpec{ diff --git a/pkg/apis/serving/v1/revision_defaults.go b/pkg/apis/serving/v1/revision_defaults.go index 01df5b626204..2d272011f3c7 100644 --- a/pkg/apis/serving/v1/revision_defaults.go +++ b/pkg/apis/serving/v1/revision_defaults.go @@ -18,6 +18,7 @@ package v1 import ( "context" + "strconv" corev1 "k8s.io/api/core/v1" "knative.dev/pkg/apis" @@ -51,52 +52,71 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) { for idx := range rs.PodSpec.Containers { if rs.PodSpec.Containers[idx].Name == "" { - rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) - } - - if rs.PodSpec.Containers[idx].Resources.Requests == nil { - rs.PodSpec.Containers[idx].Resources.Requests = corev1.ResourceList{} - } - if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU]; !ok { - if rsrc := cfg.Defaults.RevisionCPURequest; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU] = *rsrc - } - } - if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory]; !ok { - if rsrc := cfg.Defaults.RevisionMemoryRequest; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory] = *rsrc + if len(rs.PodSpec.Containers) > 1 { + rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) + "-" + strconv.Itoa(idx) + } else { + rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx) } } + rs.applyDefault(&rs.PodSpec.Containers[idx], cfg) + } +} - if rs.PodSpec.Containers[idx].Resources.Limits == nil { - rs.PodSpec.Containers[idx].Resources.Limits = corev1.ResourceList{} +func (rs *RevisionSpec) applyDefault(container *corev1.Container, cfg *config.Config) { + if container.Resources.Requests == nil { + container.Resources.Requests = corev1.ResourceList{} + } + if _, ok := container.Resources.Requests[corev1.ResourceCPU]; !ok { + if rc := cfg.Defaults.RevisionCPURequest; rc != nil { + container.Resources.Requests[corev1.ResourceCPU] = *rc } - if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU]; !ok { - if rsrc := cfg.Defaults.RevisionCPULimit; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU] = *rsrc - } + } + if _, ok := container.Resources.Requests[corev1.ResourceMemory]; !ok { + if rm := cfg.Defaults.RevisionMemoryRequest; rm != nil { + container.Resources.Requests[corev1.ResourceMemory] = *rm } - if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory]; !ok { - if rsrc := cfg.Defaults.RevisionMemoryLimit; rsrc != nil { - rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory] = *rsrc - } + } + + if container.Resources.Limits == nil { + container.Resources.Limits = corev1.ResourceList{} + } + if _, ok := container.Resources.Limits[corev1.ResourceCPU]; !ok { + if rc := cfg.Defaults.RevisionCPULimit; rc != nil { + container.Resources.Limits[corev1.ResourceCPU] = *rc } - if rs.PodSpec.Containers[idx].ReadinessProbe == nil { - rs.PodSpec.Containers[idx].ReadinessProbe = &corev1.Probe{} + } + if _, ok := container.Resources.Limits[corev1.ResourceMemory]; !ok { + if rm := cfg.Defaults.RevisionMemoryLimit; rm != nil { + container.Resources.Limits[corev1.ResourceMemory] = *rm } - if rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket == nil && - rs.PodSpec.Containers[idx].ReadinessProbe.HTTPGet == nil && - rs.PodSpec.Containers[idx].ReadinessProbe.Exec == nil { - rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{} + } + if len(rs.PodSpec.Containers) == 1 { + rs.applyProbes(container) + } else { + // If there are multiple containers then default probes will be applied to the container where user specified PORT + // default probes will not be applied for non serving containers + if len(container.Ports) != 0 { + rs.applyProbes(container) } + } - if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 { - rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1 - } + vms := container.VolumeMounts + for i := range vms { + vms[i].ReadOnly = true + } +} - vms := rs.PodSpec.Containers[idx].VolumeMounts - for i := range vms { - vms[i].ReadOnly = true - } +func (*RevisionSpec) applyProbes(container *corev1.Container) { + if container.ReadinessProbe == nil { + container.ReadinessProbe = &corev1.Probe{} + } + if container.ReadinessProbe.TCPSocket == nil && + container.ReadinessProbe.HTTPGet == nil && + container.ReadinessProbe.Exec == nil { + container.ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{} + } + + if container.ReadinessProbe.SuccessThreshold == 0 { + container.ReadinessProbe.SuccessThreshold = 1 } } diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 162cd8451d5d..12feb6620fd6 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -304,6 +304,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "helloworld", }}, @@ -319,10 +322,36 @@ func TestRevisionDefaulting(t *testing.T) { Name: "busybox", Resources: defaultResources, ReadinessProbe: defaultProbe, + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { - Name: "helloworld", - Resources: defaultResources, - ReadinessProbe: defaultProbe, + Name: "helloworld", + Resources: defaultResources, + }}, + }, + }, + }, + }, { + name: "multiple containers without container name", + in: &Revision{ + Spec: RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + }, + want: &Revision{ + Spec: RevisionSpec{ + TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "user-container-0", + Resources: defaultResources, + }, { + Name: "user-container-1", + Resources: defaultResources, }}, }, }, diff --git a/pkg/apis/serving/v1/revision_helpers.go b/pkg/apis/serving/v1/revision_helpers.go index f49762e12496..36571fe22d53 100644 --- a/pkg/apis/serving/v1/revision_helpers.go +++ b/pkg/apis/serving/v1/revision_helpers.go @@ -72,8 +72,15 @@ func (e LastPinnedParseError) Error() string { // It is never nil and should be exactly the specified container as guaranteed // by validation. func (rs *RevisionSpec) GetContainer() *corev1.Container { - if len(rs.Containers) > 0 { + switch { + case len(rs.Containers) == 1: return &rs.Containers[0] + case len(rs.Containers) > 1: + for i := range rs.Containers { + if len(rs.Containers[i].Ports) != 0 { + return &rs.Containers[i] + } + } } // Should be unreachable post-validation, but here to ease testing. return &corev1.Container{} diff --git a/pkg/apis/serving/v1/revision_lifecycle_test.go b/pkg/apis/serving/v1/revision_lifecycle_test.go index 6deb24478df2..37d0e8519109 100644 --- a/pkg/apis/serving/v1/revision_lifecycle_test.go +++ b/pkg/apis/serving/v1/revision_lifecycle_test.go @@ -773,6 +773,9 @@ func TestGetContainer(t *testing.T) { Containers: []corev1.Container{{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, { Name: "secondContainer", Image: "secondImage", @@ -782,6 +785,9 @@ func TestGetContainer(t *testing.T) { want: &corev1.Container{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, }, }} for _, tc := range cases { diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 7b5765a251a1..97de29553e34 100644 --- a/pkg/apis/serving/v1/revision_types.go +++ b/pkg/apis/serving/v1/revision_types.go @@ -136,6 +136,12 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1/revision_validation_test.go b/pkg/apis/serving/v1/revision_validation_test.go index 4cdd94864ad5..fc09d859c555 100644 --- a/pkg/apis/serving/v1/revision_validation_test.go +++ b/pkg/apis/serving/v1/revision_validation_test.go @@ -373,7 +373,7 @@ func TestRevisionSpecValidation(t *testing.T) { }, want: apis.ErrMissingField("containers"), }, { - name: "too many containers", + name: "too many containers without container port", rs: &RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -383,7 +383,44 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "too many containers with one container port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + want: nil, + }, { + name: "too many containers with multiple container port", + rs: &RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + }, + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, }, { name: "exceed max timeout", rs: &RevisionSpec{ diff --git a/pkg/apis/serving/v1/zz_generated.deepcopy.go b/pkg/apis/serving/v1/zz_generated.deepcopy.go index bfff38f19787..ae7f28d4e883 100644 --- a/pkg/apis/serving/v1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1/zz_generated.deepcopy.go @@ -230,6 +230,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion.go b/pkg/apis/serving/v1alpha1/revision_conversion.go index c6314d33f402..1e8f21536207 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion.go @@ -62,10 +62,8 @@ func (source *RevisionSpec) ConvertTo(ctx context.Context, sink *v1.RevisionSpec Volumes: source.Volumes, ImagePullSecrets: source.ImagePullSecrets, } - case len(source.Containers) == 1: + case len(source.Containers) != 0: sink.PodSpec = source.PodSpec - case len(source.Containers) > 1: - return apis.ErrMultipleOneOf("containers") default: return apis.ErrMissingOneOf("container", "containers") } @@ -82,6 +80,7 @@ func (source *RevisionStatus) ConvertTo(ctx context.Context, sink *v1.RevisionSt sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } // ConvertFrom implements apis.Convertible @@ -114,4 +113,5 @@ func (sink *RevisionStatus) ConvertFrom(ctx context.Context, source v1.RevisionS sink.ServiceName = source.ServiceName sink.LogURL = source.LogURL sink.ImageDigest = source.ImageDigest + sink.ImageDigests = source.ImageDigests } diff --git a/pkg/apis/serving/v1alpha1/revision_conversion_test.go b/pkg/apis/serving/v1alpha1/revision_conversion_test.go index 87a2fa553069..58a591a78962 100644 --- a/pkg/apis/serving/v1alpha1/revision_conversion_test.go +++ b/pkg/apis/serving/v1alpha1/revision_conversion_test.go @@ -231,6 +231,9 @@ func TestRevisionConversionError(t *testing.T) { TimeoutSeconds: ptr.Int64(18), ContainerConcurrency: ptr.Int64(53), }, + DeprecatedContainer: &corev1.Container{ + Image: "busybox", + }, }, Status: RevisionStatus{ Status: duckv1.Status{ @@ -244,7 +247,7 @@ func TestRevisionConversionError(t *testing.T) { LogURL: "http://logger.io", }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMultipleOneOf("container, containers"), }, { name: "no containers in podspec", in: &Revision{ diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle.go b/pkg/apis/serving/v1alpha1/revision_lifecycle.go index 7c333281e4da..c961c2613576 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle.go @@ -76,8 +76,15 @@ func (rs *RevisionSpec) GetContainer() *corev1.Container { if rs.DeprecatedContainer != nil { return rs.DeprecatedContainer } - if len(rs.Containers) > 0 { + switch { + case len(rs.Containers) == 1: return &rs.Containers[0] + case len(rs.Containers) > 1: + for i := range rs.Containers { + if len(rs.Containers[i].Ports) != 0 { + return &rs.Containers[i] + } + } } // Should be unreachable post-validation, but here to ease testing. return &corev1.Container{} diff --git a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go index a819e5b37973..c272b2a36f30 100644 --- a/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go +++ b/pkg/apis/serving/v1alpha1/revision_lifecycle_test.go @@ -820,13 +820,16 @@ func TestGetContainer(t *testing.T) { Image: "foo", }, }, { - name: "get first container info even after passing multiple", + name: "get serving container info even if specified multiple containers", status: RevisionSpec{ RevisionSpec: v1.RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "secondContainer", Image: "secondImage", @@ -837,7 +840,26 @@ func TestGetContainer(t *testing.T) { want: &corev1.Container{ Name: "firstContainer", Image: "firstImage", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, + }, { + name: "get empty container if specified multiple containers without container port", + status: RevisionSpec{ + RevisionSpec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "firstContainer", + Image: "firstImage", + }, { + Name: "secondContainer", + Image: "secondImage", + }}, + }, + }, }, + want: &corev1.Container{}, }} for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index e3c4ce8f943e..23d041823f15 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -192,6 +192,12 @@ type RevisionStatus struct { // may be empty if the image comes from a registry listed to skip resolution. // +optional ImageDigest string `json:"imageDigest,omitempty"` + // ImageDigests holds the resolved digest for the image specified + // within .Spec.Container.Image. The digest is resolved during the creation + // of Revision. ImageDigests holds the digest for all the .Spec.Container.Image both serving and non serving. + // ref: https://docs.google.com/document/d/1XjIRnOGaq9UGllkZgYXQHuTQmhbECNAOk6TT6RNfJMw/edit?disco=AAAAEHNSwZg + // +optional + ImageDigests map[string]string `json:"imageDigests,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go index 8d347d821b62..92bff3f318a8 100644 --- a/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/serving/v1alpha1/zz_generated.deepcopy.go @@ -316,6 +316,13 @@ func (in *RevisionSpec) DeepCopy() *RevisionSpec { func (in *RevisionStatus) DeepCopyInto(out *RevisionStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + if in.ImageDigests != nil { + in, out := &in.ImageDigests, &out.ImageDigests + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } diff --git a/pkg/apis/serving/v1beta1/revision_defaults_test.go b/pkg/apis/serving/v1beta1/revision_defaults_test.go index 7666f37b2d18..01456a3d5193 100644 --- a/pkg/apis/serving/v1beta1/revision_defaults_test.go +++ b/pkg/apis/serving/v1beta1/revision_defaults_test.go @@ -259,6 +259,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { Name: "helloworld", }}, @@ -274,10 +277,36 @@ func TestRevisionDefaulting(t *testing.T) { Name: "busybox", Resources: defaultResources, ReadinessProbe: defaultProbe, + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, }, { - Name: "helloworld", - Resources: defaultResources, - ReadinessProbe: defaultProbe, + Name: "helloworld", + Resources: defaultResources, + }}, + }, + }, + }, + }, { + name: "multiple containers without container name", + in: &Revision{ + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{}, {}}, + }, + }, + }, + want: &Revision{ + Spec: v1.RevisionSpec{ + TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "user-container-0", + Resources: defaultResources, + }, { + Name: "user-container-1", + Resources: defaultResources, }}, }, }, diff --git a/pkg/apis/serving/v1beta1/revision_validation_test.go b/pkg/apis/serving/v1beta1/revision_validation_test.go index f8971547e5b4..cd7737a074fb 100644 --- a/pkg/apis/serving/v1beta1/revision_validation_test.go +++ b/pkg/apis/serving/v1beta1/revision_validation_test.go @@ -375,7 +375,7 @@ func TestRevisionSpecValidation(t *testing.T) { }, want: apis.ErrMissingField("containers"), }, { - name: "too many containers", + name: "too many containers without container port", rs: &v1.RevisionSpec{ PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ @@ -385,7 +385,44 @@ func TestRevisionSpecValidation(t *testing.T) { }}, }, }, - want: apis.ErrMultipleOneOf("containers"), + want: apis.ErrMissingField("containers.ports"), + }, { + name: "too many containers with one container port", + rs: &v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + }}, + }, + }, + want: nil, + }, { + name: "too many containers with multiple container port", + rs: &v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }, { + Image: "helloworld", + Ports: []corev1.ContainerPort{{ + ContainerPort: 80, + }}, + }}, + }, + }, + want: &apis.FieldError{ + Message: "More than one container port is set", + Paths: []string{"containers.ports"}, + Details: "Only a single port is allowed", + }, }, { name: "exceed max timeout", rs: &v1.RevisionSpec{ diff --git a/pkg/reconciler/revision/cruds.go b/pkg/reconciler/revision/cruds.go index 6100ed2910b0..9e9f042ac8ac 100644 --- a/pkg/reconciler/revision/cruds.go +++ b/pkg/reconciler/revision/cruds.go @@ -109,8 +109,8 @@ func (c *Reconciler) checkAndUpdateDeployment(ctx context.Context, rev *v1.Revis return d, nil } -func (c *Reconciler) createImageCache(ctx context.Context, rev *v1.Revision) (*caching.Image, error) { - image := resources.MakeImageCache(rev) +func (c *Reconciler) createImageCache(rev *v1.Revision, containerName string) (*caching.Image, error) { + image := resources.MakeImageCache(rev, containerName) return c.cachingclient.CachingV1alpha1().Images(image.Namespace).Create(image) } diff --git a/pkg/reconciler/revision/reconcile_resources.go b/pkg/reconciler/revision/reconcile_resources.go index 13360eae0cd6..b68b8693c8ab 100644 --- a/pkg/reconciler/revision/reconcile_resources.go +++ b/pkg/reconciler/revision/reconcile_resources.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/pkg/kmeta" "knative.dev/pkg/logging" "knative.dev/pkg/logging/logkey" v1 "knative.dev/serving/pkg/apis/serving/v1" @@ -113,18 +114,17 @@ func (c *Reconciler) reconcileImageCache(ctx context.Context, rev *v1.Revision) logger := logging.FromContext(ctx) ns := rev.Namespace - imageName := resourcenames.ImageCache(rev) - _, err := c.imageLister.Images(ns).Get(imageName) - if apierrs.IsNotFound(err) { - _, err := c.createImageCache(ctx, rev) - if err != nil { - return fmt.Errorf("failed to create image cache %q: %w", imageName, err) + for _, container := range rev.Spec.Containers { + imageName := kmeta.ChildName(resourcenames.ImageCache(rev), container.Name) + if _, err := c.imageLister.Images(ns).Get(imageName); apierrs.IsNotFound(err) { + if _, err := c.createImageCache(rev, container.Name); err != nil { + return fmt.Errorf("failed to create image cache %q: %w", imageName, err) + } + logger.Infof("Created image cache %q", imageName) + } else if err != nil { + return fmt.Errorf("failed to get image cache %q: %w", imageName, err) } - logger.Infof("Created image cache %q", imageName) - } else if err != nil { - return fmt.Errorf("failed to get image cache %q: %w", imageName, err) } - return nil } diff --git a/pkg/reconciler/revision/resources/deploy.go b/pkg/reconciler/revision/resources/deploy.go index 1b64963cc6cc..7271dd423b21 100644 --- a/pkg/reconciler/revision/resources/deploy.go +++ b/pkg/reconciler/revision/resources/deploy.go @@ -137,53 +137,31 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) { func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig *tracingconfig.Config, observabilityConfig *metrics.ObservabilityConfig, autoscalerConfig *autoscalerconfig.Config, deploymentConfig *deployment.Config) (*corev1.PodSpec, error) { queueContainer, err := makeQueueContainer(rev, loggingConfig, tracingConfig, observabilityConfig, autoscalerConfig, deploymentConfig) - if err != nil { return nil, fmt.Errorf("failed to create queue-proxy container: %w", err) } - userContainer := rev.Spec.GetContainer().DeepCopy() - // Adding or removing an overwritten corev1.Container field here? Don't forget to - // update the fieldmasks / validations in pkg/apis/serving - - userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) - userContainer.Lifecycle = userLifecycle - userPort := getUserPort(rev) - userPortInt := int(userPort) - userPortStr := strconv.Itoa(userPortInt) - // Replacement is safe as only up to a single port is allowed on the Revision - userContainer.Ports = buildContainerPorts(userPort) - userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr)) - userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) - // Explicitly disable stdin and tty allocation - userContainer.Stdin = false - userContainer.TTY = false - - // Prefer imageDigest from revision if available - if rev.Status.ImageDigest != "" { - userContainer.Image = rev.Status.ImageDigest - } - - if userContainer.TerminationMessagePolicy == "" { - userContainer.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError - } - - if userContainer.ReadinessProbe != nil { - if userContainer.ReadinessProbe.HTTPGet != nil || userContainer.ReadinessProbe.TCPSocket != nil { - // HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the - // user-container instead of via kubelet. - userContainer.ReadinessProbe = nil + containers := []corev1.Container{} + for i := range rev.Spec.PodSpec.Containers { + if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 || len(rev.Spec.PodSpec.Containers) == 1 { + servingContainer := makeServingContainer(rev.Spec.GetContainer().DeepCopy(), rev) + // Prefer imageDigest from revision if available + if rev.Status.ImageDigest != "" { + servingContainer.Image = rev.Status.ImageDigest + } + containers = append(containers, servingContainer) + } else { + multiContainers := makeContainer(rev.Spec.PodSpec.Containers[i].DeepCopy(), rev) + // Prefer imageDigest from revision if available + if v, ok := rev.Status.ImageDigests[multiContainers.Name]; ok { + multiContainers.Image = v + } + containers = append(containers, multiContainers) } } - - // If the client provides probes, we should fill in the port for them. - rewriteUserProbe(userContainer.LivenessProbe, userPortInt) - + containers = append(containers, *queueContainer) podSpec := &corev1.PodSpec{ - Containers: []corev1.Container{ - *userContainer, - *queueContainer, - }, + Containers: containers, Volumes: append([]corev1.Volume{varLogVolume}, rev.Spec.Volumes...), ServiceAccountName: rev.Spec.ServiceAccountName, TerminationGracePeriodSeconds: rev.Spec.TimeoutSeconds, @@ -194,14 +172,45 @@ func makePodSpec(rev *v1.Revision, loggingConfig *logging.Config, tracingConfig if observabilityConfig.EnableVarLogCollection { podSpec.Volumes = append(podSpec.Volumes, internalVolume) } - if autoscalerConfig.EnableGracefulScaledown { podSpec.Volumes = append(podSpec.Volumes, labelVolume) } - return podSpec, nil } +func makeContainer(container *corev1.Container, rev *v1.Revision) corev1.Container { + container.VolumeMounts = append(container.VolumeMounts, varLogVolumeMount) + container.Lifecycle = userLifecycle + container.Env = append(container.Env, getKnativeEnvVar(rev)...) + // Explicitly disable stdin and tty allocation + container.Stdin = false + container.TTY = false + if container.TerminationMessagePolicy == "" { + container.TerminationMessagePolicy = corev1.TerminationMessageFallbackToLogsOnError + } + return *container +} + +func makeServingContainer(servingContainer *corev1.Container, rev *v1.Revision) corev1.Container { + container := makeContainer(servingContainer, rev) + userPort := getUserPort(rev) + userPortInt := int(userPort) + userPortStr := strconv.Itoa(userPortInt) + // Replacement is safe as only up to a single port is allowed on the Revision + container.Ports = buildContainerPorts(userPort) + container.Env = append(container.Env, buildUserPortEnv(userPortStr)) + if container.ReadinessProbe != nil { + if container.ReadinessProbe.HTTPGet != nil || container.ReadinessProbe.TCPSocket != nil { + // HTTP and TCP ReadinessProbes are executed by the queue-proxy directly against the + // user-container instead of via kubelet. + container.ReadinessProbe = nil + } + } + // If the client provides probes, we should fill in the port for them. + rewriteUserProbe(container.LivenessProbe, userPortInt) + return container +} + func getUserPort(rev *v1.Revision) int32 { ports := rev.Spec.GetContainer().Ports diff --git a/pkg/reconciler/revision/resources/deploy_test.go b/pkg/reconciler/revision/resources/deploy_test.go index 8c185fc43055..f371006d2914 100644 --- a/pkg/reconciler/revision/resources/deploy_test.go +++ b/pkg/reconciler/revision/resources/deploy_test.go @@ -43,10 +43,11 @@ import ( ) var ( - containerName = "my-container-name" + servingContainerName = "user-container-0" + sidecarContainerName = "user-container-1" sidecarIstioInjectAnnotation = "sidecar.istio.io/inject" - defaultUserContainer = &corev1.Container{ - Name: containerName, + defaultServingContainer = &corev1.Container{ + Name: servingContainerName, Image: "busybox", Ports: buildContainerPorts(v1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, @@ -55,9 +56,29 @@ var ( Stdin: false, TTY: false, Env: []corev1.EnvVar{{ + Name: "K_REVISION", + Value: "bar", + }, { + Name: "K_CONFIGURATION", + Value: "cfg", + }, { + Name: "K_SERVICE", + Value: "svc", + }, { Name: "PORT", Value: "8080", - }, { + }}, + } + + defaultSidecarContainer = &corev1.Container{ + Name: sidecarContainerName, + Image: "ubuntu", + VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, + Lifecycle: userLifecycle, + TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError, + Stdin: false, + TTY: false, + Env: []corev1.EnvVar{{ Name: "K_REVISION", Value: "bar", }, { @@ -152,7 +173,7 @@ var ( Value: metrics.Domain(), }, { Name: "USER_CONTAINER_NAME", - Value: containerName, + Value: servingContainerName, }, { Name: "ENABLE_VAR_LOG_COLLECTION", Value: "false", @@ -238,12 +259,39 @@ var ( TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Image: "busybox", }}, }, }, } + defaultRevisionWithMultipleContainers = &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + UID: "1234", + Labels: map[string]string{ + serving.ConfigurationLabelKey: "cfg", + serving.ServiceLabelKey: "svc", + serving.RouteLabelKey: "im-a-route", + }, + }, + Spec: v1.RevisionSpec{ + TimeoutSeconds: ptr.Int64(45), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: servingContainerName, + Image: "busybox", + Ports: []corev1.ContainerPort{{ + ContainerPort: 8888, + }}, + }, { + Name: sidecarContainerName, + Image: "ubuntu", + }}, + }, + }, + } ) func refInt64(num int64) *int64 { @@ -262,8 +310,12 @@ func container(container *corev1.Container, opts ...containerOption) corev1.Cont return *container } -func userContainer(opts ...containerOption) corev1.Container { - return container(defaultUserContainer.DeepCopy(), opts...) +func servingContainer(opts ...containerOption) corev1.Container { + return container(defaultServingContainer.DeepCopy(), opts...) +} + +func sidecarContainer(opts ...containerOption) corev1.Container { + return container(defaultSidecarContainer.DeepCopy(), opts...) } func queueContainer(opts ...containerOption) corev1.Container { @@ -286,6 +338,12 @@ func withEnvVar(name, value string) containerOption { } } +func withImage(image string) containerOption { + return func(container *corev1.Container) { + container.Image = image + } +} + func withInternalVolumeMount() containerOption { return func(container *corev1.Container) { container.VolumeMounts = append(container.VolumeMounts, internalVolumeMount) @@ -375,6 +433,14 @@ func revision(opts ...revisionOption) *v1.Revision { return revision } +func revisionWithMultiContainer(opts ...revisionOption) *v1.Revision { + revision := defaultRevisionWithMultipleContainers.DeepCopy() + for _, option := range opts { + option(revision) + } + return revision +} + func withContainerConcurrency(cc int64) revisionOption { return func(revision *v1.Revision) { revision.Spec.ContainerConcurrency = &cc @@ -427,7 +493,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Ports[0].ContainerPort = 8888 }, @@ -471,7 +537,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Ports[0].ContainerPort = 8888 }, @@ -511,7 +577,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), ), @@ -536,9 +602,9 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(func(container *corev1.Container) { - container.Image = "busybox@sha256:deadbeef" - }), + servingContainer( + withImage("busybox@sha256:deadbeef"), + ), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), ), @@ -561,7 +627,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("SERVING_CONFIGURATION", "parent-config"), withEnvVar("CONTAINER_CONCURRENCY", "1"), @@ -581,7 +647,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), withEnvVar("SERVING_READINESS_PROBE", `{"httpGet":{"path":"/","port":8080,"host":"127.0.0.1","scheme":"HTTP","httpHeaders":[{"name":"K-Kubelet-Probe","value":"queue"}]}}`), @@ -606,7 +672,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8080,"host":"127.0.0.1"}}`), @@ -626,7 +692,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withExecReadinessProbe([]string{"echo", "hello"})), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "0"), @@ -652,7 +718,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withLivenessProbe(corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ Path: "/", @@ -685,7 +751,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( withLivenessProbe(corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{ Port: intstr.FromInt(v1.DefaultUserPort), @@ -713,7 +779,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("CONTAINER_CONCURRENCY", "1"), withEnvVar("ENABLE_VAR_LOG_COLLECTION", "true"), @@ -740,7 +806,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer(), + servingContainer(), queueContainer( withEnvVar("DOWNWARD_API_LABELS_PATH", fmt.Sprintf("%s/%s", podInfoVolumePath, metadataLabelsPath)), withPodLabelsVolumeMount(), @@ -783,7 +849,7 @@ func TestMakePodSpec(t *testing.T) { cc: &deployment.Config{}, want: podSpec( []corev1.Container{ - userContainer( + servingContainer( func(container *corev1.Container) { container.Command = []string{"/bin/bash"} container.Args = []string{"-c", "echo Hello world"} @@ -814,6 +880,50 @@ func TestMakePodSpec(t *testing.T) { withEnvVar("SERVING_SERVICE", ""), ), }), + }, { + name: "user-defined user port, queue proxy have PORT env with multiple container", + rev: revisionWithMultiContainer( + withContainerConcurrency(1), + func(revision *v1.Revision) { + revision.Spec.GetContainer().Ports = []corev1.ContainerPort{{ + ContainerPort: 9999, + }} + revision.Status = v1.RevisionStatus{ + ImageDigest: "busybox@sha256:abcdef", + ImageDigests: map[string]string{ + servingContainerName: "busybox@sha256:abcdef", + sidecarContainerName: "ubuntu@sha256:ghijkl", + }, + } + container(revision.Spec.GetContainer(), + withTCPReadinessProbe(), + ) + }, + ), + lc: &logging.Config{}, + tc: &tracingconfig.Config{}, + oc: &metrics.ObservabilityConfig{}, + ac: &autoscalerconfig.Config{}, + cc: &deployment.Config{}, + want: podSpec( + []corev1.Container{ + servingContainer( + func(container *corev1.Container) { + container.Ports[0].ContainerPort = 9999 + }, + withEnvVar("PORT", "9999"), + withImage("busybox@sha256:abcdef"), + ), + + sidecarContainer( + withImage("ubuntu@sha256:ghijkl"), + ), + queueContainer( + withEnvVar("CONTAINER_CONCURRENCY", "1"), + withEnvVar("USER_PORT", "9999"), + withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":9999,"host":"127.0.0.1"}}`), + ), + }), }} for _, test := range tests { @@ -823,21 +933,7 @@ func TestMakePodSpec(t *testing.T) { }) got, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) if err != nil { - t.Fatal("makePodSpec returned errror") - } - if diff := cmp.Diff(test.want, got, quantityComparer); diff != "" { - t.Errorf("makePodSpec (-want, +got) = %v", diff) - } - }) - - t.Run(test.name+"(podspec)", func(t *testing.T) { - quantityComparer := cmp.Comparer(func(x, y resource.Quantity) bool { - return x.Cmp(y) == 0 - }) - - got, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) - if err != nil { - t.Fatal("makePodSpec returned errror") + t.Fatalf("makePodSpec returned error: %v", err) } if diff := cmp.Diff(test.want, got, quantityComparer); diff != "" { t.Errorf("makePodSpec (-want, +got) = %v", diff) @@ -847,16 +943,14 @@ func TestMakePodSpec(t *testing.T) { } func TestMissingProbeError(t *testing.T) { - _, err := MakeDeployment(defaultRevision, + if _, err := MakeDeployment(defaultRevision, &logging.Config{}, &tracingconfig.Config{}, &network.Config{}, &metrics.ObservabilityConfig{}, &autoscalerconfig.Config{}, &deployment.Config{}, - ) - - if err == nil { + ); err == nil { t.Error("expected error from MakeDeployment") } } @@ -950,7 +1044,7 @@ func TestMakeDeployment(t *testing.T) { // Tested above so that we can rely on it here for brevity. podSpec, err := makePodSpec(test.rev, test.lc, test.tc, test.oc, test.ac, test.cc) if err != nil { - t.Fatal("makePodSpec returned errror") + t.Fatalf("makePodSpec returned error: %v", err) } test.want.Spec.Template.Spec = *podSpec got, err := MakeDeployment(test.rev, test.lc, test.tc, test.nc, test.oc, test.ac, test.cc) diff --git a/pkg/reconciler/revision/resources/imagecache.go b/pkg/reconciler/revision/resources/imagecache.go index fd90667a7bac..8eff64c84e8e 100644 --- a/pkg/reconciler/revision/resources/imagecache.go +++ b/pkg/reconciler/revision/resources/imagecache.go @@ -28,15 +28,15 @@ import ( ) // MakeImageCache makes an caching.Image resources from a revision. -func MakeImageCache(rev *v1.Revision) *caching.Image { - image := rev.Status.ImageDigest +func MakeImageCache(rev *v1.Revision, containerName string) *caching.Image { + image := rev.Status.ImageDigests[containerName] if image == "" { image = rev.Spec.GetContainer().Image } img := &caching.Image{ ObjectMeta: metav1.ObjectMeta{ - Name: names.ImageCache(rev), + Name: kmeta.ChildName(names.ImageCache(rev), containerName), Namespace: rev.Namespace, Labels: makeLabels(rev), Annotations: resources.FilterMap(rev.GetAnnotations(), func(k string) bool { diff --git a/pkg/reconciler/revision/resources/imagecache_test.go b/pkg/reconciler/revision/resources/imagecache_test.go index 966a23959974..55b7aada4ec9 100644 --- a/pkg/reconciler/revision/resources/imagecache_test.go +++ b/pkg/reconciler/revision/resources/imagecache_test.go @@ -55,13 +55,15 @@ func TestMakeImageCache(t *testing.T) { }, }, Status: v1.RevisionStatus{ - ImageDigest: "busybox@sha256:deadbeef", + ImageDigests: map[string]string{ + "bar": "busybox@sha256:deadbeef", + }, }, }, want: &caching.Image{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", - Name: "bar-cache", + Name: "bar-cachebar", Labels: map[string]string{ serving.RevisionLabelKey: "bar", serving.RevisionUID: "1234", @@ -104,7 +106,7 @@ func TestMakeImageCache(t *testing.T) { want: &caching.Image{ ObjectMeta: metav1.ObjectMeta{ Namespace: "foo", - Name: "bar-cache", + Name: "bar-cachebar", Labels: map[string]string{ serving.RevisionLabelKey: "bar", serving.RevisionUID: "1234", @@ -129,7 +131,7 @@ func TestMakeImageCache(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := MakeImageCache(test.rev) + got := MakeImageCache(test.rev, test.rev.Name) if diff := cmp.Diff(test.want, got); diff != "" { t.Errorf("MakeImageCache (-want, +got) = %v", diff) } diff --git a/pkg/reconciler/revision/resources/queue_test.go b/pkg/reconciler/revision/resources/queue_test.go index 646bea971edb..aaa123e803c3 100644 --- a/pkg/reconciler/revision/resources/queue_test.go +++ b/pkg/reconciler/revision/resources/queue_test.go @@ -125,7 +125,7 @@ func TestMakeQueueContainer(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Ports: []corev1.ContainerPort{{ ContainerPort: 1955, @@ -406,7 +406,7 @@ func TestMakeQueueContainer(t *testing.T) { if len(test.rev.Spec.PodSpec.Containers) == 0 { test.rev.Spec.PodSpec = corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, }}, } @@ -459,7 +459,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -517,7 +517,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -572,7 +572,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -626,7 +626,7 @@ func TestMakeQueueContainerWithPercentageAnnotation(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: testProbe, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ @@ -707,7 +707,7 @@ func TestProbeGenerationHTTPDefaults(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ @@ -795,7 +795,7 @@ func TestProbeGenerationHTTP(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -897,7 +897,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -936,7 +936,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{}, @@ -994,7 +994,7 @@ func TestTCPProbeGeneration(t *testing.T) { TimeoutSeconds: ptr.Int64(45), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ - Name: containerName, + Name: servingContainerName, Ports: []corev1.ContainerPort{{ ContainerPort: int32(userPort), }}, @@ -1090,7 +1090,7 @@ var defaultEnv = map[string]string{ "SYSTEM_NAMESPACE": system.Namespace(), "METRICS_DOMAIN": metrics.Domain(), "QUEUE_SERVING_PORT": "8012", - "USER_CONTAINER_NAME": containerName, + "USER_CONTAINER_NAME": servingContainerName, "ENABLE_VAR_LOG_COLLECTION": "false", "VAR_LOG_VOLUME_NAME": varLogVolumeName, "INTERNAL_VOLUME_PATH": internalVolumePath, diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 73bc952a45ba..cad73451194f 100644 --- a/pkg/reconciler/revision/revision.go +++ b/pkg/reconciler/revision/revision.go @@ -64,7 +64,7 @@ var _ revisionreconciler.Interface = (*Reconciler)(nil) func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) error { // The image digest has already been resolved. - if rev.Status.ImageDigest != "" { + if rev.Status.ImageDigest != "" && len(rev.Status.ImageDigests) == len(rev.Spec.Containers) { return nil } @@ -78,17 +78,25 @@ func (c *Reconciler) reconcileDigest(ctx context.Context, rev *v1.Revision) erro ServiceAccountName: rev.Spec.ServiceAccountName, ImagePullSecrets: imagePullSecrets, } - digest, err := c.resolver.Resolve(rev.Spec.GetContainer().Image, - opt, cfgs.Deployment.RegistriesSkippingTagResolving) - if err != nil { - err = fmt.Errorf("failed to resolve image to digest: %w", err) - rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, - v1.RevisionContainerMissingMessage( - rev.Spec.GetContainer().Image, err.Error())) - return err - } - rev.Status.ImageDigest = digest + if rev.Status.ImageDigests == nil { + rev.Status.ImageDigests = make(map[string]string, len(rev.Spec.Containers)) + } + for _, container := range rev.Spec.Containers { + digest, err := c.resolver.Resolve(container.Image, + opt, cfgs.Deployment.RegistriesSkippingTagResolving) + if err != nil { + err = fmt.Errorf("failed to resolve image to digest: %w", err) + rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, + v1.RevisionContainerMissingMessage( + container.Image, err.Error())) + return err + } + if len(rev.Spec.Containers) == 1 || len(container.Ports) != 0 { + rev.Status.ImageDigest = digest + } + rev.Status.ImageDigests[container.Name] = digest + } return nil } diff --git a/pkg/reconciler/revision/revision_test.go b/pkg/reconciler/revision/revision_test.go index 05a24ddafd0a..759eecd6f7f0 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -231,7 +231,7 @@ func addResourcesToInformers(t *testing.T, ctx context.Context, rev *v1.Revision fakepainformer.Get(ctx).Informer().GetIndexer().Add(pa) } - imageName := resourcenames.ImageCache(rev) + imageName := kmeta.ChildName(resourcenames.ImageCache(rev), rev.Spec.GetContainer().Name) image, err := fakecachingclient.Get(ctx).CachingV1alpha1().Images(rev.Namespace).Get(imageName, metav1.GetOptions{}) if err != nil { t.Errorf("Caching.Images.Get(%v) = %v", imageName, err) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9655b7abf344..2a99f9b9ee98 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" clientgotesting "k8s.io/client-go/testing" + "knative.dev/caching/pkg/apis/caching/v1alpha1" caching "knative.dev/caching/pkg/apis/caching/v1alpha1" cachingclient "knative.dev/caching/pkg/client/injection/client" kubeclient "knative.dev/pkg/client/injection/kube/client" @@ -84,7 +85,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "first-reconcile", // The first reconciliation Populates the following status properties. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, Key: "foo/first-reconcile", }, { @@ -107,7 +108,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "update-status-failure", // Despite failure, the following status properties are set. - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ Eventf(corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for %q: %v", @@ -134,7 +135,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "create-pa-failure", // Despite failure, the following status properties are set. - WithLogURL, WithInitRevConditions, + WithLogURL, WithImageDigests, WithInitRevConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ @@ -160,7 +161,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "create-user-deploy-failure", // Despite failure, the following status properties are set. - WithLogURL, WithInitRevConditions, + WithLogURL, WithImageDigests, WithInitRevConditions, MarkDeploying("Deploying")), }}, WantEvents: []string{ @@ -175,7 +176,7 @@ func TestReconcile(t *testing.T) { // state (immediately post-creation), and verify that no changes // are necessary. Objects: []runtime.Object{ - rev("foo", "stable-reconcile", WithLogURL, AllUnknownConditions), + rev("foo", "stable-reconcile", WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "stable-reconcile", WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "stable-reconcile"), @@ -189,7 +190,7 @@ func TestReconcile(t *testing.T) { // with our desired spec. Objects: []runtime.Object{ rev("foo", "fix-containers", - WithLogURL, AllUnknownConditions), + WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "fix-containers", WithReachability(asv1a1.ReachabilityUnknown)), changeContainers(deploy(t, "foo", "fix-containers")), image("foo", "fix-containers"), @@ -207,7 +208,7 @@ func TestReconcile(t *testing.T) { }, Objects: []runtime.Object{ rev("foo", "failure-update-deploy", - withK8sServiceName("whateves"), WithLogURL, AllUnknownConditions), + withK8sServiceName("whateves"), WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "failure-update-deploy"), changeContainers(deploy(t, "foo", "failure-update-deploy")), image("foo", "failure-update-deploy"), @@ -227,7 +228,7 @@ func TestReconcile(t *testing.T) { // state (port-Reserve), and verify that no changes are necessary. Objects: []runtime.Object{ rev("foo", "stable-deactivation", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, MarkInactive("NoTraffic", "This thing is inactive.")), pa("foo", "stable-deactivation", WithNoTraffic("NoTraffic", "This thing is inactive.")), @@ -246,7 +247,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-ready", withK8sServiceName("new-stuff"), - WithLogURL, + WithLogURL, WithImageDigests, // When the endpoint and pa are ready, then we will see the // Revision become ready. MarkRevisionReady), @@ -270,7 +271,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-not-ready", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, withK8sServiceName("its-not-confidential"), // When we reconcile a ready state and our pa is in an activating // state, we should see the following mutation. @@ -294,7 +295,7 @@ func TestReconcile(t *testing.T) { WithLogURL, MarkRevisionReady, // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. - MarkInactive("NoTraffic", "This thing is inactive.")), + MarkInactive("NoTraffic", "This thing is inactive."), WithImageDigests), }}, Key: "foo/pa-inactive", }, { @@ -312,7 +313,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pa-inactive", - WithLogURL, MarkRevisionReady, + WithLogURL, WithImageDigests, MarkRevisionReady, withK8sServiceName("pa-inactive-svc"), // When we reconcile an "all ready" revision when the PA // is inactive, we should see the following change. @@ -334,7 +335,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "fix-mutated-pa", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When our reconciliation has to change the service // we should see the following mutations to status. withK8sServiceName("fix-mutated-pa"), WithLogURL, MarkRevisionReady), @@ -350,7 +351,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ rev("foo", "fix-mutated-pa-fail", withK8sServiceName("some-old-stuff"), - WithLogURL, AllUnknownConditions), + WithLogURL, WithImageDigests, AllUnknownConditions), pa("foo", "fix-mutated-pa-fail", WithProtocolType(networking.ProtocolH2C), WithReachability(asv1a1.ReachabilityUnknown)), deploy(t, "foo", "fix-mutated-pa-fail"), image("foo", "fix-mutated-pa-fail"), @@ -382,7 +383,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "deploy-timeout", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the PDE state. MarkProgressDeadlineExceeded("I timed out!")), @@ -403,7 +404,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "deploy-replica-failure", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, // When the revision is reconciled after a Deployment has // timed out, we should see it marked with the FailedCreate state. MarkResourcesUnavailable("FailedCreate", "I replica failed!")), @@ -422,7 +423,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pull-backoff", - WithLogURL, AllUnknownConditions, + WithLogURL, WithImageDigests, AllUnknownConditions, MarkResourcesUnavailable("ImagePullBackoff", "can't pull it")), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -445,7 +446,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-error", - WithLogURL, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!"))), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkContainerExiting(5, v1.RevisionContainerExitingMessage("I failed man!"))), }}, Key: "foo/pod-error", }, { @@ -463,7 +464,7 @@ func TestReconcile(t *testing.T) { }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "pod-schedule-error", - WithLogURL, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkResourcesUnavailable("Insufficient energy", "Unschedulable")), }}, Key: "foo/pod-schedule-error", }, { @@ -480,7 +481,7 @@ func TestReconcile(t *testing.T) { image("foo", "steady-ready"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "steady-ready", withK8sServiceName("steadier-even"), WithLogURL, + Object: rev("foo", "steady-ready", withK8sServiceName("steadier-even"), WithLogURL, WithImageDigests, // All resources are ready to go, we should see the revision being // marked ready MarkRevisionReady), @@ -500,7 +501,7 @@ func TestReconcile(t *testing.T) { image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, + Object: rev("foo", "missing-owners", withK8sServiceName("lesser-revision"), WithLogURL, WithImageDigests, MarkRevisionReady, // When we're missing the OwnerRef for PodAutoscaler we see this update. MarkResourceNotOwned("PodAutoscaler", "missing-owners")), @@ -520,7 +521,7 @@ func TestReconcile(t *testing.T) { image("foo", "missing-owners"), }, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, + Object: rev("foo", "missing-owners", withK8sServiceName("youre-gonna-lose"), WithLogURL, WithImageDigests, MarkRevisionReady, // When we're missing the OwnerRef for Deployment we see this update. MarkResourceNotOwned("Deployment", "missing-owners-deployment")), @@ -543,7 +544,7 @@ func TestReconcile(t *testing.T) { WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: rev("foo", "image-pull-secrets", WithImagePullSecrets("foo-secret"), - WithLogURL, AllUnknownConditions, MarkDeploying("Deploying")), + WithLogURL, WithImageDigests, AllUnknownConditions, MarkDeploying("Deploying")), }}, Key: "foo/image-pull-secrets", }} @@ -697,7 +698,11 @@ func image(namespace, name string, co ...configOption) *caching.Image { opt(config) } - return resources.MakeImageCache(rev(namespace, name)) + var image = &v1alpha1.Image{} + for _, container := range rev(namespace, name).Spec.Containers { + image = resources.MakeImageCache(rev(namespace, name), container.Name) + } + return image } func pa(namespace, name string, ko ...PodAutoscalerOption) *asv1a1.PodAutoscaler { diff --git a/pkg/testing/v1/revision.go b/pkg/testing/v1/revision.go index 40a5594704f4..5f437d1a018a 100644 --- a/pkg/testing/v1/revision.go +++ b/pkg/testing/v1/revision.go @@ -74,6 +74,13 @@ func WithLogURL(r *v1.Revision) { r.Status.LogURL = "http://logger.io/test-uid" } +// WithImageDigests sets the .Status.LogURL to the expected value. +func WithImageDigests(r *v1.Revision) { + r.Status.ImageDigests = map[string]string{ + "user-container": "", + } +} + // WithCreationTimestamp sets the Revision's timestamp to the provided time. // TODO(mattmoor): Ideally this could be a more generic Option and use meta.Accessor, // but unfortunately Go's type system cannot support that.