eventing icon indicating copy to clipboard operation
eventing copied to clipboard

Defaulting for Channel should resolve the namespace from context, not the resource

Open aliok opened this issue 3 years ago • 14 comments

Describe the bug When I use Kube REST api in the way kn uses it, I can see behavior differences when I post these 2:

  • {"kind":"Channel","apiVersion":"messaging.knative.dev/v1beta1","metadata":{"name":"my-channel"}}
  • {"kind":"Channel","apiVersion":"messaging.knative.dev/v1beta1","metadata":{"name":"my-channel", "namespace":"knativetutorial"}}

URL posted is same: apis/messaging.knative.dev/v1beta1/namespaces/knativetutorial/channels

This problem makes eventing webhook pick the wrong default channel for namespace.

See following.

Make IMC cluster default and KafkaChannel knativetutorial namespace default:

$ 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

Start proxy:

$ kubectl proxy --port=8080 &

Post 2 channel requests to the same namespace URL; 1 with namespace in metadata, other not:

$ curl -XPOST  \
    -d '{"kind":"Channel","apiVersion":"messaging.knative.dev/v1beta1","metadata":{"name":"my-channel-1", "namespace":"knativetutorial"}}' \
    -H "Content-Type: application/json" \
    'http://localhost:8080/apis/messaging.knative.dev/v1beta1/namespaces/knativetutorial/channels'

$ curl -XPOST  \
    -d '{"kind":"Channel","apiVersion":"messaging.knative.dev/v1beta1","metadata":{"name":"my-channel-2"}}' \
    -H "Content-Type: application/json" \
    'http://localhost:8080/apis/messaging.knative.dev/v1beta1/namespaces/knativetutorial/channels'

First channel is created with KafkaChannel, the 2nd created with IMC.

$ k get channels -A
NAMESPACE         NAME           URL                                                                AGE   READY   REASON
knativetutorial   my-channel-1   http://my-channel-1-kn-channel.knativetutorial.svc.cluster.local   6s    True    
knativetutorial   my-channel-2   http://my-channel-2-kn-channel.knativetutorial.svc.cluster.local   5s    True    

$ k get kafkachannels -A
NAMESPACE         NAME           READY   REASON   URL                                                                AGE
knativetutorial   my-channel-1   True             http://my-channel-1-kn-channel.knativetutorial.svc.cluster.local   15s

$ k get imc -A
NAMESPACE         NAME           URL                                                                AGE   READY   REASON
knativetutorial   my-channel-2   http://my-channel-2-kn-channel.knativetutorial.svc.cluster.local   11s   True    

Expected behavior Defaulting to use the context namespace.

To Reproduce Steps are above.

Knative release version 0.17, 0.18, 0.19

Additional context Add any other context about the problem here such as proposed priority

aliok avatar Nov 12 '20 09:11 aliok

This doesn't happen when you apply YAML with kubectl since kubectl embeds the namespace in the request body.

aliok avatar Nov 12 '20 09:11 aliok

This doesn't happen when you apply YAML with kubectl since kubectl embeds the namespace in the request body.

kn could carry this as workaround for now, it'll embed the namespace in the request body.

navidshaikh avatar Nov 12 '20 09:11 navidshaikh

sounds good!

aliok avatar Nov 12 '20 09:11 aliok

so,

  • this problem doesn't happen with kubectl
  • kn is adding a workaround, that should also be fine
  • all the other usecases where people are creating Channels programmatically with namespace defaults: just embed the namespace in the request as a workaround

aliok avatar Nov 12 '20 09:11 aliok

This might also affect broker class defaulting by namespace (if we even have that feature, I can't remember)

grantr avatar Nov 16 '20 18:11 grantr

Since the kn workaround seems like was merged, can we close this one @aliok ?

slinkydeveloper avatar Feb 11 '21 16:02 slinkydeveloper

Let's keep this open since this is a good-first-issue? At least until the bot marks it stale?

aliok avatar Feb 15 '21 11:02 aliok

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar May 17 '21 01:05 github-actions[bot]

/remove-lifecycle stale

aliok avatar May 24 '21 08:05 aliok

Since the kn workaround seems like was merged, can we close this one @aliok ?

I still think that the issue described here is more than just helping kn to run the e2e tests and is a general issue with other clients as well that do not add the namespace to the request (which IMO is totally legit)

rhuss avatar Jul 05 '21 07:07 rhuss

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.

github-actions[bot] avatar Oct 07 '21 01:10 github-actions[bot]

/remove-lifecycle stale

rhuss avatar Oct 11 '21 10:10 rhuss

/assign

gab-satchi avatar Jan 07 '22 21:01 gab-satchi

/assign

vishal-chdhry avatar Mar 18 '23 07:03 vishal-chdhry