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 security context #1695

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Conversation

MinerYang
Copy link
Collaborator

No description provided.

@Kajot-dev
Copy link
Contributor

I have one comment. As far as I can see, database and redis use different uids and gids from the rest of the components, so I guess global values.containerSecurityContext would have to either:

  • not include runAsUser and fsGroup and these would be hardcoded in the manifests
  • in case of database/redis use helm's omit like this omit "runAsUser" "fsGroup" and only have hardcoded uids/gids in in redis/database

@MinerYang
Copy link
Collaborator Author

MinerYang commented Feb 21, 2024

Hi @Kajot-dev ,

Thanks for your review and sorry for the late response since i just came back from holidays.

We don't expose runAsUser and fsGroup in values.yaml and it is hardcoded in the manifest
And indeed, none of our containers are running as root, however we need explicitly claim this runAsNonRoot: true to meet our restricted pod security standards. https://kubernetes.io/docs/concepts/security/pod-security-standards/#restricted

What I initially want to write in values.yaml is a empty yaml looks like below and users could configure it on demand. Current version is just filled with a common template for convenience.

containerSecurityContext: {}

@MinerYang
Copy link
Collaborator Author

@MinerYang MinerYang requested a review from wy65701436 February 21, 2024 03:21
@MinerYang MinerYang force-pushed the test_security_context branch from 074a6f4 to bb79b1b Compare February 22, 2024 06:08
values.yaml Outdated Show resolved Hide resolved
@MinerYang MinerYang force-pushed the test_security_context branch from bb79b1b to 478756a Compare February 22, 2024 10:05
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@rgarcia89 rgarcia89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@Kajot-dev Kajot-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one thing in core-pre-upgrade job, the rest looks good to me

templates/core/core-pre-upgrade-job.yaml Outdated Show resolved Hide resolved
Signed-off-by: yminer <[email protected]>

update for trivy and db

update db-ss init container

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

update core-pre-upgrade-job.yaml

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

update values.yaml

fix core-pre-upgrade-job typo
@MinerYang MinerYang force-pushed the test_security_context branch from 478756a to 7913d7e Compare February 23, 2024 03:26
@MinerYang MinerYang merged commit bd7da40 into goharbor:main Feb 23, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants