project-zot / helm-charts

7 stars 18 forks source link

Add extra volume support #26

Closed ericgraf closed 10 months ago

ericgraf commented 10 months ago

What type of PR is this?

bug + feature

Which issue does this PR fix:

What does this PR do / Why do we need it:

Feature: Adds support for defining extra volumes and extra volume mounts.

Bug: Following the restricted PSS standards, found here https://kubernetes.io/docs/concepts/security/pod-security-standards/, Root filesystem should be read-only and pods run as non root.

When securityContext.runAsUser is set to 1000 the pod fails trying to write to /var/lib/registry folder.

panic: open /var/lib/registry/repo.db: permission denied

This can be solved by mounting a pvc at /var/lib/registry but this doesn't work for HA multi-replica setups. The current chart creates a single pvc and mounts it to all the pods which is not suitable for all situations.

The solution is to allow for more flexible complex mounts like allowing for Emptydir volume mounts for read-only root filesystems.

If an issue # is not available please add repro steps and logs showing the issue:

To reproduce:

kind create cluster
helm repo add project-zot http://zotregistry.io/helm-charts
helm install zot project-zot/zot  \
  --set securityContext.runAsUser=1000

The pod will crashloop with the below error.

panic: open /var/lib/registry/repo.db: permission denied

goroutine 1 [running]:
zotregistry.io/zot/pkg/cli.newServeCmd.func1(0xc001520900?, {0xc0015eb600, 0x1, 0x1?})
        zotregistry.io/zot/pkg/cli/root.go:66 +0x11f
github.com/spf13/cobra.(*Command).execute(0xc001520900, {0xc0015eb5c0, 0x1, 0x1})
        github.com/spf13/cobra@v1.7.0/command.go:944 +0x847
github.com/spf13/cobra.(*Command).ExecuteC(0xc001520600)
        github.com/spf13/cobra@v1.7.0/command.go:1068 +0x3bd
github.com/spf13/cobra.(*Command).Execute(0xc0000061a0?)
        github.com/spf13/cobra@v1.7.0/command.go:992 +0x19
main.main()
        zotregistry.io/zot/cmd/zot/main.go:10 +0x1e

Testing done on this change:

helm install zot .\
 --set securityContext.runAsUser=1000 \
 --set extraVolumes[0].name=data \
 --set extraVolumes[0].emptydir={} \
 --set extraVolumeMounts[0].name=data \
 --set extraVolumeMounts[0].mountPath=/var/lib/registry 

helm test zot
NAME: zot
LAST DEPLOYED: Mon Oct 30 14:47:29 2023
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE:     zot-test-connection-fails
Last Started:   Mon Oct 30 14:48:02 2023
Last Completed: Mon Oct 30 14:48:05 2023
Phase:          Succeeded
TEST SUITE:     zot-test-connection
Last Started:   Mon Oct 30 14:47:55 2023
Last Completed: Mon Oct 30 14:48:02 2023
Phase:          Succeeded
NOTES:
Get the application URL by running these commands:
  export NODE_PORT=$(kubectl get --namespace default -o jsonpath="{.spec.ports[0].nodePort}" services zot)
  export NODE_IP=$(kubectl get nodes --namespace default -o jsonpath="{.items[0].status.addresses[0].address}")
  echo http://$NODE_IP:$NODE_PORT

Automation added to e2e:

This change modifies the deployment method not sure how to test it here. lib/integration.sh also doesn't exist in this repo.

Will this break upgrades or downgrades?

No this change is backward compatible since it introduces new fields in the values file extraVolume, extraVolumeMount and volumeClaimTemplates .

Does this PR introduce any user-facing change?:

Add support for extraVolume and extraVolumeMounts.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rchincha commented 10 months ago

@ericgraf thanks for the PR. We will review this.

ericgraf commented 10 months ago

@rchincha I think this would help solve the issue https://github.com/project-zot/zot/issues/1760

rchincha commented 10 months ago

@ericgraf can you pls take a look at the CI failures.

Andreea-Lupu commented 10 months ago

Hi @ericgraf, pls rebase your PR and bump up chart version to 0.1.35 in Chart.yaml file.

ericgraf commented 10 months ago

@rchincha @Andreea-Lupu Sorry I didn't respond.

I bumped the version chart version to 0.1.39 which is the next patch version in Chart.yaml.

I noticed https://github.com/project-zot/helm-charts/pull/31 was opened did we want to use that PR instead or continue with this one?

andaaron commented 10 months ago

Hi @ericgraf we merged #31 as it was including the new test as well. Thanks again for your contribution!

ericgraf commented 10 months ago

Closing since changes have been merged in with https://github.com/project-zot/helm-charts/pull/31.