Istio MCS Generating ServiceExports #29763

josephpeacock · 2020-12-23T21:31:19Z

This PR accomplishes stage 2 in the Istio MCS RFC

In short, it allows Istio to be configured to create ServiceExport objects for non cluster-local services in an Istio-managed cluster.

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ X ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes

Please check any characteristics that apply to this pull request.

[ ] Does not have any changes that may affect Istio users.

istio-policy-bot · 2020-12-23T21:31:22Z

😊 Welcome @josephpeacock! This is either your first contribution to the Istio istio repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

josephpeacock · 2020-12-23T21:32:32Z

This PR accomplishes #29385 and is dependent upon #29763

hzxuzhonghu

From the implement, could not see when the serviceExport will be consumed and by who

hzxuzhonghu · 2020-12-24T03:23:02Z

manifests/charts/base/files/gen-istio-cluster.yaml

@@ -3620,6 +3620,11 @@ rules:
  - apiGroups: [""]
    resources: ["secrets"]
    verbs: ["get", "watch", "list"]
+
+  #Used for MCS serviceexport management
+  - apiGroups: [""]


Is this in core group, i think it can not be.

I'm a bit unfamiliar with the manifests, could you recommend a more appropriate place?

hzxuzhonghu · 2020-12-24T03:24:36Z

manifests/charts/base/files/gen-istio-cluster.yaml

+  #Used for MCS serviceexport management
+  - apiGroups: [""]
+    resources: ["serviceexports"]
+    verbs: ["create", "delete"]


no get list needed?

and watch, probably update and patch will be needed as well?

This PR only needs create/delete on serviceexports. The eventual PR to consume them will likely need the ability to watch, but I want to add the minimal set of permissions necessary for this functionality.

hzxuzhonghu · 2020-12-24T03:34:53Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	queue           queue.Instance
+	serviceInformer cache.SharedInformer
+
+	clusterLocalHosts []string //hosts marked as cluster-local, which will not have serviceeexports generated


what is this for, concerned about the peformance

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

hzxuzhonghu · 2020-12-24T03:37:15Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	return false
+}
+
+func (sc *ServiceExportController) createServiceExportIfNotPresent(service *v1.Service) error {


Who will consume the ServiceExports created here

The short answer is anyone will be able to. I'm working in parallel with @nmittler on the implementation of the Istio MCS RFC. I took "phase 2", and there is another issue open for "phase 1" (#29384)

The consumption of ServiceExports is out of scope for this PR.

It is not clear how this works from the RFC, should wait for the draft implement.

As i read through the mcs-api desgin, it is not clear to me how this can solve multicluster topology issue.

Can you elaborate on how the EndpointSlice create is used? https://github.com/kubernetes/enhancements/tree/master/keps/sig-multicluster/1645-multi-cluster-services-api#importing-services

@hzxuzhonghu

The basic MCS flow is:

Something generates a ServiceExport for a service in a cluster

The MCS controller (Istio?) sees the ServiceExport and creates ServiceImport and EndpointSlices in all of the other clusters in the clusterset (i.e. mesh).

In the other clusters, the ServiceImport and EndpointSlices are used by K8s to program kube proxy to allow service calls to span clusters.

What this PR is attempting to accomplish is 1. This simply creates the ServiceExport resource ... it does nothing with EndpointSlices. This will have no real impact on the system if there is no MCS controller performing the ServiceImport.

howardjohn

I don't really understand why Istio is responsible for this. I presume MCSD will have multiple implementors, otherwise its not really useful. Will linkerd, consul, etc all be sending similar PRs, writing the same exact implementation of the same thing in 3 slightly different ways?

I would have expected the MCSD repo to have a controller, which is either deployed standalone, or maybe, once its past experimental, linked into Istiod as a library to reduce the deployment complexity.

I am a bit concerned that every project is going to implement the same controller slightly different and we will not actually have a consistent API like was desired.

howardjohn · 2020-12-28T00:07:18Z

manifests/charts/base/files/gen-istio-cluster.yaml

+  #Used for MCS serviceexport management
+  - apiGroups: [""]
+    resources: ["serviceexports"]
+    verbs: ["create", "delete"]


and watch, probably update and patch will be needed as well?

howardjohn · 2020-12-28T00:08:14Z

pilot/pkg/serviceregistry/kube/controller/multicluster.go

@@ -79,6 +79,14 @@ type Multicluster struct {
 	secretNamespace  string
 	secretController *secretcontroller.Controller
 	syncInterval     time.Duration
+
+	//MCS Settings - settings for behavior of controllers relating to MCS
+	mcsSettings *MCSSettings


nit: there is no need for this to be a pointer as far as I know

howardjohn · 2020-12-28T00:09:28Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	queue           queue.Instance
+	serviceInformer cache.SharedInformer
+
+	clusterLocalHosts []string //hosts marked as cluster-local, which will not have serviceeexports generated


I am concerned about the API here. Why are we mixing an Istio API ("cluster local") with the MCSD api? Isn't the goal of MCSD that we are just another implementor?

Isn't the goal of MCSD that we are just another implementor?

Yes, but one implementation detail that we get to determine is which services are exported (the API intentionally leaves that up to whatever entity is generating serviceExports). So we're piggybacking off of this Istio API with the assumption that cluster local hosts should not be exported for MCS.

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

howardjohn · 2020-12-28T00:11:58Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+
+	_, err := sc.client.MulticlusterV1alpha1().ServiceExports(service.Namespace).Create(context.TODO(), &serviceExport, metav1.CreateOptions{})
+
+	if err != nil && strings.Contains(err.Error(), "already exists") {


nit: use errors.IsAlreadyExists

howardjohn · 2020-12-28T00:12:22Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	_, err := sc.client.MulticlusterV1alpha1().ServiceExports(service.Namespace).Create(context.TODO(), &serviceExport, metav1.CreateOptions{})
+
+	if err != nil && strings.Contains(err.Error(), "already exists") {
+		err = nil //This is the error thrown by the client if there is already an object with the name in the namespace. If that's true, we do nothing


what if the contents are wrong?

For this implementation, we are making the intentional choice that we don't care, and don't support cases in which some other entity (either human or 3rd party MCS controller) modifies a ServiceExport. So if it exists, we stop processing (to minimize cases where the other entity overwrite may be desired behavior).

howardjohn · 2020-12-28T00:13:10Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+}
+
+func (sc *ServiceExportController) doInitialFullSync() {
+	allServices, err := sc.serviceClient.Services("").List(context.TODO(), metav1.ListOptions{})


Calling .List() is not acceptable for performance, we should be using the informer cache. Direct API calls like this have bit us many times in the past

josephpeacock · 2021-01-04T16:13:59Z

@howardjohn @hzxuzhonghu Maybe this is something we should discuss at the next Environments working group meeting? I was under the impression that the approved RFC represented an agreement that we were going to build the specified functionality into Istio. In meeting with @nmittler, we decided that phases one and two from that document could be done in parallel, and that I would put together a PR encompassing only stage two.

I'm happy to shelve or permanently close this PR if it's agreed that building MCS functionality into Istio is the wrong direction for the project. You both had some very good specific feedback in the code as well, and I appreciate the prompt review. But I'd like for us to agree on the big picture direction before I spend more time rebasing and/or updating this PR.

howardjohn · 2021-01-04T16:47:23Z

To clarify a bit. I am not *against* this, I am just not sure it's the right thing to do without more info (answers to questions above about the wider ecosystem). The RFC is a bit light on the details. Discussion in env WG (or this PR) SGTM! Thanks for working on this

…

On Mon, Jan 4, 2021 at 8:14 AM josephpeacock ***@***.***> wrote: @howardjohn <https://github.com/howardjohn> @hzxuzhonghu <https://github.com/hzxuzhonghu> Maybe this is something we should discuss at the next Environments working group meeting? I was under the impression that the approved RFC <https://docs.google.com/document/d/1K8hvQ83UcJ9a7U8oqXIefwr6pFJn-VBEi40Ak-fwQtk/edit#heading=h.xw1gqgyqs5b> represented an agreement that we were going to build the specified functionality into Istio. In meeting with @nmittler <https://github.com/nmittler>, we decided that phases one and two from that document could be done in parallel, and that I would put together a PR encompassing only stage two. I'm happy to shelve or permanently close this PR if it's agreed that building MCS functionality into Istio is the wrong direction for the project. You both had some very good specific feedback in the code as well, and I appreciate the prompt review. But I'd like for us to agree on the big picture direction before I spend more time rebasing and/or updating this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29763 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXM5F5DUYPPLCJH24FTSYHSNPANCNFSM4VHOXF5Q> .

josephpeacock · 2021-01-04T20:22:20Z

Fair enough, let me answer the questions that I feel that I can speak to:

I don't really understand why Istio is responsible for this. I presume MCSD will have multiple implementors, otherwise its not really useful. Will linkerd, consul, etc all be sending similar PRs, writing the same exact implementation of the same thing in 3 slightly different ways?

I think this is the intention of the k8s spec. They explicitly say in their document that anything should be able to be an MCS controller:

mcs-controller - A controller that syncs services across clusters and makes them available for multi-cluster service discovery and connectivity. There may be multiple implementations, this doc describes expected common behavior. The controller may be a single controller, multiple decentralized controllers, or a human using kubectl to create resources. This document aims to support any implementation that fulfills the behavioral expectations of this API.

In this case, I'd argue it's a question of value-add. Do we see value in building out MCS controller functionality in Istio? (with the alternative being having users need to use another MCS controller in tandem with Istio if they want that functionality). And I do think being able to use service names to route across multiple clusters would really simplify some multicluster Istio installations.

I would have expected the MCSD repo to have a controller, which is either deployed standalone, or maybe, once its past experimental, linked into Istiod as a library to reduce the deployment complexity.

The only official k8s MCS repo that I know of at the moment is for the MCS API. I don't know if there is a plan for an official k8s MCS controller, or if they are intending to leave it entirely up to the community.

I am a bit concerned that every project is going to implement the same controller slightly different and we will not actually have a consistent API like was desired.

You're probably right that there may be opportunities for a wider conversation about how specific the API is and should be moving forward on the k8s side. But I'm not sure if that's relevant to Istio's implementation, unless we want to wait to implement until the spec is more well-defined.

nmittler · 2021-01-06T18:42:33Z

pilot/pkg/bootstrap/options.go

+
+	MCSServiceExportEnabled bool
+
+	ClusterLocalHosts []string


Doesn't this duplicate the configuration in MeshConfig?

nmittler · 2021-01-06T18:43:28Z

pilot/pkg/bootstrap/server.go

@@ -232,6 +232,11 @@ func NewServer(args *PilotArgs) (*Server, error) {
 	s.initMeshNetworks(args, s.fileWatcher)
 	s.initMeshHandlers()

+	e.PushContext.Mesh = e.Mesh()
+	args.RegistryOptions.ClusterLocalHosts = e.PushContext.GetClusterLocalHosts(e)


Can't meshconfig be updated?

nmittler · 2021-01-06T18:49:59Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	return sc.deleteServiceExportIfPresent(obj)
+}
+
+func convertToService(obj interface{}) (*v1.Service, error) {


We do this elsewhere ... can we use a common function?

nmittler · 2021-01-06T18:50:57Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	return cm, nil
+}
+
+func (sc *ServiceExportController) isServiceClusterLocal(service *v1.Service) bool {


Can we move this function to the context?

I'm not sure what you mean by this, can you clarify?

The context is where you're getting information regarding cluster-local services. Could it just expose this method instead of defining it here?

nmittler · 2021-01-06T18:52:26Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+	//cannot use the auto-generated client as it hardcodes the namespace in the client struct, and we can't have one client per watched ns
+	err := sc.client.MulticlusterV1alpha1().ServiceExports(service.Namespace).Delete(context.TODO(), service.Name, metav1.DeleteOptions{})
+
+	if err != nil && strings.Contains(err.Error(), "not found") {


Is there a standard error here, similar to errors.IsAlreadyExists?

josephpeacock · 2021-01-07T02:25:22Z

Ready for re-review.

rcernich · 2021-03-11T16:59:20Z

pilot/pkg/serviceregistry/kube/controller/serviceexportcontroller.go

+func (sc *ServiceExportController) createServiceExportIfNotPresent(service *v1.Service) error {
+	serviceExport := v1alpha1.ServiceExport{}
+	serviceExport.Namespace = service.Namespace
+	serviceExport.Name = service.Name


Do you want to add an owner reference to this? This would also eliminate the need for handling deletions.

That is actually something that is explicitly called out by the K8s MCS Spec, and was decided against. Instead, they chose to rely entirely on the name mapping. I tried to make this implementation match the spec as closely as possible.

istio-testing · 2021-03-11T19:25:38Z

@josephpeacock: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
integ-pilot-multicluster-tests_istio	`e8a4426`	link	`/test integ-pilot-multicluster-tests_istio`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nmittler · 2021-03-11T19:26:33Z

@josephpeacock thanks for all the help. I'm taking this over in #31401. Shall we just close this one out?

josephpeacock · 2021-03-11T19:44:36Z

Yup, let's do that. Thanks for taking this over!

Taken over from istio#29763

Taken over from #29763 Co-authored-by: Joe Peacock <joe.peacock.joe@gmail.com>

josephpeacock requested review from costinm, howardjohn, linsun, nmittler and a team as code owners December 23, 2020 21:31

istio-policy-bot added the area/networking label Dec 23, 2020

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 23, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2020

josephpeacock changed the title ~~Serviceexport~~ Istio MCS Generating ServiceExports Dec 23, 2020

josephpeacock requested review from a team as code owners December 23, 2020 22:16

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 23, 2020

hzxuzhonghu requested changes Dec 24, 2020

View reviewed changes

howardjohn requested changes Dec 28, 2020

View reviewed changes

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 1, 2021

nmittler reviewed Jan 6, 2021

View reviewed changes

josephpeacock force-pushed the serviceexport branch from 917fb42 to 937afc4 Compare January 7, 2021 01:26

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 7, 2021

josephpeacock added 11 commits March 11, 2021 08:53

it was a permission issue

e5d13d3

fixing rebase issues

aa64c20

Fixing build error

c2dfdec

Restarting istiod after the serviceexport CRD is applied

8780f71

Re-ordering the setuo to apply the CRD before installing istio

163560e

code review feedback

8a97476

code review feedback

f34338f

forgot to un-export one method

2bf0e29

Undoing env var if check in the client creation

ff30349

Quick functional test fix

2f3333d

Multicluster test experiment

10fa826

rcernich reviewed Mar 11, 2021

View reviewed changes

josephpeacock force-pushed the serviceexport branch from 1986696 to 10fa826 Compare March 11, 2021 17:13

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 11, 2021

back to baseline

e8a4426

nmittler mentioned this pull request Mar 11, 2021

Istio MCS Generating ServiceExports #31401

Merged

josephpeacock closed this Mar 11, 2021

nmittler pushed a commit to nmittler/istio that referenced this pull request Mar 18, 2021

Istio MCS Generating ServiceExports

3b328e0

Taken over from istio#29763

istio-testing pushed a commit that referenced this pull request Mar 18, 2021

Istio MCS Generating ServiceExports (#31401)

9d5ba69

Taken over from #29763 Co-authored-by: Joe Peacock <joe.peacock.joe@gmail.com>


		_, err := sc.client.MulticlusterV1alpha1().ServiceExports(service.Namespace).Create(context.TODO(), &serviceExport, metav1.CreateOptions{})

		if err != nil && strings.Contains(err.Error(), "already exists") {

Istio MCS Generating ServiceExports #29763

Istio MCS Generating ServiceExports #29763

Conversation

josephpeacock commented Dec 23, 2020 • edited by istio-policy-bot

istio-policy-bot commented Dec 23, 2020

josephpeacock commented Dec 23, 2020

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephpeacock commented Jan 4, 2021

howardjohn commented Jan 4, 2021 via email

josephpeacock commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmittler Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephpeacock commented Jan 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

istio-testing commented Mar 11, 2021

nmittler commented Mar 11, 2021

josephpeacock commented Mar 11, 2021

josephpeacock commented Dec 23, 2020 •

edited by istio-policy-bot

nmittler Jan 6, 2021 •

edited