-
Notifications
You must be signed in to change notification settings - Fork 128
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
Fix/sql allowed authorized networks.rego #126
Fix/sql allowed authorized networks.rego #126
Conversation
…; 'data' is upstream to 'settings' in CAI
…orizedNetworks' does not exist, or contains no content
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed CLA. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Referencing #124 |
…into fix/sql_allowed_authorized_networks.rego
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.
Thanks for the contribution - please make sure tests pass (using make test
). Right now I see two potential issues:
- The package name for the constraint and its test must match
- The test fixtures also need to be updated to properly include the "data" field.
You are welcome. And thanks for the suggestion. I will work on it. |
Merge branch 'master' of https://github.com/forseti-security/policy-library into fix/sql_allowed_authorized_networks.rego
…tworks/assets/data.json; modify package statement of validator/sql_allowed_authorized_networks_test.rego
test fixture and test rego file for sql_allowed_authorized_networks have been updated. All tests passed using |
@morgante any other comments on this? |
This reverts commit f2709a2. Due to inability to install PyYaml, will revert this change and move work to my personal laptop
@morgante does this look good to you now? |
@AdrienWalkowiak @FanchenBao we shouldn't edit the fixture |
If I remember correctly, the real environment CAI has |
@FanchenBao Can you rebase? |
@morgante Just rebased my fork onto current master, but I am still seeing conflict files. |
@FanchenBao You'll have to resolve the conflicts manually. Also probably worth looking at the current implementation on master and seeing if your change still makes sense. |
@FanchenBao Looks like tests fail, likely due to the addition of Can you please run a CAI export and verify that it actually has that structure? Then make sure |
@morgante Yes, at least from my end, the CAI file has "data" field. For example:
I also double-checked |
The rego file "sql_allowed_authorized_networks.rego" has two issues.
templates.gcp.GCPSQLAllowedAuthorizedNetworksV1
needs be changed intotemplates.gcp.GCPSQLAllowedAuthorizedNetworksConstraintV1
in order for the template file "gcp_sql_allowed_authorized_networks_v1.yaml" to work.data
field needs to be added in betweenresource
andsettings
to correctly retrieve info fromauthorizedNetworks
.I also modified the logic so that the rule can detect when
authorizedNetworks
does not exist or has no content.