You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/08/09 03:08:25 UTC
[GitHub] [camel-k] claudio4j opened a new pull request, #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
claudio4j opened a new pull request, #3523:
URL: https://github.com/apache/camel-k/pull/3523
https://github.com/apache/camel-k/issues/3522
* Sets the bindings.knative.dev/include=true label in the namespace if running in openshift
* Add the Role permission to get namespace object
* Sets the bindings.knative.dev/include=true label when creating the KnativeService, otherwise, updating the KnativeService from the knative trait, seems to adding new revisions which starts new pods.
**Release Note**
```release-note
Sets the bindings.knative.dev/include=true label in the namespace if running in openshift
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1209281000
> So we would need to setup a very specific test infrastructure to verify this one in an e2e test.
Some mocking could be added for the non e2e testing. I consider this behavior important to test, since it could put the worker node in NotReady status.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941274251
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> With the current approach, camel-k-operator only sets the label when there is no label in the namespace and logs an INFO when setting the label in the namespace. So, if the namespace contains the `exclusion` label, meaning the user had explicitly set this configuration, then camel-k-operator would not override it.
But what should be the behavior if SINK_BINDING_SELECTION_MODE is set to inclusion ? My limited understanding is that in such case a namespace should be explicit labelled to enable the injection and in such case, it is expected that the camel-k operator sets the label ? It may be possible that namespace does not have the label on purpose.
Whatever we decide I think the behavior should be configurable so the administrator may decide to disable the automatic labeling by camel-k.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1208860407
@christophd @squakez for review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941244407
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> I wonder if it is expected that the camel-k operator is the one setting the label on the namespace
I considered to instead to let camel-k-operator to perform the change, to document this setting when using openshift serverless or any knative with `(binding mode = inclusion)` and log a warning/error in case the user wanted to run a specific route which resulted in `SinkBinding`, then the integration would not run and set a status waiting for this configuration. But, in the end I wondered this situation would restrict Camel K out of the box approach for integrations, having to wait for an administrator to set a label.
With the current approach, camel-k-operator only sets the label when there is no label in the namespace and logs an INFO when setting the label in the namespace. So, if the namespace contains the `exclusion` label, meaning the user had explicitly set this configuration, then camel-k-operator would not override it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941259832
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> 1.wait for the namespace to be updated before proceeding with deploying the integration
In this case, camel-k-operator would need additional privileges to look at the `eventing-webhook` deployment in `knative-eventing` namespace, since the configuration is an environment variable [SINK_BINDING_SELECTION_MODE](https://knative.dev/docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/#optional-choose-sinkbinding-namespace-selection-behavior).
> 2. add an additional condition to report this status
In case we decide to go for (1), a new condition would be required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
squakez commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1210636790
> CI build was failing due to lint issues, just fixed using `make lint-fix`
I see this last operation is adding a lot of files changes that should be not really needed. The previous validation complained about the missing formatting that had to be solved via `gofmt -s -w <your file>.go`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r940971544
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
+1 for #2 and the additional status condition reported. Maybe we could also make this configurable via IntegrationPlatform setting so an admin can prohibit this behavior while setting up the IP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j closed pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j closed pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
URL: https://github.com/apache/camel-k/pull/3523
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r940947302
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
I wonder if it is expected that the camel-k operator is the one setting the label on the namespace as this basically may override any set-up done by an administrator (i.e. it may be possible that you want to restrict some user to not being to use some functionalities) and also, it require additional privileges.
Maybe we should:
1. wait for the namespace to be updated before proceeding with deploying the integration
2. add an additional condition to report this status
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1208989846
thanks @claudio4j! LGTM some golang lint validation failed though.
@squakez the issue is only on OpenShift 4.x and in combination with Knative serving (binding mode = inclusion) and Knative eventing. The default binding mode setting for Knative serving is "exclusion". So we would need to setup a very specific test infrastructure to verify this one in an e2e test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1210641513
Closing this PR, as this change requires another approach to fix this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1210201725
CI build was failing due to lint issues, just fixed using `make lint-fix`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941241734
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> Is it possible to add an e2e test and make sure the wrong behavior is tested?
Sure. I have planned to add the e2e, but wanted your review while I work on the tests, it seems I should have set this as draft. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r940971544
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
+1 for 2. and the additional status condition reported. Maybe it makes sense to also make this configurable via IntegrationPlatform setting so an admin can prohibit this behavior while setting up the IP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r940971544
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
+1 for 2. and the additional status condition reported. Maybe we could also make this configurable via IntegrationPlatform setting so an admin can prohibit this behavior while setting up the IP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941244407
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> I wonder if it is expected that the camel-k operator is the one setting the label on the namespace
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#issuecomment-1209278891
> Is it possible to add an e2e test and make sure the wrong behavior is tested?
Sure. I have planned to add the e2e, but wanted your review while I work on the tests, it seems I should have set this as draft. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] claudio4j commented on a diff in pull request #3523: fix(knative): Set a knative label in the namespace to allow sinkbinding injection
Posted by GitBox <gi...@apache.org>.
claudio4j commented on code in PR #3523:
URL: https://github.com/apache/camel-k/pull/3523#discussion_r941261998
##########
pkg/platform/operator.go:
##########
@@ -133,6 +139,35 @@ func IsOperatorAllowedOnNamespace(ctx context.Context, c ctrl.Reader, namespace
return !alreadyOwned, nil
}
+// EnableKnativeBindInNamespace sets the "bindings.knative.dev/include=true" label to the namespace if is openshift and knative is installed
+// Returns true if the label was set in the namespace
+// see https://knative.dev/v1.3-docs/eventing/custom-event-source/sinkbinding/create-a-sinkbinding/
+func EnableKnativeBindInNamespace(ctx context.Context, client client.Client, namespace string) (bool, error) {
Review Comment:
> Maybe it makes sense to also make this configurable via IntegrationPlatform setting so an admin can prohibit this behavior while setting up the IP.
Yes, this could be done, I will wait feedback on the previous comments to see how to proceed. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org