-
Notifications
You must be signed in to change notification settings - Fork 487
feat(scaleway): add iam policy support (+ small fix) #2295
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
feat(scaleway): add iam policy support (+ small fix) #2295
Conversation
7d67f45 to
7c33b51
Compare
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.
3 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cartography/models/scaleway/iam/rule.py">
<violation number="1" location="cartography/models/scaleway/iam/rule.py:37">
P1: Rule violated: **Data model follows project structure**
sub_resource_relationship must target a tenant-like node with a RESOURCE relationship label and INWARD direction. This schema uses ScalewayPolicy with rel_label "HAS", which violates the required tenant-scoped RESOURCE linkage.</violation>
</file>
<file name="cartography/intel/scaleway/iam/policies.py">
<violation number="1" location="cartography/intel/scaleway/iam/policies.py:43">
P2: ScalewayRule cleanup skips rules for deleted/undiscovered policies, leaving stale rule nodes</violation>
<violation number="2" location="cartography/intel/scaleway/iam/policies.py:129">
P2: Cleanup order is reversed (parent before child); rules are cleaned after policies, contrary to required child-first cleanup for sub-resources, risking stale children or premature parent deletion.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
820ff43 to
9fd692f
Compare
jychp
left a comment
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.
Thx for your contribution. Very nice job, some comments but the overall quality is here, thx for making our review job easier!
| formatted_permission_set = scaleway_obj_to_dict(permission_set) | ||
| # Convert scope_type enum to string | ||
| if formatted_permission_set.get("scope_type"): | ||
| formatted_permission_set["scope_type"] = str( |
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.
Suggestion: Using str(enum) may produce the full enum name (e.g., "PermissionSetsScopeType.ALL") depending on how the Scaleway SDK defines the enum. Consider using .value for explicit value extraction:
formatted_permission_set["scope_type"] = formatted_permission_set["scope_type"].valueSame applies to permission_sets_scope_type in policies.py:75.
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.
I've added the enum check inside the scaleway_obj_to_dict instead so that we can safely call the value directly.
| @@ -0,0 +1,64 @@ | |||
| from copy import deepcopy | |||
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.
Blocking: Integration tests exist for other IAM entities (users, groups, applications, apikeys) in tests/integration/cartography/intel/scaleway/test_iam.py, but not for the new policies, rules, and permission sets.
| # Get and load rules for each policy | ||
| for policy in policies: | ||
| rules = get_rules(api, policy.id) | ||
| formatted_rules = transform_rules(rules, policy.id) |
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.
Aggregate rules and call load only once for more efficient batching.
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.
done
| formatted_rule = scaleway_obj_to_dict(rule) | ||
| formatted_rule["policy_id"] = policy_id | ||
| # Convert permission_sets_scope_type enum to string | ||
| if formatted_rule.get("permission_sets_scope_type"): |
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.
same comment about str() on enum, prefer using .value
9fd692f to
cd9ac3c
Compare
Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com>
Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com>
Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com>
Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com>
Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com>
cd9ac3c to
5413059
Compare
jychp
left a comment
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.
LGTM, thanks for your contribution.
Do not hesitate to join us on CNCF Slack!
### Type of change <!-- Mark the relevant option with an "x" --> - [x] Bug fix (non-breaking change that fixes an issue) - [x] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring (no functional changes) - [x] Documentation update - [ ] Other (please describe): ### Summary This PR is related to the scaleway intel module, 1st commit is a fix for instances that had no public ips and also adds supports for private nics (which were stored as a maps instead of being flattened like the ips / volumes) The remaining commits are to add support for IAM policies and their associated rules, which allows fetching principals without any policies attached, or with too broad permissions etc. ### How was this tested? <!-- Describe how you tested your changes. Include relevant details such as test configuration, commands run, or manual testing steps. --> Multiple runs were made against my scaleway infrastructure (multiple projects, 50+ policies) with removal of policies between runs to ensure that changes were reflected. Happy to run any other tests required. ### Checklist #### General - [x] I have read the [contributing guidelines](https://cartography-cncf.github.io/cartography/dev/developer-guide.html). - [x] The linter passes locally (`make lint`). - [x] I have added/updated tests that prove my fix is effective or my feature works. #### Proof of functionality <!-- Provide at least one of the following to help reviewers verify your changes: --> - [ ] Screenshot showing the graph before and after changes. - [x] New or updated unit/integration tests. #### If you are adding or modifying a synced entity - [x] Included Cartography sync logs from a real environment demonstrating successful synchronization of the new/modified entity. ``` INFO:cartography.intel.scaleway.iam.permissionsets:Loading 158 Scaleway PermissionSets into Neo4j. INFO:cartography.graph.statement:Completed ScalewayPermissionSet statement #1 INFO:cartography.graph.statement:Completed ScalewayPermissionSet statement #2 INFO:cartography.graph.job:Finished job ScalewayPermissionSet INFO:cartography.intel.scaleway.iam.policies:Loading 52 Scaleway Policies into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy '9a598f7f-0449-4672-ad34-xxxxxx' into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy '1c096b8e-2c29-497c-aa1b-xxxxxx' into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy 'e82be2a8-913a-4c95-acf5-xxxxxx' into Neo4j. [...] INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule ``` #### If you are changing a node or relationship - [x] Updated the [schema documentation](https://github.com/cartography-cncf/cartography/tree/master/docs/root/modules). - [ ] Updated the [schema README](https://github.com/cartography-cncf/cartography/blob/master/docs/schema/README.md). I've updated the module schema.md file, not sure if something else should be updated ? ### Notes for reviewers Disclaimer: Claude code was used to assist on this PR --------- Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com> Co-authored-by: EmFl <6998656+EmFl@users.noreply.github.com>
### Type of change <!-- Mark the relevant option with an "x" --> - [x] Bug fix (non-breaking change that fixes an issue) - [x] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring (no functional changes) - [x] Documentation update - [ ] Other (please describe): ### Summary This PR is related to the scaleway intel module, 1st commit is a fix for instances that had no public ips and also adds supports for private nics (which were stored as a maps instead of being flattened like the ips / volumes) The remaining commits are to add support for IAM policies and their associated rules, which allows fetching principals without any policies attached, or with too broad permissions etc. ### How was this tested? <!-- Describe how you tested your changes. Include relevant details such as test configuration, commands run, or manual testing steps. --> Multiple runs were made against my scaleway infrastructure (multiple projects, 50+ policies) with removal of policies between runs to ensure that changes were reflected. Happy to run any other tests required. ### Checklist #### General - [x] I have read the [contributing guidelines](https://cartography-cncf.github.io/cartography/dev/developer-guide.html). - [x] The linter passes locally (`make lint`). - [x] I have added/updated tests that prove my fix is effective or my feature works. #### Proof of functionality <!-- Provide at least one of the following to help reviewers verify your changes: --> - [ ] Screenshot showing the graph before and after changes. - [x] New or updated unit/integration tests. #### If you are adding or modifying a synced entity - [x] Included Cartography sync logs from a real environment demonstrating successful synchronization of the new/modified entity. ``` INFO:cartography.intel.scaleway.iam.permissionsets:Loading 158 Scaleway PermissionSets into Neo4j. INFO:cartography.graph.statement:Completed ScalewayPermissionSet statement #1 INFO:cartography.graph.statement:Completed ScalewayPermissionSet statement #2 INFO:cartography.graph.job:Finished job ScalewayPermissionSet INFO:cartography.intel.scaleway.iam.policies:Loading 52 Scaleway Policies into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy '9a598f7f-0449-4672-ad34-xxxxxx' into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy '1c096b8e-2c29-497c-aa1b-xxxxxx' into Neo4j. INFO:cartography.intel.scaleway.iam.policies:Loading 1 Scaleway Rules for policy 'e82be2a8-913a-4c95-acf5-xxxxxx' into Neo4j. [...] INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule INFO:cartography.graph.statement:Completed ScalewayRule statement #1 INFO:cartography.graph.statement:Completed ScalewayRule statement #2 INFO:cartography.graph.statement:Completed ScalewayRule statement #3 INFO:cartography.graph.job:Finished job ScalewayRule ``` #### If you are changing a node or relationship - [x] Updated the [schema documentation](https://github.com/cartography-cncf/cartography/tree/master/docs/root/modules). - [ ] Updated the [schema README](https://github.com/cartography-cncf/cartography/blob/master/docs/schema/README.md). I've updated the module schema.md file, not sure if something else should be updated ? ### Notes for reviewers Disclaimer: Claude code was used to assist on this PR --------- Signed-off-by: EmFl <6998656+EmFl@users.noreply.github.com> Co-authored-by: EmFl <6998656+EmFl@users.noreply.github.com> Signed-off-by: Jeremy Chapeau <jeremy@subimage.io>
Type of change
Summary
This PR is related to the scaleway intel module, 1st commit is a fix for instances that had no public ips and also adds supports for private nics (which were stored as a maps instead of being flattened like the ips / volumes)
The remaining commits are to add support for IAM policies and their associated rules, which allows fetching principals without any policies attached, or with too broad permissions etc.
How was this tested?
Multiple runs were made against my scaleway infrastructure (multiple projects, 50+ policies) with removal of policies between runs to ensure that changes were reflected.
Happy to run any other tests required.
Checklist
General
make lint).Proof of functionality
If you are adding or modifying a synced entity
If you are changing a node or relationship
I've updated the module schema.md file, not sure if something else should be updated ?
Notes for reviewers
Disclaimer: Claude code was used to assist on this PR