skupperproject / skupper

Skupper is an implementation of a Virtual Application Network, enabling rich hybrid cloud communication.
http://skupper.io
Apache License 2.0
559 stars 71 forks source link

`--platform` needs to be the first flag on the commandline, otherwise it is ignored #1488

Closed hash-d closed 1 month ago

hash-d commented 1 month ago

Describe the bug

If a different flag precedes --platform on the command line, the value given to it is ignored.

How To Reproduce

Try a skupper init with --platform not as the first flag:

$ skupper init --ingress-host localhost --platform podman --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Secret already exists:  skupper-local-server
Secret already exists:  skupper-local-client
Secret already exists:  skupper-service-client
Secret already exists:  skupper-console-users
Secret already exists:  skupper-site-server
Secret already exists:  skupper-console-certs
Waiting for status...
Skupper is now installed in namespace 'dh-1248'.  Use 'skupper status' to get more information.

Notice that the command above installed skupper on the K8S namespace dh-1248, instead of the requested podman install.

See that changing the placement of the --platform flag causes the correct behavior to take place.

$ skupper init --platform podman --ingress-host localhost --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Skupper is now installed for user 'dhashimo'.  Use 'skupper status' to get more information.

Also, note that the value of the --platform is completely ignored:

$ skupper init --ingress-host localhost --platform asdf --enable-console --enable-flow-collector --site-name skgw --console-auth=internal --console-user=admin --console-password=password
Secret already exists:  skupper-local-server
Secret already exists:  skupper-local-client
Secret already exists:  skupper-service-client
Secret already exists:  skupper-console-users
Secret already exists:  skupper-site-server
Secret already exists:  skupper-console-certs
Waiting for status...
Skupper is now installed in namespace 'dh-1248'.  Use 'skupper status' to get more information.

On the example above, the invalid value asdf was given to --platform and yet it was allowed to continue.

Expected behavior

Either:

I did not see anything on the documentation pointing to the necessity of --platform to be the first in the command line. Even in that case, an error should be generated when that's not in its proper place, instead of continuing with a configuration different from what the user specified.

Environment details

Additional context

N/A

hash-d commented 1 month ago

I've done some quick testing, and it seems the other flags, placed before or after --platform are unaffected (ie, their presence and values are respected).

hash-d commented 1 month ago

My guess is that this is occurring because of the unchecked rootCmd.ParseFlags at the start of the init() function on cmd/skupper/skupper.go, before the actual flags are defined later on that function.

I think that call is failing because of the unknown (at the time of parsing) first option passed to the command line. Cobra probably even parses --platform correctly later on the execution, but only after the call to config.GetPlatform() that is used to select the SkuppeClient has already occurred.

hash-d commented 1 month ago

@fgiorgetti FYI

hash-d commented 1 month ago

Well... After opening the ticket I checked the status on main, and this is actually a duplicate of #1147, already fixed on #1451

I had not seen those before, as I searched for --platform instead of just platform.

hash-d commented 1 month ago

Closing this as a dup