Default Channel support for Channel CRDs by nachocano · Pull Request #1560 · knative/eventing

nachocano · 2019-07-24T22:10:32Z

Helps with #1343
A separate documentation PR plus defaulting Sequence's channels should fix it.

Proposed Changes

Introducing a new ConfigMap with cluster and namespace defaults for being able to set default Channel CRDs. This is similar to the current defaulting behavior for Channel provisioners.
Created a new Channel type, within messaging.knative.dev API group, that proxies an underlying Channel CRD implementation, as per @Harwayne's design. If a user just wants a Channel without caring about the actual implementation, she will create such object and some ConfigMap defaults will be used to determine the underlying implementation (e.g., InMemoryChannel, KafkaChannel, etc).
The underlying impl is set in the admission mutating webhook in channel.spec.channelTemplate, in case such value hasn't been set, which we assume will be the typical case. In other words, if the user knows she wants an InMemoryChannel, she will directly instantiate it, instead of through this proxy Channel object.
Setting defaults to Broker in the webhook to populate broker.spec.channelTemplateSpec if it hasn't been set, in order to enable default channel CRDs when using the namespace injection.
There were some not so nice renamings due to naming clashes, but all should be back to normal once we remove the old Channel provisioners stuff.

Release Note

A new Channel object in messaging.knative.dev API group has been introduced to be used in case you want a Channel but are not interested in its actual implementation (e.g., whether it is an InMemoryChannel, KafkaChannel, NatssChannel, etc.). Such implementation will be determined using a new Channel CRD defaulter, which can be configured in the newly introduced ConfigMap `default-ch-webhook`, mimicking the soon to be deprecated `default-channel-webhook`. Furthermore, Brokers injected with the namespace eventing annotation now leverage this new Channel CRD defaulter mechanism, to be able to configure their underlying channels implementations.
Note that the old Channel in eventing.knative.dev that uses the Provisioner model will be deprecated in 0.9, so we recommend moving to CRDs asap.

# Conflicts: # pkg/reconciler/sequence/resources/channel.go

vaikas · 2019-07-25T16:23:33Z

Just a quick question, regarding the proposed changes:
Last but not least, changing the ConfigMap at runtime won't have an impact on existing Channels nor Brokers that have been created with this default mechanism, as we have already set their channelTemplate to certain value. Example, if we change the ConfigMap from InMemoryChannel to KafkaChannel, and remove a Broker injected through a namespace annotation that used the previous ConfigMap configuration, such Broker will be recreated with the previous InMemoryChannel default, instead of KafkaChannel.

I find this interesting. I would expect no changes to existing channels or brokers, but what you seem to describe in your example. Changing the configmap, then deleting a broker I find interesting. I think I would expect the behaviour to be that if you change the configmap, then recreate the broker, the new defaults would be applied.

nachocano · 2019-07-25T16:34:52Z

I find this interesting. I would expect no changes to existing channels or brokers, but what you seem to describe in your example. Changing the configmap, then deleting a broker I find interesting. I think I would expect the behaviour to be that if you change the configmap, then recreate the broker, the new defaults would be applied.

Good catch. My previous impl did that, not the current one. If you change the defaults and delete a broker, it will get re-created with the new config. Sorry for the confusion. Let me remove that from the PR description...

vaikas

Awesome sauce, thanks for doing this, will definitely help with usability. Just a couple of minor things, mainly around documentation, tracking bugs, etc.

vaikas · 2019-07-25T16:54:06Z

Looks like the channel_types.go not having any test coverage is causing the test failure?

Harwayne · 2019-07-25T16:37:08Z

+		cs.Address = &v1alpha1.Addressable{}
+	}
+	if address != nil && address.URL != nil {
+		cs.Address.Hostname = address.URL.Host


This looks dangerous. We can only set Hostname if nothing in the URL is needed (i.e. path, query string, etc. are both empty).

copy/paste from Matt's changes when he migrated to the new Addressable. We are doing this all over...
You'd rather not set anything in the hostname? I'm more inclined to leave it as is, and maybe then revisit how we are doing it everywhere...

lionelvillard · 2019-07-25T17:08:34Z

code coverage must be at least 50% for each file to pass the test.

nachocano · 2019-07-25T18:46:11Z

Looks like the channel_types.go not having any test coverage is causing the test failure?

Added it, and a couple others. Let's see now.

knative-metrics-robot · 2019-07-25T19:54:30Z

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/apis/messaging/v1alpha1/channel_defaults.go	Do not exist	100.0%
pkg/apis/messaging/v1alpha1/channel_lifecycle.go	Do not exist	100.0%
pkg/apis/messaging/v1alpha1/channel_types.go	Do not exist	100.0%
pkg/apis/messaging/v1alpha1/channel_validation.go	Do not exist	93.3%
pkg/apis/messaging/v1alpha1/in_memory_channel_types.go	0.0%	100.0%	100.0
pkg/apis/messaging/v1alpha1/sequence_types.go	0.0%	100.0%	100.0
pkg/defaultchannel/channel_defaulter.go	Do not exist	91.7%
pkg/reconciler/channel/channel.go	83.3%	81.9%	-1.4
pkg/reconciler/eventingchannel/channel.go	Do not exist	83.3%
pkg/reconciler/eventingchannel/controller.go	Do not exist	100.0%

nachocano · 2019-07-25T20:23:05Z

code coverage must be at least 50% for each file to pass the test.

Thanks @lionelvillard, wasn't aware of that!

vaikas · 2019-07-25T20:47:53Z

/approve
Leaving lgtm to @Harwayne since there were some comments.

knative-prow-robot · 2019-07-25T20:48:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, vaikas-google

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~~ [vaikas-google]

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

nachocano · 2019-07-25T21:21:04Z

fyi initial documentation in this PR

Harwayne

/lgtm

nachocano added 30 commits July 22, 2019 11:12

adding messaging.channel resource

1802578

channel messaging

755ac4a

using ChannelTemplateSpec

b1f91d8

updating channel controller

7d34bcf

channel validation

71187ba

setting to channelTemplate

109e8c6

to json

2b54e41

channel defaulter

eac06fe

working

a7b0c63

removing logs

b419077

Merge remote-tracking branch 'upstream/master' into default-channel

f5d4997

# Conflicts: # pkg/reconciler/sequence/resources/channel.go

patching subscriptions... infinite loop

4727ae3

updates

d1723c1

immutable

970ada4

problems with cyclic dependencies

870a146

moving channel defaulter to duck to avoid cyclic dependency

5e5b8bd

channel defaulter test

d0ebf80

updating UTs

5cdb7e1

channel validation test

9043e37

updating validation

db7127b

updating lifecycle

eee7e1d

updates to broker

e1925ac

fixing sequence

c0e7af4

broker types

4916884

updating deprecated message

ef016c5

updating broker validation

dadac47

cosmetic

961b0f1

injection magic

d30d7ba

update to broker test

4761836

updating UTs

42de64b

knative-prow-robot requested a review from vaikas July 24, 2019 22:12

nachocano added 2 commits July 24, 2019 15:22

compilation, my bad...

c018c2d

hopefully the last one

025462e

vaikas reviewed Jul 25, 2019

View reviewed changes

Harwayne reviewed Jul 25, 2019

View reviewed changes

nachocano mentioned this pull request Jul 25, 2019

Document new Channel CRDs Default Mechanism knative/docs#1618

Closed

Harwayne reviewed Jul 25, 2019

View reviewed changes

nachocano added 2 commits July 25, 2019 10:27

Ville's comments

678e589

Adam's comments, plus cosmetic tests to try to make coverage pass.

356ca7c

mikehelmick mentioned this pull request Jul 25, 2019

Add a bad qualified name test for Channel Validation #342

Closed

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

nachocano mentioned this pull request Jul 25, 2019

Support for default channel #1343

Closed

Harwayne reviewed Jul 25, 2019

View reviewed changes

knative-prow-robot assigned Harwayne Jul 25, 2019

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

knative-prow-robot merged commit c0af697 into knative:master Jul 25, 2019

This was referenced Jul 25, 2019

Default channel for Sequence #1569

Merged

[BUG] Injected Broker relies on the default Channel provisioner #1530

Closed

chizhg mentioned this pull request Jul 29, 2019

Removing In-Memory CCP binaries and config, and updating release script #1484

Merged

lionelvillard mentioned this pull request Dec 12, 2019

adding lionelvillard to approvers #2314

Merged

Conversation

nachocano commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

vaikas commented Jul 25, 2019

Uh oh!

nachocano commented Jul 25, 2019

Uh oh!

vaikas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vaikas commented Jul 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Harwayne Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

nachocano Jul 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lionelvillard commented Jul 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nachocano commented Jul 25, 2019

Uh oh!

knative-metrics-robot commented Jul 25, 2019

Uh oh!

nachocano commented Jul 25, 2019

Uh oh!

vaikas commented Jul 25, 2019

Uh oh!

knative-prow-robot commented Jul 25, 2019

Uh oh!

nachocano commented Jul 25, 2019

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nachocano commented Jul 24, 2019 •

edited

Loading