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.

Sign up for GitHub

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

🐛 Apply conditions patch should preserve LastTransitionTime #3179

Merged
merged 1 commit into from
Jun 11, 2020
Merged

🐛 Apply conditions patch should preserve LastTransitionTime #3179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 27 additions & 28 deletions util/conditions/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,15 @@ const (
RemoveConditionPatch PatchOperationType = "Remove"
)

// NewPatch returns the list of Patch required to align source conditions to target conditions.
func NewPatch(base Getter, target Getter) Patch {
// NewPatch returns the list of Patch required to align source conditions to after conditions.
func NewPatch(before Getter, after Getter) Patch {
var patch Patch

// Identify AddCondition and ModifyCondition changes.
targetConditions := target.GetConditions()
targetConditions := after.GetConditions()
for i := range targetConditions {
targetCondition := targetConditions[i]
currentCondition := Get(base, targetCondition.Type)
currentCondition := Get(before, targetCondition.Type)
if currentCondition == nil {
patch = append(patch, PatchOperation{Op: AddConditionPatch, Target: &targetCondition})
continue
Expand All @@ -67,10 +67,10 @@ func NewPatch(base Getter, target Getter) Patch {
}

// Identify RemoveCondition changes.
baseConditions := base.GetConditions()
baseConditions := before.GetConditions()
for i := range baseConditions {
baseCondition := baseConditions[i]
targetCondition := Get(target, baseCondition.Type)
targetCondition := Get(after, baseCondition.Type)
if targetCondition == nil {
patch = append(patch, PatchOperation{Op: RemoveConditionPatch, Base: &baseCondition})
}
Expand All @@ -79,61 +79,60 @@ func NewPatch(base Getter, target Getter) Patch {
}

// Apply executes a three-way merge of a list of Patch.
// When merge conflicts are detected (the source deviated from the original base recorded when creating the patch
// in an incompatible way), an error is returned.
func (p Patch) Apply(source Setter) error {
// When merge conflicts are detected (latest deviated from before in an incompatible way), an error is returned.
func (p Patch) Apply(latest Setter) error {
if len(p) == 0 {
return nil
}

for _, conditionPatch := range p {
switch conditionPatch.Op {
case AddConditionPatch:
// If the condition is already on source, check if source and target agree on the change; if not, this is a conflict.
if sourceCondition := Get(source, conditionPatch.Target.Type); sourceCondition != nil {
// If source and target agree on the change, then it is a conflict.
// If the condition is already on latest, check if latest and after agree on the change; if not, this is a conflict.
if sourceCondition := Get(latest, conditionPatch.Target.Type); sourceCondition != nil {
// If latest and after agree on the change, then it is a conflict.
if !hasSameState(sourceCondition, conditionPatch.Target) {
return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/AddCondition conflict", conditionPatch.Target.Type)
}
// otherwise, the source is already as intended.
// NOTE: We are preserving LastTransitionTime from the source in order to avoid altering the existing value.
// otherwise, the latest is already as intended.
// NOTE: We are preserving LastTransitionTime from the latest in order to avoid altering the existing value.
continue
}
// If the condition does not exists on the source, add the new target condition.
Set(source, conditionPatch.Target)
// If the condition does not exists on the latest, add the new after condition.
Set(latest, conditionPatch.Target)

case ChangeConditionPatch:
sourceCondition := Get(source, conditionPatch.Target.Type)
sourceCondition := Get(latest, conditionPatch.Target.Type)

// If the condition does not exist anymore on the source, this is a conflict.
// If the condition does not exist anymore on the latest, this is a conflict.
if sourceCondition == nil {
return errors.Errorf("error patching conditions: The condition %q on was deleted by a different process and this caused a merge/ChangeCondition conflict", conditionPatch.Target.Type)
}

// If the condition on the source is different from the base condition, check if
// the target state corresponds to the desired value. If not this is a conflict.
// If the condition on the latest is different from the base condition, check if
// the after state corresponds to the desired value. If not this is a conflict.
if !reflect.DeepEqual(sourceCondition, conditionPatch.Base) {
if !hasSameState(sourceCondition, conditionPatch.Target) {
return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/ChangeCondition conflict", conditionPatch.Target.Type)
}
// Otherwise the source is already as intended.
// NOTE: We are preserving LastTransitionTime from the source in order to avoid altering the existing value.
// Otherwise the latest is already as intended.
// NOTE: We are preserving LastTransitionTime from the latest in order to avoid altering the existing value.
continue
}

// Otherwise apply the new target condition.
Set(source, conditionPatch.Target)
// Otherwise apply the new after condition.
Set(latest, conditionPatch.Target)

case RemoveConditionPatch:
// If the condition is still on the source, check if it is changed in the meantime;
// If the condition is still on the latest, check if it is changed in the meantime;
// if so then this is a conflict.
if sourceCondition := Get(source, conditionPatch.Base.Type); sourceCondition != nil {
if sourceCondition := Get(latest, conditionPatch.Base.Type); sourceCondition != nil {
if !hasSameState(sourceCondition, conditionPatch.Base) {
return errors.Errorf("error patching conditions: The condition %q on was modified by a different process and this caused a merge/RemoveCondition conflict", conditionPatch.Base.Type)
}
}
// Otherwise the source and target agreed on the delete operation, so there's nothing to change.
Delete(source, conditionPatch.Base.Type)
// Otherwise the latest and after agreed on the delete operation, so there's nothing to change.
Delete(latest, conditionPatch.Base.Type)
}
}
return nil
Expand Down
134 changes: 82 additions & 52 deletions util/conditions/patch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ package conditions

import (
"testing"
"time"

. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
)

Expand All @@ -29,26 +32,26 @@ func TestNewPatch(t *testing.T) {

tests := []struct {
name string
base Getter
target Getter
before Getter
after Getter
want Patch
}{
{
name: "No changes return empty patch",
base: getterWithConditions(),
target: getterWithConditions(),
before: getterWithConditions(),
after: getterWithConditions(),
want: nil,
},
{
name: "No changes return empty patch",
base: getterWithConditions(fooTrue),
target: getterWithConditions(fooTrue),
before: getterWithConditions(fooTrue),
after: getterWithConditions(fooTrue),
want: nil,
},
{
name: "Detects AddConditionPatch",
base: getterWithConditions(),
target: getterWithConditions(fooTrue),
before: getterWithConditions(),
after: getterWithConditions(fooTrue),
want: Patch{
{
Base: nil,
Expand All @@ -59,8 +62,8 @@ func TestNewPatch(t *testing.T) {
},
{
name: "Detects ChangeConditionPatch",
base: getterWithConditions(fooTrue),
target: getterWithConditions(fooFalse),
before: getterWithConditions(fooTrue),
after: getterWithConditions(fooFalse),
want: Patch{
{
Base: fooTrue,
Expand All @@ -71,8 +74,8 @@ func TestNewPatch(t *testing.T) {
},
{
name: "Detects RemoveConditionPatch",
base: getterWithConditions(fooTrue),
target: getterWithConditions(),
before: getterWithConditions(fooTrue),
after: getterWithConditions(),
want: Patch{
{
Base: fooTrue,
Expand All @@ -87,7 +90,7 @@ func TestNewPatch(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

got := NewPatch(tt.base, tt.target)
got := NewPatch(tt.before, tt.after)

g.Expect(got).To(Equal(tt.want))
})
Expand All @@ -101,97 +104,97 @@ func TestApply(t *testing.T) {

tests := []struct {
name string
base Getter
target Getter
source Setter
before Getter
after Getter
latest Setter
want clusterv1.Conditions
wantErr bool
}{
{
name: "No patch return same list",
base: getterWithConditions(fooTrue),
target: getterWithConditions(fooTrue),
source: setterWithConditions(fooTrue),
before: getterWithConditions(fooTrue),
after: getterWithConditions(fooTrue),
latest: setterWithConditions(fooTrue),
want: conditionList(fooTrue),
wantErr: false,
},
{
name: "Add: When a condition does not exists, it should add",
base: getterWithConditions(),
target: getterWithConditions(fooTrue),
source: setterWithConditions(),
before: getterWithConditions(),
after: getterWithConditions(fooTrue),
latest: setterWithConditions(),
want: conditionList(fooTrue),
wantErr: false,
},
{
name: "Add: When a condition already exists but without conflicts, it should add",
base: getterWithConditions(),
target: getterWithConditions(fooTrue),
source: setterWithConditions(fooTrue),
before: getterWithConditions(),
after: getterWithConditions(fooTrue),
latest: setterWithConditions(fooTrue),
want: conditionList(fooTrue),
wantErr: false,
},
{
name: "Add: When a condition already exists but with conflicts, it should error",
base: getterWithConditions(),
target: getterWithConditions(fooTrue),
source: setterWithConditions(fooFalse),
before: getterWithConditions(),
after: getterWithConditions(fooTrue),
latest: setterWithConditions(fooFalse),
want: nil,
wantErr: true,
},
{
name: "Remove: When a condition was already deleted, it should pass",
base: getterWithConditions(fooTrue),
target: getterWithConditions(),
source: setterWithConditions(),
before: getterWithConditions(fooTrue),
after: getterWithConditions(),
latest: setterWithConditions(),
want: conditionList(),
wantErr: false,
},
{
name: "Remove: When a condition already exists but without conflicts, it should delete",
base: getterWithConditions(fooTrue),
target: getterWithConditions(),
source: setterWithConditions(fooTrue),
before: getterWithConditions(fooTrue),
after: getterWithConditions(),
latest: setterWithConditions(fooTrue),
want: conditionList(),
wantErr: false,
},
{
name: "Remove: When a condition already exists but with conflicts, it should error",
base: getterWithConditions(fooTrue),
target: getterWithConditions(),
source: setterWithConditions(fooFalse),
before: getterWithConditions(fooTrue),
after: getterWithConditions(),
latest: setterWithConditions(fooFalse),
want: nil,
wantErr: true,
},
{
name: "Change: When a condition exists without conflicts, it should change",
base: getterWithConditions(fooTrue),
target: getterWithConditions(fooFalse),
source: setterWithConditions(fooTrue),
before: getterWithConditions(fooTrue),
after: getterWithConditions(fooFalse),
latest: setterWithConditions(fooTrue),
want: conditionList(fooFalse),
wantErr: false,
},
{
name: "Change: When a condition exists with conflicts but there is agreement on the final state, it should change",
base: getterWithConditions(fooFalse),
target: getterWithConditions(fooTrue),
source: setterWithConditions(fooTrue),
before: getterWithConditions(fooFalse),
after: getterWithConditions(fooTrue),
latest: setterWithConditions(fooTrue),
want: conditionList(fooTrue),
wantErr: false,
},
{
name: "Change: When a condition exists with conflicts but there is no agreement on the final state, it should error",
base: getterWithConditions(fooWarning),
target: getterWithConditions(fooFalse),
source: setterWithConditions(fooTrue),
before: getterWithConditions(fooWarning),
after: getterWithConditions(fooFalse),
latest: setterWithConditions(fooTrue),
want: nil,
wantErr: true,
},
{
name: "Change: When a condition was deleted, it should error",
base: getterWithConditions(fooTrue),
target: getterWithConditions(fooFalse),
source: setterWithConditions(),
before: getterWithConditions(fooTrue),
after: getterWithConditions(fooFalse),
latest: setterWithConditions(),
want: nil,
wantErr: true,
},
Expand All @@ -201,16 +204,43 @@ func TestApply(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

patch := NewPatch(tt.base, tt.target)
patch := NewPatch(tt.before, tt.after)

err := patch.Apply(tt.source)
err := patch.Apply(tt.latest)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
return
}
g.Expect(err).ToNot(HaveOccurred())

g.Expect(tt.source.GetConditions()).To(haveSameConditionsOf(tt.want))
g.Expect(tt.latest.GetConditions()).To(haveSameConditionsOf(tt.want))
})
}
}

func TestApplyDoesNotAlterLastTransitionTime(t *testing.T) {
g := NewWithT(t)

before := &clusterv1.Cluster{}
after := &clusterv1.Cluster{
Status: clusterv1.ClusterStatus{
Conditions: clusterv1.Conditions{
clusterv1.Condition{
Type: "foo",
Status: corev1.ConditionTrue,
LastTransitionTime: metav1.NewTime(time.Now().UTC().Truncate(time.Second)),
},
},
},
}
latest := &clusterv1.Cluster{}

// latest has no conditions, so we are actually adding the condition but in this case we should not set the LastTransition Time
// but we should preserve the LastTransition set in after

diff := NewPatch(before, after)
err := diff.Apply(latest)

g.Expect(err).ToNot(HaveOccurred())
g.Expect(latest.GetConditions()).To(Equal(after.GetConditions()))
}
Loading