Remove Istio dependency from Eventing (Part - 1) by akashrv · Pull Request #1044 · knative/eventing

akashrv · 2019-04-10T01:12:24Z

Part of series of PRs for #294

Proposed Changes

This is part of a series of PRs that I will send to keep PR size small.
Overall change:

Create unique ExternalName K8s service for each channel in user namespace
Set the ExternalName to the dispatcher service in eventing namespace
Dispatcher needs to know how to map the host header (which is the External K8s service fqdn) to the Channel
Have a watch on Channels in each dispatcher and update in-memory map of hostname to channel name.
As part of this change remove dependency on ConfigMaps that were created and shared by the controller and the dispatcher. Now the controller updates the channel and dispatcher watches the channel to get the updates instead of configmap.

What is included in this PR:

Removed Istio virtual service dependency from in-memory channel
Sidecar is still deployed and will be removed in the last PR once all virtual service dependencies are removed.

There is an issue I identified when working with istio and externalname k8s service. I am following up with them separately, and there is a workaround in the code for it.

Release Note

Knative Eventing will create a unique K8s service of type ExternalName for each channel when using in-memory and in-memory-channel clusterchannelprovisioner

akashrv · 2019-04-10T13:52:29Z

/assign @evankanderson

Harwayne · 2019-04-10T15:22:15Z

/assign

akashrv · 2019-04-10T16:56:34Z

/assign @bbrowning

tests against channels other than in-memory

…xternalName

Harwayne · 2019-04-11T18:17:15Z

I don't think this fixes #294, I want this PR to reference it rather than closing it.

knative-metrics-robot · 2019-04-15T22:03:31Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/provisioners/channel_util.go	92.4%	90.8%	-1.5
pkg/provisioners/inmemory/channel/controller.go	50.0%	64.3%	14.3
pkg/provisioners/inmemory/channel/reconcile.go	94.4%	78.6%	-15.8
pkg/provisioners/inmemory/clusterchannelprovisioner/reconcile.go	93.4%	93.8%	0.3
pkg/provisioners/provisioner_util.go	78.9%	73.1%	-5.9

Harwayne · 2019-04-15T22:25:14Z

/lgtm
/approve

knative-prow-robot · 2019-04-15T22:25:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akashrv, Harwayne

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Harwayne]

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

evankanderson

Some comments now, since you asked for them anyway. :-D

evankanderson · 2019-04-15T22:36:25Z

 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	crlog "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+	// Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters).
+	// _ "k8s.io/client-go/plugin/pkg/client/auth/gcp"


This doesn't seem right -- why not just remove it?

I kept it around as it helps with debugging. When I started working on controllers or anything that connects to GKE from my local machine I would get some config related error. After searching online I found threads that it is because of missing package. So I added it here and then commented it out as a nicety that could help others working on the code base.

evankanderson · 2019-04-15T22:36:59Z

+	"sigs.k8s.io/controller-runtime/pkg/manager"
 	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+	// Uncomment the following line to load the gcp plugin (only required to authenticate against GKE clusters).
+	// _ "k8s.io/client-go/plugin/pkg/client/auth/gcp"


Again, why not remove this here? It's clearly not needed, and seems like it would be simpler to just remove it.

I kept it around as it helps with debugging. When I started working on controllers or anything that connects to GKE from my local machine I would get some config related error. After searching online I found threads that it is because of missing package. So I added it here and then commented it out as a nicety that could help others working on the code base.

evankanderson · 2019-04-15T22:39:30Z

-	}
-	if err != nil {
+	if err = v1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
+		logger.Error("Error while adding eventing scheme to manager.", zap.Error(err))


Should this include the scheme?

Is the question whether scheme is needed or should it be in this function?

Adding this scheme is necessary or else the watch through manager won't work

I considered whether I should add it here or in the channelwatcher.New function. Looking at rest of the code where controllers are created, I found that schemes are added in cmd/main rather than inside the controller. So I kept it this way to be consistent with rest of the code.

evankanderson · 2019-04-15T22:47:31Z

-		return err
+func listAllChannels(ctx context.Context, c client.Client) ([]v1alpha1.Channel, error) {
+	channels := make([]v1alpha1.Channel, 0)
+	for {


It might be worth a comment here indicating that this is a do... while loop on opts.Raw.Continue.

In particular, I'm surprised that the client.Client interface doesn't perform the full listing.

A reading of the ListOptions struct suggests that pagination may only be provided if limit is set (and I suspect that many clients would have failures if this wasn't the case). Do you have evidence that the more complicated code with continue tokens is needed?

This is probably copied from other code that added pagination support defensively (@Harwayne may be able to provide context). Based on my reading of the original design for apiserver pagination, it appears @evankanderson is correct: pagination is opt-in by specifying the limit parameter.

I am paranoid. I don't think it gets used today. I believe @akashrv looked into the Controller Runtime code in particular and confirmed that it isn't used. But the fact that it is in the interface makes me wary of leaving it out. I agree that ListOptions.Limit clearly talks about Continue with regards to having a limit set, but ListOptions.Continue doesn't specify that is the only time Continue will be used.

I agree with your reading of the origin design that this is purely an opt-in feature and that not providing a limit means get everything (even if that is 500 MB). Having been shown this, I am OK removing it.

Sounds good. I'll simplify it and revert it to how it was in first iteration.

evankanderson · 2019-04-15T22:48:36Z

-		return err
+func shouldWatch(ch *v1alpha1.Channel) bool {
+	if ch.Spec.Provisioner != nil && ch.Spec.Provisioner.Namespace == "" {
+		for _, v := range channelProvisioners {


Would it make more sense to make channelProvisioners a map at init time, so you can use a map membership check?

We don't expect to have too many provisioners. Most cases this should be just one check without a for loop.
Moreover when we rename fanoutsidecar to be in-memory provisioner and deprecate the old in-memory-channel. This code will change to a single value check.
Hence left it this way.

evankanderson · 2019-04-15T22:51:48Z

-	if err != nil {
-		return err
+func shouldWatch(ch *v1alpha1.Channel) bool {
+	if ch.Spec.Provisioner != nil && ch.Spec.Provisioner.Namespace == "" {


Maybe make namespace non-empty an early exit with a comment that we only support cluster-scoped provisioners (so it's easier to extend to namespace-scoped provisioners later)?

Didn't understand the comment. This function is used to filter out channels in the handler inside the watch. It is not used in the request path.

I was suggesting writing this as:

if ch.Spec.Provisioner == nil || ch.Spec.Provisioner.Namespace != "" { // Only support cluster-level provisioners right now. return false } ...

evankanderson

Last cleanup comments; the rest of the logic seems fine, though it might be better to split cleanups (like gofmt) from meaningful changes, and possibly even to break up the PR into smaller chunks based on API boundaries.

evankanderson · 2019-04-16T21:43:03Z

 			Ports: []corev1.ServicePort{
 				{
-					Name:       "http",
+					Protocol:   corev1.ProtocolTCP,


Why remove the name?

If Istio is running sidecar injection, wouldn't you want to mark it for Istio?

Check this one: istio/istio#13193.
Istio has a bug where if I name it then it wont work since it is an ExternalName service. So the change is to make it work in case pod that calls the channel has istio sidecar in it. Without istio sidecar it works irrespective of whether the port is named or not

evankanderson · 2019-04-16T21:44:10Z

+}
+
+func (r *reconciler) Reconcile(req reconcile.Request) (reconcile.Result, error) {
+	ctx := logging.WithLogger(context.TODO(), r.logger.With(zap.Any("request", req)))


I think this is reasonably context.Background(). (This makes it easier to find actual TODO contexts where the context should be flowed through later.)

Will change it in the one of the follow-up PRs

evankanderson · 2019-04-16T21:54:28Z

-		return err
+func listAllChannels(ctx context.Context, c client.Client) ([]v1alpha1.Channel, error) {
+	channels := make([]v1alpha1.Channel, 0)
+	for {


In particular, I'm surprised that the client.Client interface doesn't perform the full listing.

A reading of the ListOptions struct suggests that pagination may only be provided if limit is set (and I suspect that many clients would have failures if this wasn't the case). Do you have evidence that the more complicated code with continue tokens is needed?

evankanderson · 2019-04-16T22:25:01Z

-					Name:       "http",
+					// There is a bug in Istio where named port doesn't work when connecting using an ExternalName service
+					// Refer to https://github.com/istio/istio/issues/13193 for more details.
+					// TODO: Uncomment Name:"http" when ISTIO fixes the issue


Does this need to be copied to the comment I made above?

Yes this is the reason I removed it. Didn't put the comment in UT, because when we revert this UTs will anyways fail and we will have to update UTs.

Co-authored-by: serverless-qe <serverless-support@redhat.com>

akashrv and others added 5 commits April 5, 2019 10:13

WIP

99fddec

Merge branch 'noistio2' of github.com:akashrv/eventing into noistio2

c08546a

WIP - In-memory working with E2E tests

c642cea

WIP - remove istio dependency from in-memroy channel

2a4faae

UTs pass, E2E tests pass with in-memory as well as kafka

bd7ae68

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 10, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 10, 2019

knative-prow-robot requested review from Harwayne and vaikas April 10, 2019 01:12

fixed uts that failed due to last K8s service change

df4487f

knative-prow-robot assigned evankanderson Apr 10, 2019

knative-prow-robot assigned Harwayne Apr 10, 2019

knative-prow-robot assigned bbrowning Apr 10, 2019

akashrv added 3 commits April 10, 2019 16:14

Removed unnecessary space from a line

23ae8b4

Merge branch 'noistio2' of github.com:akashrv/eventing into noistio2

a04d6d9

dding istio annotation to test POD. This will ve needed when running E2E

bb7ab3e

tests against channels other than in-memory

akashrv commented Apr 11, 2019

View reviewed changes

Comment thread pkg/provisioners/inmemory/channel/reconcile.go

Bug fix to set clusterIp of K8s service only when it is not of type E…

c646dcd

…xternalName

Harwayne reviewed Apr 11, 2019

View reviewed changes

Updated code based on PR comments

feb5e64

Harwayne reviewed Apr 15, 2019

View reviewed changes

Comment thread pkg/provisioners/inmemory/channel/reconcile.go Outdated

Comment thread pkg/provisioners/inmemory/controller/main.go Outdated

akashrv added 5 commits April 15, 2019 14:11

Updates based on PR comments

d2c831f

Merge branch 'master' of github.com:knative/eventing into noistio2

d61cd1a

Updates based on PR comments

16a6ffc

Fixed UTs

67611dc

Updated VENDOR_LICENSE

2cc8525

Harwayne reviewed Apr 15, 2019

View reviewed changes

Comment thread pkg/provisioners/channel_util.go

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2019

knative-prow-robot merged commit f8317dd into knative:master Apr 15, 2019

evankanderson reviewed Apr 15, 2019

View reviewed changes

akashrv mentioned this pull request Apr 16, 2019

Remove Istio dependency from Eventing (Part - 2) #1058

Merged

Harwayne mentioned this pull request Apr 16, 2019

Move in-memory channel dispatcher config map related code to utils #607

Closed

evankanderson reviewed Apr 16, 2019

View reviewed changes

grantr mentioned this pull request Apr 18, 2019

Reconcile using pkg/controller #1057

Merged

akashrv mentioned this pull request Apr 19, 2019

Remove Istio dependency from Eventing (Part - 3) - Natss #1074

Merged

Harwayne mentioned this pull request Apr 25, 2019

[WIP] In-memory Channels without Istio sidecar #524

Closed

This was referenced Apr 25, 2019

Remove Istio dependency from Eventing (Part - 4) - gcp-pubsub #1107

Merged

Remove Istio dependency from Eventing (Part - 5) - final cleanup #1119

Merged

creydr pushed a commit to creydr/knative-eventing that referenced this pull request Feb 5, 2025

Update dependabot configurations (knative#1044)

0f815ad

Co-authored-by: serverless-qe <serverless-support@redhat.com>

Conversation

akashrv commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

akashrv commented Apr 10, 2019

Uh oh!

Harwayne commented Apr 10, 2019

Uh oh!

akashrv commented Apr 10, 2019

Uh oh!

Uh oh!

Harwayne commented Apr 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Apr 15, 2019

Uh oh!

Uh oh!

Harwayne commented Apr 15, 2019

Uh oh!

knative-prow-robot commented Apr 15, 2019

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

akashrv commented Apr 10, 2019 •

edited

Loading