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

fix: canonicalize config keys #2206

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Conversation

emcake
Copy link
Contributor

@emcake emcake commented Feb 24, 2024

Description

When merging supplied storage options with environment variables, we need to canonicalize the items in the storage options first. This is because for some keys there is multiple valid definitions (AWS_ENDPOINT_URL, AWS_ENDPOINT) and also casing (AWS_ACCESS_KEY_ID, aws_access_key_id). It seems like the 'more canonical' lowercase versions win, which can cause surprise if you have one set of access keys in your environment and one in your storage options. This is exacerbated as the S3StorageOptions construction also sets envrionment variables, and it always does so into the canonical lowercase versions. I found this when opening two buckets with different credentials, and the credentials for the first were being used to open the second.

@github-actions github-actions bot added the binding/rust Issues for the Rust crate label Feb 24, 2024
@rtyler rtyler self-assigned this Feb 24, 2024
@rtyler rtyler added this to the Rust v0.18 milestone Feb 24, 2024
rtyler
rtyler previously approved these changes Feb 24, 2024
@rtyler rtyler enabled auto-merge (rebase) February 24, 2024 19:28
@rtyler
Copy link
Member

rtyler commented Feb 24, 2024

Looks like the failure is just in some tests which are not expecting some environment variables to be set. Our option I think is to either have these new tests reset their environment variables, or update the other tests.

auto-merge was automatically disabled February 25, 2024 09:29

Head branch was pushed to by a user without write access

@emcake
Copy link
Contributor Author

emcake commented Feb 25, 2024

Looks like the failure is just in some tests which are not expecting some environment variables to be set. Our option I think is to either have these new tests reset their environment variables, or update the other tests.

I introduced a small utility to all of the tests to restore the environment to how it was when they captured it. Let me know if you think this approach is reasonable?

rtyler
rtyler previously approved these changes Feb 25, 2024
@rtyler rtyler enabled auto-merge (rebase) February 25, 2024 14:56
auto-merge was automatically disabled February 25, 2024 21:22

Head branch was pushed to by a user without write access

@emcake
Copy link
Contributor Author

emcake commented Feb 25, 2024

☝️ I briefly flirted with changing the tests to be a lot more careful about which environment variables they touched. This was because the integration tests workflow itself actually sets some extra environment vars, which was messing with the test. However on reflection on doing so, by being canoncalizing the keys under test, I managed to partly defeat the purpose of the test...

In the end I went for the simpler solution from the other tests of just unsetting those variables.

@rtyler rtyler enabled auto-merge (rebase) February 26, 2024 17:37
@rtyler rtyler merged commit 51f1cd0 into delta-io:main Feb 26, 2024
20 checks passed
@emcake emcake deleted the canonicalize-config-keys branch February 26, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants