Skip to content

Commit

Permalink
Add Conditions to core status
Browse files Browse the repository at this point in the history
GetCondition and something like UpdateCondition have worked well in the
OperatorPolicy. The fakepolicy tests have been updated to use conditions
instead of the various special fields it was collecting in its status.

Signed-off-by: Justin Kulikauskas <[email protected]>
  • Loading branch information
JustinKuli committed May 9, 2024
1 parent c4bb986 commit 7ea4fed
Show file tree
Hide file tree
Showing 12 changed files with 573 additions and 131 deletions.
58 changes: 58 additions & 0 deletions api/v1beta1/conditions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Copyright Contributors to the Open Cluster Management project

package v1beta1

import (
"sort"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// GetCondition returns the existing index and condition on the status matching the given type. If
// no condition of that type is found, it will return -1 as the index.
func (status PolicyCoreStatus) GetCondition(condType string) (int, metav1.Condition) {
for i, cond := range status.Conditions {
if cond.Type == condType {
return i, cond
}
}

return -1, metav1.Condition{}
}

// UpdateCondition modifies the specified condition in the status or adds it if not present,
// ensuring conditions remain sorted by Type. Returns true if the condition was updated or added.
func (status *PolicyCoreStatus) UpdateCondition(newCond metav1.Condition) (changed bool) {
idx, existingCond := status.GetCondition(newCond.Type)
if idx == -1 {
if newCond.LastTransitionTime.IsZero() {
newCond.LastTransitionTime = metav1.Now()
}

status.Conditions = append(status.Conditions, newCond)

sort.SliceStable(status.Conditions, func(i, j int) bool {
return status.Conditions[i].Type < status.Conditions[j].Type
})

return true
} else if condSemanticallyChanged(newCond, existingCond) {
if newCond.LastTransitionTime.IsZero() {
newCond.LastTransitionTime = metav1.Now()
}

status.Conditions[idx] = newCond

// Do not sort in this case, assume that they are in order.

return true
}

return false
}

func condSemanticallyChanged(newCond, oldCond metav1.Condition) bool {
return newCond.Message != oldCond.Message ||
newCond.Reason != oldCond.Reason ||
newCond.Status != oldCond.Status
}
171 changes: 171 additions & 0 deletions api/v1beta1/conditions_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
// Copyright Contributors to the Open Cluster Management project

package v1beta1

import (
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func getSampleStatus() PolicyCoreStatus {
return PolicyCoreStatus{
Conditions: []metav1.Condition{{
Type: "Apple",
Status: metav1.ConditionTrue,
Reason: "NoDoctor",
Message: "an apple a day...",
}, {
Type: "Compliant",
Status: metav1.ConditionTrue,
Reason: "Compliant",
Message: "everything is good",
}, {
Type: "Bonus",
Status: metav1.ConditionFalse,
Reason: "Compliant",
Message: "this condition is out of order",
}},
}
}

func TestGetCondition(t *testing.T) {
t.Parallel()

tests := map[string]struct {
inputType string
wantIdx int
wantMsg string
}{
"Apple is found": {
inputType: "Apple",
wantIdx: 0,
wantMsg: "an apple a day...",
},
"Compliant is found": {
inputType: "Compliant",
wantIdx: 1,
wantMsg: "everything is good",
},
"Imaginary is not found": {
inputType: "Imaginary",
wantIdx: -1,
wantMsg: "",
},
}

for name, tcase := range tests {
gotIdx, gotCond := getSampleStatus().GetCondition(tcase.inputType)

if gotIdx != tcase.wantIdx {
t.Errorf("Expected index %v in test %q, got %v", tcase.wantIdx, name, gotIdx)
}

if tcase.wantIdx == -1 {
continue
}

if gotCond.Message != tcase.wantMsg {
t.Errorf("Expected message %q in test %q, got %q", tcase.wantMsg, name, gotCond.Message)
}
}
}

func TestUpdateCondition(t *testing.T) {
t.Parallel()

baseLen := len(getSampleStatus().Conditions)

tests := map[string]struct {
newCond metav1.Condition
wantChange bool
wantLen int
wantIdx int
}{
"Imaginary should be added to the end": {
newCond: metav1.Condition{
Type: "Imaginary",
Status: metav1.ConditionFalse,
Reason: "Existent",
Message: "not just imaginary",
},
wantChange: true,
wantLen: baseLen + 1,
wantIdx: baseLen,
},
"Basic should be added in the middle": {
newCond: metav1.Condition{
Type: "Basic",
Status: metav1.ConditionTrue,
Reason: "Easy",
Message: "should be simple enough",
},
wantChange: true,
wantLen: baseLen + 1,
wantIdx: 1,
},
"Bonus should be updated but not moved": {
newCond: metav1.Condition{
Type: "Bonus",
Status: metav1.ConditionTrue,
Reason: "Compliant",
Message: "this condition is now in order",
},
wantChange: true,
wantLen: baseLen,
wantIdx: baseLen - 1,
},
"Apple should be updated if the message is different": {
newCond: metav1.Condition{
Type: "Apple",
Status: metav1.ConditionTrue,
Reason: "NoDoctor",
Message: "an apple a day keeps the doctor away",
},
wantChange: true,
wantLen: baseLen,
wantIdx: 0,
},
"Apple should not be updated if the message is the same": {
newCond: metav1.Condition{
Type: "Apple",
Status: metav1.ConditionTrue,
Reason: "NoDoctor",
Message: "an apple a day...",
},
wantChange: false,
wantLen: baseLen,
wantIdx: 0,
},
}

for name, tcase := range tests {
status := getSampleStatus()

gotChanged := status.UpdateCondition(tcase.newCond)

if tcase.wantChange && !gotChanged {
t.Errorf("Expected test %q to change the conditions, but it didn't", name)
} else if !tcase.wantChange && gotChanged {
t.Errorf("Expected test %q not to change the conditions, but it did", name)
}

gotLen := len(status.Conditions)
if gotLen != tcase.wantLen {
t.Errorf("Expected conditions to be length %v after test %q, got length %v",
tcase.wantLen, name, gotLen)
}

gotIdx, gotCond := status.GetCondition(tcase.newCond.Type)

if gotIdx != tcase.wantIdx {
t.Errorf("Expected condition to be at index %v after test %q, got index %v",
tcase.wantIdx, name, gotIdx)
}

if gotCond.Message != tcase.newCond.Message {
t.Errorf("Expected condition to have message %q after test %q, got %q",
tcase.newCond.Message, name, gotCond.Message)
}
}
}
15 changes: 8 additions & 7 deletions api/v1beta1/policycore_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

// NOTE: json tags are required. Any new fields you add must have json tags for
// the fields to be serialized.
// Important: Run "make" to regenerate code after modifying this file

// PolicyCoreSpec defines fields that policies must implement to be part of the
// PolicyCoreSpec defines fields that policies should implement to be part of the
// Open Cluster Management policy framework. The intention is for controllers
// to embed this struct in their *Spec definitions.
type PolicyCoreSpec struct {
// Severity defines how serious the situation is when the policy is not
// compliant. The severity should not change the behavior of the policy, but
// compliant. The severity might not change the behavior of the policy, but
// may be read and used by other tools. Accepted values include: low,
// medium, high, and critical.
Severity Severity `json:"severity,omitempty"`
Expand Down Expand Up @@ -156,11 +152,16 @@ func (l *namespaceResList) ObjectList() client.ObjectList {
type NonEmptyString string

// PolicyCoreStatus defines fields that policies should implement as part of
// the Open Cluster Management policy framework.
// the Open Cluster Management policy framework. The intent is for controllers
// to embed this struct in their *Status definitions.
type PolicyCoreStatus struct {
// ComplianceState indicates whether the policy is compliant or not.
// Accepted values include: Compliant, NonCompliant, and UnknownCompliancy
ComplianceState ComplianceState `json:"compliant,omitempty"`

// Conditions represent the latest available observations of the object's status. One of these
// items should have Type=Compliant and a message detailing the current compliance.
Conditions []metav1.Condition `json:"conditions,omitempty"`
}

//+kubebuilder:validation:Enum=Compliant;NonCompliant;UnknownCompliancy
Expand Down
9 changes: 8 additions & 1 deletion api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ spec:
type: object
spec:
description: |-
PolicyCoreSpec defines fields that policies must implement to be part of the
PolicyCoreSpec defines fields that policies should implement to be part of the
Open Cluster Management policy framework. The intention is for controllers
to embed this struct in their *Spec definitions.
properties:
Expand Down Expand Up @@ -119,7 +119,7 @@ spec:
severity:
description: |-
Severity defines how serious the situation is when the policy is not
compliant. The severity should not change the behavior of the policy, but
compliant. The severity might not change the behavior of the policy, but
may be read and used by other tools. Accepted values include: low,
medium, high, and critical.
enum:
Expand All @@ -136,7 +136,8 @@ spec:
status:
description: |-
PolicyCoreStatus defines fields that policies should implement as part of
the Open Cluster Management policy framework.
the Open Cluster Management policy framework. The intent is for controllers
to embed this struct in their *Status definitions.
properties:
compliant:
description: |-
Expand All @@ -147,6 +148,78 @@ spec:
- NonCompliant
- UnknownCompliancy
type: string
conditions:
description: |-
Conditions represent the latest available observations of the object's status. One of these
items should have Type=Compliant and a message detailing the current compliance.
items:
description: "Condition contains details for one aspect of the current
state of this API Resource.\n---\nThis struct is intended for
direct use as an array at the field path .status.conditions. For
example,\n\n\n\ttype FooStatus struct{\n\t // Represents the
observations of a foo's current state.\n\t // Known .status.conditions.type
are: \"Available\", \"Progressing\", and \"Degraded\"\n\t //
+patchMergeKey=type\n\t // +patchStrategy=merge\n\t // +listType=map\n\t
\ // +listMapKey=type\n\t Conditions []metav1.Condition `json:\"conditions,omitempty\"
patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,1,rep,name=conditions\"`\n\n\n\t
\ // other fields\n\t}"
properties:
lastTransitionTime:
description: |-
lastTransitionTime is the last time the condition transitioned from one status to another.
This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable.
format: date-time
type: string
message:
description: |-
message is a human readable message indicating details about the transition.
This may be an empty string.
maxLength: 32768
type: string
observedGeneration:
description: |-
observedGeneration represents the .metadata.generation that the condition was set based upon.
For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date
with respect to the current state of the instance.
format: int64
minimum: 0
type: integer
reason:
description: |-
reason contains a programmatic identifier indicating the reason for the condition's last transition.
Producers of specific condition types may define expected values and meanings for this field,
and whether the values are considered a guaranteed API.
The value should be a CamelCase string.
This field may not be empty.
maxLength: 1024
minLength: 1
pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
type: string
status:
description: status of the condition, one of True, False, Unknown.
enum:
- "True"
- "False"
- Unknown
type: string
type:
description: |-
type of condition in CamelCase or in foo.example.com/CamelCase.
---
Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
useful (see .node.status.conditions), the ability to deconflict is important.
The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
maxLength: 316
pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
type: array
type: object
type: object
served: true
Expand Down
Loading

0 comments on commit 7ea4fed

Please sign in to comment.