Skip to content

Conversation

@Friede80
Copy link
Contributor

Which issue does this PR close?

Closes #538.

Rationale for this change

Setting web_identity_token_file and role_arn via the config would be ignored as the builder was still only checking the env vars.

What changes are included in this PR?

Are there any user-facing changes?

Setting the AWS web identity fields via the config builder will no longer be ignored

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Is this a breaking change? Or is self.web_identity_token_file already seeded with the value of AWS_WEB_IDENTITY_TOKEN_FILE?

@Friede80
Copy link
Contributor Author

Is this a breaking change? Or is self.web_identity_token_file already seeded with the value of AWS_WEB_IDENTITY_TOKEN_FILE?

That's a good point. self.web_identity_token_file will have the value of AWS_WEB_IDENTITY_TOKEN_FILE if the builder is initialized via from_env(). This is more consistent with how all of the other AWS_ env vars are handled. But I think you are right that if someone was depending on these env vars being pulled in even while using AmazonS3Builder::new() this would break for them.

That feels like an unusual use case, and this feature has existed for such a short period of time, that I doubt it will affect anyone. But I'm happy to document the breaking change somewhere (I'm not sure what the process here is for breaking changes)

I could also make it backwards compatible if necessary, but it would then be a weird config option that behaves different than all the others which doesn't seem like a great option.

@kylebarron
Copy link
Member

I'd love a review from @alamb too

std::env::var("AWS_WEB_IDENTITY_TOKEN_FILE"),
std::env::var("AWS_ROLE_ARN"),
) {
} else if let (Some(token_path), Some(role_arn)) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes sense to me -- thank you @Friede80 and @kylebarron

I double checked that the environment variables are still honored and I think they will be -- specifically, they will be parsed when creating from the environment here:

https://github.com/apache/arrow-rs-object-store/blob/1b8ecc7f41eab2a3c2c373f1c2fe21bebe391727/src/aws/builder.rs#L566-L565

"aws_web_identity_token_file" | "web_identity_token_file" => {
Ok(Self::WebIdentityTokenFile)
}
"aws_role_arn" | "role_arn" => Ok(Self::RoleArn),

@alamb alamb merged commit 1558217 into apache:main Nov 21, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AmazonS3ConfigKey::WebIdentityTokenFile is ignored

3 participants