-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Confcom Release 1.5.0 #9485
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.
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
Confcom Release 1.5.0 #9485
Conversation
Addresses - Azure#9222 - [x] Update the code to restore the "attach to first image in input" behaviour - [x] Add two new commands: `fragment push` and `fragment attach` to allow the user to explicitly do one or the other (or both!) - [x] Add new tests which run a local docker registry, and test that the fragments are generated, signed, pushed and attached as expected (as well as the default behaviour) --- This checklist is used to make sure that common guidelines for a pull request are followed. <!--- Please provide the related command with az {command} if you can, so that we can quickly route to the related person to review. ---> - [x] Have you run `azdev style <YOUR_EXT>` locally? (`pip install azdev` required) - [x] Have you run `python scripts/ci/test_index.py -q` locally? (`pip install wheel==0.30.0` required) - [x] My extension version conforms to the [Extension version schema](https://github.com/Azure/azure-cli/blob/release/doc/extensions/versioning_guidelines.md)
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| confcom acifragmentgen | cmd confcom acifragmentgen added parameter out_signed_fragment |
||
| confcom fragment | sub group confcom fragment added |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
|
@necusjz Here is the fixed PR. I've both addressed the source of the error and the bug in the tests that allowed it to slip through. The issue here is CI runs against the source code not the final built package. For confcom, I've tried to implement something that approximates this via pytest fixtures, but it would be amazing if it the CI natively did this |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR bumps the confcom extension version from 1.4.5 to 1.5.0, introducing new commands for fragment management and various bug fixes and improvements to the test infrastructure.
Key Changes
- Added
confcom fragment pushandconfcom fragment attachcommands for explicit fragment management - Restored
--upload-fragmentbehavior to attach fragments to the first image in input - Improved certificate chain creation script to support custom output directories
- Enhanced test infrastructure with better isolation and cleanup coordination
- Fixed bugs related to temporary file paths in tests
- Added validation for bundle ID format in policy rules
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Version bump from 1.4.5 to 1.5.0 |
| samples/certs/create_certchain.sh | Added support for custom output directory parameter and updated all paths to use the new variable |
| test_confcom_virtual_node.py | Updated certificate generation to use temp directory with script argument, changed from shell=True to shell=False |
| test_confcom_tar.py | Fixed bug where temp file was created in wrong directory |
| test_confcom_fragment.py | Fixed bug where temp file was created in wrong directory, updated certificate generation similar to virtual_node tests |
| test_confcom_arm.py | Added locking mechanism for docker container cleanup to prevent race conditions in parallel test execution |
| test_confcom_acifragmentgen.py | New test file for fragment generation, signing, uploading, pushing, and attaching functionality |
| conftest.py | Updated module path replacement logic and added assertion for built package validation |
| rules.rego | Extracted bundle ID regex pattern as a constant for reusability and added validation to bundle path checking |
| genpolicy-settings.json | Added comprehensive configuration file for policy generation settings |
| custom.py | Added out_signed_fragment parameter support, new wrapper functions for fragment_push and fragment_attach, fixed upload_fragment behavior |
| commands.py | Registered new fragment push and attach commands under confcom fragment group |
| fragment_push.py | New command implementation for pushing standalone fragments to registry |
| fragment_attach.py | New command implementation for attaching fragments to container images |
| command/init.py | New package initialization file |
| _params.py | Added parameter definitions for fragment push/attach commands with stdin support |
| _help.py | Added help documentation for new fragment commands with usage examples |
| HISTORY.rst | Updated release notes for version 1.5.0 |
| linter_exclusions.yml | Added exclusions for positional parameters in new fragment commands |
| text: az confcom acifragmentgen --chain my.cert.pem --key my_key.pem --svn "1" --namespace contoso --feed "test-feed" --input ./fragment_spec.json | az confcom fragment push --manifest-tag myregistry.azurecr.io/fragment:latest | ||
| """ | ||
|
|
||
| helps[ | ||
| "confcom fragment attach" | ||
| ] = """ | ||
| type: command | ||
| short-summary: Attach a Confidential Container Policy Fragment to an image in an ORAS registry. | ||
| parameters: | ||
| - name: --manifest-tag | ||
| type: string | ||
| short-summary: 'The reference to attach the signed fragment to' | ||
| examples: | ||
| - name: Attach a signed fragment to a registry | ||
| text: az confcom fragment attach ./fragment.reg.cose --manifest-tag myregistry.azurecr.io/image:latest | ||
| - name: Attach the output of acifragmentgen to a registry | ||
| text: az confcom acifragmentgen --chain my.cert.pem --key my_key.pem --svn "1" --namespace contoso --feed "test-feed" --input ./fragment_spec.json | az confcom fragment attach --manifest-tag myregistry.azurecr.io/image:latest |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing required flag in help examples. The examples on lines 304 and 322 show piping acifragmentgen output to fragment push/attach commands, but the acifragmentgen command is missing the --out-signed-fragment flag that causes it to output the signed fragment to stdout. Without this flag, acifragmentgen will write files to disk and print text output instead of binary fragment data. Add --out-signed-fragment to these example commands.
| registry_id = subprocess.run( | ||
| ["docker", "run", "-d", "-p", "0:5000", "registry:2"], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ).stdout | ||
|
|
||
| registry_port = subprocess.run( | ||
| ["docker", "port", registry_id], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ).stdout.split(":")[-1].strip() | ||
|
|
||
| test_container_ref = f"localhost:{registry_port}/hello-world:latest" | ||
| subprocess.run(["docker", "pull", "hello-world"]) | ||
| subprocess.run(["docker", "tag", "hello-world", test_container_ref]) | ||
| subprocess.run(["docker", "push", test_container_ref]) | ||
|
|
||
| with tempfile.NamedTemporaryFile(mode="w+", encoding="utf-8", delete=True) as temp_file: | ||
| json.dump({ | ||
| "version": "1.0.0", | ||
| "containers": [ | ||
| { | ||
| "name": "hello-world", | ||
| "properties": { | ||
| "image": test_container_ref, | ||
| }, | ||
| } | ||
| ] | ||
| }, temp_file) | ||
| temp_file.flush() | ||
|
|
||
| yield test_container_ref, temp_file.name | ||
|
|
||
| subprocess.run(["docker", "stop", registry_id]) | ||
|
|
||
|
|
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docker container cleanup missing error handling. If the fixture fails before yielding (e.g., during docker run, pull, tag, or push), the registry container will not be stopped and removed, potentially leaving orphaned containers. Consider wrapping the setup in a try/finally block or using a context manager to ensure cleanup occurs even on failure.
| registry_id = subprocess.run( | |
| ["docker", "run", "-d", "-p", "0:5000", "registry:2"], | |
| stdout=subprocess.PIPE, | |
| text=True, | |
| ).stdout | |
| registry_port = subprocess.run( | |
| ["docker", "port", registry_id], | |
| stdout=subprocess.PIPE, | |
| text=True, | |
| ).stdout.split(":")[-1].strip() | |
| test_container_ref = f"localhost:{registry_port}/hello-world:latest" | |
| subprocess.run(["docker", "pull", "hello-world"]) | |
| subprocess.run(["docker", "tag", "hello-world", test_container_ref]) | |
| subprocess.run(["docker", "push", test_container_ref]) | |
| with tempfile.NamedTemporaryFile(mode="w+", encoding="utf-8", delete=True) as temp_file: | |
| json.dump({ | |
| "version": "1.0.0", | |
| "containers": [ | |
| { | |
| "name": "hello-world", | |
| "properties": { | |
| "image": test_container_ref, | |
| }, | |
| } | |
| ] | |
| }, temp_file) | |
| temp_file.flush() | |
| yield test_container_ref, temp_file.name | |
| subprocess.run(["docker", "stop", registry_id]) | |
| registry_id = None | |
| try: | |
| registry_id = subprocess.run( | |
| ["docker", "run", "-d", "-p", "0:5000", "registry:2"], | |
| stdout=subprocess.PIPE, | |
| text=True, | |
| ).stdout | |
| registry_port = subprocess.run( | |
| ["docker", "port", registry_id], | |
| stdout=subprocess.PIPE, | |
| text=True, | |
| ).stdout.split(":")[-1].strip() | |
| test_container_ref = f"localhost:{registry_port}/hello-world:latest" | |
| subprocess.run(["docker", "pull", "hello-world"]) | |
| subprocess.run(["docker", "tag", "hello-world", test_container_ref]) | |
| subprocess.run(["docker", "push", test_container_ref]) | |
| with tempfile.NamedTemporaryFile(mode="w+", encoding="utf-8", delete=True) as temp_file: | |
| json.dump({ | |
| "version": "1.0.0", | |
| "containers": [ | |
| { | |
| "name": "hello-world", | |
| "properties": { | |
| "image": test_container_ref, | |
| }, | |
| } | |
| ] | |
| }, temp_file) | |
| temp_file.flush() | |
| yield test_container_ref, temp_file.name | |
| finally: | |
| if registry_id: | |
| # Best-effort cleanup of the registry container; ignore errors if it's already stopped/removed. | |
| subprocess.run(["docker", "stop", registry_id], check=False) | |
| subprocess.run(["docker", "rm", registry_id], check=False) |
| registry_id = subprocess.run( | ||
| ["docker", "run", "-d", "-p", "0:5000", "registry:2"], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ).stdout | ||
|
|
||
| registry_port = subprocess.run( | ||
| ["docker", "port", registry_id], | ||
| stdout=subprocess.PIPE, | ||
| text=True, | ||
| ).stdout.split(":")[-1].strip() | ||
|
|
||
| test_container_ref = f"localhost:{registry_port}/hello-world:latest" | ||
| subprocess.run(["docker", "pull", "hello-world"]) | ||
| subprocess.run(["docker", "tag", "hello-world", test_container_ref]) | ||
| subprocess.run(["docker", "push", test_container_ref]) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for subprocess calls. The docker commands (run, port, pull, tag, push) do not check for errors, which could lead to silent failures or cryptic errors later in the test. The registry_port extraction on line 33 will fail with an IndexError if the 'docker port' command fails or returns unexpected output. Add check=True or explicit error handling for these subprocess calls.
| "push", | ||
| "--artifact-type", "application/x-ms-ccepolicy-frag", | ||
| manifest_tag, | ||
| os.path.relpath(signed_fragment.name, start=os.getcwd()), |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File path handling issue with relative paths. Using os.path.relpath with start=os.getcwd() can produce incorrect results if the current working directory changes between when the file is created and when oras is invoked. Additionally, if signed_fragment.name is an absolute path, the relpath call may produce unexpected results. Consider either using the absolute path directly or ensuring the working directory is consistent.
| os.path.relpath(signed_fragment.name, start=os.getcwd()), | |
| os.path.abspath(signed_fragment.name), |
| "attach", | ||
| "--artifact-type", "application/x-ms-ccepolicy-frag", | ||
| manifest_tag, | ||
| os.path.relpath(signed_fragment.name, start=os.getcwd()), |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File path handling issue with relative paths. Using os.path.relpath with start=os.getcwd() can produce incorrect results if the current working directory changes between when the file is created and when oras is invoked. Additionally, if signed_fragment.name is an absolute path, the relpath call may produce unexpected results. Consider either using the absolute path directly or ensuring the working directory is consistent.
| os.path.relpath(signed_fragment.name, start=os.getcwd()), | |
| os.path.abspath(signed_fragment.name), |
| not any (m.__name__.startswith(module.__name__) for m in modules_to_test) | ||
| ): | ||
| del sys.modules[module.__name__] | ||
| assert importlib.util.find_spec(module.__name__).origin # This catches modules not in the built packages |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertion may fail during module cleanup. The assertion on line 91 checks if importlib.util.find_spec can find the module after it's been deleted from sys.modules. However, if the module is not in the built package (which is what the comment suggests this is trying to catch), the assertion will raise an AttributeError when trying to access .origin on None, rather than failing the assertion. Consider checking if find_spec returns None first before accessing .origin.
| assert importlib.util.find_spec(module.__name__).origin # This catches modules not in the built packages | |
| spec = importlib.util.find_spec(module.__name__) | |
| assert spec is not None and spec.origin # This catches modules not in the built packages |
| except (docker.errors.APIError, requests.exceptions.ReadTimeout) as exc: | ||
| # Ignore conflicts (another prune in flight) or slow daemon timeouts. | ||
| status = getattr(getattr(exc, "response", None), "status_code", None) | ||
| if status not in (409, None) or not isinstance(exc, requests.exceptions.ReadTimeout): |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling logic may suppress unintended exceptions. The condition on line 5023 uses 'or' instead of 'and', which means it will suppress the exception if EITHER the status is not 409/None OR it's not a ReadTimeout. This is likely incorrect - the intent appears to be to ignore conflicts (status 409) OR timeouts (ReadTimeout), but the current logic will re-raise the exception only if status is not in (409, None) AND it's not a ReadTimeout. Consider changing to: 'if status not in (409, None) and not isinstance(exc, requests.exceptions.ReadTimeout):'
| if status not in (409, None) or not isinstance(exc, requests.exceptions.ReadTimeout): | |
| if status not in (409, None) and not isinstance(exc, requests.exceptions.ReadTimeout): |
|
[Release] Update index.json for extension [ confcom ] : https://dev.azure.com/msazure/One/_build/results?buildId=146970829&view=results |
Why
This PR contains all currently ready PRs for the confcom extension. They are grouped together to speed up merging and releasing time.
How
The only commits in this PR should be merged PRs for individual changes, and merges from main.
This checklist is used to make sure that common guidelines for a pull request are followed.
Related command
General Guidelines
azdev style <YOUR_EXT>locally? (pip install azdevrequired)python scripts/ci/test_index.py -qlocally? (pip install wheel==0.30.0required)