Repository: submariner-io/lighthouse (go)

Issue:
Submariner ServiceExport does not have unique condition types to wait for readiness
<!-- Please use this template while reporting a bug and provide as much info as
possible. Not doing so may result in your bug not being addressed in a timely
manner. Thanks!

If the matter is security related, please disclose it privately to the
Submariner Owners: https://github.com/orgs/submariner-io/teams/submariner-core
-->


**What happened**:
After creating service export with subctl, ther's no way to validate it with kubectl/oc wait command,
since Submariner ServiceExport does not have unique condition types to wait for readiness (all types are the same "Valid").

For example, on a working service export status it failed on:

$ oc wait serviceexport nginx-cl-bc --for=condition=Valid
error: timed out waiting for the condition on serviceexports/nginx-cl-bc


**What you expected to happen**:
If the service export is working, it should return something like:

multicluster.x-k8s.io/ServiceExportList condition met

**How to reproduce it (as minimally and precisely as possible)**:
Create a service export:
$ oc get serviceexport
NAME AGE
nginx-cl-bc 3h5m

$ oc get serviceexport -o yaml
apiVersion: v1
items:

apiVersion: multicluster.x-k8s.io/v1alpha1
kind: ServiceExport
metadata:
creationTimestamp: "2022-03-21T08:25:40Z"
generation: 1
name: nginx-cl-bc
namespace: test-submariner
resourceVersion: "115495575"
selfLink: /apis/multicluster.x-k8s.io/v1alpha1/namespaces/test-submariner/serviceexports/nginx-cl-bc
uid: 7af4a9ae-24f6-4d1d-9ac5-100664e03dd9
status:
conditions:
lastTransitionTime: "2022-03-21T08:25:39Z"
message: Service doesn't have a global IP yet
reason: ServiceGlobalIPUnavailable
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T08:25:39Z"
message: Awaiting sync of the ServiceImport to the broker
reason: AwaitingSync
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T08:25:39Z"
message: Service was successfully synced to the broker
reason: ""
status: "True"
type: Valid
lastTransitionTime: "2022-03-21T09:59:32Z"
message: Service to be exported doesn't exist
reason: ServiceUnavailable
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T09:59:32Z"
message: Service doesn't have a global IP yet
reason: ServiceGlobalIPUnavailable
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T09:59:33Z"
message: Awaiting sync of the ServiceImport to the broker
reason: AwaitingSync
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T09:59:35Z"
message: Service was successfully synced to the broker
reason: ""
status: "True"
type: Valid
lastTransitionTime: "2022-03-21T10:20:16Z"
message: Service to be exported doesn't exist
reason: ServiceUnavailable
status: "False"
type: Valid
lastTransitionTime: "2022-03-21T10:20:16Z"
message: Service doesn't have a global IP yet
reason: ServiceGlobalIPUnavailable
status: "False"
type: Valid
kind: List
metadata:
resourceVersion: ""
selfLink: ""
$ oc wait serviceexport nginx-cl-bc --for=condition=Valid
error: timed out waiting for the condition on serviceexports/nginx-cl-bc


**Anything else we need to know?**:

**Environment**:
Submariner version 0.12.0

- Diagnose information (use `subctl diagnose all`):
- Gather information (use `subctl gather`):
- Cloud provider or hardware configuration:
- Install tools:
- Others:


Pull request description:
Only retain last state for condition types
Condition types are expected to be singelton and only last
state of a given condition type shoud be maintained. We currently
maintain a list of all transitions to a state as a list with max of
10. This causes problems with condition check APIs. Currently
support just one type, Valid.

Fixes #714

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>

<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our developer guide: https://submariner.io/development/
2. Ensure you have added the appropriate tests for your PR: https://submariner.io/development/code-review/#test-new-functionality
3. Read the code review guide to ease the review process: https://submariner.io/development/code-review/
4. If the PR is unfinished, mark it as a draft: https://submariner.io/development/code-review/#mark-work-in-progress-prs-as-drafts
5. If you are using CI to debug, use your private fork: https://submariner.io/development/code-review/#use-private-forks-for-debugging-prs-by-running-ci
6. Add labels to the PR as appropriate.

This template is based on the K8s/K8s template:

https://github.com/kubernetes/kubernetes/blob/master/.github/PULL_REQUEST_TEMPLATE.md
-->


Patch:
diff --git a/pkg/agent/controller/agent.go b/pkg/agent/controller/agent.go
index f948412..3d9c1f9 100644
--- a/pkg/agent/controller/agent.go
+++ b/pkg/agent/controller/agent.go
@@ -56,8 +56,6 @@ type AgentConfig struct {
 	ServiceExportCounterName string
 }
 
-var MaxExportStatusConditions = 10
-
 // nolint:gocritic // (hugeParam) This function modifies syncerConf so we don't want to pass by pointer.
 func New(spec *AgentSpecification, syncerConf broker.SyncerConfig, kubeClientSet kubernetes.Interface,
 	syncerMetricNames AgentConfig) (*Controller, error) {
@@ -410,20 +408,14 @@ func (a *Controller) updateExportedServiceStatus(name, namespace string, status
 			Message:            &msg,
 		}
 
-		numCond := len(toUpdate.Status.Conditions)
-		if numCond > 0 && serviceExportConditionEqual(&toUpdate.Status.Conditions[numCond-1], &exportCondition) {
+		// TODO: Currently we only check for conditionType Valid. Revisit this when Conflict is supported.
+		if len(toUpdate.Status.Conditions) > 0 && serviceExportConditionEqual(&toUpdate.Status.Conditions[0], &exportCondition) {
 			klog.V(log.TRACE).Infof("Last ServiceExportCondition for (%s/%s) is equal - not updating status: %#v",
-				namespace, name, toUpdate.Status.Conditions[numCond-1])
+				namespace, name, toUpdate.Status.Conditions[0])
 			return nil
 		}
 
-		if numCond >= MaxExportStatusConditions {
-			copy(toUpdate.Status.Conditions[0:], toUpdate.Status.Conditions[1:])
-			toUpdate.Status.Conditions = toUpdate.Status.Conditions[:MaxExportStatusConditions]
-			toUpdate.Status.Conditions[MaxExportStatusConditions-1] = exportCondition
-		} else {
-			toUpdate.Status.Conditions = append(toUpdate.Status.Conditions, exportCondition)
-		}
+		toUpdate.Status.Conditions = []mcsv1a1.ServiceExportCondition{exportCondition}
 
 		raw, err := resource.ToUnstructured(toUpdate)
 		if err != nil {


Test patch:
diff --git a/pkg/agent/controller/controller_suite_test.go b/pkg/agent/controller/controller_suite_test.go
index 1bfc304..749bbd5 100644
--- a/pkg/agent/controller/controller_suite_test.go
+++ b/pkg/agent/controller/controller_suite_test.go
@@ -554,7 +554,7 @@ func (t *testDriver) awaitNoEndpointSlice(client dynamic.ResourceInterface) {
 	test.AwaitNoResource(client, t.endpoints.Name+"-"+clusterID1)
 }
 
-func (t *testDriver) awaitServiceExportStatus(atIndex int, expCond ...*mcsv1a1.ServiceExportCondition) {
+func (t *testDriver) awaitServiceExportStatus(expCond ...*mcsv1a1.ServiceExportCondition) {
 	var found *mcsv1a1.ServiceExport
 
 	err := wait.PollImmediate(50*time.Millisecond, 5*time.Second, func() (bool, error) {
@@ -572,24 +572,21 @@ func (t *testDriver) awaitServiceExportStatus(atIndex int, expCond ...*mcsv1a1.S
 
 		found = se
 
-		if (atIndex + len(expCond)) != len(se.Status.Conditions) {
+		if (len(expCond)) != len(se.Status.Conditions) {
 			return false, nil
 		}
 
-		j := atIndex
-		for _, exp := range expCond {
-			actual := se.Status.Conditions[j]
-			j++
-			Expect(actual.Type).To(Equal(exp.Type))
-			Expect(actual.Status).To(Equal(exp.Status))
-			Expect(actual.LastTransitionTime).To(Not(BeNil()))
-			Expect(actual.Reason).To(Not(BeNil()))
-			Expect(*actual.Reason).To(Equal(*exp.Reason))
-			Expect(actual.Message).To(Not(BeNil()))
-
-			if exp.Message != nil {
-				Expect(*actual.Message).To(Equal(*exp.Message))
-			}
+		exp := expCond[0]
+		actual := se.Status.Conditions[0]
+		Expect(actual.Type).To(Equal(exp.Type))
+		Expect(actual.Status).To(Equal(exp.Status))
+		Expect(actual.LastTransitionTime).To(Not(BeNil()))
+		Expect(actual.Reason).To(Not(BeNil()))
+		Expect(*actual.Reason).To(Equal(*exp.Reason))
+		Expect(actual.Message).To(Not(BeNil()))
+
+		if exp.Message != nil {
+			Expect(*actual.Message).To(Equal(*exp.Message))
 		}
 
 		return true, nil
@@ -600,7 +597,7 @@ func (t *testDriver) awaitServiceExportStatus(atIndex int, expCond ...*mcsv1a1.S
 			Fail("ServiceExport not found")
 		}
 
-		Fail(format.Message(found.Status.Conditions, fmt.Sprintf("to contain at index %d", atIndex), expCond))
+		Fail(format.Message(found.Status.Conditions, fmt.Sprintf("to contain at index %d", 0), expCond))
 	} else {
 		Expect(err).To(Succeed())
 	}
@@ -637,15 +634,13 @@ func (t *testDriver) awaitNotServiceExportStatus(notCond *mcsv1a1.ServiceExportC
 	}
 }
 
-func (t *testDriver) awaitServiceExported(serviceIP string, statusIndex int) int {
+func (t *testDriver) awaitServiceExported(serviceIP string) {
 	t.cluster1.awaitServiceImport(t.service, mcsv1a1.ClusterSetIP, serviceIP)
+
 	t.awaitBrokerServiceImport(mcsv1a1.ClusterSetIP, serviceIP)
 	t.cluster2.awaitServiceImport(t.service, mcsv1a1.ClusterSetIP, serviceIP)
 
-	t.awaitServiceExportStatus(statusIndex, newServiceExportCondition(corev1.ConditionFalse, "AwaitingSync"),
-		newServiceExportCondition(corev1.ConditionTrue, ""))
-
-	return statusIndex + 2
+	t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionTrue, ""))
 }
 
 func (t *testDriver) awaitHeadlessServiceImport() {
@@ -678,8 +673,8 @@ func (t *testDriver) awaitHeadlessServiceUnexported() {
 	t.awaitNoEndpointSlice(t.cluster1.localEndpointSliceClient)
 }
 
-func (t *testDriver) awaitServiceUnavailableStatus(atIndex int) {
-	t.awaitServiceExportStatus(atIndex, newServiceExportCondition(corev1.ConditionFalse, "ServiceUnavailable"))
+func (t *testDriver) awaitServiceUnavailableStatus() {
+	t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionFalse, "ServiceUnavailable"))
 }
 
 func (t *testDriver) endpointIPs() []string {
diff --git a/pkg/agent/controller/globalnet_test.go b/pkg/agent/controller/globalnet_test.go
index 3b97611..cddcba6 100644
--- a/pkg/agent/controller/globalnet_test.go
+++ b/pkg/agent/controller/globalnet_test.go
@@ -61,7 +61,7 @@ var _ = Describe("Globalnet enabled", func() {
 				})
 
 				It("should sync a ServiceImport with the global IP", func() {
-					t.awaitServiceExported(globalIP1, 0)
+					t.awaitServiceExported(globalIP1)
 				})
 			})
 		})
@@ -69,10 +69,10 @@ var _ = Describe("Globalnet enabled", func() {
 		Context("and it does not initially have a global IP", func() {
 			Context("due to missing GlobalIngressIP", func() {
 				It("should update the ServiceExport status appropriately and eventually sync a ServiceImport", func() {
-					t.awaitServiceExportStatus(0, newServiceExportCondition(corev1.ConditionFalse, "ServiceGlobalIPUnavailable"))
+					t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionFalse, "ServiceGlobalIPUnavailable"))
 
 					t.createGlobalIngressIP(ingressIP)
-					t.awaitServiceExported(globalIP1, 1)
+					t.awaitServiceExported(globalIP1)
 				})
 			})
 
@@ -84,11 +84,11 @@ var _ = Describe("Globalnet enabled", func() {
 				})
 
 				It("should update the ServiceExport status appropriately and eventually sync a ServiceImport", func() {
-					t.awaitServiceExportStatus(0, newServiceExportCondition(corev1.ConditionFalse, "ServiceGlobalIPUnavailable"))
+					t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionFalse, "ServiceGlobalIPUnavailable"))
 
 					setIngressAllocatedIP(ingressIP, globalIP1)
 					test.UpdateResource(t.cluster1.localIngressIPClient, ingressIP)
-					t.awaitServiceExported(globalIP1, 1)
+					t.awaitServiceExported(globalIP1)
 				})
 			})
 		})
@@ -110,7 +110,7 @@ var _ = Describe("Globalnet enabled", func() {
 			It("should update the ServiceExport status with the condition details", func() {
 				c := newServiceExportCondition(corev1.ConditionFalse, condition.Reason)
 				c.Message = &condition.Message
-				t.awaitServiceExportStatus(0, c)
+				t.awaitServiceExportStatus(c)
 			})
 		})
 	})
diff --git a/pkg/agent/controller/service_export_failures_test.go b/pkg/agent/controller/service_export_failures_test.go
index 2780d8f..8e0eda8 100644
--- a/pkg/agent/controller/service_export_failures_test.go
+++ b/pkg/agent/controller/service_export_failures_test.go
@@ -51,7 +51,7 @@ var _ = Describe("Service export failures", func() {
 		})
 
 		It("should eventually update the ServiceExport status", func() {
-			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 		})
 	})
 })
diff --git a/pkg/agent/controller/service_import_sync_test.go b/pkg/agent/controller/service_import_sync_test.go
index e4e6314..7ef52e8 100644
--- a/pkg/agent/controller/service_import_sync_test.go
+++ b/pkg/agent/controller/service_import_sync_test.go
@@ -20,7 +20,6 @@ package controller_test
 
 import (
 	. "github.com/onsi/ginkgo"
-	"github.com/submariner-io/lighthouse/pkg/agent/controller"
 	corev1 "k8s.io/api/core/v1"
 	mcsv1a1 "sigs.k8s.io/mcs-api/pkg/apis/v1alpha1"
 )
@@ -45,17 +44,17 @@ var _ = Describe("ServiceImport syncing", func() {
 			It("should correctly sync a ServiceImport and update the ServiceExport status", func() {
 				t.createService()
 				t.createServiceExport()
-				t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+				t.awaitServiceExported(t.service.Spec.ClusterIP)
 			})
 		})
 
 		When("the Service doesn't initially exist", func() {
 			It("should initially update the ServiceExport status to Initialized and eventually sync a ServiceImport", func() {
 				t.createServiceExport()
-				t.awaitServiceUnavailableStatus(0)
+				t.awaitServiceUnavailableStatus()
 
 				t.createService()
-				t.awaitServiceExported(t.service.Spec.ClusterIP, 1)
+				t.awaitServiceExported(t.service.Spec.ClusterIP)
 			})
 		})
 	})
@@ -64,7 +63,7 @@ var _ = Describe("ServiceImport syncing", func() {
 		It("should delete the ServiceImport", func() {
 			t.createService()
 			t.createServiceExport()
-			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 
 			t.deleteServiceExport()
 			t.awaitServiceUnexported()
@@ -75,14 +74,14 @@ var _ = Describe("ServiceImport syncing", func() {
 		It("should delete and recreate the ServiceImport", func() {
 			t.createService()
 			t.createServiceExport()
-			nextStatusIndex := t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 
 			t.deleteService()
 			t.awaitServiceUnexported()
-			t.awaitServiceUnavailableStatus(nextStatusIndex)
+			t.awaitServiceUnavailableStatus()
 
 			t.createService()
-			t.awaitServiceExported(t.service.Spec.ClusterIP, nextStatusIndex+1)
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 		})
 	})
 
@@ -96,7 +95,7 @@ var _ = Describe("ServiceImport syncing", func() {
 			t.createServiceExport()
 
 			message := "AwaitingSync"
-			t.awaitServiceExportStatus(0, newServiceExportCondition(corev1.ConditionFalse, message))
+			t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionFalse, message))
 
 			t.awaitNotServiceExportStatus(&mcsv1a1.ServiceExportCondition{
 				Type:    mcsv1a1.ServiceExportValid,
@@ -105,27 +104,7 @@ var _ = Describe("ServiceImport syncing", func() {
 			})
 
 			t.cluster1.localServiceImportClient.PersistentFailOnCreate.Store("")
-			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
-		})
-	})
-
-	When("the ServiceExportCondition list count reaches MaxExportStatusConditions", func() {
-		var oldMaxExportStatusConditions int
-
-		BeforeEach(func() {
-			oldMaxExportStatusConditions = controller.MaxExportStatusConditions
-			controller.MaxExportStatusConditions = 1
-		})
-
-		AfterEach(func() {
-			controller.MaxExportStatusConditions = oldMaxExportStatusConditions
-		})
-
-		It("should correctly truncate the ServiceExportCondition list", func() {
-			t.createService()
-			t.createServiceExport()
-
-			t.awaitServiceExportStatus(0, newServiceExportCondition(corev1.ConditionTrue, ""))
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 		})
 	})
 
@@ -138,7 +117,7 @@ var _ = Describe("ServiceImport syncing", func() {
 			t.createService()
 			t.createServiceExport()
 
-			t.awaitServiceExportStatus(0, newServiceExportCondition(corev1.ConditionFalse, "UnsupportedServiceType"))
+			t.awaitServiceExportStatus(newServiceExportCondition(corev1.ConditionFalse, "UnsupportedServiceType"))
 			t.awaitNoServiceImport(t.brokerServiceImportClient)
 		})
 	})
@@ -162,7 +141,7 @@ var _ = Describe("ServiceImport syncing", func() {
 		It("should set the appropriate port information in the ServiceImport", func() {
 			t.createService()
 			t.createServiceExport()
-			t.awaitServiceExported(t.service.Spec.ClusterIP, 0)
+			t.awaitServiceExported(t.service.Spec.ClusterIP)
 		})
 	})
 })