Skip to content
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

[AWS] Resource creation for VM #454

Merged
merged 10 commits into from
Aug 31, 2024
Merged

Conversation

seankimkdy
Copy link
Collaborator

Resolves #453. Some unique things/limitations about AWS

Faking AWS Server for Unit Testing
Unlike GCP and Azure, faking an HTTP server did not work. I used middlewares instead which is a bit of a different concept. fakeDeserializeMiddleware should look a lot like getFakeServerHandler in GCP or Azure.

Using Namespaces for Resource Cleanup during Integration Test
AWS does not have resource groups in the sense that Azure does, nor does it have a nice isolation mechanism like GCP project does. There's no clean way to delete resources by a group, so I had to write a custom deletion by hand in TeardownAwsTesting. This is also why namespace has to be unique as there can be many different tests going on in the same account for AWS.

VPC Quota Limit
There's currently a limit of default 5 VPCs per region in AWS. This will not do as we tend to have more >5 ongoing PRs. We should ask for an service quota increase.

Signed-off-by: Sean Kim <[email protected]>
Signed-off-by: Sean Kim <[email protected]>
Signed-off-by: Sean Kim <[email protected]>
Signed-off-by: Sean Kim <[email protected]>
Signed-off-by: Sean Kim <[email protected]>
@seankimkdy seankimkdy requested a review from a team as a code owner July 30, 2024 05:39
@seankimkdy seankimkdy temporarily deployed to functional-tests July 30, 2024 05:39 — with GitHub Actions Inactive
Copy link

github-actions bot commented Jul 30, 2024

Unit Test Results

321 tests  +5   321 ✅ +5   4s ⏱️ +2s
 50 suites +1     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 7f081c1. ± Comparison against base commit ddba24e.

This pull request removes 2 and adds 7 tests. Note that renamed tests count towards both.
github.com/paraglider-project/paraglider/pkg/ibm ‑ TestCreateResourceExistingVPCSubnet
github.com/paraglider-project/paraglider/pkg/ibm ‑ TestCreateResourceNewVPC
github.com/paraglider-project/paraglider/pkg/aws ‑ TestCreateResource
github.com/paraglider-project/paraglider/pkg/aws ‑ TestCreateResource/ExistingNetwork
github.com/paraglider-project/paraglider/pkg/aws ‑ TestCreateResource/FromScratch
github.com/paraglider-project/paraglider/pkg/aws ‑ TestCreateResource/WrongAvailabilityZone
github.com/paraglider-project/paraglider/pkg/ibm ‑ TestCreateResourceEndpointGatewayNewVPC
github.com/paraglider-project/paraglider/pkg/ibm ‑ TestCreateResourceInstanceExistingVPCSubnet
github.com/paraglider-project/paraglider/pkg/ibm ‑ TestCreateResourceInstanceNewVPC

♻️ This comment has been updated with latest results.

Signed-off-by: Sean Kim <[email protected]>
@seankimkdy seankimkdy temporarily deployed to functional-tests July 30, 2024 07:17 — with GitHub Actions Inactive
@seankimkdy seankimkdy temporarily deployed to functional-tests July 30, 2024 17:06 — with GitHub Actions Inactive
Copy link

51.8

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 51.8 %
  • main branch coverage: 0 %
  • diff coverage: 51.8 %

The coverage result does not include the functional test results.

@seankimkdy seankimkdy temporarily deployed to functional-tests July 30, 2024 18:05 — with GitHub Actions Inactive
Copy link
Collaborator

@smcclure20 smcclure20 left a comment

Choose a reason for hiding this comment

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

Thank you for figuring this all out! Looks great with a few notes!

return nil, fmt.Errorf("unable to wait for default security group: %w", err)
}

// Remove default inbound and outbound rules from default security group
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be easier to just put a deny-all rule at a higher priority than these defaults? That's what we do in Azure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think AWS has deny rules for security groups. From https://docs.aws.amazon.com/vpc/latest/userguide/security-group-rules.html#security-group-rule-characteristics

You can specify allow rules, but not deny rules.

There are network ACLs (which have both allow and deny) but I believe they apply over an entire subnet? Let me know if I'm wrong about that though cus I'm not 100% sure.

Signed-off-by: Sean Kim <[email protected]>
Copy link

51.8

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 51.8 %
  • main branch coverage: 0 %
  • diff coverage: 51.8 %

The coverage result does not include the functional test results.

Copy link
Collaborator

@smcclure20 smcclure20 left a comment

Choose a reason for hiding this comment

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

Approved with the note that we should fix how AZs are handled when creating a VPC in a region. Per discussion with @seankimkdy, we should provision a subnet in each AZ on VPC creation by splitting the VPC address space into N equal chunks.

@seankimkdy seankimkdy merged commit dc09db2 into main Aug 31, 2024
9 checks passed
@seankimkdy seankimkdy deleted the seankimkdy/aws-resource-creation-vm branch August 31, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AWS] Resource creation (VMs only)
2 participants