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.

Sign up for GitHub

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

Add configurable user agent prefix flag for mount-s3 #548

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Add configurable user agent prefix flag for mount-s3 #548

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5e8f983
Add configurable user agent prefix flag for mount-s3
dannycjones Oct 9, 2023
bf09bf8
Add CHANGELOG.md entry
dannycjones Oct 17, 2023
187227f
Merge branch 'main' into user-agent-prefix-cli-flag
dannycjones Oct 18, 2023
07b7136
Move `--user-agent-prefix` argument to bottom of 'Client options' sec…
dannycjones Oct 19, 2023
6d51973
Move `--user-agent-prefix` argument to new 'Advanced options' section
dannycjones Oct 19, 2023
5c692eb
Fix mountpoint-s3 CHANGELOG.md order
dannycjones Oct 19, 2023
7338a1d
Merge branch 'main' into user-agent-prefix-cli-flag
dannycjones Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion mountpoint-s3/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ struct CliArgs {
)]
pub allow_other: bool,

#[clap(
long,
help = "Configure a string to be prepended to the 'User-Agent' HTTP request header for all S3 requests",
value_name = "PREFIX",
help_heading = CLIENT_OPTIONS_HEADER
)]
pub user_agent_prefix: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

I kind of think this should be an undocumented option. It's really only useful for us right now, right? Not sure it helps customers for it to show up in --help. But it would be our first undocumented option. Maybe an alternative is a new help section for "advanced options" or something, to make clear this isn't something most customers who aren't AWS would ever need to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking it may be of value if third-parties wanted to include mount-s3 in their project/product, and include the user agent in their request.

I like the idea of moving it under an advanced options, so I'll update with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually less comfortable with moving it into a new "Advanced options", since someone looking for this kind of option would expect it to be with "Client options". Our intention is also for things like --max-threads <> and --part-size <> to be things most customers don't need to configure either.

At the moment, it shows up at the top of the list (since its based on the order it is defined). How do you feel about moving it to the bottom of the "Client options" list like below?

Client options:
      --maximum-throughput-gbps <N>  Maximum throughput in Gbps [default: auto-detected on EC2 instances, 10 Gbps elsewhere]
      --max-threads <N>              Maximum number of FUSE daemon threads [default: 16]
      --part-size <PART_SIZE>        Part size for multi-part GET and PUT [default: 8388608]
      --user-agent-prefix <PREFIX>   Configure a string to be prepended to the 'User-Agent' HTTP request header for all S3 requests

Copy link
Member

Choose a reason for hiding this comment

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

Is anyone really going to be looking for it? Only AWS really gets to see the user agent. Unlike the other options, this is something a customer would basically never configure — it's really only useful for bundling Mountpoint into something else (and that basically means only useful for us).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, let me move it to an "Advanced options" section at the bottom.


#[clap(
long,
help = "Maximum throughput in Gbps [default: auto-detected on EC2 instances, 10 Gbps elsewhere]",
Expand Down Expand Up @@ -420,11 +428,17 @@ fn mount(args: CliArgs) -> anyhow::Result<FuseSession> {
S3ClientAuthConfig::Default
};

let user_agent_prefix = if let Some(custom_prefix) = args.user_agent_prefix {
format!("{} mountpoint-s3/{}", custom_prefix, build_info::FULL_VERSION)
} else {
format!("mountpoint-s3/{}", build_info::FULL_VERSION)
};

let mut client_config = S3ClientConfig::new()
.auth_config(auth_config)
.throughput_target_gbps(throughput_target_gbps)
.part_size(args.part_size as usize)
.user_agent_prefix(&format!("mountpoint-s3/{}", build_info::FULL_VERSION));
.user_agent_prefix(&user_agent_prefix);
if args.requester_pays {
client_config = client_config.request_payer("requester");
}
Expand Down