allow default system namespace to be overridden using an env var #427

rawlingsj · 2019-01-23T23:25:43Z

I'd like to install Build Pipeline in namespace other than tekton-pipelines. This change still uses tekton-pipelines as the default but allows folks to set an environment variable to override. As an example I've hacked a Helm chart for easy install while integrating with Build Pipeline, this uses the downward api to set the new env var to the current namespace.

If it's of interest this is the chart and override the new env var here.

Also deleted unused pkg/names.go as I think it was duplicated in pkg/system/names.go.

rawlingsj · 2019-01-23T23:33:57Z

I probably ought to add the new env var to the docs somewhere too, in https://github.com/knative/build-pipeline/blob/master/DEVELOPMENT.md#environment-setup maybe? Though that would complicate things as the current ./config yaml files would then also need to be updated to remove the namespace so maybe easier to not mention it yet as this seems only useful when installing using the helm chart I created for now?

bobcatfish · 2019-01-24T19:38:32Z

Thanks for this @rawlingsj !! 🎉

I probably ought to add the new env var to the docs somewhere too

Definitely agree! I think what we really need is some docs about how to setup and configure pipelines, which we don't have yet 🤔 (kind of related to #385)

So some options:

pave new ground + start a new doc on setting up pipelines (with almost nothing in it for now - you could link to your sweet helm chart tho!!)
Put it in the DEVELOPMENT.md for now like you suggested
Add it to these (sparse) docs we have on configuring the entrypoint image

but allows folks to set an environment variable to override

What do you think about configuring the namespace in a config-map instead of an environment variable? Just wondering cuz we already have a config-map for some other controller config. Maybe this doesn't make sense tho b/c the controller wouldn't even know what namespace to look for the namespace in 🤔

bobcatfish · 2019-01-24T19:44:49Z

@shashwathi and @imjasonh pointed out that there have been similar changes to serving: knative/serving#2708 <-- so as long as we do pretty much the same thing as that, we should be good! and it looks I think like this configuration also ends up using an env var

markusthoemmes · 2019-01-25T08:44:58Z

FWIW and in addition to what @bobcatfish already pointed out, a generalized version of the serving bit was added to knative/pkg recently. I think all projects should be able to use that? knative/pkg#235

abayer · 2019-01-25T20:51:34Z

/ok-to-test

rawlingsj · 2019-01-25T21:03:04Z

ok great - thanks @bobcatfish @markusthoemmes - will take a look

bobcatfish · 2019-01-25T22:29:41Z

nice, thanks @markusthoemmes !!

rawlingsj · 2019-02-01T23:30:51Z

sorry for the delay but juggling lots of things ATM, so we're suggesting we

add a dependency on knative/pkg repo
use https://github.com/knative/pkg/blob/8e50d82/system/names.go#L30 instead of the same approach in this PR
panic if the env var isn't set using the downward api, which is different from existing behaviour where a default namespace is used
switch the ./config/*.yaml files in this repo to use the downward api and remove the namespace: knative-build-pipeline from all yamls, example https://github.com/knative/build-pipeline/blob/01853c6/config/200-serviceaccount.yaml#L18
delete the create namespace yaml and add to install docs to create a new namespace with kubectl if folks don't want it in the current namespace?

If that's all correct and folks are happy I can close this PR and create a new with above?

rawlingsj · 2019-02-01T23:44:29Z

Hmm - just started and suspect there's going to be a problem updating the ./config/*.yaml because there's a ClusterRoleBinding that needs the namespace that the serviceaccount lives in https://github.com/knative/build-pipeline/blob/01853c6/config/201-clusterrolebinding.yaml#L21, we get around this in helm world by templating the namespace https://github.com/jenkins-x-charts/knative-build-pipeline/blob/b8d1473/knative-build-pipeline/templates/200-clusterrolebinding.yaml#L21 so it's set at install time to the current namespace just as the downward api is set at install.

I'm not sure how we can use https://github.com/knative/pkg/blob/8e50d82/system/names.go#L30 from this repo and keep the current approach of installing via ko apply -f config/, I think there needs to be some kinda templating on the ClusterRoleBinding to set the current namespace or update docs to change it which doesn't seem great.

Any other thoughts or ideas?

rawlingsj · 2019-02-01T23:46:14Z

Might be nice to avoid the ClusterRoleBindings all together and create RoleBindings instead as that would get around the problem, not sure if the knative build pipeline need to be cluster wide role bindings?

abayer · 2019-02-06T17:47:23Z

Good question - @bobcatfish, do you know re ^^^?

bobcatfish · 2019-02-06T21:49:51Z

I'm not sure how we can use https://github.com/knative/pkg/blob/8e50d82/system/names.go#L30 from this repo and keep the current approach of installing via ko apply -f config/, I think there needs to be some kinda templating on the ClusterRoleBinding to set the current namespace or update docs to change it which doesn't seem great.

Yeah this is weird, im looking at knative/serving to see how they've done it, since they seem to be using system.Namespace() (e.g. here), but they've still go the namespace hardcoded in all their yaml files (and the config to create the namespace still exists)

Looking at the PR where this was added knative/serving#2708 I get the impression there was no good story at that point either about how to handle those yamls, it seems like the assumption is that a user will change them manually 🤔

My only recommendation is to leave the namespace hard-coded in our .yaml files for now, and add some docs about how folks can change them manually (and use the downward api) - not sure there are any better options (maybe @imjasonh has some ideas)

imjasonh · 2019-02-06T21:54:01Z

@bobcatfish's suggestion about leaving hard-coded values in YAML and documenting changing it SGTM, especially if that's what Serving has today. When in doubt we should go along with what the other repos do to solve these problems, even if those solutions are half-baked.

rawlingsj · 2019-02-18T00:35:48Z

Ok great, that all sounds good to me. I'll get onto it in the morning.

rawlingsj · 2019-02-20T01:59:01Z

@imjasonh @bobcatfish @abayer I was tempted to submit a few PR options for this but figured I'd start simple. I've force pushed to this existing PR but to explain I've rebased which includes the tekton rename but doesn't include the knative/pkg#235 package as suggested in an earlier comment. My thinking is to keep the current config/*.yaml as is but I added docs to explain how to install into a custom namespace.

If you want me to vendor the knative/pkg repo instead I can do that and update the controller and webhook deployments to use the downward api (as the default panics if there's no SYSTEM_NAMESPACE env var set in that pkg) and update the docs to include that too.

abayer · 2019-02-20T12:26:26Z

/lgtm

shashwathi

I have left comments on doc changes. I had trouble following instructions so thats my feedback.

shashwathi · 2019-02-20T16:02:15Z

DEVELOPMENT.md

@@ -118,6 +118,22 @@ kubectl create clusterrolebinding cluster-admin-binding \
  --user="${USER}"
 ```

+### Install in custom namespace
+1. To install into a different namespace you will need to modify resources in the `./config`
+    - remove all `namespace: tekton` resources


nit: Can you update this to "remove all namespace: tekton references"

shashwathi · 2019-02-20T16:02:45Z

DEVELOPMENT.md

+### Install in custom namespace
+1. To install into a different namespace you will need to modify resources in the `./config`
+    - remove all `namespace: tekton` resources
+    - delete the `namespace.yaml


Missing "`" at end of file name. Also can you link the config namespace file here?

shashwathi · 2019-02-20T16:04:07Z

DEVELOPMENT.md

+1. To install into a different namespace you will need to modify resources in the `./config`
+    - remove all `namespace: tekton` resources
+    - delete the `namespace.yaml
+    - modify the `subjects.namespace` value to the desired namespace


Again can you link clusterrolebinding yaml here? I think thats what you are referring here

shashwathi · 2019-02-20T16:04:34Z

DEVELOPMENT.md

+    - remove all `namespace: tekton` resources
+    - delete the `namespace.yaml
+    - modify the `subjects.namespace` value to the desired namespace
+    - add `downwardapi` entry to webhook and controller `deployment` resources and set current namespace.


Can you provide an example of how to add downwardapi entry to both deployment resources?

shashwathi · 2019-02-20T16:04:59Z

DEVELOPMENT.md

+    - add `downwardapi` entry to webhook and controller `deployment` resources and set current namespace.
+
+     e.g.
+```yaml


I think you can remove the additional space in yaml example. Markdown render looks bit weird

rawlingsj · 2019-02-20T17:29:10Z

@shashwathi thanks for the comments. Think I just resolved them, hopefully it now reads better.

bobcatfish · 2019-02-20T17:34:16Z

DEVELOPMENT.md

@@ -118,6 +118,20 @@ kubectl create clusterrolebinding cluster-admin-binding \
  --user="${USER}"
 ```

+### Install in custom namespace


i have one completely minor piece of feedback: usually we have an extra newline after a header.

i'm also happy to lgtm and let @mattmoor-sockpuppet fix that tho XD

cool - thanks, I just added the extra newline anyways.

abayer · 2019-02-20T17:56:09Z

/lgtm

shashwathi

/lgtm

abayer · 2019-02-20T18:07:13Z

/lgtm
/approve

knative-prow-robot · 2019-02-20T18:07:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer, rawlingsj, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [shashwathi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abayer · 2019-02-20T18:07:26Z

oh wait I don't have approve rights. heh.

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 23, 2019

knative-prow-robot requested review from bobcatfish and tejal29 January 23, 2019 23:25

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2019

bobcatfish self-assigned this Jan 24, 2019

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 25, 2019

rawlingsj added 2 commits February 20, 2019 02:07

allow default system namespace to be overridden by an env var

bed3db2

add docs on installing into a custom namespace

4edf57a

rawlingsj changed the title ~~allow hardcoded system namespace to be overridden using an env var~~ allow default system namespace to be overridden using an env var Feb 20, 2019

knative-prow-robot assigned abayer Feb 20, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019

shashwathi suggested changes Feb 20, 2019

View reviewed changes

tidy up docs on installing into another namespace

386e9b0

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019

bobcatfish reviewed Feb 20, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019

shashwathi approved these changes Feb 20, 2019

View reviewed changes

knative-prow-robot assigned shashwathi Feb 20, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2019

fix doc formatting

074e442

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019

knative-prow-robot merged commit fdb9102 into tektoncd:master Feb 20, 2019

mchmarny unassigned abayer and shashwathi Mar 7, 2019

allow default system namespace to be overridden using an env var #427

allow default system namespace to be overridden using an env var #427

Conversation

rawlingsj commented Jan 23, 2019 • edited Loading

rawlingsj commented Jan 23, 2019 • edited Loading

bobcatfish commented Jan 24, 2019

bobcatfish commented Jan 24, 2019

markusthoemmes commented Jan 25, 2019 • edited Loading

abayer commented Jan 25, 2019

rawlingsj commented Jan 25, 2019

bobcatfish commented Jan 25, 2019

rawlingsj commented Feb 1, 2019

rawlingsj commented Feb 1, 2019

rawlingsj commented Feb 1, 2019

abayer commented Feb 6, 2019

bobcatfish commented Feb 6, 2019

imjasonh commented Feb 6, 2019

rawlingsj commented Feb 18, 2019

rawlingsj commented Feb 20, 2019 • edited Loading

abayer commented Feb 20, 2019

shashwathi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawlingsj commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Feb 20, 2019

shashwathi left a comment

Choose a reason for hiding this comment

abayer commented Feb 20, 2019

knative-prow-robot commented Feb 20, 2019

abayer commented Feb 20, 2019

rawlingsj commented Jan 23, 2019 •

edited

Loading

rawlingsj commented Jan 23, 2019 •

edited

Loading

markusthoemmes commented Jan 25, 2019 •

edited

Loading

rawlingsj commented Feb 20, 2019 •

edited

Loading