search

openshift / openshift-velero-plugin

General Velero plugin for backup and restore of openshift workloads.
Apache License 2.0
47 stars 37 forks source link

ObjectStorage Prefix specified in BackupStorageLocation is ignored #258

Open jacksgt opened 1 month ago

jacksgt commented 1 month ago

Hello,

Velero's BackupStorageLocation (.spec.objectStorage.prefix) as well as OADP's DataProtectionApplication (.spec.backupLocations[].velero.objectStorage.prefix) allow setting a (optional) prefix for files uploaded into the S3 bucket:

Velero assumes it has control over the location you provide so you should use a dedicated bucket or prefix. If you provide a prefix, then the rest of the bucket is safe to use for multiple purposes.

apiVersion: velero.io/v1
kind: BackupStorageLocation
metadata:
  name: default
spec:
  provider: aws
  objectStorage:
    bucket: myBucket
    prefix: myPrefix

https://velero.io/docs/v1.13/api-types/backupstoragelocation/

Our BackupStorageLocation is configured with such a prefix, but the plugin currently ignores this. This leads to conflicts in the S3 bucket since ImageStream data is always uploaded to docker/registry/v2/... (when using the PluginRegistry). This appears to be the problematic part of the code: https://github.com/openshift/openshift-velero-plugin/blob/95ab2b038f0170ab6b0fa70bcb2d28268b312739/velero-plugins/imagestream/registry.go#L133

An additional environment variable REGISTRY_STORAGE_FILESYSTEM_ROOTDIRECTORY=${BSL.spec.objectStorage.prefix} should be added to address this (see example in the (registry configuration documentation](https://distribution.github.io/distribution/about/configuration/))

sseago commented 1 month ago

Supporting the registry here was one of the reasons we added the prefix in the first place -- the registry can't be in the same prefix as the velero backup, or velero will complain about the docker dir. To avoid the issue you're hitting, we'd probably need to add a separate prefix for the registry location outside of the BSL config.

jacksgt commented 1 month ago

Supporting the registry here was one of the reasons we added the prefix in the first place

You mean adding the prefix to BackupStorageLocation.spec.objectStorage.prefix, correct?

the registry can't be in the same prefix as the velero backup, or velero will complain about the docker dir.

Ah, that's interesting. So if prefix is velero, then velero would complain if /velero/docker/registry/... appears in the S3 bucket?

UPDATE: Indeed, I found the relevant code snippet here: https://github.com/vmware-tanzu/velero/blob/6c2b66b480f1f58e08f62c8a0e8ae820a9cd02c6/pkg/persistence/object_store.go#L203

But plugins are allowed to (supposed to?) write arbitrary data to the plugins directory, so maybe this data should actually go to <PREFIX>/plugins/openshift-velero-plugin/docker/registry/...:

kaovilai commented 1 month ago

so maybe this data should actually go to /plugins/openshift-velero-plugin/docker/registry/...:

Sure. But it would be a breaking change. or require some migration effort.

sseago commented 1 month ago

Supporting the registry here was one of the reasons we added the prefix in the first place

You mean adding the prefix to BackupStorageLocation.spec.objectStorage.prefix, correct?

Yes, exactly.

the registry can't be in the same prefix as the velero backup, or velero will complain about the docker dir.

Ah, that's interesting. So if prefix is velero, then velero would complain if /velero/docker/registry/... appears in the S3 bucket?

Yes -- this is actually why I implemented the prefix feature upstream in the BSL in the first place.

UPDATE: Indeed, I found the relevant code snippet here: https://github.com/vmware-tanzu/velero/blob/6c2b66b480f1f58e08f62c8a0e8ae820a9cd02c6/pkg/persistence/object_store.go#L203

But plugins are allowed to (supposed to?) write arbitrary data to the plugins directory, so maybe this data should actually go to <PREFIX>/plugins/openshift-velero-plugin/docker/registry/...:

sseago commented 1 month ago

so maybe this data should actually go to /plugins/openshift-velero-plugin/docker/registry/...:

Sure. But it would be a breaking change. or require some migration effort.

And actually, any change here is potentially a breaking change, even my earlier suggestion for a registry-specific prefix. One possibility to fix would be to have the registry creation code configure the new location under plugins but if an existing registry exists at the top level, velero could copy the full set of registry files from top level to plugins if there are no files yet under plugins.

jacksgt commented 1 month ago

so maybe this data should actually go to /plugins/openshift-velero-plugin/docker/registry/...:

Sure. But it would be a breaking change. or require some migration effort.

And actually, any change here is potentially a breaking change, even my earlier suggestion for a registry-specific prefix. One possibility to fix would be to have the registry creation code configure the new location under plugins but if an existing registry exists at the top level, velero could copy the full set of registry files from top level to plugins if there are no files yet under plugins.

Very true. I think this requirement / limitation should at least be very clearly documented.