[prometheus]: Add possibility to disable securityContext for Prometheus and Alertmanager workloads #2595

CarpathianUA · 2022-10-20T22:32:12Z

Add possibility to disable securityContext for Prometheus and Alertmanager workloads

Signed-off-by: Roman Slipchenko romanslipchenko@gmail.com

What this PR does / why we need it:

Add the possibility to disable security context for Prometheus and Alertmanager workloads for smooth deployment to OpenShift.
If securityContext is omitted, OpenShift SCC Admission controller can inject needed securityContext on chart deploy time.
More details - https://docs.openshift.com/container-platform/4.11/authentication/managing-security-context-constraints.html

Which issue this PR fixes

None

Special notes for your reviewer

Checklist

DCO signed
Chart Version bumped
Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

…ager workloads Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

monotek

please remove all unrealted format changes in the values file.
also new values needs to be in values file.

charts/prometheus/Chart.yaml

…ager workloads - fix formatting, add default values Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

…ager workloads - fix Chart version Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

charts/prometheus/templates/alertmanager/deploy.yaml

charts/prometheus/templates/alertmanager/sts.yaml

charts/prometheus/templates/server/deploy.yaml

charts/prometheus/templates/server/sts.yaml

CarpathianUA · 2022-10-24T14:01:56Z

@monotek requested changes have been applied, would appreciate a review from your side, thanks in advance!

charts/prometheus/values.yaml

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

ericpdurand · 2022-10-25T14:27:12Z

That's fine with me.
I'm wondering if the tests with "{{- if .Values.alertmanager.containerSecurityContext }}" could be simply dropped as if the content is empty there is nothing inserted.
That would be less code, more readable.
I let you decide

CarpathianUA · 2022-10-25T17:39:38Z

That's fine with me. I'm wondering if the tests with "{{- if .Values.alertmanager.containerSecurityContext }}" could be simply dropped as if the content is empty there is nothing inserted. That would be less code, more readable. I let you decide

@ericpdurand Yeah, I left it explicitly because of the known bug in Helm.
Due to that bug, and the inability to drop the map from the template via setting it's value to null, and without check for non-empty map ({{- if .Values.alertmanager.containerSecurityContext }}) Helm will render template exactly as containerSecurityContext: {}, and OpenShift will fail security constraint context (SCC) validation on that field. With these two conditions, we can avoid rendering of securityContext map at all, so OpenShift's SCC admission will pass successfully.

CarpathianUA · 2022-10-25T17:41:20Z

@zanhsieh @monotek
Please kindly review the final version of the changes, thanks in advance! I really hope that the PR will be merged soon :)

charts/prometheus/templates/alertmanager/deploy.yaml

charts/prometheus/templates/alertmanager/sts.yaml

monotek · 2022-10-26T09:44:30Z

charts/prometheus/templates/alertmanager/sts.yaml

@@ -111,8 +115,12 @@ spec:
        - name: {{ template "prometheus.name" . }}-{{ .Values.alertmanager.name }}-{{ .Values.configmapReload.alertmanager.name }}
          image: "{{ .Values.configmapReload.alertmanager.image.repository }}:{{ .Values.configmapReload.alertmanager.image.tag }}"
          imagePullPolicy: "{{ .Values.configmapReload.alertmanager.image.pullPolicy }}"
+          {{- if .Values.configmapReload.alertmanager.containerSecurityContextEnabled }}


superfluous too. see above.

charts/prometheus/templates/server/deploy.yaml

charts/prometheus/templates/server/sts.yaml

monotek · 2022-10-26T09:46:28Z

@zanhsieh
please check more carefully before approving stuff like this.
change seems not usefull to me.

CarpathianUA · 2022-10-26T09:49:01Z

@zanhsieh please check more carefully before approving stuff like this. change seems not usefull to me.

@monotek What's your concern here?

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA · 2022-10-26T10:58:34Z

@monotek @zanhsieh @ericpdurand Your requested changes have been applied. Please kindly review.

zanhsieh · 2022-10-26T14:21:08Z

@zanhsieh please check more carefully before approving stuff like this. change seems not usefull to me.

@monotek
My apologies. I already addressed my concerns previously, but the original author didn't think those are issues as fitting previous contributors format (mainly battle between with and if). So I thought just let him functionally pass maybe I make another PR myself to tidy up.

CarpathianUA · 2022-10-26T14:24:32Z

@zanhsieh please check more carefully before approving stuff like this. change seems not usefull to me.

@monotek My apologies. I already addressed my concerns previously, but the original author didn't think those are issues as fitting previous contributors format (mainly battle between with and if). So I thought just let him functionally pass maybe I make another PR myself to tidy up.

@zanhsieh
@monotek 's suggested changes were applied - with won here :) I think there is no need for another PR

zanhsieh · 2022-10-26T14:34:44Z

@zanhsieh please check more carefully before approving stuff like this. change seems not usefull to me.

@monotek My apologies. I already addressed my concerns previously, but the original author didn't think those are issues as fitting previous contributors format (mainly battle between with and if). So I thought just let him functionally pass maybe I make another PR myself to tidy up.

@zanhsieh @monotek 's suggested changes were applied - with won here :) I think there is no need for another PR

With with, if is not needed.

…p is non-empty Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA · 2022-10-26T14:56:36Z

@zanhsieh please check more carefully before approving stuff like this. change seems not usefull to me.

@monotek My apologies. I already addressed my concerns previously, but the original author didn't think those are issues as fitting previous contributors format (mainly battle between with and if). So I thought just let him functionally pass maybe I make another PR myself to tidy up.

@zanhsieh @monotek 's suggested changes were applied - with won here :) I think there is no need for another PR

With with, if is not needed.

Just pushed changes for that

charts/prometheus/templates/alertmanager/deploy.yaml

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

naseemkullah

lgtm, thanks @CarpathianUA

CarpathianUA · 2022-10-26T15:54:45Z

@monotek Please kindly review, thanks in advance!

…us and Alertmanager workloads (prometheus-community#2595) * Add possbility to disable securityContext for Prometheus and Alertmanager workloads Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Add possbility to disable securityContext for Prometheus and Alertmanager workloads - fix formatting, add default values Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Add possbility to disable securityContext for Prometheus and Alertmanager workloads - fix Chart version Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Add condition to disable containerSecurityContext Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Remove redundant containerSecurityContextEnabled variable and condtition Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Remove redundant if condition for check if containerSecurtyContext map is non-empty Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Remove redundant if condition with omit func in vafour of with iterator Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> * Remove redundant if condition Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com> Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

Add possbility to disable securityContext for Prometheus and Alertman…

f2c2fa5

…ager workloads Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA requested review from gianrubio, naseemkullah, Xtigyro and zanhsieh as code owners October 20, 2022 22:32

monotek requested changes Oct 21, 2022

View reviewed changes

charts/prometheus/Chart.yaml Outdated Show resolved Hide resolved

CarpathianUA added 2 commits October 21, 2022 18:18

Add possbility to disable securityContext for Prometheus and Alertman…

d31daa9

…ager workloads - fix formatting, add default values Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

Add possbility to disable securityContext for Prometheus and Alertman…

047beae

…ager workloads - fix Chart version Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA requested a review from monotek October 21, 2022 15:28

zanhsieh requested changes Oct 21, 2022

View reviewed changes

CarpathianUA requested review from zanhsieh and monotek and removed request for monotek and zanhsieh October 22, 2022 11:46

zanhsieh previously approved these changes Oct 22, 2022

View reviewed changes

CarpathianUA requested a review from monotek October 22, 2022 19:13

ericpdurand reviewed Oct 25, 2022

View reviewed changes

charts/prometheus/values.yaml Outdated Show resolved Hide resolved

Add condition to disable containerSecurityContext

cd4b82e

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA dismissed zanhsieh’s stale review via cd4b82e October 25, 2022 12:09

CarpathianUA requested review from ericpdurand and monotek and removed request for monotek and ericpdurand October 25, 2022 12:11

ericpdurand previously approved these changes Oct 25, 2022

View reviewed changes

zanhsieh previously approved these changes Oct 26, 2022

View reviewed changes

monotek requested changes Oct 26, 2022

View reviewed changes

Remove redundant containerSecurityContextEnabled variable and condtition

34efe4e

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA dismissed stale reviews from zanhsieh and ericpdurand via 34efe4e October 26, 2022 10:57

CarpathianUA requested review from zanhsieh and ericpdurand and removed request for zanhsieh and ericpdurand October 26, 2022 10:58

Remove redundant if condition for check if containerSecurtyContext ma…

5320328

…p is non-empty Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

naseemkullah reviewed Oct 26, 2022

View reviewed changes

charts/prometheus/templates/alertmanager/deploy.yaml Outdated Show resolved Hide resolved

CarpathianUA added 2 commits October 26, 2022 18:28

Remove redundant if condition with omit func in vafour of with iterator

c367000

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

Remove redundant if condition

24406d3

Signed-off-by: Roman Slipchenko <romanslipchenko@gmail.com>

CarpathianUA requested review from naseemkullah and removed request for zanhsieh October 26, 2022 15:34

naseemkullah approved these changes Oct 26, 2022

View reviewed changes

ericpdurand approved these changes Oct 26, 2022

View reviewed changes

monotek approved these changes Oct 26, 2022

View reviewed changes

monotek merged commit 8d06d6d into prometheus-community:main Oct 26, 2022

cm0s mentioned this pull request Dec 14, 2022

argo-repo-server issue: gpg ... --gen-key failed exit status 2 argoproj/argo-cd#9809

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[prometheus]: Add possibility to disable securityContext for Prometheus and Alertmanager workloads #2595

[prometheus]: Add possibility to disable securityContext for Prometheus and Alertmanager workloads #2595

CarpathianUA commented Oct 20, 2022 •

edited

monotek left a comment

CarpathianUA commented Oct 24, 2022 •

edited

ericpdurand commented Oct 25, 2022

CarpathianUA commented Oct 25, 2022 •

edited

CarpathianUA commented Oct 25, 2022

monotek Oct 26, 2022

monotek commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022 •

edited

zanhsieh commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

zanhsieh commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

naseemkullah left a comment

CarpathianUA commented Oct 26, 2022

[prometheus]: Add possibility to disable securityContext for Prometheus and Alertmanager workloads #2595

[prometheus]: Add possibility to disable securityContext for Prometheus and Alertmanager workloads #2595

Conversation

CarpathianUA commented Oct 20, 2022 • edited

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer

Checklist

monotek left a comment

Choose a reason for hiding this comment

CarpathianUA commented Oct 24, 2022 • edited

ericpdurand commented Oct 25, 2022

CarpathianUA commented Oct 25, 2022 • edited

CarpathianUA commented Oct 25, 2022

monotek Oct 26, 2022

Choose a reason for hiding this comment

monotek commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022 • edited

zanhsieh commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

zanhsieh commented Oct 26, 2022

CarpathianUA commented Oct 26, 2022

naseemkullah left a comment

Choose a reason for hiding this comment

CarpathianUA commented Oct 26, 2022

CarpathianUA commented Oct 20, 2022 •

edited

CarpathianUA commented Oct 24, 2022 •

edited

CarpathianUA commented Oct 25, 2022 •

edited

CarpathianUA commented Oct 26, 2022 •

edited