"s3ForcePathStyle" entry gets duplicated with AWS plugin

[vzabawski]

What steps did you take and what happened:
[A clear and concise description of what the bug is, and what commands you ran.)

I add Velero chart configuration to my values.yaml:

velero:
  enabled: true
  configuration:
    provider: aws
    backupStorageLocation:
      provider: aws
      name: default
      bucket: jenkins-backups-prod
      config:
        region: eu-de-2
        s3ForcePathStyle: true
        s3Url: https://objectstore-3.eu-de-2.example.com

Rendered yaml contains duplicated entries:

apiVersion: velero.io/v1
kind: BackupStorageLocation
metadata:
  name: default
  annotations:
    "helm.sh/hook": post-install,post-upgrade
    "helm.sh/hook-delete-policy": "before-hook-creation"
  labels:
    app.kubernetes.io/name: velero
    app.kubernetes.io/instance: jenkins
    app.kubernetes.io/managed-by: Helm
    helm.sh/chart: velero-2.14.8
spec:
  provider: aws
  objectStorage:
    bucket: jenkins-backups-prod
  config:
    region: eu-de-2
    s3ForcePathStyle: true
    s3Url: https://objectstore-3.eu-de-2.example.com
    signatureVersion: "4"
    s3ForcePathStyle: "true"
---

Quoting true in values.yaml doesn't change the behavior, it just creates 2 entries, where each true is quoted.

Environment:

• helm version (use helm version):
  version.BuildInfo{Version:"v3.5.2", GitCommit:"167aac70832d3a384f65f9745335e9fb40169dc2", GitTreeState:"dirty", GoVersion:"go1.15.7"}
• helm chart version:
  2.14.8

[jenting]

@vzabawski Thank you for reporting this. If you like, could you file a PR to addressing this? Thank you.

[vzabawski]

I suggest removing the part that quotes true since it's plugin-specific config and add a warning to plugin configuration example about s3ForcePathStyle, that it should be quoted. I don't see any other simple solution, to be honest. :)

[jenting]

I suggest removing the part that quotes true since it's plugin-specific config and add a warning to plugin configuration example about s3ForcePathStyle, that it should be quoted. I don't see any other simple solution, to be honest. :)

Yes, I agree with you.
But wouldn't this break the existing plugin behavior and force the user must upgrade the plugin version 🤔 ?

[vzabawski]

I suggest removing the part that quotes true since it's plugin-specific config and add a warning to plugin configuration example about s3ForcePathStyle, that it should be quoted. I don't see any other simple solution, to be honest. :)

Yes, I agree with you.
But wouldn't this break the existing plugin behavior and force the user must upgrade the plugin version 🤔 ?

To be honest, I can't be sure, but from what I see, the example shows that the value should be wrapped in double quotes: https://github.com/vmware-tanzu/velero-plugin-for-aws/blob/main/backupstoragelocation.md

[jenting]

May I said that if the user existing configuration with

s3ForcePathStyle: true

after this PR merged and the user performs helm upgrade or helm install with the existing configuration, it would break the velero can't access the s3-compatible provider?

[vzabawski]

I guess so. At the same time, duplicated entries do not block the deployment since duplicated keys are dropped in k8s. At least, deployed backupstoragelocation resource does not contain duplicated keys.

[jenting]

I'd prefer not to change this. /cc @carlisia WDYT?

[carlisia]

I guess so. At the same time, duplicated entries do not block the deployment since duplicated keys are dropped in k8s. At least, deployed backupstoragelocation resource does not contain duplicated keys.

Yes, I'm inclined to leave this alone. We can leave it open and clean it up if/when there's an inevitable change that brakes backwards compatibility.

Thanks for the effort tho, @vzabawski 👍

[irizzant]

If the yaml rendered by the Helm chart is then given as input to kustomize version 3.9 and later, kustomize fails with:

Error: accumulating resources: accumulation err='accumulating resources from '../base': '/home/local/INTRANET/ivan.rizzante/kubernetes/gitops/velero/base' must resolve to a file': recursed accumulation of path '/home/local/INTRANET/ivan.rizzante/kubernetes/gitops/velero/base': received error yaml: unmarshal errors:
  line 22: mapping key "s3ForcePathStyle" already defined at line 20 for the following resource:
# Source: velero/templates/backupstoragelocation.yaml

This makes it a blocking issue

[jenting]

@irizzant Thanks for the reporting, would you mind provides the kustomize command on how to apply the manifest to us? This would make us easier to understand it. Thank you.

[irizzant]

@jenting
I use the following values.yaml

configuration:
  # Cloud provider being used (e.g. aws, azure, gcp).
  provider: aws

  # Parameters for the `default` BackupStorageLocation. See
  # https://velero.io/docs/v1.5/api-types/backupstoragelocation/
  backupStorageLocation:
    # name is the name of the backup storage location where backups should be stored. If a name is not provided,
    # a backup storage location will be created with the name "default". Optional.
    name:
    # provider is the name for the backup storage location provider. If omitted
    # `configuration.provider` will be used instead.
    provider:
    # bucket is the name of the bucket to store backups in. Required.
    bucket: k8s-backups
    # caCert defines a base64 encoded CA bundle to use when verifying TLS connections to the provider.
    caCert:
    # prefix is the directory under which all Velero data should be stored within the bucket. Optional.
    prefix:

    # Additional provider-specific configuration. See link above
    # for details of required/optional fields for your provider.
    config: 
      region: sdb
      s3ForcePathStyle: "true"

And generate an upstream configuration like this with helm:
helm template velero --namespace velero -f values.yaml --include-crds vmware-tanzu/velero > base/upstream.yaml

Then I just run kustomize build base and I get that error.

[jenting]

@irizzant Thank you. I filed a PR to address it, if you like, please try it 🙏

[irizzant]

@jenting looks like your PR with the values.yaml reported here generates:

# Source: velero/templates/backupstoragelocation.yaml
apiVersion: velero.io/v1
kind: BackupStorageLocation
metadata:
  name: default
  annotations:
    "helm.sh/hook": post-install,post-upgrade
    "helm.sh/hook-delete-policy": "before-hook-creation"
  labels:
    app.kubernetes.io/name: velero
    app.kubernetes.io/instance: velero
    app.kubernetes.io/managed-by: Helm
    helm.sh/chart: velero-2.16.1
spec:
  provider: aws
  objectStorage:
    bucket: k8s-backups
  config:
    region: "sdb"
    s3ForcePathStyle: "true"

Which as you can see has only one s3ForcePathStyle entry, so it looks good to me thanks