fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 #11909

fhochleitner · 2023-09-30T06:51:15Z

add Warning message on Version mismatch between CLI and Server.
implemented a check whether argocli and arger-server version are based on the same git-tag and print a warning when versions are mismatched.

Fixes #9212

Motivation

Wanted to start contributing and found an issue that seemed to be a nice start for this journey

Modifications

added function to compare cli and server version and prints a warning in case versions do not match. function fails with warning messages if it cannot contact server as it should not break everything in case of an error.

Verification

built cli and tested it against argo-workflows running on a cluster. happy to implement some e2e tests if required but would need some pointers.

fhochleitner · 2023-10-03T06:39:18Z

I've placed the version check into the root command so it get's executed and prints the warning no matter command the user actually uses and gets notified in case. but I'm not sure whether that's the right as it does execute a few network calls for all commands. happy to implement changes if you have a way to implement this in a smarter way

agilgur5 · 2023-10-05T15:10:59Z

Wanted to start contributing and found an issue that seemed to be a nice start for this journey

Great start, thanks!

happy to implement some e2e tests if required but would need some pointers.

There are some unit tests for the CLI already that you can use as an example. Same directory, *_test.go files

but I'm not sure whether that's the right as it does execute a few network calls for all commands. happy to implement changes if you have a way to implement this in a smarter way

If we wanted to optimize this specific call, we could run it async (e.g. in a goroutine) so it doesn't block other commands.
To have less network requests, one common way is to pull the version out of a header or something from an existing network call.

cmd/argo/commands/root.go

agilgur5 · 2023-10-05T15:15:47Z

cmd/argo/commands/root.go

+
+// printVersionMismatchWarning logs a warning if the CLI version does not match the server version
+func printVersionMismatchWarning(command *cobra.Command) {
+	// if ARGO_SERVER isn't set there's no need to compare server and cli version


I'm not sure that this is necessarily accurate. Any usage of the CLI could have unexpected results if there is a version mismatch. For example, through the k8s API, the CLI could be using an outdated spec

you're right. a check would definitely be useful in this case as well. do you know whether there's a way to get a version that's comparable with the cli version directly via api access? I haven't been able to find one but then again I am not very familiar with the codebase yet.

While I agree with:

Any usage of the CLI could have unexpected results if there is a version mismatch.

Even the version command does not check the server version if ARGO_SERVER is not defined. I'm assuming is not that simple to determine the version in that case.

https://github.com/argoproj/argo-workflows/blob/main/cmd/argo/commands/version.go#L23

My comment was about the "there's no need to compare" comment. That is inaccurate.
You may not be able to retrieve it without ARGO_SERVER set, but it is nonetheless inaccurate to say "there is no need compare". It would be more apt to say "you can't compare". But the comment is a bit redundant at that point

Great, I agree.

…ixes argoproj#9212 Signed-off-by: fhochleitner <[email protected]>

isubasinghe · 2024-01-28T10:58:51Z

cmd/argo/commands/root.go

+	if serverVersion.GitTag != argo.GetVersion().GitTag {
+		log.Warnf("CLI version (%s) does not match server version (%s). This can lead to unexpected behavior.", argo.GetVersion().GitTag, serverVersion.GitTag)
+	}


I think it might be a better idea if you separated out the logic that checks for a mismatch from the code that presents this information to the user.

Only saying this because we could make this a bit more strict then, we could fail on a version mismatch and have a flag to force any action to be performed regardless.

ah sorry for not having able to continue working on this PR. work and private life got quite busy as of late.
I'm not sure this will change a lot in the coming months, but I do think that's a great idea @isubasinghe

No worries, take your time!

driv · 2024-07-06T09:17:16Z

Since this is marked as stale and relatively simple, I can take a look.

…ixes argoproj#9212 This adds a warning message to the CLI when it detects a mismatch between the client and server versions. There was another PR (argoproj#11909) for this implemented it by making a blocking API call to `/api/v1/version` in a `PersistentPreRun` hook. This PR takes a different approach: have the server send the version in a new header called `argo-version`, which the client will detect and extract. There's several advantages to this approach: 1. Negligible performance impact, since no additional requests are needed. 2. Warning is only shown when the command would normally send an API request, so we 3. Can be useful for bug triaging, since the header can be seen in `curl` output. Exposing the version information has security implictions, since it could be used by attackers to identify vulnerable Argo servers. To mitigate that, the header is not sent on 401 errors. Of course, if a user is exposing their Argo server to the internet without authentication, then an attacker could see this header, but then they've got bigger problems (and an attacker could just call `/api/v1/version`). This is implemented on the client and server side using [grpc-go interceptors](https://github.com/grpc/grpc-go/blob/master/examples/features/interceptor/README.md). On the server side, there's interceptors to set the version header in the response. On the client side, there's an interceptor to check the response for the header and stash it in a global variable (which is obviously not ideal, but I couldn't think of a better way to handle that). Testing process: 1. Manually changed the version to `v9.99`: https://github.com/argoproj/argo-workflows/blob/ce7f9bfb9b45f009b3e85fabe5e6410de23c7c5f/Makefile#L95 2. Ran `make cli && cp dist/argo argo2` 3. Ran `make start API=true` 4. Ran `ARGO_SECURE=false ARGO_TOKEN="Bearer $(kubectl get secret argo-server.service-account-token -o=jsonpath='{.data.token}' | base64 --decode)" ARGO_SERVER=localhost:2746 ./dist/argo2 list` Output: ``` No workflows found WARN[2024-09-20T18:03:26.116Z] CLI version (v9.99+303bcce.dirty) does not match server version (latest+303bcce.dirty). This can lead to unexpected behavior. ```

…ixes argoproj#9212 This adds a warning message to the CLI when it detects a mismatch between the client and server versions. There was another PR (argoproj#11909) for this implemented it by making a blocking API call to `/api/v1/version` in a `PersistentPreRun` hook. This PR takes a different approach: have the server send the version in a new header called `argo-version`, which the client will detect and extract. There's several advantages to this approach: 1. Negligible performance impact, since no additional requests are needed. 2. Warning is only shown when the command would normally send an API request. 3. Can be useful for bug triaging, since the header can be seen in `curl` output. Exposing the version information has security implictions, since it could be used by attackers to identify vulnerable Argo servers. To mitigate that, the header is not sent on 401 errors. Of course, if a user is exposing their Argo server to the internet without authentication, then an attacker could see this header, but then they've got bigger problems (and an attacker could just call `/api/v1/version`). This is implemented on the client and server side using [grpc-go interceptors](https://github.com/grpc/grpc-go/blob/master/examples/features/interceptor/README.md). On the server side, there's interceptors to set the version header in the response. On the client side, there's an interceptor to check the response for the header and stash it in a global variable (which is obviously not ideal, but I couldn't think of a better way to handle that). Testing process: 1. Manually changed the version to `v9.99`: https://github.com/argoproj/argo-workflows/blob/ce7f9bfb9b45f009b3e85fabe5e6410de23c7c5f/Makefile#L95 2. Ran `make cli && cp dist/argo argo2` 3. Ran `make start API=true` 4. Ran `ARGO_SECURE=false ARGO_TOKEN="Bearer $(kubectl get secret argo-server.service-account-token -o=jsonpath='{.data.token}' | base64 --decode)" ARGO_SERVER=localhost:2746 ./dist/argo2 list` Output: ``` No workflows found WARN[2024-09-20T18:03:26.116Z] CLI version (v9.99+303bcce.dirty) does not match server version (latest+303bcce.dirty). This can lead to unexpected behavior. ``` Signed-off-by: Mason Malone <[email protected]>

agilgur5 · 2024-09-21T23:56:02Z

Superseded by #13635

fhochleitner marked this pull request as draft September 30, 2023 06:52

fhochleitner changed the title ~~WIP fix: add Warning message on Version mismatch between CLI and Server. fixes #9212~~ fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 Sep 30, 2023

fhochleitner force-pushed the master branch 4 times, most recently from 71d24fd to 9b5f40b Compare October 3, 2023 06:08

fhochleitner marked this pull request as ready for review October 3, 2023 06:37

fhochleitner force-pushed the master branch from 9b5f40b to f7d0b34 Compare October 4, 2023 06:38

agilgur5 added the area/cli The `argo` CLI label Oct 5, 2023

agilgur5 reviewed Oct 5, 2023

View reviewed changes

fhochleitner force-pushed the master branch from f7d0b34 to def13c6 Compare October 9, 2023 06:19

fix: add Warning message on Version mismatch between CLI and Server. F…

20469de

…ixes argoproj#9212 Signed-off-by: fhochleitner <[email protected]>

fhochleitner force-pushed the master branch from def13c6 to 20469de Compare October 16, 2023 19:46

isubasinghe self-assigned this Jan 22, 2024

isubasinghe reviewed Jan 28, 2024

View reviewed changes

isubasinghe mentioned this pull request Jan 30, 2024

REQUEST: Promotion to Approver for @isubasinghe argoproj/argoproj#232

Closed

7 tasks

juliev0 added problem/stale This has not had a response in some time problem/more information needed Not enough information has been provide to diagnose this issue. labels Jun 28, 2024

github-actions bot removed problem/stale This has not had a response in some time problem/more information needed Not enough information has been provide to diagnose this issue. labels Jun 30, 2024

juliev0 added the problem/more information needed Not enough information has been provide to diagnose this issue. label Jul 3, 2024

This comment was marked as resolved.

Sign in to view

github-actions bot added the problem/stale This has not had a response in some time label Jul 21, 2024

This comment was marked as resolved.

Sign in to view

github-actions bot closed this Aug 4, 2024

MasonM mentioned this pull request Sep 20, 2024

feat(cli): add version header + warning on client-server mismatch. Fixes #9212 #13635

Merged

agilgur5 removed problem/stale This has not had a response in some time problem/more information needed Not enough information has been provide to diagnose this issue. labels Sep 21, 2024

agilgur5 added the solution/superseded This PR or issue has been superseded by another one (slightly different from a duplicate) label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 #11909

fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 #11909

fhochleitner commented Sep 30, 2023

fhochleitner commented Oct 3, 2023

agilgur5 commented Oct 5, 2023

agilgur5 Oct 5, 2023 •

edited

Loading

fhochleitner Oct 9, 2023

driv Jul 6, 2024

agilgur5 Jul 6, 2024 •

edited

Loading

driv Jul 6, 2024 •

edited

Loading

isubasinghe Jan 28, 2024

fhochleitner Feb 1, 2024

isubasinghe Feb 3, 2024

driv commented Jul 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

agilgur5 commented Sep 21, 2024

fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 #11909

fix: add Warning message on Version mismatch between CLI and Server. fixes #9212 #11909

Conversation

fhochleitner commented Sep 30, 2023

Motivation

Modifications

Verification

fhochleitner commented Oct 3, 2023

agilgur5 commented Oct 5, 2023

agilgur5 Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

fhochleitner Oct 9, 2023

Choose a reason for hiding this comment

driv Jul 6, 2024

Choose a reason for hiding this comment

agilgur5 Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

driv Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

isubasinghe Jan 28, 2024

Choose a reason for hiding this comment

fhochleitner Feb 1, 2024

Choose a reason for hiding this comment

isubasinghe Feb 3, 2024

Choose a reason for hiding this comment

driv commented Jul 6, 2024

This comment was marked as resolved.

This comment was marked as resolved.

agilgur5 commented Sep 21, 2024

agilgur5 Oct 5, 2023 •

edited

Loading

agilgur5 Jul 6, 2024 •

edited

Loading

driv Jul 6, 2024 •

edited

Loading