Skip to content

Conversation

wkbrd
Copy link

@wkbrd wkbrd commented Oct 1, 2025

This PR contains a few changes we worked. These changes enable the container to run more securely by-default.

  1. Security improvements: Pod Security Restricted support
  2. Security improvements: Ability to run with read only root filesystem in the container (immutable container). This includes making mimeConfig configurable
  3. Security improvements: Do not automount security account (automountServiceAccountToken)
  4. Security improvement: Customization for OpenShift support
  5. Add GitHub build plan action to publish the Helm chart to the GitHub Container Registry. This allows pulling chart releases from OCI compliant tools/registries

Please reach out if you have questions.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@sklarsa sklarsa left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

There are a few great additions here, and I think it would be helpful from a review and commit-history standpoint to separate them into 2 PRs:

  1. mimetype config change (since this is straightforward and has limited risk)
  2. security-related features (pod/security context & automountServiceToken additions). This requires a bit more review and testing to ensure we don't break any existing installations.

@@ -0,0 +1,35 @@
# from https://github.com/wkbrd/docker-registry.helm/blob/main/.github/workflows/helm_release.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you revert changes to this file? We'll update it as part of the release process

Copy link
Author

Choose a reason for hiding this comment

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

Hi @sklarsa : Do you publish your chart to an OCI registry? If not, that is something that this change would enable. Using an OCI registry standardizes how software is deployed and helps improve the tooling around a secure supply chain.

Comment on lines +53 to +58
- name: tmpfs-tmp
mountPath: /tmp
- name: tmpfs-questdb-import
mountPath: /var/lib/questdb/import
- name: tmpfs-questdb-public
mountPath: /var/lib/questdb/public
Copy link
Contributor

Choose a reason for hiding this comment

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

These mounts are breaking a simple helm install questdb ./charts/questdb with the following logs:

questdb No arguments found in the configuration, start with default arguments                                             
questdb Running as questdb user                                                                                           
questdb Log configuration loaded using factory defaults.                                                                  
questdb 2025-10-03T18:34:29.594330Z A server-main QuestDB 9.0.3. Copyright (C) 2014-2025, all rights reserved.            
questdb 2025-10-03T18:34:29.684387Z A server-main linux-x86-64 [AVX512,10, 64 bits, 12 processors]                        
questdb 2025-10-03T18:34:29.684447Z A server-main fs.file-max checked [limit=2147483584]                                  
questdb 2025-10-03T18:34:29.684523Z A server-main vm.max_map_count checked [limit=1048576]                                
questdb 2025-10-03T18:34:29.688161Z I server-main Web Console is up to date                                               
questdb 2025-10-03T18:34:29.693775Z A server-main Server config: /var/lib/questdb/conf/server.conf                        
questdb Server configuration file does not exist! /var/lib/questdb/conf/server.conf    

I think if we remove readOnlyRootFilesystem in the security context, we can do away with these tmp volumes and mounts. I don't believe that this is an openshift requirement and would prefer to leave this extra security-scoping to the user instead of baking into the default chart.

Comment on lines +145 to +150
- name: tmpfs-tmp
emptyDir: {}
- name: tmpfs-questdb-import
emptyDir: {}
- name: tmpfs-questdb-public
emptyDir: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

see above comment

type: RuntimeDefault

securityContext:
readOnlyRootFilesystem: true
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, let's remove this and let the user add it as necessary

@wkbrd
Copy link
Author

wkbrd commented Oct 6, 2025

Thank you for your contribution!

There are a few great additions here, and I think it would be helpful from a review and commit-history standpoint to separate them into 2 PRs:

  1. mimetype config change (since this is straightforward and has limited risk)
  2. security-related features (pod/security context & automountServiceToken additions). This requires a bit more review and testing to ensure we don't break any existing installations.

Hi @sklarsa , I am breaking the work into several PRs. Please watch for them. I am leaving this one open for now while the work is being split/migrated.

Further changes are dependent on 222/223/224, and so I will wait to submit further PRs until these are reviewed/resolved.

Please reach out with comments/feedback. Thanks!

@sklarsa
Copy link
Contributor

sklarsa commented Oct 6, 2025

thanks @wkbrd! I'm going through your changes now.

Have you signed the CLA? We will need to do that before merging

@wkbrd
Copy link
Author

wkbrd commented Oct 6, 2025

thanks @wkbrd! I'm going through your changes now.

Have you signed the CLA? We will need to do that before merging

I have not signed it yet. Please proceed to review the PRs though. Thanks much!

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.

3 participants