cli: remove install cli options used for overriding chart values in favor of using --set option

[shashankram]

Please describe the Improvement and/or Feature Request This issue proposes removing CLI options exposed in cmd/cli/install.go used specifically to override the default chart values defined in charts/osm/values.yaml, in favor of using the --set option to override the chart values.

The following install CLI flags should be removed from cmd/cli/install.go, and all corresponding documentation should be updated. Further, commands that have validations should be validated using charts/osm/values.schema.json.

• --cert-manager-issuer-group
• --cert-manager-issuer-kind
• --certificate-manager
• --container-registry
• --container-registry-secret
• --deploy-grafana
• --deploy-jaeger
• --deploy-prometheus
• --enable-debug-server
• --enable-egress
• --enable-fluentbit
• --enable-permissive-traffic-policy
• --enable-prometheus-scraping
• --envoy-log-level
• --osm-image-pull-policy
• --osm-image-tag
• --prometheus-retention-time
• --service-cert-validity-duration
• --vault-host
• --vault-protocol
• --vault-role
• --vault-token

The options I think should stay for osm install because of being very tightly related to the install process:

• --atomic
• --mesh-name
• --enforce-single-mesh

Scope (please mark with X where applicable)

• New Functionality [ ]
• Install [X]
• SMI Traffic Access Policy [ ]
• SMI Traffic Specs Policy [ ]
• SMI Traffic Split Policy [ ]
• Permissive Traffic Policy [ ]
• Ingress [ ]
• Egress [ ]
• Envoy Control Plane [ ]
• CLI Tool [X]
• Metrics [ ]
• Certificate Management [ ]
• Sidecar Injection [ ]
• Logging [ ]
• Debugging [ ]
• Tests [ ]
• CI System [ ]
• Demo [ ]
• Project Release [ ]

Possible use cases Make install CLI command options consistent.

[draychev]

For context see: https://github.com/openservicemesh/osm/pull/2638#pullrequestreview-600940623

[C123R]

Hi there, Can I give it a shot?

[shashankram]

Hi there, Can I give it a shot?

Hi @C123R, thanks a lot for your interest in contributing. I'll assign this one to you!

I wanted to share more details on how you could potentially approach this task:

1. Go through the install flags defined in https://github.com/openservicemesh/osm/blob/main/cmd/cli/install.go
2. Based on the proposal above, remove the corresponding CLI flags
3. Update the code and documentaion in the repo that makes use of these options. There will be a lot of references to the older options:

   cmd/cli/install.go:  $ osm install --osm-namespace hello-world
   cmd/cli/install.go:  $ osm install --mesh-name "hello-osm"
   demo/run-osm-demo.sh:  bin/osm install \
   demo/run-osm-demo.sh:  bin/osm install \
   docs/content/docs/install/_index.md:By default, the control plane components are installed into a Kubernetes Namespace called `osm-system` and the control plane is given a unique identifier attribute `mesh-name` defaulted to `osm`. Both the Namespace and mesh-name can be configured with flags to the `osm install` command. Running `osm install --help` provides details on the various flags that can be configured.
   docs/content/docs/install/_index.md:    osm install --set="OpenServiceMesh.enablePrivilegedInitContainer=true"
   docs/content/docs/install/manual_demo/_index.md:osm install \
   docs/content/docs/install/manual_demo/_index.md:osm install --enable-permissive-traffic-policy
   docs/content/docs/tasks_usage/logs.md:osm install --enable-fluentbit
   docs/content/docs/tasks_usage/logs.md:    osm install --enable-fluentbit [--set OpenServiceMesh.controllerLogLevel=<desired log level>]
   docs/content/docs/tasks_usage/logs.md:osm install --enable-fluentbit --set OpenServiceMesh.fluentBit.enableProxySupport=true,OpenServiceMesh.fluentBit.httpProxy=<http-proxy-host:port>,OpenServiceMesh.fluentBit.httpsProxy=<https-proxy-host:port>
   docs/content/docs/tasks_usage/logs.md:    osm install --enable-fluentbit
   docs/content/docs/tasks_usage/metrics.md:osm install --help
   docs/content/docs/tasks_usage/tracing.md:osm install --deploy-jaeger --set OpenServiceMesh.tracing.enable=true
   docs/content/docs/tasks_usage/tracing.md:osm install --set OpenServiceMesh.tracing.enable=true,OpenServiceMesh.tracing.address=<tracing server hostname>,OpenServiceMesh.tracing.port=<tracing server port>,OpenServiceMesh.tracing.endpoint=<tracing server endpoint>
   docs/content/docs/tasks_usage/traffic_management/egress.md: osm install --enable-egress
   docs/content/docs/tasks_usage/traffic_management/egress.md: bin osm install --enable-egress=false
   docs/content/docs/tasks_usage/traffic_management/egress.md:    osm install --enable-egress=true
   docs/content/docs/tasks_usage/traffic_management/iptables_redirection.md:    osm install --set="OpenServiceMesh.outboundIPRangeExclusionList={1.1.1.1/32,2.2.2.2/24}
   docs/content/docs/tasks_usage/traffic_management/iptables_redirection.md:    osm install --enable-egress=false
   docs/content/docs/tasks_usage/traffic_management/permissive_traffic_policy_mode.md:osm install --enable-permissive-traffic-policy=true
   docs/content/docs/tasks_usage/traffic_management/permissive_traffic_policy_mode.md:osm install --enable-permissive-traffic-policy=false
   docs/content/docs/tasks_usage/traffic_management/permissive_traffic_policy_mode.md:    osm install --enable-permissive-traffic-policy=true
   docs/content/docs/troubleshooting/observability/grafana.md:    When installed with `osm install --deploy-grafana`, a Grafana Pod named something like `osm-grafana-7c88b9687d-tlzld` should exist in the namespace of the other OSM control plane components which named `osm-system` by default.
   docs/content/docs/troubleshooting/observability/prometheus.md:    When installed with `osm install --deploy-prometheus`, a Prometheus Pod named something like `osm-prometheus-5794755b9f-rnvlr` should exist in the namespace of the other OSM control plane components which named `osm-system` by default.
   docs/wip/demo_prometheus.md:    $ osm install --deploy-prometheus --enable-permissive-traffic-policy

   A flag that is overriding a specific chart value in https://github.com/openservicemesh/osm/blob/main/charts/osm/values.yaml should be deprecated in favor of using the --set flag with osm install.

For ex, the --enable-egress flag in install.go should be removed, and references should be updated to use --set OpenServiceMesh.enableEgress=true.

Please let us know if you have additional questions.

[C123R]

@shashankram Perfect, Thanks for the explanation. I will check it over weekend and get back to you if there are any questions 👍

[C123R]

Hi @shashankram,

I've already started working on this issue, but I'd like some advice on how to validate install flags:

For example:

Considering there won't be deployPrometheus or enablePrometheusScraping flag, should we validate it by parsing set option values or using values.schema.json validation(I am looking into it if it's really feasible with conditional schema validation)?

if i.deployPrometheus {
     if !i.enablePrometheusScraping {
      _, _ = fmt.Fprintf(i.out, "Prometheus scraping is disabled. To enable it, set prometheus_scraping in %s/%s to true.\n", settings.Namespace(), constants.OSMConfigMap)
     }
}

Parsing set options during validation:

       // parse setOptions as its done in resolveValues()

        setOptions := s["OpenServiceMesh"].(map[string]interface{})
    if setOptions["deployPrometheus"] == "true" {
        if setOptions["enablePrometheusScraping"] == "false" {
            _, _ = fmt.Fprintf(i.out, "Prometheus scraping is disabled. To enable it, set prometheus_scraping in %s/%s to true.\n", settings.Namespace(), constants.OSMConfigMap)
        }
    }

The same goes for certificateManager validation when it's set to "vault".

[nojnhuh]

Hi @C123R, I think I can answer your question.

I think we should leverage the values.schema.json as much as possible. There are a few values where we have more complex validation that can't be defined in the schema though, all of which is done in that validateOptions() method. I think you have the right idea of replacing references to the flag variables with the result from --set.

[C123R]

Hi @C123R, I think I can answer your question.

I think we should leverage the values.schema.json as much as possible. There are a few values where we have more complex validation that can't be defined in the schema though, all of which is done in that validateOptions() method. I think you have the right idea of replacing references to the flag variables with the result from --set.

@nojnhuh Thanks for clarifying!

I will come up with [WIP] PR soon with validations parsing --set options.