Add route table assocations to internet gateway #149

nnbu · 2023-09-13T04:17:30Z

Issue # aws-controllers-k8s/community#1892

Description of changes:
New custom field RouteTables is added to internetGateway CRD. Route Table to Internet Gateway association is done using AssociateRouteTable API call and association is removed using DisassociateRouteTable API call.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2023-09-13T04:17:40Z

Hi @nnbu. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nnbu · 2023-09-13T04:21:18Z

I have followed the same model that introduced routeTables field in subnet spec. It allowed route table association to subnet.

a-hilaly

hold as we will release a new code gen version in the next few hours/days.
/ok-to-test
/hold

nnbu · 2023-09-18T21:13:32Z

hold as we will release a new code gen version in the next few hours/days. /ok-to-test /hold

Were you referencing this issue aws-controllers-k8s/community#1893 ? Now that it is closed, do I need to generate the code again?

a-hilaly · 2023-09-18T21:16:18Z

@nnbu yes, there is some rebase conflicts, ideally you should use code-generator 0.27.1, thank you!

nnbu · 2023-09-19T17:32:33Z

@RedbackThomson Will you be reviewing this PR please? It is similar to the change you did for subnet to routeTable association.

a-hilaly

Thank you @nnbu ! Left few comments in-line

a-hilaly · 2023-09-20T15:01:17Z

pkg/resource/internet_gateway/hooks.go

+	}
+
+	toAdd := make([]*string, 0)
+	toDelete := make([]*svcsdk.RouteTableAssociation, 0)


just for consistency can we make this array of string type? make([]*string, 0)

I could do that. However, I am using svcsdk.RouteTableAssociation to print Debug message with 2 fields 1. route table id and 2. association id. *[]string in case of toDelete would need to hold association id only.

In case of toAdd, association is not established yet. so, there is no association id to print.

I can still make the change and remove the debug statement, because printing association id without mentioning routing table id would be not much of a use.

I see what you did there. Good points.

a-hilaly · 2023-09-20T15:02:05Z

pkg/resource/internet_gateway/hooks.go

+		resp, err := rm.sdkapi.DescribeRouteTablesWithContext(ctx, input)
+		if err != nil || resp == nil {
+			break
+		}


I believe we should just return an error here. The break when resp.NextToken == nil|"" is enough

if err != nil { return nil, err }

I am not fully sure, but if resp is nil ever (without err being nil), then the for loop at line 439 would crash while dereferencing resp.

pkg/resource/internet_gateway/hooks.go

test/e2e/tests/helper.py

a-hilaly · 2023-09-20T15:16:19Z

test/e2e/tests/test_internet_gateway.py

+        ec2_validator.assert_internet_gateway(resource_id, exists=False)
+
+def simple_route_table(vpc_id: int):


Can we just reuse https://github.com/aws-controllers-k8s/ec2-controller/blob/main/test/e2e/tests/test_route_table.py#L36-L37 ?

I did think about before copying over this function here. :)
But this one is simpler route table that does not have additional entries associated with like. e.g. IGW_ID(internet gateway), DEST_CIDR_BLOCK among others.

https://github.com/aws-controllers-k8s/ec2-controller/blob/main/test/e2e/tests/test_route_table.py#L47C5-L49C1

It would need unnecessary functionality to create additional internet gateway while this test is testing internet gateway itself.

I see that you're having a separate resource file for this function, which make things ok! Great!

a-hilaly · 2023-09-20T15:17:21Z

/unhold

Description of changes: New custom field RouteTables is added to internetGateway CRD. Route Table to Internet Gateway association is done using AssociateRouteTable API call and association is removed using DisassociateRouteTable API call. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly

Thank you very much sir! @nnbu
/lgtm

ack-prow · 2023-09-21T21:19:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, nnbu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 13, 2023

ack-prow bot requested review from jljaco and RedbackThomson September 13, 2023 04:17

a-hilaly reviewed Sep 13, 2023

View reviewed changes

nnbu force-pushed the ig-rt-association branch 3 times, most recently from 116fcdd to b60551d Compare September 18, 2023 23:11

a-hilaly reviewed Sep 20, 2023

View reviewed changes

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2023

nnbu force-pushed the ig-rt-association branch from b60551d to ad8eef7 Compare September 21, 2023 05:02

nnbu mentioned this pull request Sep 21, 2023

release artifacts for release v1.0.7 #152

Merged

a-hilaly approved these changes Sep 21, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Sep 21, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2023

ack-prow bot added the approved label Sep 21, 2023

ack-prow bot merged commit d86f411 into aws-controllers-k8s:main Sep 21, 2023

		ec2_validator.assert_internet_gateway(resource_id, exists=False)

		def simple_route_table(vpc_id: int):

Add route table assocations to internet gateway #149

Add route table assocations to internet gateway #149

Uh oh!

Conversation

nnbu commented Sep 13, 2023

Uh oh!

ack-prow bot commented Sep 13, 2023

Uh oh!

nnbu commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

nnbu commented Sep 18, 2023

Uh oh!

a-hilaly commented Sep 18, 2023

Uh oh!

nnbu commented Sep 19, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Sep 20, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Sep 21, 2023

Uh oh!

Uh oh!

nnbu commented Sep 13, 2023 •

edited

Loading