apiextensions: always set lastTransitionTime when CRD condition changes

Author: CaoShuFeng

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/helpers.go

@@ -27,18 +27,19 @@ var swaggerMetadataDescriptions = metav1.ObjectMeta{}.SwaggerDoc()
 
 // SetCRDCondition sets the status condition. It either overwrites the existing one or creates a new one.
 func SetCRDCondition(crd *CustomResourceDefinition, newCondition CustomResourceDefinitionCondition) {
+	newCondition.LastTransitionTime = metav1.NewTime(time.Now())
+
 	existingCondition := FindCRDCondition(crd, newCondition.Type)
 	if existingCondition == nil {
-		newCondition.LastTransitionTime = metav1.NewTime(time.Now())
 		crd.Status.Conditions = append(crd.Status.Conditions, newCondition)
 		return
 	}
 
-	if existingCondition.Status != newCondition.Status {
-		existingCondition.Status = newCondition.Status
+	if existingCondition.Status != newCondition.Status || existingCondition.LastTransitionTime.IsZero() {
 		existingCondition.LastTransitionTime = newCondition.LastTransitionTime
 	}
 
+	existingCondition.Status = newCondition.Status
 	existingCondition.Reason = newCondition.Reason
 	existingCondition.Message = newCondition.Message
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/helpers_test.go

@@ -202,6 +202,67 @@ func TestSetCRDCondition(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "set new condition which doesn't have lastTransitionTime set",
+			crdCondition: []CustomResourceDefinitionCondition{
+				{
+					Type:               Established,
+					Status:             ConditionTrue,
+					Reason:             "Accepted",
+					Message:            "the initial names have been accepted",
+					LastTransitionTime: metav1.Date(2018, 1, 1, 0, 0, 0, 0, time.UTC),
+				},
+			},
+			newCondition: CustomResourceDefinitionCondition{
+				Type:    Established,
+				Status:  ConditionFalse,
+				Reason:  "NotAccepted",
+				Message: "Not accepted",
+			},
+			expectedcrdCondition: []CustomResourceDefinitionCondition{
+				{
+					Type:               Established,
+					Status:             ConditionFalse,
+					Reason:             "NotAccepted",
+					Message:            "Not accepted",
+					LastTransitionTime: metav1.Date(2018, 1, 2, 0, 0, 0, 0, time.UTC),
+				},
+			},
+		},
+		{
+			name: "append new condition which doesn't have lastTransitionTime set",
+			crdCondition: []CustomResourceDefinitionCondition{
+				{
+					Type:               Established,
+					Status:             ConditionTrue,
+					Reason:             "Accepted",
+					Message:            "the initial names have been accepted",
+					LastTransitionTime: metav1.Date(2018, 1, 1, 0, 0, 0, 0, time.UTC),
+				},
+			},
+			newCondition: CustomResourceDefinitionCondition{
+				Type:    Terminating,
+				Status:  ConditionFalse,
+				Reason:  "NeverEstablished",
+				Message: "resource was never established",
+			},
+			expectedcrdCondition: []CustomResourceDefinitionCondition{
+				{
+					Type:               Established,
+					Status:             ConditionTrue,
+					Reason:             "Accepted",
+					Message:            "the initial names have been accepted",
+					LastTransitionTime: metav1.Date(2018, 1, 1, 0, 0, 0, 0, time.UTC),
+				},
+				{
+					Type:               Terminating,
+					Status:             ConditionFalse,
+					Reason:             "NeverEstablished",
+					Message:            "resource was never established",
+					LastTransitionTime: metav1.Date(2018, 2, 1, 0, 0, 0, 0, time.UTC),
+				},
+			},
+		},
 	}
 	for _, tc := range tests {
 		crd := generateCRDwithCondition(tc.crdCondition)
@@ -213,6 +274,9 @@ func TestSetCRDCondition(t *testing.T) {
 			if !IsCRDConditionEquivalent(&tc.expectedcrdCondition[i], &crd.Status.Conditions[i]) {
 				t.Errorf("%v expected %v, got %v", tc.name, tc.expectedcrdCondition, crd.Status.Conditions)
 			}
+			if crd.Status.Conditions[i].LastTransitionTime.IsZero() {
+				t.Errorf("%q lastTransitionTime should not be null: %v", tc.name, i, crd.Status.Conditions)
+			}
 		}
 	}
 }