diff --git a/pkg/apis/messaging/v1beta1/kafka_channel_validation.go b/pkg/apis/messaging/v1beta1/kafka_channel_validation.go index 753ce38884..1fd3228b94 100644 --- 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 index 3d049b7b41..a470b36b06 100644 --- 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 {