Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(validation): add webhook validations for CVC replica scale #1621

Merged
merged 7 commits into from
Mar 5, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 25 additions & 31 deletions pkg/webhook/cvc.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ func validateCVCSpecChanges(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error {
validateFuncList := []validateFunc{validateReplicaCount,
validatePoolListChanges,
validateReplicaScaling,
validateStatusPoolList,
}
for _, f := range validateFuncList {
err := f(cvcOldObj, cvcNewObj)
Expand Down Expand Up @@ -106,12 +105,14 @@ func validateReplicaCount(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error {
// validatePoolListChanges returns error if user modified existing pool names with new
// pool name(s) or if user performed more than one replica scale down at a time
func validatePoolListChanges(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error {
oldDesiredPoolNames := cvc.GetDesiredReplicaPoolNames(cvcOldObj)
// Check the new CVC spec changes with old CVC status(Comparing with status
// is more appropriate than comparing with spec)
oldCurrentPoolNames := cvcOldObj.Status.PoolInfo
newDesiredPoolNames := cvc.GetDesiredReplicaPoolNames(cvcNewObj)
modifiedPoolNames := util.ListDiff(oldDesiredPoolNames, newDesiredPoolNames)
if len(newDesiredPoolNames) >= len(oldDesiredPoolNames) {
// If no.of pools on new spec >= no.of pools in old spec(scaleup as well
// as migration then there all the pools in old spec must present in new
modifiedPoolNames := util.ListDiff(oldCurrentPoolNames, newDesiredPoolNames)
if len(newDesiredPoolNames) >= len(oldCurrentPoolNames) {
// If no.of pools on new spec >= no.of pools in old status(scaleup as well
// as migration case then all the pools in old status must present in new
// spec)
if len(modifiedPoolNames) > 0 {
return errors.Errorf(
Expand All @@ -120,7 +121,7 @@ func validatePoolListChanges(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error
)
}
} else {
// If no.of pools in new spec < no.of pools in old spec(scale down
// If no.of pools in new spec < no.of pools in old status(scale down
// volume replica case) then there should at most one change in
// oldSpec.PoolInfo - newSpec.PoolInfo
if len(modifiedPoolNames) > 1 {
Expand All @@ -130,6 +131,23 @@ func validatePoolListChanges(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error
)
}
}
// Reject the request if someone perform scaling when CVC is not in Bound
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this new changes to top of function

// state
// NOTE: We should not reject the controller request which Updates status as
// Bound as well as pool info in status and spec
// TODO: Make below check as cvcOldObj.ISBound()
// If CVC Status is not bound then reject
if cvcOldObj.Status.Phase != apis.CStorVolumeClaimPhaseBound {
// If controller is updating pool info then new CVC will be in bound state
if cvcNewObj.Status.Phase != apis.CStorVolumeClaimPhaseBound &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this check

Copy link
Author

@mittachaitu mittachaitu Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Performed scaling operation on CVC
len(oldCurrentPoolNames) != len(newDesiredPoolNames) {
return errors.Errorf(
"Can't perform scaling of volume replicas when CVC is not in %s state",
apis.CStorVolumeClaimPhaseBound,
)
}
}
return nil
}

Expand All @@ -148,30 +166,6 @@ func validateReplicaScaling(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error {
return nil
}

// validateStatusPoolList return error if content of pools under status doesn't
// match with desired pool names(spec.poolNames)
// NOTE: This validation is only to reject invalid changes of pool information
// on CVC status.
func validateStatusPoolList(cvcOldObj, cvcNewObj *apis.CStorVolumeClaim) error {
replicaPoolNames := []string{}
if len(cvcOldObj.Spec.Policy.ReplicaPoolInfo) == 0 &&
len(cvcOldObj.Status.PoolInfo) == 0 {
// Might be a case where controller updating Bound status along with
// spec and status pool names
replicaPoolNames = cvc.GetDesiredReplicaPoolNames(cvcNewObj)
} else {
replicaPoolNames = cvc.GetDesiredReplicaPoolNames(cvcOldObj)
}
// get pool names which are in status but not under spec
invalidStatusPoolNames := util.ListDiff(cvcNewObj.Status.PoolInfo, replicaPoolNames)
if len(invalidStatusPoolNames) > 0 {
return errors.Errorf("invalid poolInfo %v changes on CVC status",
invalidStatusPoolNames,
)
}
return nil
}

// validatePoolNames returns error if there is repeatition of pool names either
// under spec or status of cvc
func validatePoolNames(cvcObj *apis.CStorVolumeClaim) error {
Expand Down
77 changes: 31 additions & 46 deletions pkg/webhook/cvc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
Name: "cvc1",
Namespace: "openebs",
},
Status: apis.CStorVolumeClaimStatus{
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
getCVCObj: fakeGetCVCError,
Expand All @@ -82,6 +85,9 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
Spec: apis.CStorVolumeClaimSpec{
ReplicaCount: 3,
},
Status: apis.CStorVolumeClaimStatus{
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -91,6 +97,9 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
Spec: apis.CStorVolumeClaimSpec{
ReplicaCount: 4,
},
Status: apis.CStorVolumeClaimStatus{
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
getCVCObj: getCVCObject,
Expand Down Expand Up @@ -121,6 +130,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
},
Status: apis.CStorVolumeClaimStatus{
Phase: apis.CStorVolumeClaimPhaseBound,
PoolInfo: []string{"pool1", "pool2", "pool3"},
},
},
Expand All @@ -145,6 +155,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -165,6 +176,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
Expand All @@ -188,6 +200,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -207,6 +220,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
Expand All @@ -230,6 +244,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -250,6 +265,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
Expand All @@ -273,6 +289,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -290,29 +307,20 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
getCVCObj: getCVCObject,
},
"When Status Was Updated With Non Spec Pool Names": {
"When ScaleUp was Performed Before CVC In Bound State": {
existingObj: &apis.CStorVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "cvc8",
Namespace: "openebs",
},
Spec: apis.CStorVolumeClaimSpec{
ReplicaCount: 3,
Policy: apis.CStorVolumePolicySpec{
ReplicaPoolInfo: []apis.ReplicaPoolInfo{
apis.ReplicaPoolInfo{PoolName: "pool1"},
apis.ReplicaPoolInfo{PoolName: "pool2"},
apis.ReplicaPoolInfo{PoolName: "pool3"},
},
},
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3"},
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -330,41 +338,6 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
},
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3", "pool4"},
},
},
expectedRsp: false,
getCVCObj: getCVCObject,
},
"When Spec & Status Pool Names Was Updated By Controller With Invalid Pool Names Under Status": {
existingObj: &apis.CStorVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "cvc9",
Namespace: "openebs",
},
Spec: apis.CStorVolumeClaimSpec{
ReplicaCount: 3,
},
},
requestedObj: &apis.CStorVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "cvc9",
Namespace: "openebs",
},
Spec: apis.CStorVolumeClaimSpec{
ReplicaCount: 3,
Policy: apis.CStorVolumePolicySpec{
ReplicaPoolInfo: []apis.ReplicaPoolInfo{
apis.ReplicaPoolInfo{PoolName: "pool1"},
apis.ReplicaPoolInfo{PoolName: "pool2"},
apis.ReplicaPoolInfo{PoolName: "pool3"},
},
},
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool3", "pool4"},
},
},
expectedRsp: false,
getCVCObj: getCVCObject,
Expand All @@ -385,6 +358,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -404,6 +378,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: true,
Expand All @@ -426,6 +401,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -443,6 +419,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: true,
Expand All @@ -465,6 +442,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -483,6 +461,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: true,
Expand All @@ -504,6 +483,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -521,6 +501,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: true,
Expand All @@ -543,6 +524,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -562,6 +544,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
Expand All @@ -585,6 +568,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
requestedObj: &apis.CStorVolumeClaim{
Expand All @@ -604,6 +588,7 @@ func TestValidateCVCUpdateRequest(t *testing.T) {
},
Status: apis.CStorVolumeClaimStatus{
PoolInfo: []string{"pool1", "pool2", "pool2"},
Phase: apis.CStorVolumeClaimPhaseBound,
},
},
expectedRsp: false,
Expand Down