Feat(cli): version operator and warning compatibility message

[squakez]

• With 89503f6 I am adding a kamel version --operator that connect to the cluster (current or user provided namespace) and return the operator version.

$ kamel version --operator
Camel K Operator 1.3.0
$ kamel version --operator -n kamel2
Camel K Operator 1.2.1

• With e177158 I am adding a possible check on every "online" command in order to verify compatibility between client and operator.

$ kamel run test323
Warning: you're using Camel K 1.3.0-SNAPSHOT client against a 1.2.1 cluster operator
integration "test323" created

Closes #1652

Release Note

Feat(cli): version operator and warning compatibility message

[squakez]

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?

[astefanutti]

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
• 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.

[astefanutti]

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. 

[squakez]

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.

[astefanutti]

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?

I agree, if that is easy enough, it can print a message that no operator version can be retrieve. It may also happen when no operator is deployment in the current context, or there is no default context.

* 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.

I agree. An informative message is an acceptable tradeoff.

[astefanutti]

I just had a quick look at kubectl version. It has a --client option to filter out the server version. It is a bit different than our case, as the operator version may be missing also when there is no operator deployment in the current context.

It also has an --output option for structured result:

$ kubectl version -h
Print the client and server version information for the current context

Examples:
  # Print the client and server versions for the current context
  kubectl version

Options:
      --client=false: Client version only (no server required).
  -o, --output='': One of 'yaml' or 'json'.
      --short=false: Print just the version number.

[astefanutti]

Another point about retrieving the operator version is that it may be there are multiple replicas of the operator for HA. Ideally, the exec request should be made into the operator that has been elected leader among these replicas, which can be done by getting the ConfigMap used to hold the lock, even if I see no reason the other replicas could have a different version.

[nicolaferraro]

Note that kamel version is probably used by IDE plugins / didact to check if the CLI is present on the system, and also that recent additions added a way to run integrations locally without a cluster, so I would be ok with keeping the --operator to check the cluster, so that kamel version can work also offline.

[squakez]

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.

[astefanutti]

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?

[squakez]

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.

[astefanutti]

Great, let's dive into the version endpoint approach 👍🏼.

The health and monitoring endpoints are delegated to controller-runtime:

camel-k/pkg/cmd/operator/operator.go

Lines 130 to 131 in aaeb457

	HealthProbeBindAddress: ":" + strconv.Itoa(int(healthPort)),
	MetricsBindAddress: ":" + strconv.Itoa(int(monitoringPort)),

.

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 😉.

[squakez]

Great, let's dive into the version endpoint approach 👍🏼.

The health and monitoring endpoints are delegated to controller-runtime:

camel-k/pkg/cmd/operator/operator.go

Lines 130 to 131 in aaeb457

	HealthProbeBindAddress: ":" + strconv.Itoa(int(healthPort)),
	MetricsBindAddress: ":" + strconv.Itoa(int(monitoringPort)),

.

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?

[astefanutti]

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 😇!

[squakez]

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 ;)

[squakez]

Ready for review now. @astefanutti if you want to have a look :)

[astefanutti]

Looks great! Eager to try it! Thanks a lot.

[astefanutti]

I've relaunched CI to pass flaky cron e2e tests.