-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[confcom] Fix multiple issues with acipolicygen --diff
#9258
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
base: main
Are you sure you want to change the base?
Conversation
️✔️Azure CLI Extensions Breaking Change Test
|
Hi @DomAyre, |
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>
|
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 fixes multiple issues with the acipolicygen --diff
functionality to handle cases where ARM templates contain existing ccePolicy
fields, particularly when they contain "allow_all" policies that don't define container specifications.
Key changes include:
- Only parsing existing policies when
--diff
mode is enabled - Treating missing container definitions as empty lists rather than hard errors
- Adding comprehensive test coverage for diff functionality
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/confcom/samples/policies/allow_all.rego |
Adds sample allow_all policy for testing |
src/confcom/samples/aci/existing_policy_allow_all/* |
Adds test ARM templates and policies with allow_all configurations |
src/confcom/samples/aci/existing_policy/* |
Adds test ARM templates and policies with existing policy configurations |
src/confcom/azext_confcom/tests/latest/test_confcom_acipolicygen_arm.py |
Adds comprehensive tests for diff functionality |
src/confcom/azext_confcom/template_util.py |
Changes error handling from eprint to logger warning |
src/confcom/azext_confcom/security_policy.py |
Updates policy loading and validation to handle empty container lists |
Hi @DomAyre Release SuggestionsModule: confcom
Notes
|
Why
We recently discovered that running acipolicygen with an allow_all policy already in the
ccePolicy
field causes policy generation to break.This is because of two separate reasons
--diff
)The two issues are tightly coupled so I have decided to fix both in one PR
How
load_policy_from_arm_template_str
code to only attempt to load the existing policy if diff mode is enabled--diff
and assert expected differencesThis 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 azdev
required)python scripts/ci/test_index.py -q
locally? (pip install wheel==0.30.0
required)This is a small change so no version bump is needed