flexibility in using Helm chart: args, env, probes & better security and namespaces usage #126

laimison · 2023-03-21T15:29:03Z

Hi,

I have added more ways to use the chart. Those ways are optional and don't change the way templates are rendered now, except:
this block bellow is added by default on Deployment - it improves security:

          securityContext:
            allowPrivilegeEscalation: false
            capabilities:
              drop:
              - ALL
            privileged: false
            runAsGroup: 1000
            runAsUser: 1000

Also, missing parameters for probes are added.

Let me know, if PR looks good or needs adjustments to be agreed eg not forcing better security in values.yaml file at deployment.securityContext.

I have tested this on cluster.

I would like to add these improvements to be able to use this chart and depend on it. So I hope flexibilities and improvements can help other people too. It would be great to have helm chart released.

Let me know your thoughts and also things I should be aware of.

Thanks

…and namespaces usage

bakito · 2023-03-21T16:01:24Z

@laimison thank you very much for the PR.

I agree with your demand of being able to have more flexibility, but I have a simpler approach than mapping evry single property.

for example defining proves and security context in the values and importing the whole yaml into the deployment.
This give even more flexibility, than mapping each value separately

        {{- with .Values.deployment.livenessProbe }}
          livenessProbe:
            {{- toYaml . | nindent 12 }}
          {{- end }}

bakito · 2023-03-21T16:02:38Z

chart/templates/deployment.yaml

@@ -1,3 +1,4 @@
+{{ if .Values.deployment.create }}


why ould you not want to create the deployment, when you install this chart?

Thanks for suggestions, these are great. I will amend PR once have a moment.

In terms of this item, I just added it, in case people find something not flexible enough as me. They can use their Deployment and submit PR here. So there is no gap in waiting when chart can offer what they need.
If you want, I can exclude this change. By default, deployment has to be created based on values.yaml.

I'd rader have this conditon removed as I do not see the use case.

But anyway thank you very much for your initiative. I'm looking forward to the update.

Don,'t forget to run make docs before pushing to have the readme updated.

Changes have been submitted and PR is ready for review. Please check if you want to have limit-rpm - requests limit per IP on Ingress and if version increase is okay. Nothing else additional.

bakito · 2023-03-21T16:03:53Z

chart/templates/deployment.yaml

          ports:
            - name: http
              containerPort: 8080
              protocol: TCP
-          livenessProbe:
+          readinessProbe:
+            failureThreshold: {{ .Values.deployment.readinessProbe.failureThreshold }}


I prefer having the whole probe config in the values and include the whole yaml as mentioned in the comment

bakito · 2023-03-21T16:04:06Z

chart/templates/deployment.yaml

            httpGet:
              path: /_health
              port: http
-          readinessProbe:
+          livenessProbe:
+            failureThreshold: {{ .Values.deployment.livenessProbe.failureThreshold }}


bakito · 2023-03-21T16:04:14Z

chart/templates/deployment.yaml

            httpGet:
              path: /_health
              port: http
+          {{- if .Values.deployment.securityContext }}
+          securityContext:
+            {{- if .Values.deployment.securityContext.allowPrivilegeEscalation }}


bakito · 2023-03-21T16:06:42Z

chart/values.yaml

-  #    - path: /ssw(/|$)(.*)
-  #      pathType: Prefix
+  hosts:
+  - host: example.internal


I agree with changing the example, but would not set a default host in the values.

bakito · 2023-03-21T16:08:27Z

chart/templates/deployment.yaml

+          {{- end }}
+          {{- if .Values.deployment.env }}
+          env:
+          {{- if .Values.deployment.env.SEALEDSECRETSCONTROLLERNAMESPACE }}


use better readable camelCase keys for the env variable in the values it only need to be uppercase in the variable name, not the helm values.

…metic changes & limit-rpm added to an ingress

bakito · 2023-03-24T06:09:49Z

chart/values.yaml

-  # nginx.ingress.kubernetes.io/ssl-redirect: 'true'
+  annotations:
+    # -- Specifies number of requests accepted from a given IP each minute
+    nginx.ingress.kubernetes.io/limit-rpm: "180"


