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

Kopia Integration: Unified Repository Provider - Repo Password #5167

Merged
merged 1 commit into from Aug 4, 2022

Conversation

Lyndon-Li
Copy link
Contributor

Add changes for Kopia Integration: Unified Repository Provider - Repo Password
Related Issue: #5079

@Lyndon-Li Lyndon-Li changed the title repo credentials Kopia Integration: Unified Repository Provider - Repo Password Aug 1, 2022
@Lyndon-Li Lyndon-Li added this to the 1.10 milestone Aug 1, 2022
@Lyndon-Li Lyndon-Li removed this from the 1.10 milestone Aug 2, 2022
credentialsSecretName = "velero-restic-credentials"
credentialsKey = "repository-password"

encryptionKey = "static-passw0rd"
Copy link
Contributor

Choose a reason for hiding this comment

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

which means that our kopia repo paasword is plain text and invariable? and which may lead to some security issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the existing code and file, this keys.go is moved from Restic package to the repository package.
As the agreement, in v1.10, we will keep the existing behaviors for Kopia integration.

In future, as you can see, the provider construct accepts a CredentialGetter, in which, we can add whatever implementation to get a password. For example, we may get repo password from KMS in future, then we can add something like KmsStore interface into it.

}

// Buffer returns the secret key defined by the given selector.
func (n *namespacedSecretStore) Buffer(selector *corev1api.SecretKeySelector) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

whether we should change it to one easily understood function name instead of Buffer? this function is to get a secret for a specific namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Buffer means getting a buffer stream, since the interface has already named as SecretStore, we know that it is to get the stream buffer from Secret, so no need to add something like "secret" in the method name. This is also to comply with FileStore's Path method.

As the discuss, we leave it as it in this PR. Later, if there is any conflict or better name, we come back to change it.

qiuming-best
qiuming-best previously approved these changes Aug 4, 2022

// SecretStore defines operations for interacting with credentials
// that are stored in Secret.
type SecretStore interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems it isn't a store? It's just a secret getter and stores nothing. Can we call kubeClient.Get() to get the secret directly without defining this interface?

If it's needed, I have the same comment with @qiuming-best , the function name Buffer confuses me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We add this SecretStore in parallel with the existing FileStore, the FileStore gets the password from Secret then save to a temp file; SecretStore gets the password from Secret and then return a stream buffer.

We don't call kubeClient.Get() directly for below purpose:

  • We may get the password/credential keys from different sources, for example, from a temp file, from memory, from KMS, so we wrap the interfaces for all the sources in the CredentialGetter structure.
  • At present, in Unified Repo, we are using repo password got from memory, cloud provider credentials from a temp file
  • In future, we may add a new interface called something like KmsStore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the method name of SecretStore, changed from Buffer to Get

Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
@ywk253100 ywk253100 merged commit 088eb9b into vmware-tanzu:main Aug 4, 2022
@Lyndon-Li Lyndon-Li deleted the udmrepo-dev-01 branch August 4, 2022 08:41
danfengliu pushed a commit to danfengliu/velero that referenced this pull request Sep 13, 2022
Signed-off-by: Lyndon-Li <lyonghui@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.

None yet

3 participants