Skip to content

fix(clp-package): Temporarily remove support for implicit AWS credentials. #682

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

Merged
kirkrodrigues merged 5 commits into from
Jan 21, 2025
Merged

fix(clp-package): Temporarily remove support for implicit AWS credentials. #682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c01b29d
fix
haiqi96 Jan 20, 2025
86600d1
Entirely remove support for empty credentials.
kirkrodrigues Jan 21, 2025
ee3839c
Make condition more explicit.
kirkrodrigues Jan 21, 2025
8ab90df
Replace S3InputConfig.input.aws_* with S3Credentials
kirkrodrigues Jan 21, 2025
2b4bb6a
Address missed change.
kirkrodrigues Jan 21, 2025
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
12 changes: 6 additions & 6 deletions components/clp-package-utils/clp_package_utils/scripts/compress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def _generate_compress_cmd(
aws_access_key_id, aws_secret_access_key = _parse_aws_credentials_file(
pathlib.Path(parsed_args.aws_credentials_file), default_credentials_user
)
if aws_access_key_id and aws_secret_access_key:
if bool(aws_access_key_id) and bool(aws_secret_access_key):
compress_cmd.append("--aws-access-key-id")
compress_cmd.append(aws_access_key_id)
compress_cmd.append("--aws-secret-access-key")
Expand Down Expand Up @@ -192,11 +192,11 @@ def _validate_s3_input_args(
" aws_secret_access_key."
)

elif bool(aws_access_key_id) != bool(aws_secret_access_key):
args_parser.error(
"aws_access_key_id and aws_secret_access_key must be specified together or left"
" unspecified."
)
else:
if not bool(aws_access_key_id):
args_parser.error("aws_access_key_id not specified or empty")
if not bool(aws_secret_access_key):
args_parser.error("aws_secret_access_key not specified or empty")


def main(argv):
Expand Down
8 changes: 5 additions & 3 deletions components/clp-package-utils/clp_package_utils/scripts/native/compress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import brotli
import msgpack
from clp_py_utils.clp_config import COMPRESSION_JOBS_TABLE_NAME
from clp_py_utils.clp_config import COMPRESSION_JOBS_TABLE_NAME, S3Credentials
from clp_py_utils.pretty_size import pretty_size
from clp_py_utils.s3_utils import parse_s3_url
from clp_py_utils.sql_adapter import SQL_Adapter
Expand Down Expand Up @@ -148,8 +148,10 @@ def _generate_clp_io_config(
region_code=region_code,
bucket=bucket_name,
key_prefix=key_prefix,
aws_access_key_id=parsed_args.aws_access_key_id,
aws_secret_access_key=parsed_args.aws_secret_access_key,
credentials=S3Credentials(
access_key_id=parsed_args.aws_access_key_id,
secret_access_key=parsed_args.aws_secret_access_key,
),
Comment on lines +151 to +154

@coderabbitai coderabbitai Bot Jan 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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,
),

timestamp_key=parsed_args.timestamp_key,
)
else:
Expand Down
15 changes: 7 additions & 8 deletions components/clp-package-utils/clp_package_utils/scripts/start_clp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,15 +943,14 @@ def start_log_viewer_webui(
)

access_key_id, secret_access_key = s3_config.get_credentials()
if access_key_id is not None and secret_access_key is not None:
container_cmd_extra_opts.extend(
[
"-e",
f"AWS_ACCESS_KEY_ID={access_key_id}",
"-e",
f"AWS_SECRET_ACCESS_KEY={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}",
)
)
Comment on lines +946 to +953

@coderabbitai coderabbitai Bot Jan 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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}",
)
)


settings_json = read_and_update_settings_json(settings_json_path, settings_json_updates)
with open(settings_json_path, "w") as settings_json_file:
Expand Down
8 changes: 4 additions & 4 deletions components/clp-py-utils/clp_py_utils/clp_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ class S3Config(BaseModel):
bucket: str
key_prefix: str

credentials: Optional[S3Credentials]
credentials: S3Credentials

@validator("region_code")
def validate_region_code(cls, field):
Expand All @@ -354,9 +354,9 @@ def validate_key_prefix(cls, field):
raise ValueError('key_prefix must end with "/"')
return field

def get_credentials(self) -> Tuple[Optional[str], Optional[str]]:
if self.credentials is None:
return None, None
# 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.
def get_credentials(self) -> Tuple[str, str]:
return self.credentials.access_key_id, self.credentials.secret_access_key


Expand Down
4 changes: 2 additions & 2 deletions components/clp-py-utils/clp_py_utils/s3_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ def s3_get_object_metadata(s3_input_config: S3InputConfig) -> List[FileMetadata]
s3_client = boto3.client(
"s3",
region_name=s3_input_config.region_code,
aws_access_key_id=s3_input_config.aws_access_key_id,
aws_secret_access_key=s3_input_config.aws_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,
Comment on lines +80 to +81

@coderabbitai coderabbitai Bot Jan 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

)

file_metadata_list: List[FileMetadata] = list()
Expand Down
4 changes: 2 additions & 2 deletions components/job-orchestration/job_orchestration/executor/compress/compression_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ def make_clp_s_command_and_env(
if InputType.S3 == clp_config.input.type:
compression_env_vars = {
**os.environ,
"AWS_ACCESS_KEY_ID": clp_config.input.aws_access_key_id,
"AWS_SECRET_ACCESS_KEY": clp_config.input.aws_secret_access_key,
"AWS_ACCESS_KEY_ID": clp_config.input.credentials.access_key_id,
"AWS_SECRET_ACCESS_KEY": clp_config.input.credentials.secret_access_key,
}
compression_cmd.append("--auth")
compression_cmd.append("s3")
Expand Down
3 changes: 0 additions & 3 deletions components/job-orchestration/job_orchestration/executor/query/extract_stream_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ def _make_clp_s_command_and_env_vars(
))
# fmt: on
aws_access_key_id, aws_secret_access_key = s3_config.get_credentials()
if aws_access_key_id is None or aws_secret_access_key is None:
logger.error("Missing credentials for accessing archives on S3")
return None, None
env_vars = {
**os.environ,
"AWS_ACCESS_KEY_ID": aws_access_key_id,
Expand Down
3 changes: 0 additions & 3 deletions components/job-orchestration/job_orchestration/executor/query/fs_search_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,6 @@ def _make_core_clp_s_command_and_env_vars(
))
# fmt: on
aws_access_key_id, aws_secret_access_key = s3_config.get_credentials()
if aws_access_key_id is None or aws_secret_access_key is None:
logger.error("Missing credentials for accessing archives on S3")
return None, None
env_vars = {
**os.environ,
"AWS_ACCESS_KEY_ID": aws_access_key_id,
Expand Down
4 changes: 2 additions & 2 deletions components/job-orchestration/job_orchestration/scheduler/job_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import typing
from enum import auto

from clp_py_utils.clp_config import S3Credentials
from pydantic import BaseModel, validator
from strenum import LowercaseStrEnum

Expand Down Expand Up @@ -34,8 +35,7 @@ class S3InputConfig(BaseModel):
bucket: str
key_prefix: str

aws_access_key_id: typing.Optional[str] = None
aws_secret_access_key: typing.Optional[str] = None
credentials: S3Credentials


class OutputConfig(BaseModel):
Expand Down