From e43b1c1454a0e357470188551ebb12ad0c3af0ed Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes Date: Mon, 2 Dec 2024 13:24:46 -0600 Subject: [PATCH 1/3] Add customResourceDefinition trackability --- pkg/controllers/work/apply_controller.go | 28 ++++ pkg/controllers/work/apply_controller_test.go | 128 ++++++++++++++++++ pkg/utils/common.go | 6 + 3 files changed, 162 insertions(+) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 0b49e96a9..574a7032c 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -29,6 +29,7 @@ import ( "go.uber.org/atomic" appv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -448,6 +449,9 @@ func trackResourceAvailability(gvr schema.GroupVersionResource, curObj *unstruct case utils.ServiceGVR: return trackServiceAvailability(curObj) + case utils.CustomResourceDefinitionGVR: + return trackCRDAvailability(curObj) + default: if isDataResource(gvr) { klog.V(2).InfoS("Data resources are available immediately", "gvr", gvr, "resource", klog.KObj(curObj)) @@ -458,6 +462,30 @@ func trackResourceAvailability(gvr schema.GroupVersionResource, curObj *unstruct } } +func trackCRDAvailability(curObj *unstructured.Unstructured) (ApplyAction, error) { + var crd apiextensionsv1.CustomResourceDefinition + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(curObj.Object, &crd); err != nil { + return errorApplyAction, controller.NewUnexpectedBehaviorError(err) + } + // Check if there is a condition of type "Established" + var establishedCondition *apiextensionsv1.CustomResourceDefinitionCondition + for i := range crd.Status.Conditions { + condition := crd.Status.Conditions[i] // Create a new variable + if condition.Type == apiextensionsv1.Established { + establishedCondition = &condition + break + } + } + + // If the condition is found, and it is True, the CRD is available + if establishedCondition != nil && establishedCondition.Status == apiextensionsv1.ConditionTrue { + klog.V(2).InfoS("CustomResourceDefinition is available", "customResourceDefinition", klog.KObj(curObj)) + return manifestAvailableAction, nil + } + klog.V(2).InfoS("Still need to wait for CustomResourceDefinition to be available", "customResourceDefinition", klog.KObj(curObj)) + return manifestNotAvailableYetAction, nil +} + func trackDeploymentAvailability(curObj *unstructured.Unstructured) (ApplyAction, error) { var deployment appv1.Deployment if err := runtime.DefaultUnstructuredConverter.FromUnstructured(curObj.Object, &deployment); err != nil { diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 5cd2715d4..71247e276 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -1235,6 +1235,134 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestNotTrackableAction, err: nil, }, + "Test CustomResourceDefinition available": { + gvr: utils.CustomResourceDefinitionGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]interface{}{ + "name": "testresources.example.com", + }, + "spec": map[string]interface{}{ + "group": "example.com", + "names": map[string]interface{}{ + "plural": "testresources", + "singular": "testresource", + "kind": "TestResource", + "shortNames": []string{"tr"}, + }, + "scope": "Namespaced", + "versions": []interface{}{ + map[string]interface{}{ + "name": "v1", + "served": true, + "storage": true, + "schema": map[string]interface{}{ + "openAPIV3Schema": map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "spec": map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "field": map[string]interface{}{ + "type": "string", + }, + }, + }, + }, + }, + }, + }, + }, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Established", + "status": "True", + "lastTransitionTime": metav1.Now(), + "reason": "CRDEstablished", + "message": "The CustomResourceDefinition has been established.", + }, + map[string]interface{}{ + "type": "NamesAccepted", + "status": "True", + "lastTransitionTime": metav1.Now(), + "reason": "NoConflicts", + "message": "The CustomResourceDefinition name was accepted.", + }, + }, + }, + }, + }, + expected: manifestAvailableAction, + err: nil, + }, + "Test CustomResourceDefinition unavailable": { + gvr: utils.CustomResourceDefinitionGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]interface{}{ + "name": "testresources.example.com", + }, + "spec": map[string]interface{}{ + "group": "example.com", + "names": map[string]interface{}{ + "plural": "testresources", + "singular": "testresource", + "kind": "TestResource", + "shortNames": []string{"tr"}, + }, + "scope": "Namespaced", + "versions": []interface{}{ + map[string]interface{}{ + "name": "v1", + "served": true, + "storage": true, + "schema": map[string]interface{}{ + "openAPIV3Schema": map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "spec": map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{ + "field": map[string]interface{}{ + "type": "string", + }, + }, + }, + }, + }, + }, + }, + }, + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Established", + "status": "False", + "lastTransitionTime": metav1.Now(), + "reason": "CRDEstablished", + "message": "The CustomResourceDefinition has been established.", + }, + map[string]interface{}{ + "type": "NamesAccepted", + "status": "True", + "lastTransitionTime": metav1.Now(), + "reason": "NoConflicts", + "message": "The CustomResourceDefinition name was accepted.", + }, + }, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, } for name, tt := range tests { diff --git a/pkg/utils/common.go b/pkg/utils/common.go index bf8be3b60..788f495fc 100644 --- a/pkg/utils/common.go +++ b/pkg/utils/common.go @@ -167,6 +167,12 @@ var ( Kind: "CustomResourceDefinition", } + CustomResourceDefinitionGVR = schema.GroupVersionResource{ + Group: apiextensionsv1.SchemeGroupVersion.Group, + Version: apiextensionsv1.SchemeGroupVersion.Version, + Resource: "customresourcedefinitions", + } + EndpointSliceExportMetaGVK = metav1.GroupVersionKind{ Group: fleetnetworkingv1alpha1.GroupVersion.Group, Version: fleetnetworkingv1alpha1.GroupVersion.Version, From c3398d107d44a557388bd6cab6d0883c4dcfd25a Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes Date: Fri, 6 Dec 2024 11:50:14 -0800 Subject: [PATCH 2/3] add names accpeted condition & more test cases --- pkg/controllers/work/apply_controller.go | 17 +++-- pkg/controllers/work/apply_controller_test.go | 71 +++++++++++++++++-- 2 files changed, 75 insertions(+), 13 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 574a7032c..8cbeecf15 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -467,21 +467,26 @@ func trackCRDAvailability(curObj *unstructured.Unstructured) (ApplyAction, error if err := runtime.DefaultUnstructuredConverter.FromUnstructured(curObj.Object, &crd); err != nil { return errorApplyAction, controller.NewUnexpectedBehaviorError(err) } - // Check if there is a condition of type "Established" - var establishedCondition *apiextensionsv1.CustomResourceDefinitionCondition + + // Check if both "Established" and "NamesAccepted" conditions exist and are True + var establishedCondition, namesAcceptedCondition *apiextensionsv1.CustomResourceDefinitionCondition for i := range crd.Status.Conditions { condition := crd.Status.Conditions[i] // Create a new variable - if condition.Type == apiextensionsv1.Established { + switch condition.Type { + case apiextensionsv1.Established: establishedCondition = &condition - break + case apiextensionsv1.NamesAccepted: + namesAcceptedCondition = &condition } } - // If the condition is found, and it is True, the CRD is available - if establishedCondition != nil && establishedCondition.Status == apiextensionsv1.ConditionTrue { + // If both conditions are True, the CRD is available + if establishedCondition != nil && establishedCondition.Status == apiextensionsv1.ConditionTrue && + namesAcceptedCondition != nil && namesAcceptedCondition.Status == apiextensionsv1.ConditionTrue { klog.V(2).InfoS("CustomResourceDefinition is available", "customResourceDefinition", klog.KObj(curObj)) return manifestAvailableAction, nil } + klog.V(2).InfoS("Still need to wait for CustomResourceDefinition to be available", "customResourceDefinition", klog.KObj(curObj)) return manifestNotAvailableYetAction, nil } diff --git a/pkg/controllers/work/apply_controller_test.go b/pkg/controllers/work/apply_controller_test.go index 71247e276..3722982ff 100644 --- a/pkg/controllers/work/apply_controller_test.go +++ b/pkg/controllers/work/apply_controller_test.go @@ -1282,15 +1282,15 @@ func TestTrackResourceAvailability(t *testing.T) { "type": "Established", "status": "True", "lastTransitionTime": metav1.Now(), - "reason": "CRDEstablished", - "message": "The CustomResourceDefinition has been established.", + "reason": "InitialNamesAccepted", + "message": "the initial names have been accepted", }, map[string]interface{}{ "type": "NamesAccepted", "status": "True", "lastTransitionTime": metav1.Now(), "reason": "NoConflicts", - "message": "The CustomResourceDefinition name was accepted.", + "message": "no conflicts found", }, }, }, @@ -1299,7 +1299,7 @@ func TestTrackResourceAvailability(t *testing.T) { expected: manifestAvailableAction, err: nil, }, - "Test CustomResourceDefinition unavailable": { + "Test CustomResourceDefinition unavailable (not established)": { gvr: utils.CustomResourceDefinitionGVR, obj: &unstructured.Unstructured{ Object: map[string]interface{}{ @@ -1346,15 +1346,72 @@ func TestTrackResourceAvailability(t *testing.T) { "type": "Established", "status": "False", "lastTransitionTime": metav1.Now(), - "reason": "CRDEstablished", - "message": "The CustomResourceDefinition has been established.", + "reason": "Installing", + "message": "the initial names have been accepted", }, map[string]interface{}{ "type": "NamesAccepted", "status": "True", "lastTransitionTime": metav1.Now(), "reason": "NoConflicts", - "message": "The CustomResourceDefinition name was accepted.", + "message": "no conflicts found", + }, + }, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, + "Test CustomResourceDefinition unavailable (name not accepted)": { + gvr: utils.CustomResourceDefinitionGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]interface{}{ + "name": "testresources.example.com", + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "NamesAccepted", + "status": "False", + "lastTransitionTime": metav1.Now(), + "reason": "NameConflict", + "message": "names conflict", + }, + }, + }, + }, + }, + expected: manifestNotAvailableYetAction, + err: nil, + }, + "Test CustomResourceDefinition unavailable (established but name not accepted)": { + gvr: utils.CustomResourceDefinitionGVR, + obj: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apiextensions.k8s.io/v1", + "kind": "CustomResourceDefinition", + "metadata": map[string]interface{}{ + "name": "testresources.example.com", + }, + "status": map[string]interface{}{ + "conditions": []interface{}{ + map[string]interface{}{ + "type": "Established", + "status": "True", + "lastTransitionTime": metav1.Now(), + "reason": "InitialNamesAccepted", + "message": "the initial names have been accepted", + }, + map[string]interface{}{ + "type": "NamesAccepted", + "status": "False", + "lastTransitionTime": metav1.Now(), + "reason": "NotAccepted", + "message": "not all names are accepted", }, }, }, From 0f953db381c03e03081156661b3c557cda93d523 Mon Sep 17 00:00:00 2001 From: Britania Rodriguez Reyes Date: Fri, 6 Dec 2024 13:30:55 -0800 Subject: [PATCH 3/3] address comments --- pkg/controllers/work/apply_controller.go | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/pkg/controllers/work/apply_controller.go b/pkg/controllers/work/apply_controller.go index 8cbeecf15..cb0ed7c87 100644 --- a/pkg/controllers/work/apply_controller.go +++ b/pkg/controllers/work/apply_controller.go @@ -29,6 +29,7 @@ import ( "go.uber.org/atomic" appv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" + apiextensionshelpers "k8s.io/apiextensions-apiserver/pkg/apihelpers" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -468,21 +469,8 @@ func trackCRDAvailability(curObj *unstructured.Unstructured) (ApplyAction, error return errorApplyAction, controller.NewUnexpectedBehaviorError(err) } - // Check if both "Established" and "NamesAccepted" conditions exist and are True - var establishedCondition, namesAcceptedCondition *apiextensionsv1.CustomResourceDefinitionCondition - for i := range crd.Status.Conditions { - condition := crd.Status.Conditions[i] // Create a new variable - switch condition.Type { - case apiextensionsv1.Established: - establishedCondition = &condition - case apiextensionsv1.NamesAccepted: - namesAcceptedCondition = &condition - } - } - // If both conditions are True, the CRD is available - if establishedCondition != nil && establishedCondition.Status == apiextensionsv1.ConditionTrue && - namesAcceptedCondition != nil && namesAcceptedCondition.Status == apiextensionsv1.ConditionTrue { + if apiextensionshelpers.IsCRDConditionTrue(&crd, apiextensionsv1.Established) && apiextensionshelpers.IsCRDConditionTrue(&crd, apiextensionsv1.NamesAccepted) { klog.V(2).InfoS("CustomResourceDefinition is available", "customResourceDefinition", klog.KObj(curObj)) return manifestAvailableAction, nil }