Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kn channel create does not honour default-ch-webhook settings #1100

Closed
kameshsampath opened this issue Nov 6, 2020 · 11 comments
Closed

kn channel create does not honour default-ch-webhook settings #1100

kameshsampath opened this issue Nov 6, 2020 · 11 comments
Labels
good first issue Denotes an issue ready for a new contributor. kind/bug Categorizes issue or PR as related to a bug.

Comments

@kameshsampath
Copy link

Bug report

Using the docs https://knative.dev/docs/eventing/channels/default-channels/, we can configure the default channel, assume Kafka is installed and I configure the default-ch-webhook CM to be like:

apiVersion: v1
kind: ConfigMap
metadata:
  name: default-ch-webhook
  namespace: knative-eventing
data:
  default-ch-config: |
    clusterDefault:
      apiVersion: messaging.knative.dev/v1
      kind: InMemoryChannel
    namespaceDefaults:
      knativetutorial:
        apiVersion: messaging.knative.dev/v1beta1
        kind: KafkaChannel
        spec:
          numPartitions: 2
          replicationFactor: 1

NOTE: Kafka is configured and running correctly and so is KafkaChannel resource

When I do kn channel create my-events-ch -n knativetutorial, I expect the channel to be created as KafkaChannel type but its getting defaulted to InMemoryChannel

e.g.

$ kn channel -n knativetutorial ls

NAME           TYPE              URL                                                                AGE     READY   REASON
my-events-ch   InMemoryChannel   http://my-events-ch-kn-channel.knativetutorial.svc.cluster.local   4m33s   True

Expected behavior

cat <<EOF | kubectl create -f  -
apiVersion: messaging.knative.dev/v1
kind: Channel
metadata:
  name: my-events-ch
  namespace: knativetutorial
EOF
 kn channel ls
NAME           TYPE           URL                                                                AGE   READY   REASON
my-events-ch   KafkaChannel   http://my-events-ch-kn-channel.knativetutorial.svc.cluster.local   13s   True

Steps to reproduce the problem

  1. Set default channel webhook settings to be like
apiVersion: v1
kind: ConfigMap
metadata:
  name: default-ch-webhook
  namespace: knative-eventing
data:
  default-ch-config: |
    clusterDefault:
      apiVersion: messaging.knative.dev/v1
      kind: InMemoryChannel
    namespaceDefaults:
      knativetutorial:
        apiVersion: messaging.knative.dev/v1beta1
        kind: KafkaChannel
        spec:
          numPartitions: 2
          replicationFactor: 1
  1. Run the command kn channel create -n knativetutorial my-events-ch

kn version

Version:      v0.18.0
Build Date:   2020-10-07 19:07:26
Git Revision: 60f2c2c5
Supported APIs:
* Serving
  - serving.knative.dev/v1 (knative-serving v0.18.0)
* Eventing
  - sources.knative.dev/v1alpha2 (knative-eventing v0.18.0)
  - eventing.knative.dev/v1beta1 (knative-eventing v0.18.0)

Knative (serving/eventing) version

0.17.0

/kind good-first-issue

@kameshsampath kameshsampath added the kind/bug Categorizes issue or PR as related to a bug. label Nov 6, 2020
@knative-prow-robot knative-prow-robot added the good first issue Denotes an issue ready for a new contributor. label Nov 6, 2020
@rhuss
Copy link
Contributor

rhuss commented Nov 6, 2020

We switched to use the default "Channel" CRD which actually uses the default as configured for the cluster. So actually I would expect it to be just "Channel" as type in the list. @navidshaikh do you know whether this was included in 0.18 ?

@kameshsampath
Copy link
Author

kameshsampath commented Nov 6, 2020

@rhuss not sure I got what you meant, you mean default as the CM name ? can we not leverage the default-ch-webhook CM during the channel creation?

@aliok
Copy link
Member

aliok commented Nov 10, 2020

FYI, I bumped into this as well.

@rhuss, the configmap is not entirely ignored. Only the namespaceDefaults part.

So, kn simply uses the clusterDefault instead of using the namespaceDefaults when creating a resource for e.g. knativetutorial above.

@navidshaikh
Copy link
Collaborator

@navidshaikh do you know whether this was included in 0.18 ?

@rhuss : Yes, we populate messagingv1beta1.Channel and hand over to eventing.

@rhuss, the configmap is not entirely ignored. Only the namespaceDefaults part.
So, kn simply uses the clusterDefault instead of using the namespaceDefaults when creating a resource for e.g. knativetutorial above.

@aliok : kn doesnt know about the default channel config set via CM at all. It populates the messagingv1beta1.Channel spec and submits to eventing, the cluster/namespace defaults are then taken care by eventing layer.

I could reproduce the issue using (v1beta1 apis) yaml + kubectl as well. Cluster wide default works while namespace default doesn't.

- Setup eventing at v0.18.4 and kafka

- Set clusterwide channel to IMC (v1beta1) and namespace (knativetutorial) to Kafka (v1alpha1)

cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: ConfigMap
metadata:
  name: default-ch-webhook
  namespace: knative-eventing
data:
  default-ch-config: |
    clusterDefault:
      apiVersion: messaging.knative.dev/v1beta1
      kind: InMemoryChannel
    namespaceDefault:
      knativetutorial:
        apiVersion: messaging.knative.dev/v1alpha1
        kind: KafkaChannel
        spec:
          numPartitions: 2
          replicationFactor: 1
EOF

create a channel in default namespace

cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1beta1
kind: Channel
metadata:
  name: my-channel
  namespace: default
EOF

create a channel in knativetutorial namespace

cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1beta1
kind: Channel
metadata:
  name: my-channel
  namespace: knativetutorial
EOF

verify using kn

kn channel list -A

NAMESPACE         NAME         TYPE              URL                                                              AGE     READY   REASON
default           my-channel   InMemoryChannel   http://my-channel-kn-channel.default.svc.cluster.local           9m38s   True    
knativetutorial   my-channel   InMemoryChannel   http://my-channel-kn-channel.knativetutorial.svc.cluster.local   8m48s   True    

notice that the channel in knativetutorial should be a kafka channel based on the CM set above

@aliok
Copy link
Member

aliok commented Nov 11, 2020

@navidshaikh thanks for trying this.

I did exactly the same as you did (creating stuff with YAML) but I did it with 0.17 of eventing, 0.17 of eventing-contrib, 0.18 of kn. I now also realized I used v1 API instead of v1beta for Channel.

I think it is most likely there's a bug in v1beta1 Channel reconciliation and since kn is creating the Channel with that version, we're having this problem.
UPDATE: see #1100 (comment)

I will verify though.

BTW, how is the process for kn to start using the new API versions?

@aliok
Copy link
Member

aliok commented Nov 11, 2020

oh, @navidshaikh I just realized the default-ch-webhook you posted above has a typo: namespaceDefault --> namespaceDefaults.

@aliok
Copy link
Member

aliok commented Nov 11, 2020

@navidshaikh sorry, the typo got me confused and I made assumptions in 2 comments earlier.

I don't understand why (since kn is not doing anything with the confirmap) but I think it happens on kn only.

Configure default-ch-webhook

$ cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: ConfigMap
metadata:
  name: default-ch-webhook
  namespace: knative-eventing
data:
  default-ch-config: |
    clusterDefault:
      apiVersion: messaging.knative.dev/v1beta1
      kind: InMemoryChannel
    namespaceDefaults:
      knativetutorial:
        apiVersion: messaging.knative.dev/v1alpha1
        kind: KafkaChannel
        spec:
          numPartitions: 2
          replicationFactor: 1
EOF

configmap/default-ch-webhook configured

Create a Channel in default ns and knativetutorial ns:

$ cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1beta1
kind: Channel
metadata:
  name: my-channel
  namespace: default
EOF

$ cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1beta1
kind: Channel
metadata:
  name: my-channel
  namespace: knativetutorial
EOF

List channels:

$ kn channels list -A
NAMESPACE         NAME         TYPE              URL                                                              AGE     READY   REASON
default           my-channel   InMemoryChannel   http://my-channel-kn-channel.default.svc.cluster.local           3m9s    True
knativetutorial   my-channel   KafkaChannel      http://my-channel-kn-channel.knativetutorial.svc.cluster.local   2m50s   True

Works as expected. I also tried using v1 of Channel and I saw the same result:

cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1
kind: Channel
metadata:
  name: my-channel
  namespace: default
EOF

cat <<EOF | kubectl apply -f -
apiVersion: messaging.knative.dev/v1
kind: Channel
metadata:
  name: my-channel
  namespace: knativetutorial
EOF

But, when I create channels with kn, I see different results:

$ kn channel create my-events-ch -n default
Channel 'my-events-ch' created in namespace 'default'.
$ kn channel create my-events-ch -n knativetutorial
Channel 'my-events-ch' created in namespace 'knativetutorial'.
$ kn channels list -A
NAMESPACE         NAME           TYPE              URL                                                                AGE   READY   REASON
default           my-events-ch   InMemoryChannel   http://my-events-ch-kn-channel.default.svc.cluster.local           4s    True    
knativetutorial   my-events-ch   InMemoryChannel   http://my-events-ch-kn-channel.knativetutorial.svc.cluster.local   4s    True  

@aliok
Copy link
Member

aliok commented Nov 11, 2020

and, I tried this with Knative eventing 0.17.8, eventing-contrib 0.17.x, kn 0.18.latest

@aliok
Copy link
Member

aliok commented Nov 12, 2020

The root cause of this issue here is this one in Knative Eventing: knative/eventing#4514

navidshaikh added a commit to navidshaikh/client that referenced this issue Nov 12, 2020
 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for knative#1100
 this changeset should be reverted when eventing#4514 is resolved
knative-prow-robot pushed a commit that referenced this issue Nov 12, 2020
* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for #1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG
@navidshaikh
Copy link
Collaborator

/close

The issue is opened against eventing and we're carrying workaround for now.

@knative-prow-robot
Copy link
Contributor

@navidshaikh: Closing this issue.

In response to this:

/close

The issue is opened against eventing and we're carrying workaround for now.

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.

navidshaikh added a commit to navidshaikh/client that referenced this issue Nov 23, 2020
…1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for knative#1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG
navidshaikh added a commit to navidshaikh/client that referenced this issue Nov 24, 2020
…1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for knative#1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG
knative-prow-robot pushed a commit that referenced this issue Nov 24, 2020
* Embed the namespace in request body while creating channels (#1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for #1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG

* Update changelog for v0.18.3
knative-prow-robot pushed a commit that referenced this issue Nov 24, 2020
* Embed the namespace in request body while creating channels (#1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for #1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG

* Update CHANGELOG for v0.17.4
navidshaikh added a commit to navidshaikh/client that referenced this issue Nov 24, 2020
…1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for knative#1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG
knative-prow-robot pushed a commit that referenced this issue Nov 25, 2020
* Embed the namespace in request body while creating channels (#1117)

* Embed the namespace in request body while creating channels

 since on the eventing side, defaulting for channel isnt picking
 the namespace from the context (see knative/eventing#4514)

 workaround for #1100
 this changeset should be reverted when eventing#4514 is resolved

* Add CHANGELOG

* Update CHANGELOG for v0.19.1

* Cross-compile the kn binary for linux/s390x (#1083)

* Update CHANGELOG for v0.19.1

* Fix date in changelog

* Fix race conditions when creating watches (#1113)

* Fix a race condition between creating a watch and initiating the action that emits the event it is watching for

* update changelog

* add PR ID to changelog entry

* Fix merge in Changelog

* Fix table format in Changelog
Kaustubh-pande pushed a commit to Kaustubh-pande/client that referenced this issue Jul 22, 2022
* [release-1.3] Add kn-plugin-func v0.25.0

* Update vendor dir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants