You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "squakez (via GitHub)" <gi...@apache.org> on 2024/03/05 17:09:57 UTC
[PR] fix(trait): watch for resource versions... [camel-k]
squakez opened a new pull request, #5218:
URL: https://github.com/apache/camel-k/pull/5218
...instead of for their content.
With this PR we fix a potentially risky behavior, as we were inspecting Configmap/Secrets content to address any change. Instead of looking directly the content, we watch for the changing resource version, which should be the way provided by the platform to know when a resource has changed without inspecting its content.
<!-- Description -->
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
fix(trait): watch for resource versions
```
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on code in PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#discussion_r1514061488
##########
pkg/util/kubernetes/lookup.go:
##########
@@ -52,6 +52,17 @@ func LookupConfigmap(ctx context.Context, c client.Client, ns string, name strin
return &cm
}
+// LookupResourceVersion will look for any k8s resource with a given name in a given namespace, returning its resource version only.
+// It makes this safe against any resource that the operator is not allowed to inspect.
+func LookupResourceVersion(ctx context.Context, c client.Client, object ctrl.Object) string {
+ if err := c.Get(ctx, ctrl.ObjectKeyFromObject(object), object); err != nil && k8serrors.IsNotFound(err) {
+ return ""
+ } else if err != nil {
+ return ""
Review Comment:
is swallowing the error expected ? if so `k8serrors.IsNotFound(err)` can probably be removed
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1979255412
Note that `resourceVersion` changes also in case of any of the metadata changes
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1980365632
@lburgazzoli I've added a check on the presence of the `hot-reload` flag and a short documentation notice to warn about the behavior. 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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1981131580
Check failure relates to #5197
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1980242368
> Maybe, should we honor the `mount.hot-reload` option ? In some cases, the user may not have full control of the configmap/secrets i.e. when those resources are generated by another controller.
The option is already used in the controller to wake up the Integration and kick the logic: https://github.com/apache/camel-k/blob/825f0ce6aaf3a52c2620f8efab31b8167973c5fc/pkg/controller/integration/integration_controller.go#L191 - do you think we need to include this again in the `getIntegrationSecretAndConfigmapResourceVersions` func? I think it may be a duplicated check, but definetely it's something that should not hurt.
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1979333480
> > Note that `resourceVersion` changes also in case of any of the metadata changes
>
> Thanks. Not sure if there is any better way to deal with this, any idea? At least we remove the inspection of a secret, with the tradeoff to potentially reload an Integration when there is a metadata change as well.
I don't think there is a better solution so is a matter of document the behavior.
Maybe, should also honor the `mount.hot-reload` option ? In some cases, the user may not have full control of the configmap/secrets i.e. when those resources are generated by another controller.
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #5218:
URL: https://github.com/apache/camel-k/pull/5218
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1979262042
> Note that `resourceVersion` changes also in case of any of the metadata changes
Thanks. Not sure if there is any better way to deal with this, any idea. At least we remove the inspection of a secret, with the tradeoff to potentially reload an Integration when there is a metadata change as well.
--
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
Re: [PR] fix(trait): watch for resource versions... [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5218:
URL: https://github.com/apache/camel-k/pull/5218#issuecomment-1980284841
> > Maybe, should we honor the `mount.hot-reload` option ? In some cases, the user may not have full control of the configmap/secrets i.e. when those resources are generated by another controller.
>
> The option is already used in the controller to wake up the Integration and kick the logic:
>
> https://github.com/apache/camel-k/blob/825f0ce6aaf3a52c2620f8efab31b8167973c5fc/pkg/controller/integration/integration_controller.go#L191
> - do you think we need to include this again in the `getIntegrationSecretAndConfigmapResourceVersions` func? I think it may be a duplicated check, but definetely it's something that should not hurt.
I don't have the full picture but my reasoning is that, the hot reload flag prevents a reconciliation to happen but, in case the reconciliation is triggered by i.e. a non destructive trait change, then we ends up in the same case as https://github.com/apache/camel-k/issues/5192, which effectively will end up in a restart/reload.
--
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