change priority of admin credentials from env varaibles #7032
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @chrislusf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request redefines the precedence for loading S3 admin credentials within the system. It establishes environment variables as a strict fallback mechanism, meaning they will only be utilized if no other S3 configuration is found via a configuration file or the filer. This change simplifies the initial setup process for users who do not provide explicit configuration files, ensuring a clear hierarchy for credential sourcing.
Highlights
- Credential Priority Change: The S3 API server's logic for loading admin credentials has been updated to prioritize explicit configuration files or filer-based settings. Environment variables are now considered a lower-priority fallback.
- Environment Variable Fallback: The
AWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEYenvironment variables will now only be used to configure S3 admin credentials if no other configuration (from a specified file or the filer) is successfully loaded. This simplifies initial setup when no explicit configuration is present. - Exclusive Fallback Configuration: When environment variables are utilized as a fallback, they now establish the sole S3 admin identity, replacing any potential partial or empty configurations, rather than merely supplementing them.
- Documentation Update: The command-line help text for
weed s3has been revised to accurately reflect that environment variables now serve as 'fallback' credentials, clarifying their new role in the configuration hierarchy. - Unit Test Alignment: Unit tests for S3 credential management have been updated to align with the new fallback logic, including clearer test case descriptions and more direct assertions that expect the environment variable identity to be the only one loaded in fallback scenarios.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes modify the loading of S3 admin credentials, prioritizing configuration files and filer configurations over environment variables. Environment variables are now used as a fallback mechanism only when no other configuration is available. The changes include updates to the s3.go command, auth_credentials.go, and auth_credentials_test.go to reflect this new behavior.
weed/s3api/auth_credentials.go
Outdated
| // Set as the only configuration | ||
| iam.m.Lock() | ||
| iam.identities = append(iam.identities, envIdentity) | ||
| iam.accessKeyIdent[accessKeyId] = envIdentity | ||
| if !iam.isAuthEnabled { | ||
| iam.isAuthEnabled = true | ||
| } | ||
| iam.identities = []*Identity{envIdentity} | ||
| iam.accessKeyIdent = map[string]*Identity{accessKeyId: envIdentity} | ||
| iam.isAuthEnabled = true | ||
| iam.m.Unlock() |
There was a problem hiding this comment.
Since environment variables are now the fallback, it might be more appropriate to set the admin identity only if no other identities exist. This would prevent potential conflicts or unexpected behavior if a configuration file or filer configuration is present but doesn't define any identities.
| // Set as the only configuration | |
| iam.m.Lock() | |
| iam.identities = append(iam.identities, envIdentity) | |
| iam.accessKeyIdent[accessKeyId] = envIdentity | |
| if !iam.isAuthEnabled { | |
| iam.isAuthEnabled = true | |
| } | |
| iam.identities = []*Identity{envIdentity} | |
| iam.accessKeyIdent = map[string]*Identity{accessKeyId: envIdentity} | |
| iam.isAuthEnabled = true | |
| iam.m.Unlock() | |
| // Set as the only configuration if no other identities exist | |
| iam.m.Lock() | |
| if len(iam.identities) == 0 { | |
| iam.identities = []*Identity{envIdentity} | |
| iam.accessKeyIdent = map[string]*Identity{accessKeyId: envIdentity} | |
| iam.isAuthEnabled = true | |
| } | |
| iam.m.Unlock() |
What problem are we solving?
How are we solving the problem?
How is the PR tested?
Checks