Define a config manager for registry auths #2326
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughIntroduces Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Pipeline
participant RC as RegistryConfig
participant DefaultAuth as Default Auth File
participant AddlAuth as Additional Auth Sources
participant TempFile as Temp Auth File
participant Tools as Container Tools
User->>RC: __enter__() - initialize context
activate RC
RC->>DefaultAuth: Read default auth (or use default_auth_file)
DefaultAuth-->>RC: Credentials dict
RC->>RC: Apply filter_patterns to default creds
RC->>AddlAuth: Read additional sources (env/params)
AddlAuth-->>RC: Additional credentials dict
RC->>RC: Apply filter_patterns to additional creds
RC->>RC: Merge credentials (additional overrides default)
rect rgba(100, 150, 200, 0.2)
note over RC: Merge Phase
RC->>TempFile: Write merged credentials to temp JSON
TempFile-->>RC: temp file path (registry-auth-*.json)
end
RC-->>User: Return temp file path
User->>Tools: Use temp file path in tool invocation<br/>(e.g., --registry-auth flag)
Tools->>TempFile: Read credentials from temp file
TempFile-->>Tools: Merged credentials
User->>RC: __exit__() - cleanup
deactivate RC
RC->>TempFile: Delete temporary file
TempFile-->>RC: File deleted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
locriandev
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
locriandev
force-pushed
the
registry-config-context-manager
branch
from
4f46df1 to
c504b19
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
artcommon/artcommonlib/registry_config.py (5)
106-107: Consider usinglogging.exceptionfor better error context.When logging errors during exception handling,
logging.exceptionautomatically includes the stack trace, which aids in debugging.🔎 Proposed change
except Exception as e: - self.logger.error(f'Failed to clean up temporary auth file {self.temp_auth_file}: {e}') + self.logger.exception(f'Failed to clean up temporary auth file {self.temp_auth_file}')Based on static analysis hints.
143-145: Uselogging.exceptionfor better error diagnostics.Similar to other exception handlers, using
logging.exceptionwill automatically include the traceback.🔎 Proposed change
except Exception as e: - self.logger.error(f'Failed to read existing auth file: {e}') + self.logger.exception('Failed to read existing auth file') raiseBased on static analysis hints.
202-204: Uselogging.exceptionfor consistency.For consistency with other error handlers and better debugging, use
logging.exception.🔎 Proposed change
except Exception as e: - self.logger.error(f'Failed to read auth file from {source_env_var}: {e}') + self.logger.exception(f'Failed to read auth file from {source_env_var}') raiseBased on static analysis hints.
236-242: Improve error handling in cleanup code.The bare
exceptclause silently suppresses all exceptions during cleanup. While this prevents cascading failures, it makes debugging difficult.🔎 Proposed improvement
except Exception: # Clean up on error if self.temp_auth_file and self.temp_auth_file.exists(): try: self.temp_auth_file.unlink() - except: - pass + except Exception as cleanup_error: + self.logger.debug(f'Failed to clean up temp file during error handling: {cleanup_error}') raiseBased on static analysis hints.
231-234: Consider extracting the error message to improve maintainability.The long error message embedded in the RuntimeError constructor could be extracted to a variable or formatted string for better readability.
🔎 Proposed refactor
if len(verify_auths) != len(self.merged_auths): - raise RuntimeError( - f'Verification failed: Expected {len(self.merged_auths)} entries, got {len(verify_auths)}' - ) + error_msg = f'Verification failed: Expected {len(self.merged_auths)} entries, got {len(verify_auths)}' + raise RuntimeError(error_msg)Based on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
artcommon/artcommonlib/registry_config.pydoozer/doozerlib/cli/images_streams.py
🧰 Additional context used
🪛 Ruff (0.14.10)
artcommon/artcommonlib/registry_config.py
106-106: Do not catch blind exception: Exception
(BLE001)
107-107: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
144-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
203-203: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
232-234: Abstract raise to an inner function
(TRY301)
232-234: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Do not use bare except
(E722)
241-242: try-except-pass detected, consider logging the exception
(S110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (4)
artcommon/artcommonlib/registry_config.py (3)
40-69: LGTM: Clean initialization with sensible defaults.The constructor properly handles optional parameters and provides a good fallback for determining the default auth file location using XDG_RUNTIME_DIR.
254-268: Clarify the credential matching logic.The bidirectional
startswithcheck at lines 267-268 implements parent/child registry matching, but the behavior might be surprising:
registry='quay.io'matchesauth_key='quay.io/org/repo'✓registry='quay.io/org/repo'matchesauth_key='quay.io'✓While this may be intentional to handle both specific and broad credentials, consider:
- Adding a docstring example demonstrating this behavior
- Verifying this logic matches container tool behavior for registry auth lookup
# Example test cases to verify expected behavior: # Case 1: Specific credential for broader query assert has_credential_for('quay.io') # when auth has 'quay.io/redhat' # Case 2: Broad credential for specific query assert has_credential_for('quay.io/redhat/repo') # when auth has 'quay.io'Would this match the authentication behavior of the container tools (oc, podman, etc.) that will consume this config?
214-216: Document the design assumption that the current working directory must be writable.Creating temporary files in the current directory (
.) requires that the working directory is always writable and accessible to the process. If this is an intentional design choice for container deployment or container image build tools, add a comment explaining this requirement clearly. Also, consider if environment-based temp directory fallback would provide better robustness without sacrificing the use case.Additionally, the bare
exceptclause in the cleanup handler (lines 241–246) silently swallows all errors. Useexcept OSErroror similar to only catch file operation errors, allowing other exceptions to propagate for debugging.doozer/doozerlib/cli/images_streams.py (1)
246-257: Verify the reordered upstream_image_mirror logic.The reordering of upstream_image_mirror mirroring to occur AFTER the primary mirror operation (rather than before) appears logical and ensures all mirror destinations receive the same version. However, this is a significant behavior change that should be thoroughly tested.
Please verify:
- All downstream consumers of
upstream_image_mirrordestinations are compatible with this timing change- The mirroring completes successfully in the new order
- No race conditions are introduced where destinations might be accessed before mirroring completes
#!/bin/bash # Search for any code that might depend on the old mirroring order rg -n -C3 -g '!*.pyc' 'upstream_image_mirror' --type=py
locriandev
force-pushed
the
registry-config-context-manager
branch
2 times, most recently
from
93b8da7 to
e9508c0
Compare
locriandev
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
locriandev
force-pushed
the
registry-config-context-manager
branch
from
a617c02 to
b5c5aeb
Compare
|
@locriandev: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pyartcd/pyartcd/pipelines/ocp4_konflux.py (1)
395-400: Consider validating required registries for CI mirroring.The current implementation merges all available credentials without validating that the required registries are present. Based on the PR description, this mirror operation needs credentials for
registry.ci.openshift.organdquay.io/openshift/cito successfully mirror ART images to CI.Consider using the
registriesparameter to ensure required credentials are available:🔎 Proposed enhancement
- # Use RegistryConfig to create a temporary auth file with merged credentials - with RegistryConfig() as auth_file: + # Use RegistryConfig to create a temporary auth file with merged credentials + # Validate that required CI registry credentials are present + with RegistryConfig( + registries=['registry.ci.openshift.org', 'quay.io/openshift/ci'] + ) as auth_file: # Mirror out ART equivalent images to CI cmd = self._doozer_base_command.copy() cmd.extend(['images:streams', 'mirror', '--registry-auth', auth_file])This will fail fast with a clear error message if the required credentials are missing, rather than allowing the mirror operation to fail partway through.
artcommon/artcommonlib/registry_config.py (3)
111-130: Improve exception logging in cleanup.The error handler on Line 127 should use
logging.exceptioninstead oflogging.errorto automatically include the exception traceback, which is helpful for debugging cleanup failures.🔎 Proposed improvement
except Exception as e: - self.logger.error(f'Failed to clean up temporary auth file {self.temp_auth_file}: {e}') + self.logger.exception(f'Failed to clean up temporary auth file {self.temp_auth_file}')Based on static analysis hints.
132-178: Improve exception logging for default credential errors.Line 173 should use
logging.exceptioninstead oflogging.errorto automatically capture the full exception traceback, making it easier to diagnose credential loading failures.🔎 Proposed improvement
except Exception as e: - self.logger.error(f'Failed to read existing auth file: {e}') + self.logger.exception('Failed to read existing auth file') raiseBased on static analysis hints.
180-245: Improve exception logging for additional credential errors.Line 241 should use
logging.exceptioninstead oflogging.errorto automatically capture the full exception traceback, which is especially important when debugging issues with environment-specified auth files.🔎 Proposed improvement
except Exception as e: - self.logger.error(f'Failed to read auth file from {source_env_var}: {e}') + self.logger.exception(f'Failed to read auth file from {source_env_var}') raiseBased on static analysis hints.
artcommon/tests/test_registry_config.py (1)
49-50: Consider specific exception handling in tearDown.The bare
exceptclause could be replaced with a specific exception type to avoid accidentally suppressing unexpected errors during test cleanup.🔎 Optional improvement
for file in Path('.').glob('registry-auth-*.json'): try: file.unlink() - except: + except OSError: passBased on static analysis hints.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
artcommon/artcommonlib/registry_config.pyartcommon/tests/test_registry_config.pypyartcd/pyartcd/pipelines/ocp4_konflux.py
🧰 Additional context used
🧬 Code graph analysis (1)
pyartcd/pyartcd/pipelines/ocp4_konflux.py (1)
artcommon/artcommonlib/registry_config.py (1)
RegistryConfig(17-328)
🪛 Ruff (0.14.10)
artcommon/tests/test_registry_config.py
49-49: Do not use bare except
(E722)
49-50: try-except-pass detected, consider logging the exception
(S110)
118-118: Abstract raise to an inner function
(TRY301)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
artcommon/artcommonlib/registry_config.py
126-126: Do not catch blind exception: Exception
(BLE001)
127-127: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
173-173: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
241-241: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
262-264: Avoid specifying long messages outside the exception class
(TRY003)
291-293: Abstract raise to an inner function
(TRY301)
291-293: Avoid specifying long messages outside the exception class
(TRY003)
300-300: Do not use bare except
(E722)
300-301: try-except-pass detected, consider logging the exception
(S110)
🔇 Additional comments (10)
pyartcd/pyartcd/pipelines/ocp4_konflux.py (1)
15-15: LGTM: Clean import of RegistryConfig.The import is properly placed and follows the existing import structure.
artcommon/artcommonlib/registry_config.py (4)
17-85: LGTM: Well-designed initialization with clear documentation.The class docstring provides excellent examples of the three usage modes (filter patterns, required registries, and no filtering). The initialization logic correctly handles default auth file location using XDG_RUNTIME_DIR with a sensible fallback.
86-109: LGTM: Clean context manager entry.The
__enter__method follows a clear sequence of operations and returns the temporary file path as expected. The flow is easy to follow and handles both filtering and validation modes correctly.
247-266: LGTM: Clear validation with helpful error messages.The validation logic correctly identifies missing registries and provides a helpful error message that includes both the missing registries and the available ones. This makes debugging credential issues much easier.
304-328: LGTM: Useful utility methods with bidirectional matching.The
get_registries()andhas_credential_for()methods provide convenient access to the merged credentials. The bidirectional prefix matching inhas_credential_for()is particularly useful for handling both parent and child registry paths.artcommon/tests/test_registry_config.py (5)
16-51: LGTM: Well-structured test fixtures.The setUp and tearDown methods properly initialize test data and clean up temporary files. The test fixtures cover realistic authentication scenarios with multiple registries.
52-179: LGTM: Comprehensive initialization and filtering tests.The tests thoroughly validate initialization with default and custom values, and verify filtering behavior with single and multiple patterns. The test coverage for credential filtering is excellent.
180-288: LGTM: Thorough testing of additional credentials and edge cases.These tests comprehensively cover additional credential sources, including handling of missing environment variables, non-existent files, and filtering interactions. The edge case coverage is excellent.
289-346: LGTM: Complete validation of utility methods.The tests for
get_registries()andhas_credential_for()verify both exact and prefix matching behavior. The bidirectional prefix matching tests are particularly valuable.
347-577: LGTM: Excellent coverage of advanced features and error cases.The tests comprehensively validate:
- Temporary file location and naming patterns
- JSON format validation
- Invalid JSON handling for both default and additional sources
- Credential override behavior
- The
registriesparameter with validation- Integration of registries parameter with additional sources
This is thorough testing that validates all the key features and error scenarios.
| def _write_merged_credentials(self): | ||
| """ | ||
| Write merged credentials to a temporary file in the current directory. | ||
| """ | ||
| try: | ||
| # Create temporary file in current directory with delete=False so we control deletion | ||
| fd, temp_path = tempfile.mkstemp(prefix='registry-auth-', suffix='.json', dir='.', text=True) | ||
|
|
||
| self.temp_auth_file = Path(temp_path) | ||
|
|
||
| # Write the merged credentials | ||
| merged_config = {'auths': self.merged_auths} | ||
| with os.fdopen(fd, 'w') as f: | ||
| json.dump(merged_config, f, indent=2) | ||
|
|
||
| self.logger.debug(f'Wrote {len(self.merged_auths)} total auth entries to {self.temp_auth_file}') | ||
| self.logger.info(f'Enabled registries: {list(self.merged_auths.keys())}') | ||
|
|
||
| # Verify writing was successful | ||
| with open(self.temp_auth_file, 'r') as f: | ||
| verify_config = json.load(f) | ||
| verify_auths = verify_config.get('auths', {}) | ||
| if len(verify_auths) != len(self.merged_auths): | ||
| raise RuntimeError( | ||
| f'Verification failed: Expected {len(self.merged_auths)} entries, got {len(verify_auths)}' | ||
| ) | ||
|
|
||
| except Exception: | ||
| # Clean up on error | ||
| if self.temp_auth_file and self.temp_auth_file.exists(): | ||
| try: | ||
| self.temp_auth_file.unlink() | ||
| except: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace bare except with specific exception type.
Lines 300-301 use a bare except clause which catches all exceptions including system exits and keyboard interrupts. This should catch a specific exception type (e.g., OSError, Exception) to avoid suppressing unexpected errors during cleanup.
🔎 Proposed fix
except Exception:
# Clean up on error
if self.temp_auth_file and self.temp_auth_file.exists():
try:
self.temp_auth_file.unlink()
- except:
+ except OSError:
pass
raiseBased on static analysis hints.
🧰 Tools
🪛 Ruff (0.14.10)
291-293: Abstract raise to an inner function
(TRY301)
291-293: Avoid specifying long messages outside the exception class
(TRY003)
300-300: Do not use bare except
(E722)
300-301: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In artcommon/artcommonlib/registry_config.py around lines 268-302, the cleanup
block currently uses a bare "except:" which can catch BaseException subclasses
like SystemExit/KeyboardInterrupt; change the outer bare except to "except
Exception:" so only normal exceptions are caught during write/verify and
re-raised, and change the inner cleanup's bare except when unlinking the temp
file to catch a more specific exception such as "except OSError:" (or "except
Exception:" if you prefer broader but not BaseException) so interrupt signals
are not suppressed.
| Context manager for managing container registry credentials from multiple sources. | ||
|
|
||
| This class merges credentials from: | ||
| 1. Existing default auth file (e.g., /run/user/<uid>/containers/auth.json) |
There was a problem hiding this comment.
The context manager is an excellent direction. I would caution against our continued dependence of a default auth. (a) it makes the source of certain auths implicit (b) it makes the source of the credentials possible to store outside of a trusted credential store (c) someone accidentally authorizing using podmand login when ssh'd in as the jenkins user can mess it up.
Thoughts about an interface like this?
with RegistryConfig(
sources=[
RegistryPath('quay.io',
# The order of preferred ways to find a credential for this registry path.
auth_options=[
RegistryAuth(encoded_auth_str=os.getenv('SOME_JENKINS_SET_ENV_WITH_BASE64')),
# Fall back to user's auth.json, but warn them if it is necessary.
RegistryAuth(auth_file=os.getenv('XDG_RUNTIME_DIR') + '/containers/auth.json', warning=True),
]
),
# Podman supports different credentials different repositories on the same registry. It should choose
# the closest match when authenticating for the repo
RegistryPath('quay.io/openshift/ci',
auth_options=[
# Plain text option that would be base64 encoded.
RegistryAuth(username_password=os.getenv('ANOTHER_JENKINS_SET_ENV_PLAINTEXT'))),
# exact=False means that RegistryConfig will use the longest substring match.
# i.e. quay.io/openshift/ci from auth.json, or quay.io/openshift as a fallback, and finally quay.io.
# In all cases, if the auth file is used, a warning will be issued since we want everything to be
# specifically wired in through credential stores / envs.
RegistryAuth(auth_file=os.getenv('XDG_RUNTIME_DIR') + '/containers/auth.json', warning=True, exact=False),
]
),
],
) as auth_file:
...
Obviously that is more than we want to type for every context manager, so it would be more like:
with RegistryConfig(
sources=[
# Specific cred required for the task
RegistryPath('quay.io',
auth_options=[RegistryAuth(username_password=os.getenv('SPECIAL_JENKINS_SET_ENV_PLAINTEXT')))]),
*doozerlib.COMMON_READ_ONLY_REGISTRY_CONFIG
]
Where registry configure would need to prioritize the first source over a duplicate quay.io RegistryPath in the COMMON_READ_ONLY_REGISTRY_CONFIG list.
2 participants
Add registry config manager
Summary
This PR introduces
RegistryConfig, a context manager for managing container registry credentials from multiple sources, and integrates it into the Konflux OCP4 pipeline for improved credential handling during image mirroring operations.Changes
New Components
1.
artcommonlib/registry_config.pyA new context manager class that provides centralized container registry credential management:
Key Features:
Usage Modes:
quay.io/redhat-user-workloads/)Example:
2.
artcommon/tests/test_registry_config.pyComprehensive unit tests covering:
Integration
pyartcd/pipelines/ocp4_konflux.pyUpdated the
mirror_streams_to_ci()method in theKonfluxOcp4Pipelineclass to useRegistryConfigfor better credential management when mirroring ART equivalent images to CI.Impact:
This change ensures that when the apiserver is rebuilt and streams need to be mirrored to CI, all registry credentials (including those for registry.ci.openshift.org and quay.io/openshift/ci) are properly available to the doozer command in an isolated, temporary auth file.
Testing
Test build: https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-konflux/23085/console