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

Reuse the aws session with cacerts and credentials for fetching region #71

Closed
wants to merge 3 commits into from

Conversation

aayushrangwala
Copy link

@aayushrangwala aayushrangwala commented Feb 23, 2021

Velero has a way to use custom CA bundle to access the objectStore behind the proxy by providing the option --cacerts at the time of velero install or velero client operations.

s3 plugin tries to communicate the objectStore for finding out the region or to push/pull the Backup manifests. If the region is not provided in the BackupStorageLocation config, while finding the region, plugin creates a new aws session config which doesnot use any session options, such as certs or credential profile.

This PR is to add the support for aws s3 plugin also to consume the ca certificate bundle passed in BackupStorageLocation with the caCerts field.

This can be tested by having a velero setup behind proxy with a sef-signed certs and pass them along with the velero install and try to perform the backup/restore operations

Fixes: vmware-tanzu/velero#3449

Signed-off-by: Ayush Rangwala arangwala@vmware.com

@aayushrangwala aayushrangwala changed the title Reuse the aws session with cacerts for fetching region Reuse the aws session with cacerts and credentials for fetching region Feb 23, 2021
@nrb
Copy link
Contributor

nrb commented Feb 24, 2021

High level question - would it be possible to support the CA cert with the VolumeSnapshotter API, too?

@aayushrangwala
Copy link
Author

aayushrangwala commented Feb 24, 2021

High level question - would it be possible to support the CA cert with the VolumeSnapshotter API, too?

@nrb Yes, its possible and we should definitely do it to make this adoption across any API calls. But this might need a bit more changes as we have to decide how should we propagate the CA certs passed via --certs, to VolumeSnapshot API. For BSL, we have an optional field in the spec caCerts which stores the encoded bundle. We can have a same field in the VolumeSnapshot CR but it will need us to change in the CRD.

@carlisia carlisia removed the request for review from nrb April 5, 2021 16:20
@carlisia carlisia requested review from nrb and removed request for carlisia, dsu-igeek and ashish-amarnath April 15, 2021 22:47
}

// Init initializes the S3 config parameters which needs to be fetched/prepared based on the passed options
func (h *S3Config) Init() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

h? s would be more appropriate.

Copy link
Author

Choose a reason for hiding this comment

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

done

return nil
}

// GetBucketRegion returns the AWS region that a bucket is in, or an error
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment case should match method

Copy link
Author

Choose a reason for hiding this comment

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

done

}

// add region to the session config
if h.s3URL == "" && h.region == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is funky. h.getBucketRegion gets called once, the structure gets changed and then it should not be called again. getBucketRegion does not need to be a method, just keep it a func and pass in session and bucket.

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Will make that change

Copy link
Author

Choose a reason for hiding this comment

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

Done

Ayush Rangwala added 2 commits June 9, 2021 22:34
Signed-off-by: Ayush Rangwala <arangwala@vmware.com>
Signed-off-by: Ayush Rangwala <arangwala@vmware.com>
Signed-off-by: Ayush Rangwala <arangwala@vmware.com>
@kaovilai
Copy link
Contributor

any updates we wanna make here?

@reasonerjt
Copy link
Contributor

This PR won't work after the SDK is bumped up to v2.
However I believe the issue vmware-tanzu/velero#3449 remains valid, I'll close this PR, assign the issue to me and try to fix it in the timeframe of v1.14

@reasonerjt reasonerjt closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants