convert cs deployment to helm chart #922

tony-schndr · 2024-12-03T19:29:04Z

What this PR does

Fixes https://issues.redhat.com/browse/ARO-11949, https://issues.redhat.com/browse/ARO-12801, & https://issues.redhat.com/browse/ARO-13258
Jira:
Link to demo recording:

Special notes for your reviewer

github-actions · 2024-12-04T14:41:27Z

Please rebase pull request.

cluster-service/deploy/helm/templates/azure-runtime-config.configmap.yaml

github-actions · 2024-12-09T10:32:45Z

Please rebase pull request.

janboll · 2024-12-10T16:19:28Z

config/config.yaml

+        azureOperatorsManagedIdentitiesConfig: |
+          controlPlaneOperatorsIdentities:
+            cloud-controller-manager:
+              minOpenShiftVersion: 4.17


Will this differ from Env to Env? Or will this see progressive updates? Just asking, cause this large key might make the config harder to read then (especially when thinking about later rollouts with more envs).

It will differ from env to env and it may see updates if CS needs to add an identity.. I don't like it but I was having a hard time templatizing this object and making it work nice with config.mk. The problem I hit was that templatize was not creating a valid multi line string in the config.mk. To work around this I treat it as a string and base64 encode it so that it becomes a single line in config.mk and decode it in the helm template.

cluster-service/deploy/openshift-templates/arohcp.devenv.md

geoberle · 2024-12-10T18:33:25Z

cluster-service/config.tmpl.mk


 DEVOPS_MSI_ID ?= {{ .aroDevopsMsiId }}

 # MGMT CLUSTER KVs
 MGMT_RESOURCEGROUP ?= {{ .mgmt.rg }}
 CX_SECRETS_KV_NAME ?= {{ .cxKeyVault.name }}
 CX_MI_KV_NAME ?= {{ .msiKeyVault.name }}
+
+AZURE_OPERATORS_MANAGED_IDENTITIES_CONFIG ?= {{ .clusterService.azureOperatorsManagedIdentitiesConfig | b64enc }}


i'm not sure how well this will translate to EV2 env vars.
but let's not block this PR because of that. we can revisite later when we deploy CS to INT.

btw. this has nothing to do with the translation to helm. we would have faced the same issue with oc templates.

geoberle

LGTM
let's tackle the EV2 related comments next sprint. this chart works nicely on DEV

machi1990 · 2024-12-12T08:53:22Z

cluster-service/deploy/helm/values.yaml

+databasePort: "5432"
+
+# The name of the managed identities data plane audience resource.
+managedIdentitiesDataPlaneAudienceResource: "https://dummy.org"


One constraint we've to take onto account regarding this field's value is that we cannot check it in code for higher envs (stage/prod). So when we get to those envs, please make sure that this is not checked in. @tony-schndr

What should the values for those environments be?

We don't know yet, but whatever the value of them, they shouldn't be checked in any repo.

To be specific,
The dev and int envs value can be any dummy https url; this can be checked in the repo.
For stage and prod envs, these will be real value of the msi dataplane. They can't be checked in any repo.

machi1990 · 2024-12-12T14:48:58Z

cluster-service/deploy/helm/values.yaml

+# TODO: This parameter isn't currently used, but kept to avoid failures in the
+# execution of saasherder. It will be removed once the version of the service
+# that doesn't use it is deployed to all environments.
+debugPort: ""


Where is this used?

machi1990 · 2024-12-12T14:49:40Z

cluster-service/deploy/helm/values.yaml

+# Duration since cluster creation after which the first notification for stale cluster should be sent.
+firstStaleClusterNotification: "24h"  # 1 day
+
+# Duration since cluster creation after which the second notification for stale cluster should be sent.
+secondStaleClusterNotification: "600h"  # 25 days
+
+# Duration after which a stale cluster can be cleaned up.
+staleClusterAutocleanupWindow: "720h"  # 30 days


These don't apply to aro-hcp.
They can be deleted.

machi1990 · 2024-12-12T14:51:46Z

cluster-service/deploy/helm/values.yaml

+# Period between executions of day-1 machine pool migration worker. Useful time units are "m" or "h".
+machinePoolMigrationWorkerPeriod: "1h"
+
+# The name of the DNS base domain for creating a user defined domains.
+# Note: this is defaulted to a commercial value. This should be overriden in fedramp app-interface
+userDefinedDnsBaseDomain: "i1.devshift.org"


These two values don't apply in aro-hcp

machi1990 · 2024-12-12T14:55:43Z

cluster-service/deploy/helm/templates/deployment.yaml

+        - --gateway-url={{ .Values.gatewayURL }}
+        - --client-id=@/secrets/service/client.id
+        - --client-secret=@/secrets/service/client.secret
+        - --client-scopes={{ .Values.clientScopes }}
+        - --user-defined-dns-base-domain={{ .Values.userDefinedDnsBaseDomain }}
+        - --jwks-url={{ .Values.jwksUrl }}
+        - --jwks-file=/configs/authentication/jwks.json
+        - --acl-file=/configs/authentication/acl.yml
+        - --token-url={{ .Values.tokenUrl }}
+        - --insecure={{ .Values.insecure }}


These flags are perhaps not needed for aro-hcp.

We can clean up later.

machi1990 · 2024-12-12T14:56:17Z

cluster-service/deploy/helm/templates/deployment.yaml

+        - serve
+        - --log-level={{ .Values.logLevel }}
+        - --namespace={{ .Release.Namespace }}
+        - --runtime-mode={{ .Values.runtimeMode }}


We could hardcode the mode here as we are in the context of aro-hcp.

Suggested change

- --runtime-mode={{ .Values.runtimeMode }}

- --runtime-mode=aro-hcp

machi1990 · 2024-12-12T14:57:13Z

cluster-service/deploy/helm/templates/deployment.yaml

+        - --default-expiration={{ .Values.defaultExpiration }}
+        - --maximum-expiration={{ .Values.maximumExpiration }}


Suggested change

- --default-expiration={{ .Values.defaultExpiration }}

- --maximum-expiration={{ .Values.maximumExpiration }}

Probably not applicable for aro-hcp, but we can cleanup later on.

machi1990 · 2024-12-12T15:02:46Z

cluster-service/deploy/helm/templates/deployment.yaml

+        - --backplane-url={{ .Values.backplaneURL }}
+        - --provision-shards-config=/secrets/shards/config
+        - --proxy-config-file=/configs/proxy/config.yaml
+        - --aws-sts-policy-directory=/configs/policies


This flag isn't applicable for aro-hcp.
We can clean up later;
At the moment if we don't define it, CS starts normally but we'll get a

2024-12-12T15:00:50Z ERROR loader/aws_sts_policy_loader.go:50 [opid='dc2b7851-64e2-4869-b48a-d4a21c5a407a'] Got error while trying to load policies: open : no such file or directory gitlab.cee.redhat.com/service/uhc-clusters-service/pkg/aws/loader.(*AWSPolicyLoader).run /cs/pkg/aws/loader/aws_sts_policy_loader.go:50 gitlab.cee.redhat.com/service/uhc-clusters-service/pkg/aws/loader.(*AWSPolicyLoader).Start cs/pkg/aws/loader/aws_sts_policy_loader.go:43 gitlab.cee.redhat.com/service/uhc-clusters-service/cmd/clusters-service/servecmd.runServ

error message at start time, which can be confusing.

machi1990

left cleanup comments that can be addressed later on.
Many of the flags may not directly apply to aro-hcp.

machi1990 · 2024-12-12T15:12:18Z

cluster-service/deploy/helm/values.yaml

+clientScopes: "openid"
+
+# Environment associated with this instance.
+environment: "aro-hcp-dev"


@geoberle @tony-schndr A note on the environment name.
It shouldn't be longer than 10 chars.
Otherwise, it breaks the cluster's base domain prefix limit.

For example "aro-hcp-dev" is 11 chars, this means that the cluster's base domain prefix can't be longer than 14 chars while CS allows for it to be <= 15 chars.

Also worth noting that, this field is immutable and it is used in multiple other places that are critical to CS.
To change it, it implies re-creating the env with a new name.

I will address this in a followup. We can enforce the length in the config json schema.

tony-schndr force-pushed the cluster-service-helm-chart branch 2 times, most recently from 090ef00 to bc89279 Compare December 4, 2024 14:36

github-actions bot added the needs-rebase label Dec 4, 2024

machi1990 reviewed Dec 4, 2024

View reviewed changes

cluster-service/deploy/helm/templates/azure-runtime-config.configmap.yaml Outdated Show resolved Hide resolved

tony-schndr force-pushed the cluster-service-helm-chart branch 2 times, most recently from 6c9d7e3 to a7643b7 Compare December 4, 2024 18:49

github-actions bot added needs-rebase and removed needs-rebase labels Dec 4, 2024

tony-schndr force-pushed the cluster-service-helm-chart branch from a7643b7 to 15c3c9c Compare December 9, 2024 16:31

github-actions bot removed the needs-rebase label Dec 9, 2024

tony-schndr force-pushed the cluster-service-helm-chart branch 7 times, most recently from 88cef58 to 0fb42dc Compare December 9, 2024 22:11

tony-schndr marked this pull request as ready for review December 9, 2024 22:18

tony-schndr requested review from bennerv, geoberle, janboll, weinong, whober0521, jharrington22, SudoBrendan, UlrichSchlueter and zgalor as code owners December 9, 2024 22:18

tony-schndr force-pushed the cluster-service-helm-chart branch from 2f2a25e to fbff667 Compare December 10, 2024 14:03

tony-schndr force-pushed the cluster-service-helm-chart branch 3 times, most recently from 40a0dcf to 9f209f5 Compare December 10, 2024 15:26

tony-schndr added 2 commits December 10, 2024 09:58

convert cs deployment to helm chart

dc8d0d2

ignore yaml linting

86c3852

tony-schndr force-pushed the cluster-service-helm-chart branch from 9f209f5 to 86c3852 Compare December 10, 2024 15:58

janboll reviewed Dec 10, 2024

View reviewed changes

cluster-service/deploy/openshift-templates/arohcp.devenv.md Outdated Show resolved Hide resolved

geoberle reviewed Dec 10, 2024

View reviewed changes

restore documentation

1454e8d

geoberle approved these changes Dec 11, 2024

View reviewed changes

machi1990 reviewed Dec 12, 2024

View reviewed changes

machi1990 approved these changes Dec 12, 2024

View reviewed changes

machi1990 reviewed Dec 12, 2024

View reviewed changes

tony-schndr merged commit ea5be9f into main Dec 13, 2024
9 checks passed

tony-schndr deleted the cluster-service-helm-chart branch December 13, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert cs deployment to helm chart #922

convert cs deployment to helm chart #922

tony-schndr commented Dec 3, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 9, 2024

janboll Dec 10, 2024

tony-schndr Dec 10, 2024

geoberle Dec 10, 2024

geoberle left a comment

machi1990 Dec 12, 2024

tony-schndr Dec 12, 2024

machi1990 Dec 12, 2024 •

edited

Loading

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

machi1990 left a comment

machi1990 Dec 12, 2024

machi1990 Dec 12, 2024

tony-schndr Dec 13, 2024

	- --runtime-mode={{ .Values.runtimeMode }}
	- --runtime-mode=aro-hcp

		- --default-expiration={{ .Values.defaultExpiration }}
		- --maximum-expiration={{ .Values.maximumExpiration }}

convert cs deployment to helm chart #922

convert cs deployment to helm chart #922

Conversation

tony-schndr commented Dec 3, 2024 • edited Loading

What this PR does

Special notes for your reviewer

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoberle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machi1990 Dec 12, 2024 • edited Loading

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

machi1990 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

tony-schndr commented Dec 3, 2024 •

edited

Loading

machi1990 Dec 12, 2024 •

edited

Loading