Skip to content

fix clouformation ec2 tests for ap-northeast-1 and validate against AWS#10563

Merged
sannya-singal merged 2 commits intomasterfrom
cfn_ec2_multi_acc
Mar 29, 2024
Merged

fix clouformation ec2 tests for ap-northeast-1 and validate against AWS#10563
sannya-singal merged 2 commits intomasterfrom
cfn_ec2_multi_acc

Conversation

@sannya-singal
Copy link
Contributor

@sannya-singal sannya-singal commented Mar 28, 2024 •
edited
Loading

Motivation

Test test_vpc_creates_default_sg and test_transit_gateway_attachmentfail when run in the workflow for region: ap-northeast-1 and throws a StackDeployError.

Changes

This PR validates test: test_vpc_creates_default_sg against AWS and deploys both the tests specifically in us-east-1.

@sannya-singal sannya-singal added the semver: patch Non-breaking changes which can be included in patch releases label Mar 28, 2024
@sannya-singal sannya-singal self-assigned this Mar 28, 2024
@sannya-singal sannya-singal marked this pull request as ready for review March 28, 2024 06:36
@sannya-singal sannya-singal marked this pull request as draft March 28, 2024 06:38
@sannya-singal sannya-singal changed the title fix test_vpc_creates_default_sg and validate against AWS fix clouformation ec2 tests for ap-northeast-1 and validate against AWS Mar 28, 2024
@sannya-singal sannya-singal marked this pull request as ready for review March 28, 2024 06:53
@github-actions
Copy link

github-actions bot commented Mar 28, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 29m 31s ⏱️ +3s
2 740 tests ±0  2 484 ✅ ±0  256 💤 ±0  0 ❌ ±0 
2 742 runs  ±0  2 484 ✅ ±0  258 💤 ±0  0 ❌ ±0 

Results for commit 67e5248. ± Comparison against base commit 3b8099b.

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Mar 28, 2024

we need to update the region values in: moto.ec2.models.availability_zones_and_regions.RegionsAndZonesBackend.

Morijarti
Morijarti approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@Morijarti Morijarti left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as not to block pipeline.
However after adding missing subnet to moto, we should revert these changes

"ap-northeast-1": [
            Zone(
                region_name="ap-northeast-1",
                name="ap-northeast-1a",
                zone_id="apne1-az4",
            ),
            Zone(
                region_name="ap-northeast-1",
                name="ap-northeast-1c",
                zone_id="apne1-az1",
            ),
            Zone(
                region_name="ap-northeast-1",
                name="ap-northeast-1d",
                zone_id="apne1-az2",
            ),
        ],

in moto/ec2/models/availability_zones_and_regions.py:143

it's a bit weird that we have 1a, 1c and 1d, but not 1b on the list

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Mar 28, 2024

when we check the availability zones for ap-northeast-1 on aws, we get the following:

{
    "AvailabilityZones": [
        {
            "State": "available",
            "OptInStatus": "opt-in-not-required",
            "Messages": [],
            "RegionName": "ap-northeast-1",
            "ZoneName": "ap-northeast-1a",
            "ZoneId": "apne1-az4",
            "GroupName": "ap-northeast-1",
            "NetworkBorderGroup": "ap-northeast-1",
            "ZoneType": "availability-zone"
        },
        {
            "State": "available",
            "OptInStatus": "opt-in-not-required",
            "Messages": [],
            "RegionName": "ap-northeast-1",
            "ZoneName": "ap-northeast-1c",
            "ZoneId": "apne1-az1",
            "GroupName": "ap-northeast-1",
            "NetworkBorderGroup": "ap-northeast-1",
            "ZoneType": "availability-zone"
        },
        {
            "State": "available",
            "OptInStatus": "opt-in-not-required",
            "Messages": [],
            "RegionName": "ap-northeast-1",
            "ZoneName": "ap-northeast-1d",
            "ZoneId": "apne1-az2",
            "GroupName": "ap-northeast-1",
            "NetworkBorderGroup": "ap-northeast-1",
            "ZoneType": "availability-zone"
        }
    ]
}

@Morijarti

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Mar 28, 2024

another fix for this could be selecting 1c instead here, what do you think @Morijarti

@Morijarti
Copy link
Contributor

Morijarti commented Mar 28, 2024

It would fix it in this case, but I think then that the real issue is in how Fn::GetAZs works. Let me give it a look @sannya-singal

@pinzon
Copy link
Member

pinzon commented Mar 28, 2024 •
edited
Loading

I'm with @Morijarti probably it would be best if you just extend the functionality of Fn::GetAZs instead of limiting the test just to force us-east-1 in the test.

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Mar 28, 2024 •
edited
Loading

I'm with @Morijarti probably it would be best if you just extend the functionality of Fn::GetAZs instead of limiting the test just to force us-east-1 in the test.

Hey @pinzon! Thanks for the review. I and @Morijarti had a discussion today regarding the root cause of the issue, which is not clear. For now this has been marked approved to unblock #10546 and created a backlog item for this.

@sannya-singal sannya-singal merged commit 6c8e43f into master Mar 29, 2024
@sannya-singal sannya-singal deleted the cfn_ec2_multi_acc branch March 29, 2024 05:17
@sannya-singal sannya-singal mentioned this pull request Apr 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

@pinzon pinzon Awaiting requested review from pinzon

@simonrw simonrw Awaiting requested review from simonrw

1 more reviewer

@Morijarti Morijarti Morijarti approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sannya-singal sannya-singal

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sannya-singal @Morijarti @pinzon