[installer]: add customization functions to components

[mrsimonemms]

Description

Adds in customisation functions to annotations, environment variables and labels. The functions currently return what's input, but the functions will be completed in a future PR - it's been done this way to reduce the number of urgent PRs submitted to each team.

It's only applied to limited components at the moment to allow for self-hosted customers the flexibility they require in installing Gitpod in their own environments.

Things to check:

• I've put the correct TypeMeta in the functions - these should be the same as in the parent object (eg, it should be a deployment in a deployment, a configmap in a configmap etc). NB pods should have their parent TypeMeta
• I've correctly entered any existing key/value pairs

I have tested it and confirm that I can build an image and open a new workspace

Context

Customers need to set custom annotations, env vars, and labels to some components. To make it easier for all teams to review, this PR does not implement this but simply extracts the setting of annotations, env vars, and labels to the functions CustomizeAnnotation, CustomizeEnvvar, and CustomizeLabel in common/customize. These functions do not change anything yet but allow us to implement this behaviour in a future PR without the need that every team has to review this again.

Uff. These are a lot of changed files. 😓

Indeed. But actually, the changes are:

• common/customize.go that introduces the customize functions that do not change something in the behavior yet but simply return the annotations/env vars/labels that are given via the existing... param (identity function 👋).
• All other changes are only the replacement of setting the annotations/env vars/labels directly in favour of calling this function.

It's just a refactoring. No new logic yet.

In a follow-up PR, we will extend the customize functions to let customers set custom annotations/env vars/labels.

Related Issue(s)

Fixes #10853

How to test

Deploy to preview environment

Release Notes

[installer]: add customization to components

Documentation

Werft options:

• /werft with-preview

[werft-gitpod-dev-com]

started the job as gitpod-build-sje-custom-overrides.2 because the annotations in the pull request description changed
(with .werft/ from main)

[geropl]

Reviewed WebApp components, and deployment LGTM 👍

[mrsimonemms]

I've put a diff file in to show the difference. orig.yaml is run without these changes, change.yaml is run with these changes and the yaml.diff shows the difference.

It's the randomly generated strings causing changes in gitpod.io/checksum_config

diff.zip

[corneliusludmann]

Looks good.

[mustard-mh]

LGTM

/hold

Will we implement that mock function?

[mrsimonemms]

Will we implement that mock function?

Yes, that's coming in a future PR #10827. We did it like this to avoid having to keep asking every team to rush and do an incremental change - do a single big change and then our team will implement the function later

[mrsimonemms]

@mrsimonemms are you going to "protect" the default labels? Changing those could impact ws-manager for instance

@aledbf No. By definition of the requirement, we're explicitly allowing users to change (and potentially break) things. The KOTS config will have a warning line, but it's definitely a risk.

As a future iteration, we could (and probably should) protect certain things, but the 🛹 is to allow for custom overrides on all labels/envvars/annotations in the supported resources

[aledbf]

If we merge this change and change app and/or component you break the connection between ws-manager and ws-daemon

[mrsimonemms]

If we merge this change and change app and/or component you break the connection between ws-manager and ws-daemon

Yes. I agree it's potentially problematic.

If you'd prefer for me to remove this from some components, I can do - would it just be labels in ws-manager and ws-daemon?

[mustard-mh]

@mustard-mh are you ok if I unhold then?

@mrsimonemms good for me

[mrsimonemms]

@aledbf if it's ok with you, I'm going to unhold this. You are absolutely correct in your observation, but it's something that a user would have to specifically do in order to achieve this - it's not something that they could do by accident.

This should be treated with the same caution that Firefox gives when going to about:config - and the KOTS form will explicitly say this

[aledbf]

I'm going to unhold this

I'm afraid I don't agree with that decision. I prefer to escalate to @csweichel

Keep in mind this is (potentially) support overhead for the self-hosted team or worse, break a Gitpod installation.

[corneliusludmann]

