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

fix(knative): Set a knative label in the namespace to allow sinkbinding injection (1.8.x) #111

Merged

Conversation

claudio4j
Copy link

@claudio4j claudio4j commented Aug 10, 2022

https://issues.redhat.com/browse/ENTESB-19425

  • Sets the bindings.knative.dev/include=true label to the namespace if ALL of the conditions are met:
    . Knative is installed and enabled in the IntegrationPlatform namespace
    . There aren't any of these labels bindings.knative.dev/include, bindings.knative.dev/exclude in the IntegrationPlatform namespace
    . The environment variable SINK_BINDING_SELECTION_MODE should be set to "inclusion" in "deploy/eventing-webhook" of "knative-eventing" namespace
  • Add the Role, RoleBindings permissions to allow handling of objects in "knative-eventing"
  • Allow uninstall of the knative-eventing roles,rolebindings
  • Set the app=camel-k label to 'addressable-resolver' role
  • Set the "bindings.knative.dev/include=true" label when creating the ksvc, this fix the pods being indefinitely created in a ping pong behavior
  • Added a namespace-label flag in knative trait to control if camel-k-operator should set the label.

Copy link
Collaborator

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It's important to remind to fix it properly upstream asap however :)

Copy link

@christophd christophd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM many thanks!

@claudio4j claudio4j marked this pull request as draft August 10, 2022 14:46
claudio4j and others added 2 commits August 10, 2022 18:54
…ng injection

https://issues.redhat.com/browse/ENTESB-19425

* Sets the bindings.knative.dev/include=true label to the namespace if ALL of the conditions are met:
. Knative is installed and enabled in the namespace
. There aren't any of these labels bindings.knative.dev/include, bindings.knative.dev/exclude in the namespace
. The environment variable SINK_BINDING_SELECTION_MODE should be set to "inclusion" in "deploy/eventing-webhook" of "knative-eventing" namespace
* Add the Role, RoleBindings permissions to allow handling of objects in "knative-eventing"
* Allow uninstall of the knative-eventing roles,rolebindings
* Set the app=camel-k label to 'addressable-resolver' role
* Set the "bindings.knative.dev/include=true" label when creating the ksvc, this fix the pods being indefinitely created in a ping pong behavior
…ng in global operator mode

Use proper operator namespace in the service account role binding subject for global operators. Was using empty global operator watch namespace before which caused errors in the cluster role binding.

(cherry picked from commit 2ffdcfa)
@claudio4j claudio4j marked this pull request as ready for review August 10, 2022 21:57
Copy link

@christophd christophd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just want to know why we need the additional permissions on knative routes and brokers. But that is nothing that prevents us from merging in order to get the new release build going.

Also I saw that you have fixed the knative addressable resolver role bindings as in main branch. so in theory we also would not need to add extra permissions on knative channels and inmemorychannels any more because this becomes obsolete with the addressable resolver role binding fix. But let's keep it this way and clean up a bit after the release.

@@ -26,6 +26,7 @@ rules:
- serving.knative.dev
resources:
- services
- routes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had cherry-picked this commit from the previous work on upstream main branch, and this just came along. Also, I had noticed missing permission for channels, so I thought this role fixed the issue.

@@ -38,6 +39,7 @@ rules:
- eventing.knative.dev
resources:
- triggers
- brokers

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

@claudio4j
Copy link
Author

LGTM, just want to know why we need the additional permissions on knative routes and brokers.

Yesterday, while testing, I had noticed the addressable-resolver was not enough, but it could be that I missed some part.

But that is nothing that prevents us from merging in order to get the new release build going.

That's good. Thanks for reviewing.

@squakez squakez merged commit 4929082 into jboss-fuse:release-1.8.x Aug 11, 2022
@claudio4j claudio4j deleted the fix_knative_pods_prod-1.8x branch August 11, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants