charts/turbinia Lock tag by release version, Make ingress more generalized, remove values-production.yaml, README updates, allow multiple instances of Turbinia #167

wajihyassine · 2024-09-17T16:28:16Z

Description of the change

Allow for multiple instantiations of Turbinia on the same cluster by fixing parts of Helm code to name more resources with the Release name
Remove deprecated Turbinia controller
Clean up Ingress specification to allow it to be generalized to other environments (dont make host or IP required)
Remove kube-prometheus from sub dependency as this should remain a seperate steps/so we don't have to download the charts even when not using it
Lock tag by release version
README updates adding instructions on how to update the image tag, upgrading helm chart, moving sections around

Applicable issues

fixes Quality of life updates to OSDFIR Infrastructure charts #170

Checklist

Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
Newly added variables are documented in the values.yaml
Title of the pull request is descriptive

…ixes

Signed-off-by: wajihyassine <[email protected]>

…astructure into turb-updates5

Signed-off-by: wajihyassine <[email protected]>

…e double indentation caused readme-generator to remove content after the table

…astructure into turb-updates5

…e double indentation caused readme-generator to remove content after the table

Signed-off-by: wajihyassine <[email protected]>

…have in doc, remove deprecated controller, remove config.override

Signed-off-by: wajihyassine <[email protected]>

…anup of docs

aarontp

LGTM, just a few minor comments.

aarontp · 2024-09-27T18:43:12Z

charts/turbinia/values-production.yaml

@@ -1,495 +0,0 @@
-## Turbinia Helm Production Values


Is this getting removed to make maintenance easier so we don't have to maintain two versions of this? I don't remember all what is different in this production version, but I do seem to recall there being some parameters that we'd want to use from this in a production install but could be wrong. I'm just wondering if that info will be kept somewhere, or if we should consider keeping a minimal or sparse version of this file with only the differences?

Looking at this file a bit closer, I think most of the values that would be commonly changed for a production instance are probably pretty obvious, and you already have some comments in the README, so I don't think we would need to keep a minimal or sparse version unless there are some other less obvious differences that are recommended (ie. things outside of VM/resource size or autoscaling), and if there is anything else we could just mention it in the README where you have the other related comments.

Yes exactly. And it is to make maintenance easier as it's hard to keep track of the changes done in values.yaml that might also apply to values-production.yaml and we don't test if the deployment works with the values-production.yaml file. Yeah really the biggest difference was the resources defined and autoscaling turned on and filestore (which made it GCP specific).

For the second part expanding the README SGTM. Still thinking about this, but in a second pass, was opting to update the README with the recommended production resource values. Then leave any GKE specific steps in the install GKE instructions (like using GCP filestore for shared storage)

aarontp · 2024-09-27T18:55:51Z

charts/turbinia/README.md

+the Turbinia deployment will automatically retrieve the latest default configs
+from the Turbinia Github repository. This method requires no further action from you.
+
+> **NOTE:**  When using the default method, you cannot update the Turbinia config file directly.


From what I can tell from the instructions below, you can upgrade the helm chart in an existing installation to be able to use a custom config with the config map, is that right? If so, can we add something to the end of this sentence like "... unless you create a config map and upgrade your Helm chart using the instructions below"?

Yep you can. I added "See the next section for instructions on using a custom Turbinia config instead." to make it more clear that there is another option

aarontp · 2024-09-27T19:00:33Z

charts/turbinia/README.md

+1. Update the ConfigMap:
+
+    ```console
+    kubectl create configmap turbinia-configs --from-file=turbinia.conf --dry-run -o yaml | kubectl replace -f -


I think --dry-run might need to be removed here?

Noo it's because without --dry-run it will complain that the configmap exists and there isn't any direct update you can use so you re-create the yaml file with the --dry-run and replace the ConfigMap file containing the turbina config. A little convoluted.. yes

aarontp · 2024-09-27T19:24:57Z

charts/turbinia/README.md

@@ -126,14 +279,14 @@ OAuth client.
    kubectl create secret generic authenticated-emails --from-file=authenticated-emails-list=authenticated-emails.txt
    ```

-8. Then to upgrade an existing release with production values, externally expose
-Turbinia through a load balancer with GCP managed certificates, and deploy the
+8. Then to upgrade an existing, externally expose Turbinia through a load balancer with GCP managed certificates, and deploy the


s/existing/existing release/ ?

nice catch, updated

aarontp · 2024-09-27T19:30:15Z

charts/turbinia/README.md

@@ -146,52 +299,14 @@ Oauth2 Proxy for authentication, run:
 plan to expose Turbinia with a public facing IP, it is highly recommended that
 the Oauth2 Proxy is deployed alongside with the command provided above.


I know this is unchanged from the previous version and should be obvious, but maybe we want to make this statement even stronger by adding something like "... otherwise Turbinia will be accessible from anyone on the internet without authentication"?

Yeah good call out that should be made as clear as possible. I've updated to match that language

wajihyassine and others added 30 commits September 17, 2024 09:26

Remove values-production.yaml, update servicemonitor, documentation f…

7e91f23

…ixes

Update README.md with readme-generator-for-helm

ad36fc0

Signed-off-by: wajihyassine <[email protected]>

Fix checksum configs, service account lookups

caf2823

Merge branch 'turb-updates5' of https://github.com/google/osdfir-infr…

d6dd461

