[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-763473046 Ready for review now. @astefanutti if you want to have a look :) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-762744944 > Right, resolving the version from the `IntegrationPlatfom` status is actually a great idea! It seems to solve all our concerns. Now I feel bad I distracted you with less relevant alternatives innocent! Haha, no, not at all. I'd been sticking with the original draft instead looking for alternatives. Also a good exercise for me to dig and learn more about the software ;) 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-762733070 > Great, let's dive into the `version` endpoint approach . > > The health and monitoring endpoints are delegated to controller-runtime: > > https://github.com/apache/camel-k/blob/aaeb4573fb62881fae1f6fa0b8f20695237414d4/pkg/cmd/operator/operator.go#L130-L131 > . > > I'm not sure it's possible to hook into these. Otherwise we can start our own listener. > > For the backward compatibility point, better sooner than later wink. I had a thought at the possibility to include a version endpoint. If we go in that direction we must expose the version endpoint publicly in order to be externally read: health and monitoring are not public endpoint. We also may have difficulty to know beforehand where a `global` operator is installed. I like the idea to expose something but I feel we need a wider discussion and a more devised design: I can open an issue to follow up with that. In the while I think we have a better way to deal with the problem and solve the `global` installation point. We can check the `IntegrationPlatfom` that contains the version and is installed in each namespace where an operator is watching (also a global one). How do you see this? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912:
URL: https://github.com/apache/camel-k/pull/1912#issuecomment-762201031
> > I am making the development to exec into the kamel operator, however I
am thinking about the possible security concerns. Should we allow the CLI to
run an arbitrary command on the operator running pod? I am not sure it's a good
choice.
>
> It has indeed security concerns, and UX as a result. For that approach to
work, the end-user has to have the permission to run the `exec` request. This
is managed by Kubernetes RBAC, so the end-user has to be authorised. And then,
he/she can run arbitrary command.
>
> Other options that I can think of would be:
>
> * Expose a `version` endpoint: the operator already expose HTTP
endpoints for health and monitoring
>
> * Simply add an annotation to the deployment with the version
information
>
>
> While the `exec` approach is more correct, these alternatives are probably
less involved security and UX wise.
>
> WDYT?
I was exactly looking at the possibility to expose a `version` endpoint as
that is the approach taken by `kubectl`:
```
$ kubectl version -v 9
I0118 12:49:13.727726 323730 loader.go:375] Config loaded from file:
/home/squake/.kube/config
I0118 12:49:13.728448 323730 round_trippers.go:423] curl -k -v -XGET -H
"User-Agent: kubectl/v1.19.0 (linux/amd64) kubernetes/e199641" -H "Accept:
application/json, */*" 'https://172.17.0.2:8443/version?timeout=32s'
I0118 12:49:13.728469 323730 cert_rotation.go:137] Starting client
certificate rotation controller
I0118 12:49:13.734369 323730 round_trippers.go:443] GET
https://172.17.0.2:8443/version?timeout=32s 200 OK in 5 milliseconds
I0118 12:49:13.734385 323730 round_trippers.go:449] Response Headers:
I0118 12:49:13.734390 323730 round_trippers.go:452] Content-Length: 261
I0118 12:49:13.734394 323730 round_trippers.go:452] Date: Mon, 18 Jan
2021 11:49:13 GMT
I0118 12:49:13.734399 323730 round_trippers.go:452] Cache-Control:
no-cache, private
I0118 12:49:13.734402 323730 round_trippers.go:452] Content-Type:
application/json
I0118 12:49:13.735425 323730 request.go:1097] Response Body: {
"major": "1",
"minor": "19",
"gitVersion": "v1.19.2",
"gitCommit": "f5743093fd1c663cb0cbc89748f730662345d44d",
"gitTreeState": "clean",
"buildDate": "2020-09-16T13:32:58Z",
"goVersion": "go1.15",
"compiler": "gc",
"platform": "linux/amd64"
}
Client Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.0",
GitCommit:"e19964183377d0ec2052d1f1fa930c4d7575bd50", GitTreeState:"clean",
BuildDate:"2020-08-26T14:30:33Z", GoVersion:"go1.15", Compiler:"gc",
Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.2",
GitCommit:"f5743093fd1c663cb0cbc89748f730662345d44d", GitTreeState:"clean",
BuildDate:"2020-09-16T13:32:58Z", GoVersion:"go1.15", Compiler:"gc",
Platform:"linux/amd64"}
```
I see this more appropriate and we can include any further info that we will
retain useful in the future (plus gitCommit and such). Can you point me at the
code where we expose health and monitoring please? I can work to include this
new endpoint. The only drawback is that there is no way to make this change
backward compatible and will work only with CLI/operators installed from newer
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-762190832 > Another point is the retrieval of the operator version. Relying on extracting the tag from the Deployment image may fail in a number of situations, like when SHA pinning is used. I would suggest to exec into the operator container and run the `kamel version` command. That would provide a reliable way to retrieve the operator version. If that approach is chosen, it may be useful to support structured output for the `kamel version` command, to ease parsing. I am making the development to `exec` into the `kamel` operator, however I am thinking about the possible security concerns. Should we allow the CLI to run an arbitrary command on the operator running pod? I am not sure it's a good choice. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-760748643 > That is a great improvement! Here are my first feedback: > > * I wonder whether the `--operator` is necessary and `kamel version` should just output both client and server versions, similarly to what `kubectl version` does > It was my first choice, but later I realized that the client version is meant to work also "offline". I can rework that part in order to skip the online/offline check and have either the operator version or a message error when disconnected, wdyt? > * For the compatibility detection, ideally a warning should be displayed when there is an actual compatibility issue. Otherwise, an informative message could be displayed to suggest the user to upgrade. I understand this is more involved, as the information has to be provided by the developers somewhere, plus forward compatibility is an issue. `kubectl` has also already approached the issue by requesting the server, as described in https://kubernetes.io/blog/2020/09/03/warnings/. That being said, `kamel` does not always interact with the operator directly, and we may not want to expose an endpoint for that purpose. So maybe outputting an informative message, for all _online_ commands is an acceptable tradeoff. This would be nice, though it requires quite a few changes, and during development we must find a way to "mark" when some change is breaking compatibility. I think for the time being an informative message is a good addition to inform the user that if something does not work, it may be because the cli/operator misalignment. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #1912: Feat(cli): version operator and warning compatibility message
squakez commented on pull request #1912: URL: https://github.com/apache/camel-k/pull/1912#issuecomment-760257361 Few points I'd like to hear about: 1. I'm considering compatibility up to patch level (ie, 1.3.1 cli is compatible with 1.3.0 operator, same for SNAPSHOT): does it make sense? 2. Not sure of the best way to provide a warning message. Right now I've just added as first line, please suggest anything better. 3. So far I am annotating only `run`, `debug` and `kit create` subcommands to make the compatibility check, we can easily extends to other that can be affected by a breaking compatibility. Or does it make sense to add to all? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
