Don't default Revision values when BYO name is unchanged. (knative#13565

) (#205) * Don't default Revision values when BYO name is unchanged. Fixes knative#11512 * Switch to using a context-passed ConfigurationSpec reference instead of hard-coding Configuration and Service. * Complete docstring for WithConfigurationSpec * Update naming per dave's feedback Co-authored-by: Evan Anderson <[email protected]>
openshift-knative · Mar 10, 2023 · bd52068 · bd52068
1 parent fb79933
commit bd52068
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 10 deletions.
diff --git a/pkg/apis/serving/v1/configuration_defaults.go b/pkg/apis/serving/v1/configuration_defaults.go
@@ -23,20 +23,49 @@ import (
 	"knative.dev/serving/pkg/apis/serving"
 )
 
+type configSpecKey struct{}
+
+// WithPreviousConfigurationSpec stores the pre-update ConfigurationSpec in the
+// context, to allow ConfigurationSpec.SetDefaults to determine whether the
+// update would create a new Revision.
+func WithPreviousConfigurationSpec(ctx context.Context, spec *ConfigurationSpec) context.Context {
+	return context.WithValue(ctx, configSpecKey{}, spec)
+}
+
+func previousConfigSpec(ctx context.Context) *ConfigurationSpec {
+	if spec, ok := ctx.Value(configSpecKey{}).(*ConfigurationSpec); ok {
+		return spec
+	}
+	return nil
+}
+
 // SetDefaults implements apis.Defaultable
 func (c *Configuration) SetDefaults(ctx context.Context) {
 	ctx = apis.WithinParent(ctx, c.ObjectMeta)
+
+	var prevSpec *ConfigurationSpec
+	if prev, ok := apis.GetBaseline(ctx).(*Configuration); ok && prev != nil {
+		prevSpec = &prev.Spec
+		ctx = WithPreviousConfigurationSpec(ctx, prevSpec)
+	}
+
 	c.Spec.SetDefaults(apis.WithinSpec(ctx))
+
 	if c.GetOwnerReferences() == nil {
-		if apis.IsInUpdate(ctx) {
-			serving.SetUserInfo(ctx, apis.GetBaseline(ctx).(*Configuration).Spec, c.Spec, c)
-		} else {
-			serving.SetUserInfo(ctx, nil, c.Spec, c)
-		}
+		serving.SetUserInfo(ctx, prevSpec, &c.Spec, c)
 	}
 }
 
 // SetDefaults implements apis.Defaultable
 func (cs *ConfigurationSpec) SetDefaults(ctx context.Context) {
+	if prev := previousConfigSpec(ctx); prev != nil {
+		newName := cs.Template.ObjectMeta.Name
+		oldName := prev.Template.ObjectMeta.Name
+		if newName != "" && newName == oldName {
+			// Skip defaulting, to avoid suggesting changes that would conflict with
+			// "BYO RevisionName".
+			return
+		}
+	}
 	cs.Template.SetDefaults(ctx)
 }
diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	authv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/ptr"
@@ -173,6 +174,46 @@ func TestConfigurationDefaulting(t *testing.T) {
 	}
 }
 
+func TestBYORevisionName(t *testing.T) {
+	new := &Configuration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        "thing",
+			Annotations: map[string]string{"annotated": "yes"},
+		},
+		Spec: ConfigurationSpec{
+			Template: RevisionTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "thing-2022",
+				},
+				Spec: RevisionSpec{
+					PodSpec: corev1.PodSpec{
+						Containers: []corev1.Container{{
+							Image: "busybox",
+						}},
+					},
+				},
+			},
+		},
+	}
+
+	old := new.DeepCopy()
+	old.ObjectMeta.Annotations = map[string]string{}
+
+	want := new.DeepCopy()
+
+	ctx := apis.WithinUpdate(context.Background(), old)
+	new.SetDefaults(ctx)
+
+	if diff := cmp.Diff(want, new); diff != "" {
+		t.Errorf("SetDefaults (-want, +got) = %v", diff)
+	}
+
+	new.SetDefaults(context.Background())
+	if cmp.Equal(want, new, ignoreUnexportedResources) {
+		t.Errorf("Expected diff, got none! object: %v", new)
+	}
+}
+
 func TestConfigurationUserInfo(t *testing.T) {
 	const (
 		u1 = "[email protected]"

diff --git a/pkg/apis/serving/v1/service_defaults.go b/pkg/apis/serving/v1/service_defaults.go
@@ -26,13 +26,16 @@ import (
 // SetDefaults implements apis.Defaultable
 func (s *Service) SetDefaults(ctx context.Context) {
 	ctx = apis.WithinParent(ctx, s.ObjectMeta)
-	s.Spec.SetDefaults(apis.WithinSpec(ctx))
 
-	if apis.IsInUpdate(ctx) {
-		serving.SetUserInfo(ctx, apis.GetBaseline(ctx).(*Service).Spec, s.Spec, s)
-	} else {
-		serving.SetUserInfo(ctx, nil, s.Spec, s)
+	var prevSpec *ServiceSpec
+	if prev, ok := apis.GetBaseline(ctx).(*Service); ok && prev != nil {
+		prevSpec = &prev.Spec
+		ctx = WithPreviousConfigurationSpec(ctx, &prev.Spec.ConfigurationSpec)
 	}
+
+	s.Spec.SetDefaults(apis.WithinSpec(ctx))
+	serving.SetUserInfo(ctx, prevSpec, &s.Spec, s)
+
 }
 
 // SetDefaults implements apis.Defaultable