Open mikemorris opened 1 month ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: mikemorris Once this PR has been reviewed and has the lgtm label, please assign pmorie for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Cilium implementation is still WIP (but a mere few weeks, maybe a month from completion). And we are planning to only have ServiceImport auto created/deleted by our controllers right now like suggested by the KEP so from a Cilium perspective this looks good to me!
As per @skitt comment in office hours he mentions that Liqo is also a MCS API stakeholder
cc @mikemorris
As per @skitt comment in office hours he mentions that Liqo is also a MCS API stakeholder
cc @mikemorris
Looking into this a bit more I’m not sure Liqo actually implements the MCS API.
Refs https://github.com/kubernetes-sigs/mcs-api/issues/48#issuecomment-2043727874
The source of truth for these values are on the exported Service and these fields should generally be written to by a controller, not a human. Manually updating these fields could cause unexpected behavior.
This would be a breaking change, necessitating a v1alpha2 API version.
We expect this change should have minimal impact on existing users of either v1alpha1 or forked CRDs though because they largely interact with ServiceImport through the
foo.clusterset.local
address, not the CRD directly.I'm hoping that fixing this directly in the upstream spec would be an alternative to #20, and that all implementations would be able to use the MCS API CRDs directly.
I'm proposing to retain the now-empty
.spec
stanza because I believe there is future additive scope (intentionally excluded from this proposal) for the ability to export a service beyond a ClusterSet to be imported/consumed in a different Cluster which may not have the same "sameness" guarantees. Notably, this likely means that having a multi-cluster controller automatically generating a ServiceImport with an appropriate name and placing it in an appropriate namespace may not be possible to automate, and this could be a place where manually creating and naming a ServiceImport becomes a way to handle this, using spec fields to handle mapping to a ClusterSet-external exported service "known" to the cluster, and likely adding a status.conditions field to ServiceImport for a controller to report if this attempted mapping was successful.Other MCS API implemetations from which to solicit feedback on this change:
"ServiceImport IPs need update"
and"updated ServiceImport"
indicating that the Cloud Map controller is modifying these fields..spec.type
and.spec.ports
fields. Would be helpful to get direct feedback on whether this is desirable for any specific functionality (such as only exporting selected ports?), or whether the UX would be simplified if a controller set these in status automatically based on the underlying exported Services.Please add other known implementations!