could you leave this empty default?
You can keep the rpm commented out.

addressed, thanks

bakito · 2023-03-24T06:11:35Z

chart/values.yaml

+      - ALL
+    privileged: false
+    runAsGroup: 1000
+    runAsUser: 1000


The Dockerfile runs with user 1001 https://github.com/bakito/sealed-secrets-web/blob/main/Dockerfile#L32
Can you change this also to the same value?

addressed, thanks

bakito · 2023-03-24T06:17:09Z

chart/values.yaml

+    failureThreshold: 3
+    successThreshold: 1
+    timeoutSeconds: 10
+    initialDelaySeconds: 15


bakito · 2023-03-24T06:25:40Z

chart/values.yaml

+    periodSeconds: 5
+    successThreshold: 1
+    timeoutSeconds: 10
+    initialDelaySeconds: 30


can you remove this part?

failureThreshold: 3 periodSeconds: 5 successThreshold: 1 timeoutSeconds: 10 initialDelaySeconds: 30

30 sec us much too high for this application as it starts fast.

The kubernetes default should be enough
https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#configure-probes

If someone needs it changed, it can be overwritten in the values.

addressed and PR is for review now, many thanks

…and namespaces usage

bakito · 2023-03-27T18:31:33Z

@laimison thank you very much for your contribution

laimison · 2023-03-28T10:59:13Z

@bakito thanks for your time and merge. Let me know when/if there are some plans to release new version of this chart so I believe it will be listed using command helm search repo bakito/sealed-secrets-web. Cheers.

bakito · 2023-03-28T12:03:45Z

@laimison I released 3.0.6 yesterday https://artifacthub.io/packages/helm/bakito/sealed-secrets-web

laimison · 2023-03-28T14:33:24Z

@bakito perfect, thank you!

laimison added 2 commits March 21, 2023 16:12

flexibility in using Helm chart: args, env, probes & better security …

a5c6f5c

…and namespaces usage

flexibility in using Helm chart: args, env, probes & better security …

02a1808

…and namespaces usage

bakito requested changes Mar 21, 2023

View reviewed changes

Helm: securityContext and probes fully managed from values file & cos…

63c4044

…metic changes & limit-rpm added to an ingress

bakito requested changes Mar 24, 2023

View reviewed changes

bakito reviewed Mar 24, 2023

View reviewed changes

laimison added 2 commits March 26, 2023 17:10

flexibility in using Helm chart: args, env, probes & better security …

a436550

…and namespaces usage

flexibility in using Helm chart: args, env, probes & better security …

c9c064b

…and namespaces usage

bakito merged commit 6d3a6cc into bakito:main Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flexibility in using Helm chart: args, env, probes & better security and namespaces usage #126

flexibility in using Helm chart: args, env, probes & better security and namespaces usage #126

laimison commented Mar 21, 2023

bakito commented Mar 21, 2023

bakito Mar 21, 2023

laimison Mar 22, 2023

bakito Mar 22, 2023

laimison Mar 23, 2023

bakito Mar 21, 2023

bakito Mar 21, 2023

bakito Mar 21, 2023

bakito Mar 21, 2023

bakito Mar 21, 2023

bakito Mar 24, 2023 •

edited

Loading

laimison Mar 26, 2023

bakito Mar 24, 2023

laimison Mar 26, 2023

bakito Mar 24, 2023

bakito Mar 24, 2023

laimison Mar 26, 2023

bakito commented Mar 27, 2023

laimison commented Mar 28, 2023

bakito commented Mar 28, 2023

laimison commented Mar 28, 2023

flexibility in using Helm chart: args, env, probes & better security and namespaces usage #126

flexibility in using Helm chart: args, env, probes & better security and namespaces usage #126

Conversation

laimison commented Mar 21, 2023

bakito commented Mar 21, 2023

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

bakito Mar 24, 2023 • 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

bakito commented Mar 27, 2023

laimison commented Mar 28, 2023

bakito commented Mar 28, 2023

laimison commented Mar 28, 2023

bakito Mar 24, 2023 •

edited

Loading