This repository was archived by the owner on Mar 23, 2026. It is now read-only.
Migrate IAM managed policies#13790
Merged
Merged
Conversation
LocalStack Community integration with Pro 2 files 2 suites 2m 36s ⏱️ For more details on these failures, see this check. Results for commit 6241c44. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 11m 23s ⏱️ For more details on these failures, see this check. Results for commit 6241c44. ♻️ This comment has been updated with latest results. |
49e13e2 to
cbcd0f4
Compare
81cb654 to
6241c44
Compare
pinzon
approved these changes
Feb 19, 2026
Comment on lines
+340
to
+413
| def _validate_resource_format(self, resource: str) -> None: | ||
| if resource != "*": | ||
| resource_partitions = resource.partition(":") | ||
|
|
||
| if resource_partitions[1] == "": | ||
| self._resource_error = f'Resource {resource} must be in ARN format or "*".' | ||
| return | ||
|
|
||
| resource_partitions = resource_partitions[2].partition(":") | ||
| if resource_partitions[0] != "*" and resource_partitions[0] not in PARTITION_NAMES: | ||
| remaining_resource_parts = resource_partitions[2].split(":") | ||
|
|
||
| arn1 = ( | ||
| remaining_resource_parts[0] | ||
| if remaining_resource_parts[0] != "" or len(remaining_resource_parts) > 1 | ||
| else "*" | ||
| ) | ||
| arn2 = remaining_resource_parts[1] if len(remaining_resource_parts) > 1 else "*" | ||
| arn3 = remaining_resource_parts[2] if len(remaining_resource_parts) > 2 else "*" | ||
| arn4 = ( | ||
| ":".join(remaining_resource_parts[3:]) | ||
| if len(remaining_resource_parts) > 3 | ||
| else "*" | ||
| ) | ||
| pt = resource_partitions[0] | ||
| self._resource_error = f'Partition "{pt}" is not valid for resource "arn:{pt}:{arn1}:{arn2}:{arn3}:{arn4}".' | ||
| return | ||
|
|
||
| if resource_partitions[1] != ":": | ||
| self._resource_error = ( | ||
| "Resource vendor must be fully qualified and cannot contain regexes." | ||
| ) | ||
| return | ||
|
|
||
| resource_partitions = resource_partitions[2].partition(":") | ||
|
|
||
| service = resource_partitions[0] | ||
| region = resource_partitions[2] | ||
| resource_partitions = resource_partitions[2].partition(":") | ||
|
|
||
| resource_partitions = resource_partitions[2].partition(":") | ||
| resource_id = resource_partitions[2] | ||
|
|
||
| if ( | ||
| service in SERVICE_TYPE_REGION_INFORMATION_ERROR_ASSOCIATIONS.keys() | ||
| and not region.startswith(":") | ||
| ): | ||
| valid_start = False | ||
|
|
||
| for valid_starting_value in SERVICE_TYPE_REGION_INFORMATION_ERROR_ASSOCIATIONS[ | ||
| service | ||
| ].get("valid_starting_values", []): | ||
| if resource_id.startswith(valid_starting_value): | ||
| valid_start = True | ||
| break | ||
|
|
||
| if not valid_start: | ||
| self._resource_error = SERVICE_TYPE_REGION_INFORMATION_ERROR_ASSOCIATIONS[ | ||
| service | ||
| ]["error_message"].format(resource=resource) | ||
| return | ||
|
|
||
| if service in VALID_RESOURCE_PATH_STARTING_VALUES.keys(): | ||
| valid_start = False | ||
| for valid_starting_value in VALID_RESOURCE_PATH_STARTING_VALUES[service]["values"]: | ||
| if resource_partitions[2].startswith(valid_starting_value): | ||
| valid_start = True | ||
| break | ||
| if not valid_start: | ||
| self._resource_error = VALID_RESOURCE_PATH_STARTING_VALUES[service][ | ||
| "error_message" | ||
| ].format( | ||
| values=", ".join(VALID_RESOURCE_PATH_STARTING_VALUES[service]["values"]) | ||
| ) |
Member
There was a problem hiding this comment.
idea: can we use the arn util to simplify this validation?
Member
Author
There was a problem hiding this comment.
Likely! However I would like to do this in a separate step, once we have more tests for this.
Comment on lines
+1890
to
+1905
| def test_policy_tag_order(self, aws_client, create_policy, snapshot): | ||
| # Create policy to check tag ordering in responses | ||
| response = create_policy( | ||
| PolicyDocument=json.dumps(SAMPLE_POLICY), | ||
| Tags=[ | ||
| {"Key": "abc", "Value": "first"}, | ||
| {"Key": "b", "Value": "second"}, | ||
| {"Key": "z", "Value": "third"}, | ||
| {"Key": "a", "Value": "fourth"}, | ||
| ], | ||
| ) | ||
| snapshot.match("create-with-case-sensitive-tags", response) | ||
| case_sensitive_arn = response["Policy"]["Arn"] | ||
|
|
||
| response = aws_client.iam.list_policy_tags(PolicyArn=case_sensitive_arn) | ||
| snapshot.match("list-tags", response) |
Member
There was a problem hiding this comment.
comment: thanks for adding this. I was a bit confuse by the extra steps you took to order the tags in the provider.
Member
Author
There was a problem hiding this comment.
Yes, the ordering is very random. I'd like to write more tests at some point, but in the end we could also just use a sorting transformer. I went a bit overboard here 😅
| def test_create_policy_invalid_document(self, create_policy, snapshot, policy_doc): | ||
| """Test that invalid policy documents are rejected with MalformedPolicyDocument.""" | ||
| # Manually add us-east-1 transformer if snapshots are run against different region | ||
| snapshot.add_transformer(snapshot.transform.regex("us-east-1", "<region>")) |
Member
There was a problem hiding this comment.
idea: we should consider adding this to the global conftest or somewhere with more visibility for tests that use policies.
Member
Author
There was a problem hiding this comment.
This is really only for the policies in the test file - usually we use the current region anyway, but here it is hardcoded to us-east-1.
Comment on lines
+271
to
+283
| def _get_custom_id_from_tags(self, tags: list[Tag]) -> str | None: | ||
| """ | ||
| Check an IAM tag list for a custom id tag, and return the value if present. | ||
|
|
||
| :param tags: List of tags | ||
| :return: Custom Id or None if not present | ||
| """ | ||
| if not tags: | ||
| return | ||
| for tag in tags: | ||
| if tag["Key"] == TAG_KEY_CUSTOM_ID: | ||
| return tag["Value"] | ||
| return None |
Member
There was a problem hiding this comment.
comment: I didn't know IAM was supposed to support his feature. Thank you for adding it. Maybe it's worth verifying that we have a test for this. 👍
dfangl
added a commit
that referenced
this pull request
Feb 26, 2026
dfangl
added a commit
that referenced
this pull request
Feb 26, 2026
dfangl
added a commit
that referenced
this pull request
Mar 4, 2026
dfangl
added a commit
that referenced
this pull request
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR migrates the IAM managed policy implementation from moto to LocalStack.
The implementation of the attachement of them to entities will be part of the respective entity implementation.
Changes
Tests
Uncommented
test_iam_policies.py, all tests passing (one with a snapshot ignore)Policy attachment tests will fail for this PR, as it is not yet integrated with roles/users/groups.
Related
Closes UNC-227