Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kube-state-metrics] set parameters for podsecurity restricted #3194

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

jcpunk
Copy link
Contributor

@jcpunk jcpunk commented Apr 5, 2023

What this PR does / why we need it

This permits installing this change in a namespace with PodSecurity set to restricted.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer

Checklist

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

@dotdc
Copy link
Member

dotdc commented Apr 6, 2023

@jcpunk, does this change require kubernetes/kube-state-metrics#2042 to be merged ?

@mrueg, do you know if this is a breaking change?

Not sure if the minor or major version should be bumped, but probably not patch.

@jcpunk
Copy link
Contributor Author

jcpunk commented Apr 6, 2023

This patch does not require the upstream one to be merged. It would be nice to have up there, but not strictly required.

@mrueg
Copy link
Member

mrueg commented Apr 11, 2023

I wouldn't consider this change breaking, but would prefer to raise the minor version as it changes some defaults.

@dotdc
Copy link
Member

dotdc commented Apr 11, 2023

@jcpunk Can you bump the minor version instead of the patch?

Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
@jcpunk
Copy link
Contributor Author

jcpunk commented Apr 11, 2023

Done, and upstream has merged the non-helm permissions update.

@mrueg mrueg merged commit f641bb6 into prometheus-community:main Apr 11, 2023
@jcpunk jcpunk deleted the kube-state-metrics-pss branch April 11, 2023 20:31
@georgekaz
Copy link

georgekaz commented Apr 12, 2023

For me this merge causes:

error validating data: [ValidationError(Deployment.spec.template.spec.securityContext): unknown field "allowPrivilegeEscalation" in io.k8s.api.core.v1.PodSecurityContext, ValidationError(Deployment.spec.template.spec.securityContext): unknown field "capabilities" in io.k8s.api.core.v1.PodSecurityContext]

I believe capabilities is only on container level securitycontext

@kaessert
Copy link

Yes, definitely broken after the merge, seeing the same as @georgekaz

@dotdc
Copy link
Member

dotdc commented Apr 12, 2023

Can you share your respective Kubernetes versions @jcpunk, @georgekaz and @atarax?

Will try to look at this when I can, in the meantime you can use the previous version.

@kaessert
Copy link

1.24 here

@georgekaz
Copy link

1.26 for me

@ramdaspotale
Copy link

it fails on v1.24.9 as well.

dotdc pushed a commit to dotdc/prometheus-community-helm-charts that referenced this pull request Apr 13, 2023
Signed-off-by: David Calvert <david.calvert@oqton.com>
@dotdc dotdc mentioned this pull request Apr 13, 2023
3 tasks
@kaessert
Copy link

it's not related to the version, like @georgekaz mentioned, capabilities is a container-level setting

@ramdaspotale
Copy link

@atarax agreed. and @dotdc i believe chart can be improved to add allowPrivilegeEscalation and capabilities fields to container level specs instead of rolling back changes?

@dotdc
Copy link
Member

dotdc commented Apr 13, 2023

Revert is to unblock the situation, don't have time to dig into the issue right now, but this will be done later.

mrueg pushed a commit that referenced this pull request Apr 13, 2023
Signed-off-by: David Calvert <david.calvert@oqton.com>
jcpunk added a commit to jcpunk/prometheus-helm-charts that referenced this pull request Apr 13, 2023
In theory this fixes the bug introduced in
prometheus-community#3194

Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
@jcpunk
Copy link
Contributor Author

jcpunk commented Apr 13, 2023

In theory I've got another patch that does this the right way now. In practice, they both seem to install on my test cluster, so I suspect my test cluster is super messed up....

#3235

jcpunk added a commit to jcpunk/prometheus-helm-charts that referenced this pull request Apr 14, 2023
In theory this fixes the bug introduced in
prometheus-community#3194

Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
jcpunk added a commit to jcpunk/prometheus-helm-charts that referenced this pull request Apr 14, 2023
In theory this fixes the bug introduced in
prometheus-community#3194

Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
Matiasmct pushed a commit to giffgaff/prometheus-charts that referenced this pull request May 16, 2023
…theus-community#3194)

Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
Co-authored-by: Manuel Rüger <manuel@rueg.eu>
Matiasmct pushed a commit to giffgaff/prometheus-charts that referenced this pull request May 16, 2023
…-community#3233)

Signed-off-by: David Calvert <david.calvert@oqton.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants