clustermesh: fix a few issues in the new mcs api service controller

[MrFreezeex]

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Are you a user of Cilium? Please add yourself to the Users doc
• Thanks for contributing!

This commit does 3 small fixes:

• Use the correct upstream MCS-API controller. The controller used
  is now the ones that sync the service IP to the ServiceImport resources.
  The rest of the controllers are Cilium specific and will (or already is)
  be implemented soon.
• Also add a shortcut on creation to save a delete/recreate on
  of the derived service if there is no ServiceImport and the local is
  headless.
• Fix the watch on Services to also issue a reconcile if the locally
  exported Service has changed

Related to #27902

Fix a few issues with the newly added MCS-API controllers

[MrFreezeex]

/test

[YutaroHayakawa]

CC: @giorio94

[YutaroHayakawa]

Could you elaborate more about how this annotation is used within the service importing/exporting flow? Currently it is very hard to follow it.

[MrFreezeex]

Could you elaborate more about how this annotation is used within the service importing/exporting flow? Currently it is very hard to follow it.

Sure! Will try to explain it a bit better on the commit description/code comment but in the meantime here is an explanation:

This mcsapi implementation relies on a derived service that inherits mainly from the ServiceImport object. The spec of this derived service (named derived-$hash) is aggregated from all the remote services that has the same name/ns (+ if it's shared/has a Service export ofc).

As the derived service is already the aggregated/conflict resolved version we need to somehow have a way to find which cluster has what property before aggregating them, this is maybe a bit unclear in this PR because this part is not present yet. The aggregation/conflict resolution will be in another controller whose job is to create ServiceImport. I made already good progress on this but I need to polish it a bit more and I wanted to split a bit the review load (but then I realized that it made this PR a bit unclear sorry).

[YutaroHayakawa]

@MrFreezeex Hi, thanks for your clarification!

As the derived service is already the aggregated/conflict resolved version we need to somehow have a way to find which cluster has what property before aggregating them, this is maybe a bit unclear in this PR because this part is not present yet. The aggregation/conflict resolution will be in another controller whose job is to create ServiceImport.

Ok, then my next question is we could get this disaggregated information from ServiceExport + the actual exported service. Then why do we need to introduce this new annotation?

[giorio94]

Ok, then my next question is we could get this disaggregated information from ServiceExport + the actual exported service. Then why do we need to introduce this new annotation?

I second Yutaro's comment, here. It would be nice to have a diagram (maybe linked to the original issue) which depicts the flows you are thinking about, both in terms of what users would do, and what Cilium will automatically do to reconcile the information. That would help a lot in being able to properly review this PR (and the follow-ups), as we are otherwise missing the context about the next steps, and it becomes difficult to provide useful comments.

[MrFreezeex]

@giorio94 @YutaroHayakawa @joamaki thanks for the feedback, I will try to create a dedicated CFP issue with some schema of what the ServiceImport auto creation is doing in more details and present a few other alternatives so that we can choose one that make sense with all the info at hands 👍.

[MrFreezeex]

/test

[MrFreezeex]

Hi! Since we are having a separate discussion for how data should be shared across clusters to allow automatic ServiceImport creation I dropped the second commit about that and only kept the first one which brought a few fixes. I wanted to keep same PR so that the assigned reviewers don't change (although now it should probably be a sig-clustermesh only change), hopefully that's ok 😅.

[MrFreezeex]

/test

[MrFreezeex]

/ci-e2e

[MrFreezeex]

/ci-runtime

[giorio94]

These cleanup changes look good to me, thanks!

And sorry for the delay in reviewing the CFP, I plan to share my comments by today.

[giorio94]

Jussi's concern has been addressed, and his review is no longer strictly required, given that all changes are clustermesh-related only now. Marking as ready to merge to unblock.