…astructure into turb-updates5

update readme

5021284

Update README.md with readme-generator-for-helm

3052782

Signed-off-by: wajihyassine <[email protected]>

Update README

f277852

Update README.md with readme-generator-for-helm

81707b9

Signed-off-by: wajihyassine <[email protected]>

Try again

76bb25c

Update README.md with readme-generator-for-helm

75a2c47

Signed-off-by: wajihyassine <[email protected]>

Try again

5bba1a0

And again

775dbb5

Update README.md with readme-generator-for-helm

b5880e9

Signed-off-by: wajihyassine <[email protected]>

And again

9f468d5

Update README.md with readme-generator-for-helm

2546468

Signed-off-by: wajihyassine <[email protected]>

And again

7dd0f73

Update README.md with readme-generator-for-helm

62dd2bc

Signed-off-by: wajihyassine <[email protected]>

Final fix hopefully for README lint errors by updating the headers th…

83b1e67

…e double indentation caused readme-generator to remove content after the table

Merge branch 'turb-updates5' of https://github.com/google/osdfir-infr…

0ec00d1

…astructure into turb-updates5

Final fix hopefully for README lint errors by updating the headers th…

6069602

…e double indentation caused readme-generator to remove content after the table

Update README.md with readme-generator-for-helm

111ed10

Signed-off-by: wajihyassine <[email protected]>

Update README

6de2817

Partially add back section to see if it will get deleted

ee523bf

Update README.md with readme-generator-for-helm

6f0fed5

Signed-off-by: wajihyassine <[email protected]>

Try again

d962fd3

Update README.md with readme-generator-for-helm

99185c9

Signed-off-by: wajihyassine <[email protected]>

Try again

0878789

Update README.md with readme-generator-for-helm

b23e4f2

Signed-off-by: wajihyassine <[email protected]>

Try again

ded44d1

Try again

039def3

wajihyassine and others added 8 commits September 19, 2024 22:04

Update README.md with readme-generator-for-helm

b815f3b

Signed-off-by: wajihyassine <[email protected]>

Try again

97c5491

Fix README

e714e5a

update README

a90a5ae

Update term grace period

a3fb4e1

Update ingress to be more generic, remove monitoring to instead just …

bd78b8c

…have in doc, remove deprecated controller, remove config.override

Update README.md with readme-generator-for-helm

a4568ef

Signed-off-by: wajihyassine <[email protected]>

Update monitoring documentation

a983c86

wajihyassine self-assigned this Sep 23, 2024

wajihyassine and others added 2 commits September 23, 2024 10:36

Lock by Turbinia release version

b34b28f

Update README.md with readme-generator-for-helm

6709e69

Signed-off-by: wajihyassine <[email protected]>

wajihyassine changed the title ~~Remove values-production.yaml, update servicemonitor, documentation f…~~ charts/turbinia Remove values-production.yaml, update servicemonitor, documentation updates, allow multiple instances of Turbinia Sep 23, 2024

wajihyassine and others added 2 commits September 24, 2024 13:46

Disable ingress

d1c2483

Update README.md with readme-generator-for-helm

0d4d5f4

Signed-off-by: wajihyassine <[email protected]>

wajihyassine marked this pull request as ready for review September 24, 2024 23:08

wajihyassine requested a review from hacktobeer September 24, 2024 23:08

wajihyassine added 2 commits September 25, 2024 13:33

Update README with upgrade instructions, image update and general cle…

8bb0334

…anup of docs

Markdown lint fix

ebce5fa

wajihyassine requested review from aarontp and removed request for hacktobeer September 27, 2024 18:02

aarontp approved these changes Sep 27, 2024

View reviewed changes

Update README review comments

ce9b205

wajihyassine merged commit 4965d07 into main Sep 27, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

charts/turbinia Lock tag by release version, Make ingress more generalized, remove values-production.yaml, README updates, allow multiple instances of Turbinia #167

charts/turbinia Lock tag by release version, Make ingress more generalized, remove values-production.yaml, README updates, allow multiple instances of Turbinia #167

wajihyassine commented Sep 17, 2024 •

edited

Loading

aarontp left a comment

aarontp Sep 27, 2024

aarontp Sep 27, 2024

wajihyassine Sep 27, 2024

aarontp Sep 27, 2024

wajihyassine Sep 27, 2024

aarontp Sep 27, 2024

wajihyassine Sep 27, 2024

aarontp Sep 27, 2024

wajihyassine Sep 27, 2024

aarontp Sep 27, 2024

wajihyassine Sep 27, 2024 •

edited

Loading

		@@ -146,52 +299,14 @@ Oauth2 Proxy for authentication, run:
		plan to expose Turbinia with a public facing IP, it is highly recommended that
		the Oauth2 Proxy is deployed alongside with the command provided above.

charts/turbinia Lock tag by release version, Make ingress more generalized, remove values-production.yaml, README updates, allow multiple instances of Turbinia #167

charts/turbinia Lock tag by release version, Make ingress more generalized, remove values-production.yaml, README updates, allow multiple instances of Turbinia #167

Conversation

wajihyassine commented Sep 17, 2024 • edited Loading

Description of the change

Applicable issues

Checklist

aarontp 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

wajihyassine Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

wajihyassine commented Sep 17, 2024 •

edited

Loading

wajihyassine Sep 27, 2024 •

edited

Loading