fix(clp-package): Temporarily remove support for implicit AWS credentials. - #682
Conversation
WalkthroughThis pull request enhances error handling and validation for AWS S3 credential management across multiple components. It introduces stricter validation for AWS credentials, ensuring that they are fully specified and validated during S3 operations. The changes encapsulate AWS credentials into a structured format, improving the organization and robustness of credential handling in the codebase. Changes
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if InputType.S3 == clp_config.input.type: | ||
| aws_access_key_id = clp_config.input.aws_access_key_id | ||
| aws_secret_access_key = clp_config.input.aws_secret_access_key | ||
| if aws_access_key_id is None or aws_secret_access_key is None: |
There was a problem hiding this comment.
technically, we shouldn't run into this case. However, clp worker will return weird error message if we ever run into a None=aws_access_key_id here, since a env=None is invalid can cause error for subprocess.run.
Just to be safe, I decided to add the guard here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
357-359: Enhance the TODO comment with specific requirements.The TODO comment could be more specific about the requirements for supporting empty credentials, particularly regarding session token handling.
Consider updating the comment to:
- # TODO: When we support empty credentials, this method should be used to return a tuple that's - # either (None, None) if empty, or the credentials otherwise. + # TODO: When implementing support for implicit AWS credentials: + # 1. Update this method to return Tuple[Optional[str], Optional[str]] + # 2. Add support for session tokens in S3Credentials + # 3. Update credential handling to work with temporary credentials from IAM roles
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/clp-package-utils/clp_package_utils/scripts/compress.py(2 hunks)components/clp-package-utils/clp_package_utils/scripts/native/compress.py(2 hunks)components/clp-package-utils/clp_package_utils/scripts/start_clp.py(1 hunks)components/clp-py-utils/clp_py_utils/clp_config.py(2 hunks)components/clp-py-utils/clp_py_utils/s3_utils.py(1 hunks)components/job-orchestration/job_orchestration/executor/compress/compression_task.py(3 hunks)components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py(0 hunks)components/job-orchestration/job_orchestration/executor/query/fs_search_task.py(0 hunks)components/job-orchestration/job_orchestration/scheduler/job_config.py(2 hunks)
💤 Files with no reviewable changes (2)
- components/job-orchestration/job_orchestration/executor/query/fs_search_task.py
- components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py
🚧 Files skipped from review as they are similar to previous changes (2)
- components/clp-package-utils/clp_package_utils/scripts/compress.py
- components/job-orchestration/job_orchestration/executor/compress/compression_task.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (2)
components/job-orchestration/job_orchestration/scheduler/job_config.py (1)
6-6: Verify the impact of making credentials mandatory.The change from optional AWS credential fields to a mandatory
credentialsattribute effectively prevents the use of implicit credentials, which aligns with the PR objective. However, this is a breaking change that might affect existing configurations.Consider adding a deprecation warning for any code still trying to use implicit credentials, to provide a better migration path.
Also applies to: 38-38
components/clp-py-utils/clp_py_utils/clp_config.py (1)
335-335: LGTM! Consider documenting the breaking change.Making
credentialsnon-optional effectively disables implicit AWS credentials as intended. This is a breaking change that requires all S3 configurations to provide explicit credentials.Let's verify the impact on existing configurations:
✅ Verification successful
The breaking change is safe and well-contained.
The codebase only uses S3 operations when explicitly configured through storage type settings. Making credentials mandatory will fail early during configuration validation, preventing any runtime issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for S3Config usage to identify affected configurations rg -A 5 "S3Config"Length of output: 5100
| credentials=S3Credentials( | ||
| access_key_id=parsed_args.aws_access_key_id, | ||
| secret_access_key=parsed_args.aws_secret_access_key, | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for AWS credentials.
While the code now enforces explicit credentials, it should validate that both credentials are provided before creating the S3Credentials object.
Add validation before creating S3Credentials:
+ if not parsed_args.aws_access_key_id or not parsed_args.aws_secret_access_key:
+ raise ValueError("Both AWS access key ID and secret access key must be provided")
credentials=S3Credentials(
access_key_id=parsed_args.aws_access_key_id,
secret_access_key=parsed_args.aws_secret_access_key,
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| credentials=S3Credentials( | |
| access_key_id=parsed_args.aws_access_key_id, | |
| secret_access_key=parsed_args.aws_secret_access_key, | |
| ), | |
| if not parsed_args.aws_access_key_id or not parsed_args.aws_secret_access_key: | |
| raise ValueError("Both AWS access key ID and secret access key must be provided") | |
| credentials=S3Credentials( | |
| access_key_id=parsed_args.aws_access_key_id, | |
| secret_access_key=parsed_args.aws_secret_access_key, | |
| ), |
| container_cmd_extra_opts.extend( | ||
| ( | ||
| "-e", | ||
| f"AWS_ACCESS_KEY_ID={access_key_id}", | ||
| "-e", | ||
| f"AWS_SECRET_ACCESS_KEY={secret_access_key}", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Add validation for AWS credentials.
The unconditional addition of AWS credentials to container environment variables could lead to issues if the credentials are None. This should be validated first.
Add validation before extending container options:
access_key_id, secret_access_key = s3_config.get_credentials()
+ if not access_key_id or not secret_access_key:
+ raise ValueError("Both AWS access key ID and secret access key must be provided for S3 storage")
container_cmd_extra_opts.extend(
(
"-e",
f"AWS_ACCESS_KEY_ID={access_key_id}",
"-e",
f"AWS_SECRET_ACCESS_KEY={secret_access_key}",
)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| container_cmd_extra_opts.extend( | |
| ( | |
| "-e", | |
| f"AWS_ACCESS_KEY_ID={access_key_id}", | |
| "-e", | |
| f"AWS_SECRET_ACCESS_KEY={secret_access_key}", | |
| ) | |
| ) | |
| if not access_key_id or not secret_access_key: | |
| raise ValueError("Both AWS access key ID and secret access key must be provided for S3 storage") | |
| container_cmd_extra_opts.extend( | |
| ( | |
| "-e", | |
| f"AWS_ACCESS_KEY_ID={access_key_id}", | |
| "-e", | |
| f"AWS_SECRET_ACCESS_KEY={secret_access_key}", | |
| ) | |
| ) |
| aws_access_key_id=s3_input_config.credentials.access_key_id, | ||
| aws_secret_access_key=s3_input_config.credentials.secret_access_key, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing credentials.
While the code now uses the structured credentials format, it should handle the case where credentials might be None.
Add validation before accessing credentials:
+ if not s3_input_config.credentials:
+ raise ValueError("AWS credentials are required")
s3_client = boto3.client(
"s3",
region_name=s3_input_config.region_code,
aws_access_key_id=s3_input_config.credentials.access_key_id,
aws_secret_access_key=s3_input_config.credentials.secret_access_key,
)Committable suggestion skipped: line range outside the PR's diff.
| db_config_file_path: pathlib.Path, | ||
| use_single_file_archive: bool, | ||
| ) -> Tuple[List[str], Optional[Dict[str, str]]]: | ||
| ) -> Tuple[Optional[List[str]], Optional[Dict[str, str]]]: |
There was a problem hiding this comment.
With your change, we won't return optional.
| logger.error(f"Unsupported storage engine {clp_storage_engine}") | ||
| return False, {"error_message": f"Unsupported storage engine {clp_storage_engine}"} | ||
|
|
||
| if compression_cmd is None: |
There was a problem hiding this comment.
same comment as the one above. can't be None
haiqi96
left a comment
haiqi96
left a comment
There was a problem hiding this comment.
Looks all good to me but I can't approve since it's my own PR.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
fix(clp-package): Temporarily remove support for implicit AWS credentials.
kirkrodrigues
changed the title
…ials. (y-scope#682) Co-authored-by: Kirk Rodrigues <2454684+kirkrodrigues@users.noreply.github.com>
2 participants
Description
Our original plan was to support implicit AWS credentials that are associated with machine, such as IAM. However, we don't have session token support ready in CLP-S executable. Depending on how the aws identity is configured, some identities may return a temporary credentials with a session token so we don't have support for such cases.
As a temporary solution, we simply disable all implicit AWS credentials support until we can properly handle session tokens.
Validation performed
Manually tested for
Verified that script returns error requiring credentials to be specified.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
The changes focus on improving input validation and error management for S3-related operations, ensuring more reliable credential handling and command generation.