k8s resource names are not configurable

[silverlyra]

Expected Behavior

When deploying Tekton Pipelines, I expect to be able to change resource names to match our conventions.

Actual Behavior

• The webhook Deployment and Service must be called tekton-pipelines-webhook
• The logging and observability ConfigMaps must be called config-logging and config-observability, despite being configurable in the controller’s Deployment
• The configuration for bucket and PVC artifacts must be called config-artifact-bucket and config-artifact-pvc

Steps to Reproduce the Problem

1. Change any of the above resource names in the Tekton Pipelines deployment YAML bundle
2. Apply them
3. Witness the CrashLoopBackOffs

Additional Info

We have a policy of not blindly applying .yml distributions to our clusters; instead, we go through them to make sure we understand what we’re deploying and how the services that comprise a system fit together.

There’s almost always things in the distributed resources that we like to clean up; for Tekton, I thought it was redundant to name resources tekton-pipelines-… when we were creating a whole tekton-pipelines namespace for them; and likewise redundant to start all ConfigMap names with config-….

[vdemeester]

/kind question
/kind feature

[imjasonh]

Thanks for bringing this up! So, this is probably a few related issues wrapped up in one:

1. tekton-pipelines-webhook is defined in config/webhook.yaml, config/400-webhook-service.yaml (the k8s service), and then again in cmd/webhook/main.go -- the copy in main.go should probably just take that value from an env var, or from the downward API, to get whatever value is specified in the YAML.

2. I was able to rename config-observability to asdfasdf and set the env var in controller.yaml to point to that without instigating a CrashLoopBackoff. Same with config-logging->qwerqwer.

3. config-artifact-pvc is defined as a const in artifacts_storage.go, this should be a method that consults an env var and falls back to that value. Likewise for config-artifact-bucket. That should be a pretty straightfoward change.

/assign

It's worth mentioning though, this is going to be pretty difficult to automatically test, at least given the limits of our current test setup. So straying from the defaults will possibly result in regressions in the future unless we're very careful, and/or invest in better tests.

[silverlyra]

hi, thanks for replying!

this is going to be pretty difficult to automatically test

yeah – I think this would not be worth doing in an e2e test, unless one day this becomes a commonly-used configuration option.

What if, instead of replacing the current consts with functions that call os.Getenv directly, they call it through an interface with a Getenv method that could be easily mocked in a unit test?

[imjasonh]

With both of those PRs merged, I believe this issue is resolved. Please let me know if you find otherwise, or find other examples of unrenameable resources.