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

Only set BSL credential field if provided #4322

Conversation

zubron
Copy link
Contributor

@zubron zubron commented Nov 7, 2021

Please add a summary of your change

Previously, the BSL credential field would always be set when using the
create command, even if no credential details were provided. This
would result in an empty SecretKeySelector in the BSL which would
cause operations using this BSL to fail as Velero would attempt to fetch
a Secret with an empty name from the K8s API server.

With this change, the Credential field is only set if credential
details have been specified. This change also includes some refactoring
to allow the change to be tested.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

Does your change fix a particular issue?

Fixes #4196

Please indicate you've done the following:

Previously, the BSL credential field would always be set when using the
`create` command, even if no credential details were provided. This
would result in an empty `SecretKeySelector` in the BSL which would
cause operations using this BSL to fail as Velero would attempt to fetch
a `Secret` with an empty name from the K8s API server.

With this change, the `Credential` field is only set if credential
details have been specified. This change also includes some refactoring
to allow the change to be tested.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
@zubron zubron force-pushed the fix-setting-empty-credential-field-in-bsl-4196 branch from c3d6778 to 6e2daa1 Compare November 7, 2021 16:40
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

LGTM in general

setErr := o.Credential.Set("my-secret=key-from-secret")
assert.NoError(t, setErr)

bsl, err = o.BuildBackupStorageLocation("velero-test-ns", false, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

bsl, err = o.BuildBackupStorageLocation("velero-test-ns", false, false)

@dsu-igeek dsu-igeek merged commit e4019f2 into vmware-tanzu:main Nov 10, 2021
@turkenh
Copy link

turkenh commented Dec 3, 2021

@zubron @sseago
Experiencing this issue with with velero v1.7.1.
Will this fix be cherry-picked to v1.7 ?

danfengliu pushed a commit to danfengliu/velero that referenced this pull request Jan 25, 2022
Previously, the BSL credential field would always be set when using the
`create` command, even if no credential details were provided. This
would result in an empty `SecretKeySelector` in the BSL which would
cause operations using this BSL to fail as Velero would attempt to fetch
a `Secret` with an empty name from the K8s API server.

With this change, the `Credential` field is only set if credential
details have been specified. This change also includes some refactoring
to allow the change to be tested.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
Previously, the BSL credential field would always be set when using the
`create` command, even if no credential details were provided. This
would result in an empty `SecretKeySelector` in the BSL which would
cause operations using this BSL to fail as Velero would attempt to fetch
a `Secret` with an empty name from the K8s API server.

With this change, the `Credential` field is only set if credential
details have been specified. This change also includes some refactoring
to allow the change to be tested.

Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
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.

CLI backup-location create command incorrectly sets empty credential field when no credential provided
5 participants