Introduced new cli options #204

eslrahc-swa · 2023-04-11T16:20:41Z

Introduced Cli options.

--no-sign-request: allow customer to omit credential.
--profile: customer can specify credentials name.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

We promised customer to maximize network throughput. The change enables automatic network throughput configuration if customer specified throughput is missing. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

1. Simplify the throughput configuration logic - main.rs(commit#:65432b) 2. Introduce "version" to network_performance.json.

The program reads network throughput form network_performance.json, adds unit tests for checking throughput correctness. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

By using #[test_case] in stead of #[test] Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

passaro · 2023-04-11T16:50:43Z

mountpoint-s3-client/src/s3_crt_client.rs

+            Some(profile_name_override) => {
+                let credentials_profile_options = CredentialsProviderProfileOptions {
+                    profile_name_override: &profile_name_override,
+                };


Shouldn't we pass client_bootstrap in this case as well?

client_bootstrap usage to profile based credential is for customer who uses assume role with STS. My understanding now is customer only uses IAM Role or IAM User as per README.md mentioned.

However, I think we should add client_bootstrap to support any customer use assume role and sending http/https request to STS

Profiles can specify a role to assume, so we do need to include the bootstrap here.

mountpoint-s3/src/main.rs

dannycjones · 2023-04-11T16:58:56Z

Looks related to:

Possibly also?: #197

mountpoint-s3-crt/src/auth/credentials.rs

mountpoint-s3-client/src/s3_crt_client.rs

passaro · 2023-04-11T17:32:13Z

mountpoint-s3-client/src/s3_crt_client.rs

-            bootstrap: &mut client_bootstrap,
+        let mut client_config = ClientConfig::new();
+
+        let mut credentials_provider = match config.profile_name_override {


Should this be moved under the if at line 113?

It should not, we need to bump ref count of CredentialProvider to 1 all the way until S3CrtClient owns the client config.

I think I see what you mean, but maybe it's the signature of init_default_signing_config that's an issue: why is it taking a (mutable) reference instead of taking ownership of CredentialProvider? Is this a similar issue to what @jamesbornholt pointed out about the options parameter below? (or am I missing something?)

jamesbornholt

We need to write tests for this, as they would have caught the missing bootstrap in the profile credentials provider.

Test mounting an anonymous-readable bucket
Test specifying profile provider with STS credentials

jamesbornholt · 2023-04-11T17:39:21Z

mountpoint-s3-crt/src/auth/credentials.rs

+#[derive(Debug)]
+pub struct CredentialsProviderProfileOptions<'a> {
+    /// The name of profile to use.
+    pub profile_name_override: &'a str,


This should be Option<&'a str>

nice catch! will fix

mountpoint-s3-crt/src/auth/credentials.rs

jamesbornholt · 2023-04-11T17:42:04Z

mountpoint-s3-crt/src/auth/credentials.rs

@@ -42,6 +54,25 @@ impl CredentialsProvider {

        Ok(Self { inner })
    }
+
+    /// Creates the profile credential provider.
+    pub fn new_profile(allocator: &Allocator, options: &CredentialsProviderProfileOptions) -> Result<Self, Error> {


We should take ownership of the options here.

Suggested change

pub fn new_profile(allocator: &Allocator, options: &CredentialsProviderProfileOptions) -> Result<Self, Error> {

pub fn new_profile(allocator: &Allocator, options: CredentialsProviderProfileOptions) -> Result<Self, Error> {

what about the new_chain_default should that method take the ownership of CredentialsProviderChainDefaultOption?

that should too

mountpoint-s3-client/src/s3_crt_client.rs

jamesbornholt · 2023-04-11T17:45:20Z

mountpoint-s3-client/src/s3_crt_client.rs

+            Some(profile_name_override) => {
+                let credentials_profile_options = CredentialsProviderProfileOptions {
+                    profile_name_override: &profile_name_override,
+                };


Profiles can specify a role to assume, so we do need to include the bootstrap here.

* --no-sign-request: allow customer to omit credential. * --profile: customer can specify credentials name. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

…ug (#201) Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

* Add error handling for FUSE worker threads that panic When encountering a panic in a FUSE operation, the filesystem was not exiting cleanly. We were not handling the panicking thread case before this change. With this change, we log at error and allow the unmount process to continue. Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> * Make thread name optional, add panic message to log if possible Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> * Appease fmt-check Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> --------- Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

…tus code (#205) * Update to debug/warn based on status code Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> * Appease fmt-check Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> * Appease clippy Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com> --------- Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

* Do some basic validation of bucket names This will help catch common CLI errors like #203. Customers coming from the AWS CLI might expect to be able to pass an s3:// URI as a bucket name, which we don't support. It's also possible to accidentally swap the bucket name and mount point arguments -- mount points will often be invalid bucket names, so some validation helps us catch some of these cases more quickly. Arguably we could support parsing s3:// URIs, including using the key as a prefix for the mount. But that would be a bigger feature and I'm not totally sure it's the right thing, so for now some validation is better than nothing. Signed-off-by: James Bornholt <bornholt@amazon.com> * Allow underscores in bucket names Signed-off-by: James Bornholt <bornholt@amazon.com> --------- Signed-off-by: James Bornholt <bornholt@amazon.com>

Bumps [h2](https://github.com/hyperium/h2) from 0.3.16 to 0.3.17. - [Release notes](https://github.com/hyperium/h2/releases) - [Changelog](https://github.com/hyperium/h2/blob/master/CHANGELOG.md) - [Commits](hyperium/h2@v0.3.16...v0.3.17) --- updated-dependencies: - dependency-name: h2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* --no-sign-request: allow customer to omit credential. address issue 181 #181 * --profile: customer can specify credentials name. address issue 151: #151 Signed-off-by: Yaosheng <zyaoshen@amazon.com>

Ubuntu added 4 commits April 6, 2023 21:24

Enable auto configure network throughput.

65432b6

We promised customer to maximize network throughput. The change enables automatic network throughput configuration if customer specified throughput is missing. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

Addressing feedbacks.

279c839

1. Simplify the throughput configuration logic - main.rs(commit#:65432b) 2. Introduce "version" to network_performance.json.

Unit tests for function get_maximum_network_throughput.

b0396d6

The program reads network throughput form network_performance.json, adds unit tests for checking throughput correctness. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

Simplify unit tests

6cd1fec

By using #[test_case] in stead of #[test] Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

eslrahc-swa had a problem deploying to PR integration tests April 11, 2023 16:20 — with GitHub Actions Failure

passaro reviewed Apr 11, 2023

View reviewed changes

mountpoint-s3-crt/src/auth/credentials.rs Show resolved Hide resolved

passaro reviewed Apr 11, 2023

View reviewed changes

mountpoint-s3-client/src/s3_crt_client.rs Show resolved Hide resolved

passaro reviewed Apr 11, 2023

View reviewed changes

jamesbornholt reviewed Apr 11, 2023

View reviewed changes

Introduced Cli options.

8bb9230

* --no-sign-request: allow customer to omit credential. * --profile: customer can specify credentials name. Signed-off-by: Charles Zhang <zyaoshen@amazon.com>

eslrahc-swa had a problem deploying to PR integration tests April 13, 2023 23:29 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 13, 2023 23:30 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 13, 2023 23:34 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 13, 2023 23:35 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 13, 2023 23:36 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 14, 2023 00:34 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 14, 2023 00:48 — with GitHub Actions Failure

eslrahc-swa had a problem deploying to PR integration tests April 14, 2023 01:59 — with GitHub Actions Failure

dannycjones and others added 7 commits April 13, 2023 22:19

Lower log level for S3CrtClient metareq response err from warn to deb…

5eed769

…ug (#201) Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

Add note regarding PATH to README.md (#196)

44a8c9b

Signed-off-by: Daniel Carl Jones <djonesoa@amazon.com>

Introduced CLI options.

a75a749

* --no-sign-request: allow customer to omit credential. address issue 181 #181 * --profile: customer can specify credentials name. address issue 151: #151 Signed-off-by: Yaosheng <zyaoshen@amazon.com>

eslrahc-swa had a problem deploying to PR integration tests April 14, 2023 05:28 — with GitHub Actions Failure

eslrahc-swa closed this Apr 14, 2023

eslrahc-swa deleted the zyaoshen-dev branch April 14, 2023 05:32

eslrahc-swa linked an issue Apr 14, 2023 that may be closed by this pull request

Auto-configure Network Interface Card (NIC) throughput #3

Closed

eslrahc-swa removed a link to an issue Apr 14, 2023

Auto-configure Network Interface Card (NIC) throughput #3

Closed

eslrahc-swa mentioned this pull request Apr 14, 2023

Add support for AWS profile and unsigned requests as command line arguments #209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced new cli options #204

Introduced new cli options #204

eslrahc-swa commented Apr 11, 2023 •

edited

Loading

passaro Apr 11, 2023

eslrahc-swa Apr 11, 2023

jamesbornholt Apr 11, 2023

dannycjones commented Apr 11, 2023

passaro Apr 11, 2023

eslrahc-swa Apr 11, 2023

passaro Apr 12, 2023

jamesbornholt left a comment

jamesbornholt Apr 11, 2023

eslrahc-swa Apr 11, 2023

jamesbornholt Apr 11, 2023

eslrahc-swa Apr 11, 2023

jamesbornholt Apr 11, 2023

jamesbornholt Apr 11, 2023

	pub fn new_profile(allocator: &Allocator, options: &CredentialsProviderProfileOptions) -> Result<Self, Error> {
	pub fn new_profile(allocator: &Allocator, options: CredentialsProviderProfileOptions) -> Result<Self, Error> {

Introduced new cli options #204

Introduced new cli options #204

Conversation

eslrahc-swa commented Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycjones commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslrahc-swa commented Apr 11, 2023 •

edited

Loading