[Feature] API Conditions unifications

[mjudeikis]

Is your feature request related to a problem? Please describe.
While you working on making Liqo GA, it would be great if you could unify object conditions to be closer to Upstream K8S.

Describe the solution you'd like

Currently, your objects are:

// Condition contains details about state of a.
type Condition struct {
	// Type of the condition.
	// +kubebuilder:validation:Enum="APIServerStatus";"NetworkConnectionStatus";"NetworkGatewayServerStatus";"NetworkGatewayClientStatus";"NetworkGatewayPresence";"NetworkConfigurationStatus";"AuthIdentityControlPlaneStatus";"AuthTenantStatus";"OffloadingVirtualNodeStatus";"OffloadingNodeStatus"
	//
	//nolint:lll // ignore long lines given by Kubebuilder marker annotations
	Type ConditionType `json:"type"`
	// Status of the condition.
	// +kubebuilder:validation:Enum="None";"Pending";"Established";"Error";"Ready";"NotReady";"SomeNotReady"
	// +kubebuilder:default="None"
	Status ConditionStatusType `json:"status"`
	// LastTransitionTime -> timestamp for when the condition last transitioned from one status to another.
	LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`
	// Reason -> Machine-readable, UpperCamelCase text indicating the reason for the condition's last transition.
	Reason string `json:"reason,omitempty"`
	// Message -> Human-readable message indicating details about the last status transition.
	Message string `json:"message,omitempty"`
}

Where upstream usually is more like this:

// Condition defines an observation of a object operational state.
type Condition struct {
	// 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.
	Type ConditionType `json:"type"`

	// Status of the condition, one of True, False, Unknown.
	Status corev1.ConditionStatus `json:"status"`

	// Severity provides an explicit classification of Reason code, so the users or machines can immediately
	// understand the current situation and act accordingly.
	// The Severity field MUST be set only when Status=False.
	// +optional
	Severity ConditionSeverity `json:"severity,omitempty"`

	// 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.
	LastTransitionTime metav1.Time `json:"lastTransitionTime"`

	// The reason for the condition's last transition in CamelCase.
	// The specific API may choose whether or not this field is considered a guaranteed API.
	// This field may not be empty.
	// +optional
	Reason string `json:"reason,omitempty"`

	// A human readable message indicating details about the transition.
	// This field may be empty.
	// +optional
	Message string `json:"message,omitempty"`
}

mainly Severity ConditionSeverity json:"severity,omitempty" and `Status corev1.ConditionStatus `json:"status"

This makes using upstream conditions setters, getters, and checkers easier. Like example we align them here:
https://github.com/kcp-dev/kcp/tree/main/sdk/apis/third_party/conditions/util/conditions
https://github.com/kcp-dev/kcp/tree/main/sdk/apis/third_party/conditions/apis/conditions/v1alpha1

In addition see example ^ how conditions apis are shared in every object.

This is good to have but makes things way easier as now you can do things like:

conditions.Set(edge, &conditionsapi.Condition{
				Type:    omnicastaiv1alpha1.PeeringReady,
				Status:  corev1.ConditionFalse,
				Message: err.Error(),
			})

And now if you look to your objects (v0.10.3 branch):

  peeringConditions:
  - lastTransitionTime: "2025-02-20T10:04:53Z"
    message: This ForeignCluster seems to be processable
    reason: ForeignClusterProcessable
    status: Success
    type: ProcessForeignClusterStatus
  - lastTransitionTime: "2025-02-20T10:04:58Z"
    message: The TunnelEndpoint has been successfully found in the Tenant Namespace
      liqo-tenant-little-meadow-a55620 and it is connected
    reason: TunnelEndpointAvailable
    status: Established
    type: NetworkStatus
  - lastTransitionTime: "2025-02-20T10:04:53Z"
    message: The Identity has been correctly accepted by the remote cluster
    reason: IdentityAccepted
    status: Established
    type: AuthenticationStatus
  - lastTransitionTime: "2025-02-20T10:04:55Z"
    message: The ResourceRequest has been accepted by the remote cluster in the Tenant
      Namespace liqo-tenant-little-meadow-a55620
    reason: ResourceRequestAccepted
    status: Established
    type: OutgoingPeering
  - lastTransitionTime: "2025-02-20T10:04:53Z"
    message: No ResourceRequest found in the Tenant Namespace liqo-tenant-little-meadow-a55620
    reason: NoResourceRequest
    status: None
    type: IncomingPeering
  - lastTransitionTime: "2025-02-20T10:04:55Z"
    message: The remote cluster API Server is ready
    reason: APIServerReady
    status: Established
    type: APIServerStatus

Statuses are None, Established, Success, etc. There is no single unification like in upstream True/False to tell if its ready or not.

[mjudeikis]

in example:

var healthyConditionsMap = map[discoveryv1alpha1.PeeringConditionType]discoveryv1alpha1.PeeringConditionStatusType{
	discoveryv1alpha1.ProcessForeignClusterStatusCondition: discoveryv1alpha1.PeeringConditionStatusSuccess,
	discoveryv1alpha1.NetworkStatusCondition:               discoveryv1alpha1.PeeringConditionStatusEstablished,
	discoveryv1alpha1.AuthenticationStatusCondition:        discoveryv1alpha1.PeeringConditionStatusEstablished,
	discoveryv1alpha1.OutgoingPeeringCondition:             discoveryv1alpha1.PeeringConditionStatusEstablished,
	discoveryv1alpha1.IncomingPeeringCondition:             discoveryv1alpha1.PeeringConditionStatusNone,
	discoveryv1alpha1.APIServerStatusCondition:             discoveryv1alpha1.PeeringConditionStatusEstablished,
}

this is a healthy state. Most of them are the same, with the exception of 2.

[claudiolor]

Hi @mjudeikis, thanks for opening this issue and bringing this topic.

I appreciate your suggestion and agree that being complaint to the standard way of doing things is always a good idea as it makes people's life simpler.
However, I fear that this would involve a massive work on the code and on the business logic, as this would require the refactoring of all the conditions to be more granular, as their value will be limited to a boolean.

The main target at the moment is finalizing the 1.0 release, which is supposed to be launched by the next week. Unfortunately, given the limited bandwidth, the risk is delaying the release or implementing these changes in a rush, with the risk of introducing bugs.

With many users waiting for the GA to start trying or using the new version of Liqo in production, the goal is delivering a release as stable and reliable as possible.

While I personally believe your proposal is valuable, and, to be honest, it would be nice doing this kind of API change before officially release 1.0, but probably, this is not crucial at the moment and we should instead focus on consolidation, leaving this to future improvements of the APIs.

That said, I’d love to hear your thoughts, do you see any critical reasons why this change should be made now?

[mjudeikis]

We started integrating the liqo "as a library" approach, and it just creates more mapping boilerplates. Nothing chatgpt cant help with :)
I agree; this is not a blocker, nor should it be. Maybe 1.2? 1.5 roadmap? :D

In general, I feel this is one of those where,e if not done now, it will never be done without v2.0, as having a stable release would require a migration strategy. I will leave it to your judgment, if you want to keep this or just close as "no fix" and move on :)