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

Add option to configure securityContext on both pods and containers #1666

Closed
wants to merge 2 commits into from

Conversation

Kajot-dev
Copy link
Contributor

@Kajot-dev Kajot-dev commented Dec 22, 2023

Fixes: #1253
Fixes: #1660

Why is it needed?

With the introduction of PodSecurityStandard (see https://kubernetes.io/docs/concepts/security/pod-security-standards/) in Kubernetes, users are able to enforce stricter security policies. In addition to that security-hardened distributions of k8s (like Openshift/OKD) changes the default behavior in 4.12+ (due to the introduction of mentioned PSP and integration with existing SecurityContextConstraints) and requires more values in securityContexts to be defined.

Because of this, it's becoming impossible for users with stricter security to deploy the chart without forking it.

What is does?

  • Adds option to configure securityContexts through the chart
  • Does not change the default behavior (except for permission initContainer in internal database deployment, see below)
  • Has a clear indication that changing the user id/fs group is not supported by official images and may break permissions
  • Makes data-permissions-ensurer initContainer in internal database deploy always run as a root, since it should be able to use chmod (if uid via podSecurityContext was set to 999 before, was it even working at all?)

Note: This PR does not make the chart compatible with stricter security k8s distribution by default, but enables users to do so on their own.

If the idea gets accepted, I will also update the README.

@Kajot-dev
Copy link
Contributor Author

Ok, I've checked and root is not necessary for chmod since it will check your permissions and elevate to root by itself.

@MinerYang
Copy link
Collaborator

MinerYang commented Jan 29, 2024

Hi @Kajot-dev ,

Could you elaborate more why should we both have pods level and container level security contexts? Since almost all of the harbor pods only have one containers. And for registry pod , although it includes registry and registryctl containers, but both of them share one PV and same permission settings for them.

@Kajot-dev
Copy link
Contributor Author

Kajot-dev commented Feb 2, 2024

Hi @Kajot-dev ,

Could you elaborate more why should we both have pods level and container level security contexts? Since almost all of the harbor pods only have one containers. And for registry pod , although it includes registry and registryctl containers, but both of them share one PV and same permission settings for them.

@MinerYang Od course! Ability to modify securityContext on the Pod scope is not necessarily required. Main goal of this PR is for user to be able to comply with PSP restricted (which requires additional fields in securityContext on the Container scope, https://kubernetes.io/docs/concepts/security/pod-security-standards/#restricted). However #1673 achieves the same goal without exposing configuration to the users and automatically forcing most safe/strict security.

In other words if #1673 gets merged, I am satisfied and this PR can be closed.

EDIT: Well, PSP restricted also requires runAsNonRoot on pod security context, so the PR that I'm mentioning would also need to do that

@MinerYang
Copy link
Collaborator

Thanks @Kajot-dev
Will close this PR and please help to review #1695 as we discussed.

@MinerYang MinerYang closed this Feb 21, 2024
@MinerYang MinerYang mentioned this pull request Feb 21, 2024
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.

Support for Security contexts in values.yaml deploy harbor on openshift
3 participants