Helm upgrades #1779

SamLR · 2022-03-22T17:44:02Z

Closes: #1778

What changed?

Replaced previous chart with one built out from the recommended helm template (i.e. the one created by helm create)
Added options to restrict service account impersonation scope. This is important for production usage as without a limited scope on this permission the app can impersonate system:masters which is unacceptable
Made the wego-admin user (now renamed wego-test-user) an optional creation
Added quality of life fixed like additionalArgs and envVars for greater flexibility

Why?

I needed to be able to pass --insecure for deployment (and handle TLS termination in the LB)... things got out of hand

How did you test it?

Running helm template with various settings, e.g.:

$ helm template -n TEST --dry-run --debug \
      --set resources.limits.cpu=320,resources.limits.memory=666 \
      --set resources.requests.cpu=125,resources.requests.memory=333 \
      --set additionalArgs='{a,b,c}' \
      --set envVars\[0\].name=TEST,envVars\[0\].value=foo,envVars\[1\].name=bar \
      --set rbac.impersonationResources='{user,group,serviceaccout}' \
      --set rbac.impersonationResourceNames='{foo,bar}' test ./charts/gitops-server

Documentation Changes

The notes from the README.md should be reflected in the main line website docs (probably in gitops-dashboard) but those look like they need a top-to-bottom overhaul so I wasn't sure where to start.

Note

Ideally the permissions granted to the service account should be further reduced. As I understand it the non-impersonate stanzas exist to support profiles which is not a default use of the UI so should probably be removed but doing so (currently) stops the UI working. I think this should be fixed.

ozamosi · 2022-03-22T17:55:52Z

The chart is called weave-gitops in a directory called gitops-server - I think the chart should move to weave-gitops as the old chart is deleted.
I believe this needs some changes to tools/helm-values-dev.yaml so that it doesn't break the dev env.

SamLR · 2022-03-22T18:00:02Z

The chart is called weave-gitops in a directory called gitops-server - I think the chart should move to weave-gitops as the old chart is deleted.

I think the question here is should this be "weave-gitops" or "gitops-server" (there're also places we refer to it as "wego-app" still). I don't mind which we go with and, yes, once one is chosen happy to update as many places as I can find.

* I believe this needs some changes to `tools/helm-values-dev.yaml` so that it doesn't break the dev env.

Good catch, will go and check

josecordaz · 2022-03-22T19:23:20Z

This work looks awesome. Assuming we are using Helm 3... should we also plan on defining a json schema for the file values.yaml ? (values.schema.json). A couple advantages I see are that we can enforce specific value types and required fields when installing, updating or using template subcommand. Or maybe we want to wait a bit more to let weave-gitops to get a bit more mature? 🤔

Callisto13 · 2022-03-23T10:49:36Z

charts/gitops-server/README.md

+* Service, this is optional as you may want to limit access to the UI to via
+  port-forwarding
+* Ingress
+* Test User -- A test user with hard-coded username & password with minimal


The test user is actually a backup admin user in the case that OIDC is failing and cluster ops need to do things.
It is currently undergoing some feature realignment, see the thread here: https://weaveworks.slack.com/archives/C03244W0C8H/p1647967858091769

Why would you have a backup admin with only read permissions?

because those are all i added so far 🙃 , probably because that is all wego core can do right now

from a point of least privilige I'd like to only make it a requirement/recommendation when it's actually in a position to fulfill those extended duties ;)

Callisto13 · 2022-03-23T10:54:21Z

charts/gitops-server/templates/test-user.yaml

+  username: {{ .Values.testUser.username | b64enc | quote }}
+  password: {{ .Values.testUser.password | b64enc | quote }}


do these need to be b64enc here? i think i remember that k8s will do that automagically when the secret is created

you do (I just checked against a kind cluster with a simple helm chart). I think the auto-b64encode is only when you use kubectl

i swear i have seen that as well when just doing like a:

secret := &v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", Namespace: "bar", }, Type: "Opaque", Data: map[string][]byte{ "key": []byte("value"), }, } return yaml.Marshal(secret)

even then it is encoded

o well, idk what the rules are 🙃

bold of you to assume there're rules ;)

Callisto13 · 2022-03-23T10:55:26Z

charts/gitops-server/values.yaml

+replicaCount: 1
+
+image:
+  # FIXME check the app name


is this a thing you wanted to fix now?

negotiations are on going https://weaveworks.slack.com/archives/C03244W0C8H/p1647972249151359 :p

Ideally yes but it's not a blocker on this and it may need a wider conversation

Callisto13 · 2022-03-23T10:56:06Z

charts/gitops-server/values.yaml

+# To enable enterprise OIDC:
+# envVars:
+# - name: WEAVE_GITOPS_AUTH_ENABLED
+#   value: true


this flag is also needed for the admin user login flow, just in case you wanted to note that.
i think it gates all login functionality, in core as well as ent

Would it make more sense to turn this into a variable then, something like:

# Sets the WEAVE_GITOPS_AUTH_ENABLED environment variable to enable login enable_login: true

yeh that sounds good

Callisto13 · 2022-03-23T10:57:25Z

charts/gitops-server/values.yaml

+  # in order to work with weave-gitops enterprise
+  viewSecrets: []
+
+# This should not be used in production


this will (could optionally if they want to) be used in production, we have hijacked the current implementation for easy dev cycles, but it is not meant for that

I'm unclear as to why this would be used in production, is there a ticket for clarifying/resolving it's use there?

see that thread I linked. the AdminUser feature is a deliberate one for prod.
I have to write up the tickets now

cf. above thread, is there a reason not to change this when those changes are implemented?

you mean when #1787 is done? yep makes sense

Callisto13 · 2022-03-23T10:58:44Z

charts/gitops-server/values.yaml

+  # capabilities:
+  #   drop:
+  #   - ALL
+  # readOnlyRootFilesystem: true


just for context, if users make the pod fs read only, i think the profiles mechanism will fail as it writes a disk-based cache

Is the profiles mechanism supposed to always be available? I was unclear as to whether it was an enterprise only feature. (this is default copy but once its use is clarified I can update & fix this).

See this thread here: https://weaveworks.slack.com/archives/C03244W0C8H/p1646387603804719

(It has no resolution and I am still confused, but just for context)

actually re-reading it does have a resolution, i'll create a ticket to have it behind a flag.
then we can decide what we want to do with it.
i would like it running as a separate service personally.

I'm unsure what it does but it feels like it's separate to what the rest of the ui is (currently) doing

yep 💯 Core v2 has no real concept of what profiles are yet. so let's shuffle it away until they come into scope

Callisto13 · 2022-03-23T11:05:28Z

charts/gitops-server/values.yaml

+  username: not-production-worthy
+  password: seriously-change-me


we should note that these need to be bcrypted not plain-text

is that in addition to base64 encoded to make them k8s secrets?

yes. the code will do this: https://github.com/weaveworks/weave-gitops/blob/main/pkg/server/auth/server.go#L308-L313

(turns out helm has a handy htpasswd function

SamLR · 2022-03-23T12:24:26Z

This work looks awesome. Assuming we are using Helm 3... should we also plan on defining a json schema for the file values.yaml ? (values.schema.json). A couple advantages I see are that we can enforce specific value types and required fields when installing, updating or using template subcommand. Or maybe we want to wait a bit more to let weave-gitops to get a bit more mature? 🤔

I think waiting until the interface is a bit more stable would be best. There are a lot of environment variables and parameters that aren't currently exposed which probably should be (I've not because I'm unsure what they all do and how they interact).

SamLR · 2022-03-23T12:44:30Z

I've created a ticket, #1784, to cover the work in exposing the rest of the various flags & envars that are available to configure the server. @Callisto13 I think this may cover some of the stuff WRT use of the admin user as well?

Callisto13 · 2022-03-23T12:49:37Z

Yep that's fine. We can leave getting the admin user stuff "correct" until we have smoothed out exactly what it is for and what it does.

Helm has a lot of conventions that our existing chart doesn't conform to. Create a new chart using `helm create gitops-server` that things can be ported over to

Set e.g. chart name etc based on values in previous version of the chart

As far as I can imagine the gitops-server should never be serving so much traffic that it needs to autoscale. If it does you probably want to do something custom anyway.

Make sure we have the correct permissions for the service account by default

As the gitops-server is effectively an ops tool it may be desirable to run it with no service resource with access carried out by port-forwarding or via some other mechanism.

We need a test-user. I'm not sure if this should exist in the main chart or be included as a sub-chart but their effectively equivalent and, in theory, as our testing matures this should be something we can remove.

The admin user name & secret location are hard coded into the server config and there are several questions outstanding for how it will be used so, rather than add further confusion by trying to change the name, go back to the original name and make sure that works.

SamLR · 2022-03-23T17:36:52Z

@Callisto13 I've reverted a load of the 'admin user' bits to closer how they were previously because they're hard coded elsewhere. I've tested it all and I don't think I've broken anything

luizbafilho

LGTM

luizbafilho · 2022-03-23T20:10:07Z

charts/gitops-server/README.md

+and specific resources that the service account can impersonate. e.g.
+```yaml
+rbac:
+  create: true


It is not like users have a choice here, they must have impersonation setup otherwise nothing works. So ideally we could have some default values as impersonaionResouces and impersonationResourceNames but allow configuration of those here.

I've improved how the defaults for impersonationResources is set in ca34285

Much as I'd love to provide defaults for impersonationResourceNames I can't think of a good way to do so. The best solution I've come up with would be to create some default groups (e.g. viewer and editor) that are then the only things to be impersonated. The problem with this is that it requires the user's k8s authentication system to be configured to support them. I suspect this is best solved via documenting common processes for it but that feels very out of the scope of this PR (I'm also not 100% sure this is the best way to do it, I'm not sure who might be the best person to cover this sort of security work).

definitely, we can't set anything there since there is no way for us to know what groups the companies use. I'm good with that, the important bit that's allowing configuration is there, it should be enough.

Using `toJson` keeps everything on one line which improves legibility

SamLR force-pushed the helm-upgrades branch from 6cfdeef to e50c6ce Compare March 22, 2022 17:58

SamLR added the type/enhancement New feature or request label Mar 22, 2022

Callisto13 reviewed Mar 23, 2022

View reviewed changes

SamLR requested a review from scottrigby March 23, 2022 11:18

SamLR mentioned this pull request Mar 23, 2022

Expose all production configuration options via the helm chart #1784

Closed

Callisto13 mentioned this pull request Mar 23, 2022

Stick all profiles stuff behind a feature flag #1785

Closed

6 tasks

Sam Cook added 11 commits March 23, 2022 17:23

Initialise a default helm chart

208cca6

Helm has a lot of conventions that our existing chart doesn't conform to. Create a new chart using `helm create gitops-server` that things can be ported over to

Add helm chart readme

67824b4

Import basic values into new chart

f1b8f7a

Set e.g. chart name etc based on values in previous version of the chart

Remove default HPA chart

b23e441

As far as I can imagine the gitops-server should never be serving so much traffic that it needs to autoscale. If it does you probably want to do something custom anyway.

Add chart ClusterRole(binding) resources

8f5c6ee

Make sure we have the correct permissions for the service account by default

Make service optional in helm chart

24dcaaa

As the gitops-server is effectively an ops tool it may be desirable to run it with no service resource with access carried out by port-forwarding or via some other mechanism.

Add test-user to helm chart

88ee418

We need a test-user. I'm not sure if this should exist in the main chart or be included as a sub-chart but their effectively equivalent and, in theory, as our testing matures this should be something we can remove.

Remove old helm chart

4da5a4b

Update tilt file & dev values to use new chart

1be4607

bcrypt hash the test user password

9caefb8

SamLR force-pushed the helm-upgrades branch from 61529b6 to 6a4cf95 Compare March 23, 2022 17:24

luizbafilho approved these changes Mar 23, 2022

View reviewed changes

Use toJson for role resources in helm chart

ca34285

Using `toJson` keeps everything on one line which improves legibility

SamLR merged commit a176104 into main Mar 24, 2022

SamLR deleted the helm-upgrades branch March 24, 2022 13:58

		username: {{ .Values.testUser.username \| b64enc \| quote }}
		password: {{ .Values.testUser.password \| b64enc \| quote }}

		username: not-production-worthy
		password: seriously-change-me

Helm upgrades #1779

Helm upgrades #1779

Conversation

SamLR commented Mar 22, 2022

ozamosi commented Mar 22, 2022

SamLR commented Mar 22, 2022

josecordaz commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Callisto13 Mar 23, 2022 • edited

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

Callisto13 Mar 23, 2022 • edited

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

Callisto13 Mar 23, 2022 • edited

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

Callisto13 Mar 23, 2022 • edited

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

Choose a reason for hiding this comment

SamLR commented Mar 23, 2022

SamLR commented Mar 23, 2022

Callisto13 commented Mar 23, 2022

SamLR commented Mar 23, 2022

luizbafilho 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

Callisto13 Mar 23, 2022 •

edited

Callisto13 Mar 23, 2022 •

edited

Callisto13 Mar 23, 2022 •

edited

Callisto13 Mar 23, 2022 •

edited