Prevent TAGS=scos unless truly CentOS #2322
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 |
WalkthroughAdds SCOS tag safety validation to KonfluxImageInspector class with new methods to extract image environment variables and labels. The validation ensures SCOS-tagged images originate only from CentOS bases, raising a ValueError during initialization if violated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
doozer/tests/test_konflux_image_inspector.py (1)
45-66: Simplify the test to focus on its stated purpose.The test name suggests it's testing environment variable extraction, but it includes an unnecessary try-except block (lines 52-55) that silently catches a ValueError without making any assertions. This adds confusion without value.
Consider removing lines 47-55 and directly testing env parsing with a valid image configuration.
🔎 Simplified test structure
def test_get_image_envs_basic(self): """Test basic environment variable extraction""" - image_info = self._create_image_info(env_vars=['PATH=/usr/bin', 'HOME=/root', 'TAGS=scos']) - build_inspector = self._create_mock_build_record_inspector() - - # This should fail due to SCOS tag check, but let's test just the env parsing - # We'll catch the exception and verify the env method works - try: - inspector = KonfluxImageInspector(self.runtime, image_info, build_inspector) - except ValueError: - pass - - # Create inspector without TAGS to test env parsing - image_info_no_tags = self._create_image_info( + image_info = self._create_image_info( env_vars=['PATH=/usr/bin', 'HOME=/root'], labels={'org.label-schema.vendor': 'Red Hat, Inc.'} ) - inspector = KonfluxImageInspector(self.runtime, image_info_no_tags, build_inspector) + build_inspector = self._create_mock_build_record_inspector() + + inspector = KonfluxImageInspector(self.runtime, image_info, build_inspector) envs = inspector.get_image_envs() self.assertEqual(envs['PATH'], '/usr/bin') self.assertEqual(envs['HOME'], '/root')
📜 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)
doozer/doozerlib/build_info.py(2 hunks)doozer/tests/test_konflux_image_inspector.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
doozer/doozerlib/build_info.py (1)
doozer/doozerlib/backend/pipelinerun_utils.py (1)
labels(296-298)
🪛 Ruff (0.14.8)
doozer/doozerlib/build_info.py
301-305: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (6)
doozer/tests/test_konflux_image_inspector.py (3)
1-20: LGTM!The imports and test setup are appropriate for the test module.
21-43: LGTM!The helper methods effectively create test fixtures with appropriate defaults and parameterization.
67-154: LGTM!The test methods comprehensively cover the SCOS tag validation logic across multiple scenarios:
- Label extraction
- SCOS tag with correct/incorrect vendor labels
- Missing vendor label handling
- Skipping validation when SCOS tag is absent
- Comma-separated TAGS handling
The test coverage is thorough and validates the expected behavior effectively.
doozer/doozerlib/build_info.py (3)
240-241: LGTM!The placement of SCOS tag validation in
__init__is appropriate, ensuring the safety check runs immediately when the inspector is instantiated.
263-276: LGTM!The environment variable extraction logic correctly parses the Env list and handles edge cases where entries lack an
=separator.
278-282: LGTM!The label extraction method correctly returns the image labels dictionary.
| def _validate_scos_tag(self): | ||
| """ | ||
| Validates that if TAGS=scos environment variable is set, the image must be built on a CentOS base. | ||
| This prevents accidentally releasing SCOS-tagged images if they are built for OCP. | ||
| Defends against https://issues.redhat.com/browse/COS-3765 incident. | ||
| :raises ValueError: If TAGS=scos is set but org.label-schema.vendor label is not CentOS | ||
| """ | ||
| envs = self.get_image_envs() | ||
| labels = self.get_image_labels() | ||
|
|
||
| # Check if TAGS environment variable contains 'scos' | ||
| tags_env = envs.get('TAGS', '') | ||
| if tags_env and 'scos' in tags_env: | ||
| # Assert that the vendor label indicates CentOS | ||
| vendor_label = labels.get('org.label-schema.vendor', '') | ||
| if vendor_label != 'CentOS': | ||
| raise ValueError( | ||
| f"SCOS tag safety check failed for image {self.get_pullspec()}: " | ||
| f"Image has TAGS=scos environment variable but org.label-schema.vendor label is '{vendor_label}' (expected 'CentOS'). " | ||
| f"This indicates the image is not built on a CentOS base image and should not be tagged as SCOS." | ||
| ) |
There was a problem hiding this comment.
Fix the SCOS tag detection to use proper parsing instead of substring matching.
Line 297 uses substring matching ('scos' in tags_env) which will produce false positives. For example, tags like "scosmos", "my-scos-tag", or "pscos" would incorrectly match. Since TAGS can be comma-separated (as tested in lines 132-154 of the test file), the code should properly parse the comma-separated list.
🔎 Proposed fix using proper tag parsing
def _validate_scos_tag(self):
"""
Validates that if TAGS=scos environment variable is set, the image must be built on a CentOS base.
This prevents accidentally releasing SCOS-tagged images if they are built for OCP.
Defends against https://issues.redhat.com/browse/COS-3765 incident.
:raises ValueError: If TAGS=scos is set but org.label-schema.vendor label is not CentOS
"""
envs = self.get_image_envs()
labels = self.get_image_labels()
# Check if TAGS environment variable contains 'scos'
tags_env = envs.get('TAGS', '')
- if tags_env and 'scos' in tags_env:
+ if tags_env:
+ # Parse comma-separated tags and check if 'scos' is present
+ tags_list = [tag.strip() for tag in tags_env.split(',')]
+ if 'scos' in tags_list:
- # Assert that the vendor label indicates CentOS
- vendor_label = labels.get('org.label-schema.vendor', '')
- if vendor_label != 'CentOS':
- raise ValueError(
- f"SCOS tag safety check failed for image {self.get_pullspec()}: "
- f"Image has TAGS=scos environment variable but org.label-schema.vendor label is '{vendor_label}' (expected 'CentOS'). "
- f"This indicates the image is not built on a CentOS base image and should not be tagged as SCOS."
- )
+ # Assert that the vendor label indicates CentOS
+ vendor_label = labels.get('org.label-schema.vendor', '')
+ if vendor_label != 'CentOS':
+ raise ValueError(
+ f"SCOS tag safety check failed for image {self.get_pullspec()}: "
+ f"Image has TAGS=scos environment variable but org.label-schema.vendor label is '{vendor_label}' (expected 'CentOS'). "
+ f"This indicates the image is not built on a CentOS base image and should not be tagged as SCOS."
+ )Optional: Consider the static analysis hint about exception message length.
The static analysis tool flagged TRY003 for the long exception message (lines 301-305). While the detailed message is helpful for debugging, you could optionally move it to a custom exception class if you prefer to follow this style guideline strictly. However, given the context and the value of the detailed error message, the current approach is acceptable.
🧰 Tools
🪛 Ruff (0.14.8)
301-305: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
doozer/doozerlib/build_info.py lines 284-305: The current check uses substring
matching ('scos' in tags_env) which yields false positives; instead split the
TAGS value on commas, strip whitespace, and perform a case-insensitive
exact-token comparison against 'scos' (e.g., any(token.strip().lower() == 'scos'
for token in tags_env.split(','))). If a match is found, keep the existing
vendor-label check and raise the ValueError as before (optionally shorten or
move the long message to a custom exception if you want to address static
analysis).
|
@jupierce: 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. |
| if tags_env and 'scos' in tags_env: | ||
| # Assert that the vendor label indicates CentOS | ||
| vendor_label = labels.get('org.label-schema.vendor', '') | ||
| if vendor_label != 'CentOS': |
There was a problem hiding this comment.
I'm a bit confused by this. In the bugged 4.19.18, the scos Env tag was set, but I don't see an org.label-schema.vendor label at all, just a vendor label:
$ oc image info -o json "$(oc adm release info --image-for installer-artifacts quay.io/openshift-release-dev/ocp-release:4.19.18-x86_64)" | jq '.config.config | {Env, Labels}' | grep 'TAGS\|vendor'
"TAGS=scos",
"vendor": "Red Hat, Inc.",I'd expect a guard to look like:
- Ensure CentOS-vendored images have the
scostag. - Ensure that non-CentOS-vendored images do not have the
scostag. - Ensure that OKD release images do have the
scostag (or the Centos vendor). - Ensure that OCP release images do not have the
scostag (or that they do have the Red Hat vendor).
But also 🤷, no worries from me if this is one step in a series of pivots to get us to that kind of place. And also certainly possible that those other guards are either not needed, or exist already, and I'm just not aware.
2 participants
When TAGS includes
scos, the installer images, and potentially others, may behave differently. Described in https://issues.redhat.com/browse/COS-3765 . https://github.com/openshift/installer/blob/4820a1064367a0fd97c2677263662502be16e1fc/hack/build-coreos-manifest.go#L74-L81 .