Support for custom prometheus registry

[christyjacob4]

This PR contains the proposed changes for allowing custom registries for the prometheus image.
Draft for #3204

[Pothulapati]

As mentioned in the issue here #3204 (comment), We don't actually need a new prometheusRegistry flag, we would just want to re-use the dockerRegistry flag.

Something like

installValues.PrometheusImage = fmt.Sprintf("%s/%s", options.dockerRegistry, installValues.PrometheusImage)

but this means that folks actually can't overwrite the prometheusImage using the CLI. So, we will have to add a prometheusImage cli flag, and overwrite it when the value is given.

Both options will still have to use the same dockerRegistry flag as the registry. It dosen't make sense for prometheus to have its own registry field.

Can you also update the PR name to reflect the changes that are being done?

[christyjacob4]

@Pothulapati Okay I will make the requested changes. :)
Sorry About that. I will update the PR message now :)

Also I should be adding a regex check to validate the prometheus image name?

[christyjacob4]

@Pothulapati Can you please explain the difference b/w options & install values?

[zaharidichev]

@christyjacob4 the options hold data that have been supplied by the command line arguments. The values is the data used to render the Helm chart. Thinkgs of it this way: the options are used to override some of the default values and customize what you actually get in the end. Does that answer the question?

[christyjacob4]

@zaharidichev thanks for that. It makes it clearer to me now. Since I've never worked with Helm before, could you please tell me what it's used for?

[Pothulapati]

@christyjacob4 Helm is like a package manager for kubernetes deployments. Applications like MySQL,etc are packaged as a helm template, which users can directly install on their cluster.

We also package Linkerd as a helm chart, which users can directly install on their cluster using helm

https://helm.sh/

https://linkerd.io/2/tasks/install-helm/

[christyjacob4]

@Pothulapati Thanks so much. Much clearer now. So its sort of like a Dockerfile ?

[Pothulapati]

Yep, you can think of it like that. Let's please keep the issue comments regarding the issue itself.

Feel free to use the Linkerd slack for conversations like this.

[christyjacob4]

@Pothulapati would this be a right regex to validate a prometheus image string?

prometheusImage = regexp.MustCompile(`^[a-zA-Z]+\/[a-zA-Z]+\:v[0-9]+\.[0-9]+\.[0-9]+$`)

I have assumed that prometheus-image would have strings like

prom/prometheus:v2.15.2

[cpretzer]

@christyjacob4 the progress looks good on this.

Please be sure to document the new flag as well.

[christyjacob4]

@cpretzer where do you want me to add the documentation?

[cpretzer]

@cpretzer where do you want me to add the documentation?

You could add it to the docs in the [linkerd/website](https://github.com/linkerd/website) repository. That being said, I see that we don't document all of the image flags there, so you may not need to add it there.

[cpretzer]

@christyjacob4 I see that this is still a draft PR. Do you plan to add support for a specific tag?

[christyjacob4]

@cpretzer it was suggested by @Pothulapati to open a draft PR so thay its clearer to others what changes I'm making.
The prometheusImage is the flag I want to add support for

[Pothulapati]

@christyjacob4 So, The problem is that we don't maintain the prometheus image in the linkerd-io image repo (like we do with grafana). So, we can't really pre-fix the dockerRegistry flag to promethues as we can't support the default install (i.e repo is linkerd-io). Should we maintain? IMO, we should not.In that case, we could just make this,

diff --git a/cli/cmd/install.go b/cli/cmd/install.go
index 2934d6c0..56b61e17 100644
--- a/cli/cmd/install.go
+++ b/cli/cmd/install.go
@@ -653,6 +653,10 @@ func (options *installOptions) buildValuesWithoutIdentity(configs *pb.All) (*l5d
                options.identityOptions.replicas = options.controllerReplicas
        }

+       if options.prometheusImage != "" {
+               installValues.PrometheusImage = options.prometheusImage
+       }
+
        globalJSON, proxyJSON, installJSON, err := config.ToJSON(configs)
        if err != nil {
                return nil, err
@@ -674,7 +678,6 @@ func (options *installOptions) buildValuesWithoutIdentity(configs *pb.All) (*l5d
        installValues.Global.HighAvailability = options.highAvailability
        installValues.Global.ImagePullPolicy = options.imagePullPolicy
        installValues.GrafanaImage = fmt.Sprintf("%s/grafana", options.dockerRegistry)
-       installValues.PrometheusImage = fmt.Sprintf("%s/%s", options.dockerRegistry, installValues.PrometheusImage)
        installValues.Global.Namespace = controlPlaneNamespace
        installValues.Global.CNIEnabled = options.cniEnabled
        installValues.OmitWebhookSideEffects = options.omitWebhookSideEffects

Thus allowing users to just give a full image name (including registry name) in the flag. Should we also consider the dockerRegistry flag when that is also passed with --prometheus-image? In that case, we could have another check inside, which pre-fixes it. @cpretzer

[christyjacob4]

So how would you like me to proceed on this @Pothulapati @cpretzer ?

[cpretzer]

@christyjacob4

@Pothulapati provides a good snippet of code about how to handle this case and it is congruent with the comments from the issue:

#3204 (comment)

#3204 (comment)

1. We shouldn't assume a registry other than the default registry
2. If the user specifies an image using this flag, then it should contain the registry and image name

Think about it this way: let's say BigBank has an airgapped environment and they use their own docker registry. They will source the prometheus image from some location and add it to their internal registry, so the repository could be something like: docker.bigbank.internal

They could even rename the image if they wanted to like: bb-prom and include their own tag 2.15.1_rc

In that case, their install command might be like:
linkerd install --prometheus-image docker.bigbank.internal/bb-prom:2.15.1_rc

For this scenario, the snippet that @Pothulapati wrote would work. Additionally, for other users like scrappystartup.com who don't use a custom registry, they can omit the --prometheus-image flag and get the official image from docker hub.

I don't see added value to having constituent parts for the repository, image, and tag in an airgapped environment.

Hope that helps!

[Pothulapati]

This looks good but I think we should update the regex now (as we include the registry name, etc stuff in the prometheusImage). Without that, I get image invalid error when I run

./bin/go-run cli install --ignore-cluster --prometheus-image docker.bigbank.internal/bb-prom:2.15.1_rc

[christyjacob4]

@Pothulapati I will do that now... :)

[christyjacob4]

@Pothulapati Do you want me to add the regex in the root.go file or install.go?

[christyjacob4]

@cpretzer @Pothulapati Are we good to go on this?

[cpretzer]

LGTM

Ran automated tests
Confirmed that the prometheus image is set when using --prometheus-image flag.

[ihcsim]

LGTM!