KEP 1645: update conditions in ServiceExport after KEP-1623

[MrFreezeex]

• One-line PR description:
  Update conditions after KEP-1623 on KEP-1645 which match the actual CRDs here: https://github.com/kubernetes-sigs/mcs-api/blob/master/pkg/apis/v1alpha1/serviceexport.go#L47.

• Issue link: Multi-Cluster Services API #1645

• Other comments: Also swap the "2/5 cluster disagree" in the conflict message example so that it's clear that the disagree sentence is for the service that we export and not from the oldest Service that we conflict with.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[lauralorenz]

/remove-lifecycle stale

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrFreezeex
Once this PR has been reviewed and has the lgtm label, please ask for approval from jeremyot. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-multicluster/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[MrFreezeex]

Following today's sig-multicluster meeting I am proposing to add a new condition to reflect if the local service export is directly involved with the conflict. I am not that familiar with Kubernetes conditions so it may be more suitable to encode that info in the reason of the existing "Conflict" condition for instance. Feel free to comment if you have opinion about how this extra info should be exposed!

[tpantelis]

I think the reason should be "ConflictingType".

[MrFreezeex]

Yes indeed, although maybe we could extend the reason to do what I wanted to do for LocalConflict instead of encoding the type as well (like having LocalConflict/ExternalConflict as a reason) WDYT?

[MrFreezeex]

So with some inspiration from your message on slack if we encode having a local conflict in the same condition it would be probably one of these:

  - type: Conflict
    status: "True"
    lastTransitionTime: "2020-03-30T01:33:55Z"
    reason: "LocalConflict"
    message: "The local service type conflicts with other constituent clusters. 2/5 clusters disagree. Using \"ClusterSetIP\" from oldest service export in \"cluster-1\"."

  - type: Conflict
    status: "True"
    lastTransitionTime: "2020-03-30T01:33:55Z"
    reason: "ConflictingType"
    message: "The local service type conflicts with other constituent clusters. 2/5 clusters disagree. Using \"ClusterSetIP\" from oldest service export in \"cluster-1\"."

So if going that route we would have to decide if we want to encode local/external conflict or the "type" of conflict within the reason.

Those are still a suggested options to implementers and not the exact message that they absolutely need to provide ofc.

[tpantelis]

nit but I don't think this should merely duplicate the type - if there's no other short reason that adds value then leave it blank.

[MrFreezeex]

I now remember why I did this, we actually cannot add a Condition with an empty reason

[tpantelis]

Do you mean the API service rejects it?

[MrFreezeex]

I am not sure who rejects it but I retested it and by removing one reason I got the following error:

time="2024-09-26T10:12:11.051078297Z" level=error msg="Reconciler error" ServiceImport="{rebel-base-mcsapi default}" controller=serviceimport controllerGroup=multicluster.x-k8s.io controllerKind=ServiceImport error="ServiceExport.multicluster.x-k8s.io \"rebel-base-mcsapi\" is invalid: status.conditions[0].reason: Invalid value: \"\": conditions[0].reason in body should be at least 1 chars long" name=rebel-base-mcsapi namespace=default reconcileID="\"9d39229b-9e9b-4c8d-81d8-5a6cfda7dd68\"" subsys=controller-runtime

[tpantelis]

K8s rejects it b/c Reason is required in metav1.Condition:

// This field may not be empty
// +required
// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Pattern=`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`
Reason string `json:"reason" protobuf:"bytes,5,opt,name=reason"`

In the previous ServiceExportCondition it wasn't.