As far as I can see (@mrsimonemms correct me when I'm wrong), the decision of whether we protect existing annotations/env vars/labels or not is independent of this PR. Let's merge this PR and discuss whether we want to protect existing once in the issue (#10461).

[mrsimonemms]

@corneliusludmann That is correct. This PR applies the dummy functions only. We can remove/amend/add protection prior to merging the implementation of the functions.

I can also see why @aledbf is (rightly) protective of the ws-daemon and ws-manager because, once this is merged, we won't require any approval from @gitpod-io/engineering-workspace in the implementation of the functions.

As a compromise, shall I remove the controversial sections from this PR? I don't believe we have any current customers wishing to make customisations here (@lucasvaltl can you confirm?) then then we can add this in when necessary?

[corneliusludmann]

As a compromise, shall I remove the controversial sections from this PR?

Just to be sure: To my understanding, the concern is about removing/overriding existing annotations and not about adding annotations to these components at all, right?

once this is merged, we won't require any approval from teams/engineering-workspace in the implementation of the functions

Rising this concern here is totally fine and that we need to agree on something is obvious. However, we could agree on not overriding existing annotations in the issue and anyone can trust us that we implement this accordingly. 😇 In my opinion, reviews, the way we use them, are mainly an instrument to ensure quality (second pair of eyes) instead of getting the “permit” to change something.

That said, given the fact that the CustomOverride... functions don't do anything yet (in this PR), I don't think that it makes sense to implement something here to preserve existing annotations. This should be solved in the PR that implements the functions.

tl;dr – I would vote for: merging this PR now, agreeing on not overriding existing annotations in the issue, and implementing this accordingly in the follow-up PR.

[csweichel]

No. By definition of the requirement, we're explicitly allowing users to change (and potentially break) things. The KOTS config will have a warning line, but it's definitely a risk.

That's not how I understand what we're trying to solve here. We want to enable the addition of labels and annotations to enable things like attributing cost to departments, or to customise behaviour on certain cloud platforms. We do not want to endanger the functioning of Gitpod which heavily relies on the correct labels and annotations being present. I see no value in breaking these invariants which would lead to nothing but a broken Gitpod installation.

Keep in mind this is (potentially) support overhead for the self-hosted team or worse, break a Gitpod installation.

I very much agree with this. We want to make it easy to do the right thing. Breaking Gitpod is not the right thing, hence should not be easy. @mrzarquon (and others) needs to have an easier time in adapting a Gitpod installation to the requirements of a customer's infrastructure.

We can remove/amend/add protection prior to merging the implementation of the functions.

This is a really easy change with the potential to avoid a harm down the road. If we come across a situation where we will want to be able to override a Gitpod-set label/annotation, we should either find out if that label/annotation needs to be there - or if it really makes sense at that point to enable the override of such values.

In my opinion, reviews, the way we use them, are mainly an instrument to ensure quality (second pair of eyes) instead of getting the “permit” to change something.

Indeed, ensuring quality is the primary function of those reviews. Part of this quality assurance is also that the teams who are responsible for certain functions of the system can ensure that functionality. It's not permission as much as it is stewardship.

In the end a decision needs to be made, and it looks like we're at an impasse. What we want to achieve here is that a Gitpod installation is easier to adapt to a customer's environment. I fail to see a use-case where overriding a Gitpod annotation/label which the system likely depends on makes sense today. If we come across such an (unlikely) use-case we can still fix that particular situation.
We want to make it easy to adapt Gitpod, not to break it.

I'd very much vote for not overriding annotations and labels for the reasons outlined above. It's a super easy change to make, which might just save us a ton of headache.

[mrsimonemms]

In the end a decision needs to be made, and it looks like we're at an impasse

A decision does need to be made, although I'm not sure we're at an impasse - I always think of an impasse as something where's there no way forward, whereas this is just a different understanding of the requirements/consequences that we'll solve with open dialogue.

How would you prefer me to proceed?

1. Remove the label override from ws-manager, ws-daemon and anything else?
2. Prevent overriding of existing labels/annotations/envvars?

If I understand the two options, the former this PR will need a change. If it's the latter, this PR is good as-is*. As a third option, I'm happy with any other suggestion on how we can change the implementation.

* Except for the conflict

[werft-gitpod-dev-com]

started the job as gitpod-build-sje-custom-overrides.6 because the annotations in the pull request description changed
(with .werft/ from main)

[corneliusludmann]

Honestly, I don't know why we are still discussing here whether we want to override existing annotations. We are already all more or less on the same page. We all don't want to make it easy to break Gitpod (that is why we stubbornly refuse to offer general post-processing, even though it is repeatedly requested). I see, the statement above was a bit misleading/overshot the mark, but it was retracted two comments later.

Part of this quality assurance is also that the teams who are responsible for certain functions of the system can ensure that functionality. It's not permission as much as it is stewardship.

100 % agree. And that is what has happened here properly. My statement was only about whether we need to implement the code that ensures we don't overwrite annotations in this PR (which is also reviewed by Team Workspace), or in the follow-up PR that implements the actual function (which is not necessarily reviewed by Team Workspace).

In the end a decision needs to be made, and it looks like we're at an impasse.

I don't see an impasse at all. It was the original idea of the PR opener to allow to override of existing ideas, the concerns that this can break Gitpod have been raised and acknowledged, and we will respect this in the implementation of the functions.

Again: This PR, in particular, does not change anything in the behavior. This is just a refactoring that allows implementing on a central position accessing annotations, env vars, and labels of components to do stuff based on the config. We split that out to make the review easier (smaller PRs). And that's why I suggested moving the discussion (which isn't really necessary anymore) to the issue (or the follow-up PR). That's all.

(We could at most discuss here whether the naming with “override” in the file/function is possibly misleading.)

What do you all think? Are we on the same page?

[csweichel]

It was the original idea of the PR opener to allow to override of existing ideas, the concerns that this can break Gitpod have been raised and acknowledged

👍

What do you all think? Are we on the same page?

Yes - let's do this :)

[mrsimonemms]

Changes made as proposed - I'll leave the removal of the "hold" label to @csweichel or @aledbf as a final approval

[csweichel]

/hold cancel