Allow retention duration empty to default (#954)

* Allow the retentionDuration to be changed from empty to the default * Also allow empty string to P0D * imports * imports * imports * imports * Simplify to allow any change from empty-string * Don't allow other immutable changes just because retentionDuration is changing * style * Linter doesn't care for passing nil contexts * Skip the recursion and just alter the ignoreArguments * imports
knative-extensions · Oct 27, 2021 · 90e6e62 · 90e6e62
1 parent a7255df
commit 90e6e62
Show file tree

Hide file tree

Showing 2 changed files with 135 additions and 2 deletions.
diff --git a/pkg/apis/messaging/v1beta1/kafka_channel_validation.go b/pkg/apis/messaging/v1beta1/kafka_channel_validation.go
@@ -20,6 +20,8 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"knative.dev/eventing/pkg/apis/eventing"
 	"knative.dev/pkg/apis"
@@ -82,8 +84,20 @@ func (kc *KafkaChannel) CheckImmutableFields(_ context.Context, original *KafkaC
 		return nil
 	}
 
-	ignoreArguments := cmpopts.IgnoreFields(KafkaChannelSpec{}, "ChannelableSpec")
-	if diff, err := kmp.ShortDiff(original.Spec, kc.Spec, ignoreArguments); err != nil {
+	ignoreArguments := []cmp.Option{cmpopts.IgnoreFields(KafkaChannelSpec{}, "ChannelableSpec")}
+
+	// In the specific case of the original RetentionDuration being an empty string, allow it
+	// as an exception to the immutability requirement.
+	//
+	// KafkaChannels created pre-v0.26 will not have a RetentionDuration field (thus an empty
+	// string), and in v0.26 there is a post-install job that updates this to its proper value.
+	// This immutability check was added after the post-install job, and without this exception
+	// it will fail attempting to upgrade those pre-v0.26 channels.
+	if original.Spec.RetentionDuration == "" && kc.Spec.RetentionDuration != "" {
+		ignoreArguments = append(ignoreArguments, cmpopts.IgnoreFields(KafkaChannelSpec{}, "RetentionDuration"))
+	}
+
+	if diff, err := kmp.ShortDiff(original.Spec, kc.Spec, ignoreArguments...); err != nil {
 		return &apis.FieldError{
 			Message: "Failed to diff KafkaChannel",
 			Paths:   []string{"spec"},

diff --git a/pkg/apis/messaging/v1beta1/kafka_channel_validation_test.go b/pkg/apis/messaging/v1beta1/kafka_channel_validation_test.go
@@ -27,6 +27,8 @@ import (
 	"knative.dev/eventing/pkg/apis/eventing"
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/webhook/resourcesemantics"
+
+	"knative.dev/eventing-kafka/pkg/common/constants"
 )
 
 func TestKafkaChannelValidation(t *testing.T) {
@@ -328,6 +330,123 @@ func TestKafkaChannelImmutability(t *testing.T) {
 				}
 			}(),
 		},
+		"updating immutable retentionDuration (empty to default, immutability exception)": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: constants.DefaultRetentionISO8601Duration,
+				},
+			},
+		},
+		"updating immutable retentionDuration (empty to canonical zero P0D, immutability exception)": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "P0D",
+				},
+			},
+		},
+		"updating immutable retentionDuration (non-empty to default)": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "PT1H",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: constants.DefaultRetentionISO8601Duration,
+				},
+			},
+			want: func() *apis.FieldError {
+				return &apis.FieldError{
+					Message: "Immutable fields changed (-old +new)",
+					Paths:   []string{"spec"},
+					Details: "{v1beta1.KafkaChannelSpec}.RetentionDuration:\n\t-: \"PT1H\"\n\t+: \"PT168H\"\n",
+				}
+			}(),
+		},
+		"updating immutable retentionDuration (empty to non-default, immutability exception)": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "PT100H",
+				},
+			},
+		},
+		"updating immutable retentionDuration (immutability exception) and numPartitions": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     2,
+					ReplicationFactor: 1,
+					RetentionDuration: "PT100H",
+				},
+			},
+			want: func() *apis.FieldError {
+				return &apis.FieldError{
+					Message: "Immutable fields changed (-old +new)",
+					Paths:   []string{"spec"},
+					Details: "{v1beta1.KafkaChannelSpec}.NumPartitions:\n\t-: \"1\"\n\t+: \"2\"\n",
+				}
+			}(),
+		},
+		"updating immutable retentionDuration (immutability exception) and numPartitions and replicationFactor": {
+			original: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     1,
+					ReplicationFactor: 1,
+					RetentionDuration: "",
+				},
+			},
+			updated: &KafkaChannel{
+				Spec: KafkaChannelSpec{
+					NumPartitions:     2,
+					ReplicationFactor: 3,
+					RetentionDuration: "PT100H",
+				},
+			},
+			want: func() *apis.FieldError {
+				return &apis.FieldError{
+					Message: "Immutable fields changed (-old +new)",
+					Paths:   []string{"spec"},
+					Details: "{v1beta1.KafkaChannelSpec}.NumPartitions:\n\t-: \"1\"\n\t+: \"2\"\n{v1beta1.KafkaChannelSpec}.ReplicationFactor:\n\t-: \"1\"\n\t+: \"3\"\n",
+				}
+			}(),
+		},
 	}
 
 	for n, test := range testCases {