Conversation
…its own AWS dependencies
…e maintaining backward compatibility for traditional Maven repos
BackendBits
requested review from
GlimmerCape,
chethana-shastry-p and
miyamuraga
as code owners
…_ecr_authorization_token
| python-dateutil==2.8.2 | ||
| PyYAML==6.0.1 | ||
| s3transfer==0.7.0 | ||
| PyYAML>=6.0.2 |
There was a problem hiding this comment.
there is no version management, only strict versions are allowed for now
|
|
||
|
|
||
| def download_sd_by_appver(app_name: str, version: str, plugins: PluginEngine) -> dict[str, object]: | ||
| if 'SNAPSHOT' in version: |
There was a problem hiding this comment.
This needs to be clarified with Evgenii, snapshots have been supported for reg v1 for time, not just now, maybe there is another motivation for this validation
scripts/build_env/process_sd.py
Outdated
| artifact_info = asyncio.run(artifact.check_artifact_async(app_def, artifact.FileExtension.JSON, version)) | ||
| cred = None | ||
| auth_headers = None | ||
| if isinstance(app_def.registry, artifact_models.RegistryV2): |
There was a problem hiding this comment.
Maybe it would be easier to make common parent Registry with common fields and methods like resolve_auth_headers for children. I also don't see any authorization for v1 here.
| logger.info(f"RegDef file: {file}") | ||
| validate_yaml_by_scheme_or_fail(file, str(SCHEMAS_DIR / "regdef.schema.json")) | ||
| content = openYaml(file) | ||
| if isinstance(content, dict) and content.get("version") == REGDEF_V2_VERSION: |
There was a problem hiding this comment.
checking that this is dict is unnecessary
| resolved_version = app_version | ||
| dd_artifact_info = asyncio.run(artifact.check_artifact_async(app_def, FileExtension.JSON, app_version, cred)) | ||
| dd_artifact_info = asyncio.run(artifact.check_artifact_async(app_def, FileExtension.JSON, app_version, | ||
| cred=cred, auth_headers=auth_headers)) |
There was a problem hiding this comment.
maybe you can leave auth_headers headers
| PyGithub==1.55 | ||
| certifi==2022.6.15 | ||
|
|
||
| boto3==1.29.3 |
There was a problem hiding this comment.
why are they deleted? also don't forget to delete in test_requirements.txt
| flatten_dict==0.4.2 | ||
|
|
||
| # V2 RegDef authentication support (GCP/AWS) | ||
| google-auth~=2.34.0 |
There was a problem hiding this comment.
only strict versions are allowed
| class AuthConfig(BaseSchema): | ||
| credentials_id: str | ||
| auth_type: Optional[str] = None | ||
| provider: Optional[Provider] = None |
There was a problem hiding this comment.
provider is mandatory. pls align mandatory and optional of each field with @popoveugene
There was a problem hiding this comment.
also need to fix schema and doc to extend types for provider field @popoveugene
| auth_type: Optional[str] = None | ||
| provider: Optional[Provider] = None | ||
| auth_method: Optional[str] = None | ||
| aws_region: Optional[str] = None |
There was a problem hiding this comment.
not all of them are optional, some have dependencies between each other. @popoveugene
| target_release: str | ||
| snapshot_group: Optional[str] = "" | ||
| release_group: Optional[str] = "" | ||
| is_nexus: bool = False |
There was a problem hiding this comment.
there is no such field is_nexus in the scheme, it is better not to add it
| def resolve_v2_auth_headers(registry: RegistryV2, env_creds: dict) -> Optional[dict]: | ||
| """Resolve V2 registry authConfig into HTTP Authorization headers. | ||
| Returns None for anonymous access.""" | ||
| auth_ref = registry.maven_config.auth_config |
There was a problem hiding this comment.
rename to pointer like in doc
| def _is_anonymous(cred_data: dict) -> bool: | ||
| return (not cred_data.get(CRED_FIELD_USERNAME) | ||
| and not cred_data.get(CRED_FIELD_PASSWORD) | ||
| and not cred_data.get(CRED_FIELD_SECRET)) |
There was a problem hiding this comment.
password and secret can't be in 1 cred at the same time
| return None | ||
|
|
||
| if auth_cfg.provider == Provider.AWS: | ||
| if auth_cfg.auth_method not in (AUTH_METHOD_SECRET, AUTH_METHOD_ASSUME_ROLE): |
There was a problem hiding this comment.
in original issue i see only secret method is available for aws
| def _aws_bearer(auth_cfg: AuthConfig, cred_data: dict) -> dict: | ||
| """Get ECR authorization token and return as Bearer token header.""" | ||
| if not auth_cfg.aws_region: | ||
| raise ValueError("AWS authConfig must specify 'awsRegion'") |
There was a problem hiding this comment.
so this field in the model should be mandatory, right?
|
|
||
| token = provider.get_ecr_authorization_token() | ||
|
|
||
| logger.info(f"AWS ECR token obtained for region '{auth_cfg.aws_region}'") |
There was a problem hiding this comment.
maybe it's better not to clog up logs and put them on debug level
| return _aws_bearer(auth_cfg, cred_data) | ||
|
|
||
| if auth_cfg.provider == Provider.GCP: | ||
| if auth_cfg.auth_method == AUTH_METHOD_FEDERATION: |
There was a problem hiding this comment.
in original issue i see only service_account method is available for gcp
| logger.info(f"Resolving basic auth for {auth_cfg.provider.value} registry '{registry.name}'") | ||
| return _basic_auth(cred_data) | ||
|
|
||
| if auth_cfg.auth_method == AUTH_METHOD_USER_PASS or auth_cfg.provider is None: |
There was a problem hiding this comment.
checking nexus and artifactory providers is the same with checking auth method == user_pass. It is necessary to leave only way
| curl \ | ||
| wget | ||
| wget \ | ||
| git |
There was a problem hiding this comment.
why is git added here?
| app.registry.maven_config.repository_domain_name = registry_url | ||
|
|
||
| auth = BasicAuth(login=cred.username, password=cred.password) if cred else None | ||
| if auth_headers: |
| logger.warning( | ||
| f"[Task {task_id}] [Application: {app.name}: {version}] - Artifact not found at URL {full_url}, status: {response.status}") | ||
|
|
||
| # Fallback for timestamped SNAPSHOT: try using original version as folder name (AWS CodeArtifact stores timestamped SNAPSHOTs in timestamped folders) |
There was a problem hiding this comment.
how do we know that this particular artifact is aws? such condition may be suitable for another artifact type and we will waste time on unnecessary call for another type.
2 participants
Pull Request
Summary
Provide a concise description of what this pull request does and why it is needed.
Issue
Link to the issue(s) this PR addresses (e.g.,
Fixes #123orCloses #456). If no issue exists, explain why this change is necessary.Breaking Change?
If yes, describe the breaking change and its impact (e.g., API changes, behavior changes, or required updates for users).
Scope / Project
Specify the component, module, or project area affected by this change (e.g.,
docs,actions,workflows).Implementation Notes
Provide details on how the change was implemented, including any technical considerations, trade-offs, or notable design decisions. Leave blank if not applicable.
Tests / Evidence
Describe how the changes were verified, including:
Additional Notes
Include any extra information, such as:
Leave blank if not applicable.