diff --git a/pkg/apis/config/defaults.go b/pkg/apis/config/defaults.go index ea4d47e13011..915560d668d0 100644 --- a/pkg/apis/config/defaults.go +++ b/pkg/apis/config/defaults.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "math" "strconv" + "strings" "text/template" corev1 "k8s.io/api/core/v1" @@ -68,6 +69,18 @@ func defaultConfig() *Defaults { func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) { nc := defaultConfig() + // Process bool fields. + b := struct { + key string + field *bool + defaultValue bool + }{ + key: "enable-multi-container", + field: &nc.EnableMultiContainer, + defaultValue: false, + } + nc.EnableMultiContainer = strings.EqualFold(data[b.key], "true") + // Process int64 fields for _, i64 := range []struct { key string @@ -158,6 +171,9 @@ func NewDefaultsConfigFromConfigMap(config *corev1.ConfigMap) (*Defaults, error) // Defaults includes the default values to be populated by the webhook. type Defaults struct { + // Feature flag to enable multi container support + EnableMultiContainer bool + RevisionTimeoutSeconds int64 // This is the timeout set for cluster ingress. // RevisionTimeoutSeconds must be less than this value. diff --git a/pkg/apis/serving/v1/revision_types.go b/pkg/apis/serving/v1/revision_types.go index 7b5765a251a1..759384a98dfd 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: http://bit.ly/image-digests + // +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/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_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index e3c4ce8f943e..8c1b8cddd39b 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: http://bit.ly/image-digests + // +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/reconciler/revision/queueing_test.go b/pkg/reconciler/revision/queueing_test.go index 7a10c480080e..323333e30ff8 100644 --- a/pkg/reconciler/revision/queueing_test.go +++ b/pkg/reconciler/revision/queueing_test.go @@ -110,6 +110,23 @@ func testRevision() *v1.Revision { rev.SetDefaults(context.Background()) return rev } +func testRevisionForMultipleContainer() *v1.Revision { + return &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rev-multi-container", + Namespace: testNamespace, + }, + Spec: v1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "gcr.io/repo/image", + }, { + Image: "docker.io/repo/image", + }}, + }, + }, + } +} func getTestDeploymentConfig() *deployment.Config { c, _ := deployment.NewConfigFromConfigMap(getTestDeploymentConfigMap()) @@ -117,6 +134,12 @@ func getTestDeploymentConfig() *deployment.Config { return c } +func getTestDefaultConfig() *config.Defaults { + c, _ := config.NewDefaultsConfigFromConfigMap(getTestDefaultsConfigMap()) + // ignoring error as test controller is generated + return c +} + func getTestDeploymentConfigMap() *corev1.ConfigMap { return &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/reconciler/revision/revision.go b/pkg/reconciler/revision/revision.go index 73bc952a45ba..02882088c2bd 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,31 @@ 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 + 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.ImageDigest = digest + // In order to keep consistency with original behavior of single container handled below scenario + // revision status should not contains imageDigests field in these scenario + // 1. If flag is disabled + // 2. If flag enabled but applied revision has single container + if cfgs.Defaults.EnableMultiContainer && len(rev.Spec.Containers) > 1 { + 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 6b4120b616a4..5a2094231b0d 100644 --- a/pkg/reconciler/revision/revision_test.go +++ b/pkg/reconciler/revision/revision_test.go @@ -338,6 +338,40 @@ func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { } } +func TestRevWithImageDigests(t *testing.T) { + deploymentConfig := getTestDeploymentConfig() + ctx, _, controller, _ := newTestControllerWithConfig(t, deploymentConfig, []*corev1.ConfigMap{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace(), + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-multi-container": "true", + }, + }, + }) + // Flag is enabled and spec contains single container then rev status should not have image digests field + createdRev := getCreatedRev(ctx, t, controller, testRevision()) + if len(createdRev.Status.ImageDigests) != 0 { + t.Errorf("Revision status does not have imageDigests") + } + // Flag is enabled and spec contains multiple container so rev status should contain image digests field + createdRev = getCreatedRev(ctx, t, controller, testRevisionForMultipleContainer()) + if len(createdRev.Status.ImageDigests) == 0 { + t.Errorf("Revision status does not have imageDigests") + } +} + +func getCreatedRev(ctx context.Context, t *testing.T, controller *controller.Impl, rev *v1.Revision) *v1.Revision { + revClient := fakeservingclient.Get(ctx).ServingV1().Revisions(testNamespace) + createRevision(t, ctx, controller, rev) + createdRev, err := revClient.Get(rev.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Couldn't get revision: %v", err) + } + return createdRev +} + // TODO(mattmoor): Remove when we have coverage of EnqueueEndpointsRevision func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) { ctx, cancel, _, ctl, _ := newTestController(t) diff --git a/pkg/reconciler/revision/table_test.go b/pkg/reconciler/revision/table_test.go index 9655b7abf344..08d9ca697fc1 100644 --- a/pkg/reconciler/revision/table_test.go +++ b/pkg/reconciler/revision/table_test.go @@ -747,5 +747,6 @@ func ReconcilerTestConfig() *config.Config { Logging: &logging.Config{}, Tracing: &tracingconfig.Config{}, Autoscaler: &autoscalerconfig.Config{}, + Defaults: getTestDefaultConfig(), } }