osm install --set arguments should override default flag values Go

Bug description: If the --set flag is used for osm install but the value being passed in already has a default since it has its own flag for install, the argument passed to --set should override the default.

More details: https://github.com/openservicemesh/osm/pull/2784/files#r591035725 Affected area (please mark with X where applicable): - Install [X] - SMI Traffic Access Policy [ ] - SMI Traffic Specs Policy [ ] - SMI Traffic Split Policy [ ] - Permissive Traffic Policy [ ] - Ingress [ ] - Egress [ ] - Envoy Control Plane [ ] - CLI Tool [ ] - Metrics [ ] - Certificate Management [ ] - Sidecar Injection [ ] - Logging [ ] - Debugging [ ] - Tests [ ] - CI System [ ]

Expected behavior: osm install --set="OpenServiceMesh.enablePrivilegedInitContainer=true" --set="OpenServiceMesh.envoyLogLevel=info" The above command should override the envoyLogLevel Steps to reproduce the bug (as precisely as possible):

osm install --set="OpenServiceMesh.enablePrivilegedInitContainer=true" --set="OpenServiceMesh.envoyLogLevel=info"

How was OSM installed?: osm CLI Anything else we need to know?:

Environment: - OSM version (use osm version): 0.8.0 - Kubernetes version (use kubectl version): 1.18 - Size of cluster (number of worker nodes in the cluster): 2 - Others:

Asked Oct 07 '21 04:10
avatar ksubrmnn
ksubrmnn

5 Answer:

This behavior is intended, though I understand it's not ideal.

The help text for the flag does address this behavior somewhat:

--set stringArray                         Set arbitrary chart values not settable by another flag...

--set doesn't override the other flags because currently the validation for those values is tied to the CLI flag. The current behavior ensures that validation can't be bypassed using --set.

1
Answered Mar 10 '21 at 20:40
avatar  of nojnhuh
nojnhuh

--set doesn't override the other flags because currently the validation for those values is tied to the CLI flag. The current behavior ensures that validation can't be bypassed using --set.

This should be resolvable with a bit of refactoring, right? We'd just have to "merge" the set values first, then validate, then resolve.

1
Answered Mar 10 '21 at 20:56
avatar  of ksubrmnn
ksubrmnn

Yeah, that should work. My only concern is the error messages will probably say something like OpenServiceMesh.envoyLogLevel not valid... if a user does osm install --envoy-log-level=invalid. Usually the mapping between osm install flags and values is easy enough to figure out, but I could see that being unclear.

1
Answered Mar 10 '21 at 21:15
avatar  of nojnhuh
nojnhuh

Can we make the usage of chart param specific flags consistent with either using the CLI flags or using --set and ensuring we can validate every param passed to --set? There needs to be a clear guidance on when to use a CLI flag vs --set. Otherwise this is going to introduce more inconsistency in the code and make it hard to reason. I don't believe we should be providing multiple ways to set the same chart param, its just harder to maintain and reason.

1
Answered Mar 10 '21 at 22:17
avatar  of shashankram
shashankram

Commit bff18e72c702478ee588433547d80b17240d8e83 removed custom CLI flags for the install commands in favor of using --set, so this issue should be resolved.

1
Answered Sep 10 '21 at 17:44
avatar  of shashankram
shashankram