Skip to content

Commit

Permalink
fix: disallow updating an existing isbsvc's persistence strategy (#1376)
Browse files Browse the repository at this point in the history 
Signed-off-by: Keran Yang <yangkr920208@gmail.com>
  • Loading branch information
@KeranYang
KeranYang committed Nov 18, 2023
1 parent 92b8a8d commit 44a38f4
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 5 deletions.
22 changes: 20 additions & 2 deletions webhook/validator/isbsvc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"

admissionv1 "k8s.io/api/admission/v1"
"k8s.io/apimachinery/pkg/api/equality"

dfv1 "github.com/numaproj/numaflow/pkg/apis/numaflow/v1alpha1"
isbsvccontroller "github.com/numaproj/numaflow/pkg/reconciler/isbsvc"
Expand All @@ -42,17 +43,34 @@ func (v *isbsvcValidator) ValidateCreate(_ context.Context) *admissionv1.Admissi
}

func (v *isbsvcValidator) ValidateUpdate(_ context.Context) *admissionv1.AdmissionResponse {
// check the new ISB Service is valid
if err := isbsvccontroller.ValidateInterStepBufferService(v.newISBService); err != nil {
return DeniedResponse(err.Error())
}
switch {
case v.oldISBService.Spec.JetStream != nil:
// check the type of ISB Service is not changed
if v.newISBService.Spec.Redis != nil {
return DeniedResponse("Can not change ISB Service type from Jetstream to Redis")
return DeniedResponse("can not change ISB Service type from Jetstream to Redis")
}
// check the persistence of ISB Service is not changed
if !equality.Semantic.DeepEqual(v.oldISBService.Spec.JetStream.Persistence, v.newISBService.Spec.JetStream.Persistence) {
return DeniedResponse("can not change persistence of Jetstream ISB Service")
}
case v.oldISBService.Spec.Redis != nil:
// check the type of ISB Service is not changed
if v.newISBService.Spec.JetStream != nil {
return DeniedResponse("Can not change ISB Service type from Redis to Jetstream")
return DeniedResponse("can not change ISB Service type from Redis to Jetstream")
}
// nil check for Redis Native, if one of them is nil and the other is not, it is NOT allowed
if oldRedisNative, newRedisNative := v.oldISBService.Spec.Redis.Native, v.newISBService.Spec.Redis.Native; oldRedisNative != nil && newRedisNative == nil {
return DeniedResponse("can not remove Redis Native from Redis ISB Service")
} else if oldRedisNative == nil && newRedisNative != nil {
return DeniedResponse("can not add Redis Native to Redis ISB Service")
}
// check the persistence of ISB Service is not changed
if oldRedisNative, newRedisNative := v.oldISBService.Spec.Redis.Native, v.newISBService.Spec.Redis.Native; oldRedisNative != nil && newRedisNative != nil && !equality.Semantic.DeepEqual(oldRedisNative.Persistence, newRedisNative.Persistence) {
return DeniedResponse("can not change persistence of Redis ISB Service")
}
}
return AllowedResponse()
Expand Down
103 changes: 103 additions & 0 deletions webhook/validator/isbsvc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"testing"

"github.com/stretchr/testify/assert"
apiresource "k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

dfv1 "github.com/numaproj/numaflow/pkg/apis/numaflow/v1alpha1"
)
Expand All @@ -42,6 +44,107 @@ func TestValidateISBServiceUpdate(t *testing.T) {
{name: "changing ISB Service type is not allowed - redis to jetstream", old: fakeRedisISBSvc(), new: fakeJetStreamISBSvc(), want: false},
{name: "changing ISB Service type is not allowed - jetstream to redis", old: fakeJetStreamISBSvc(), new: fakeRedisISBSvc(), want: false},
{name: "valid new ISBSvc spec", old: fakeRedisISBSvc(), new: fakeRedisISBSvc(), want: true},
{name: "removing persistence is not allowed - jetstream", old: fakeJetStreamISBSvc(),
new: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
JetStream: &dfv1.JetStreamBufferService{
Version: "1.1.1",
}}}, want: false},
{name: "removing persistence is not allowed - redis", old: fakeJetStreamISBSvc(),
new: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
Redis: &dfv1.RedisBufferService{
Native: &dfv1.NativeRedis{
Version: "6.2.6",
},
},
},
}, want: false},
{name: "adding persistence is not allowed - jetstream", old: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
JetStream: &dfv1.JetStreamBufferService{
Version: "1.1.1",
}}},
new: fakeJetStreamISBSvc(), want: false},
{name: "adding persistence is not allowed - redis", old: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
Redis: &dfv1.RedisBufferService{
Native: &dfv1.NativeRedis{
Version: "6.2.6",
},
},
},
},
new: fakeRedisISBSvc(), want: false},
{name: "changing persistence is not allowed - jetstream", old: fakeRedisISBSvc(),
new: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
JetStream: &dfv1.JetStreamBufferService{
Version: "1.1.1",
Persistence: &dfv1.PersistenceStrategy{
StorageClassName: &testStorageClassName,
VolumeSize: &apiresource.Quantity{},
},
},
},
}, want: false},
{name: "changing persistence is not allowed - redis", old: fakeRedisISBSvc(),
new: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
Redis: &dfv1.RedisBufferService{
Native: &dfv1.NativeRedis{
Version: "6.2.6",
Persistence: &dfv1.PersistenceStrategy{
StorageClassName: &testStorageClassName,
VolumeSize: &apiresource.Quantity{},
},
},
},
},
}, want: false},
{name: "changing redis isbsvc native from nil to non-nil", old: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
Redis: &dfv1.RedisBufferService{Native: nil},
},
}, new: fakeRedisISBSvc(), want: false},
{name: "changing redis isbsvc native from non-nil to nil", old: fakeRedisISBSvc(),
new: &dfv1.InterStepBufferService{
ObjectMeta: metav1.ObjectMeta{
Namespace: testNamespace,
Name: dfv1.DefaultISBSvcName,
},
Spec: dfv1.InterStepBufferServiceSpec{
Redis: &dfv1.RedisBufferService{Native: nil},
},
}, want: false},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand Down
13 changes: 10 additions & 3 deletions webhook/validator/test_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ const (
)

var (
fakeK8sClient = fakeClient.NewSimpleClientset()
fakePipelineClient = fake.FakePipelines{}
fakeNumaClient = fake.FakeNumaflowV1alpha1{}
fakeK8sClient = fakeClient.NewSimpleClientset()
fakePipelineClient = fake.FakePipelines{}
fakeNumaClient = fake.FakeNumaflowV1alpha1{}
testStorageClassName = "test-sc"
)

func fakeRedisISBSvc() *dfv1.InterStepBufferService {
Expand All @@ -28,6 +29,9 @@ func fakeRedisISBSvc() *dfv1.InterStepBufferService {
Redis: &dfv1.RedisBufferService{
Native: &dfv1.NativeRedis{
Version: "6.2.6",
Persistence: &dfv1.PersistenceStrategy{
StorageClassName: &testStorageClassName,
},
},
},
},
Expand All @@ -43,6 +47,9 @@ func fakeJetStreamISBSvc() *dfv1.InterStepBufferService {
Spec: dfv1.InterStepBufferServiceSpec{
JetStream: &dfv1.JetStreamBufferService{
Version: "1.1.1",
Persistence: &dfv1.PersistenceStrategy{
StorageClassName: &testStorageClassName,
},
},
},
}
Expand Down

0 comments on commit 44a38f4

Please sign in to comment.