Adding support for VPC association in DHCPOptions #143

nishant221 · 2023-07-10T10:58:39Z

Issue #, if available:

Description of changes:
Adding support in EC2 controller to associate VPC in DHCPOptions resource.

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

ack-prow · 2023-07-10T10:58:50Z

Hi @nishant221. 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.

LikithaVemulapalli

Thanks @nishant221 for adding this support for VPC association in DHCPOptions, left few inline comments :)

test/e2e/tests/test_dhcp_options.py

a-hilaly · 2023-07-13T16:00:18Z

/ok-to-test

pkg/resource/dhcp_options/hooks.go

nishant221 · 2023-07-14T10:58:52Z

/test ec2-kind-e2e

a-hilaly · 2023-07-15T13:04:52Z

@nishant221 from the controller logs running the e2e:

2023-07-14T11:14:02.217Z	ERROR	Reconciler error	{"controller": "dhcpoptions", "controllerGroup": "ec2.services.k8s.aws", "controllerKind": "DHCPOptions", "DHCPOptions": {"name":"dhcpoptions-vpc-ref-p21r","namespace":"default"}, "namespace": "default", "name": "dhcpoptions-vpc-ref-p21r", "reconcileID": "5485841e-0f6f-4249-b5ca-935a1c86cd46", "error": "DependencyViolation: The dhcpOptions 'dopt-0daa5af75770e9472' has dependencies and cannot be deleted.\n\tstatus code: 400, request id: 2fb3d484-6bb2-44cc-bd43-c21daa6de178"}
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:329
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:274
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2
	/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:235
2023-07-14T11:14:09.216Z	DEBUG	exporter.field-export-reconciler	error did not need requeue	{"error": "the source resource is not synced yet"}

Looks like we need to support detaching, and make sure to detach from a VPC before deleting the resource

nishant221 · 2023-07-17T12:15:08Z

@a-hilaly Added following supports:

Support for defining multiple VPC(s) in DHCPOptions object.
Updating VPC list to which DHCPOptions added/removed.
Added support for disassociating VPCs from DHCP as well.

a-hilaly

Looks good to me! Thanks @nishant221
I have few nit comments below

pkg/resource/dhcp_options/hooks.go

templates/hooks/dhcp_options/sdk_delete_pre_build_request.go.tpl

templates/hooks/dhcp_options/sdk_read_many_post_set_output.go.tpl

pkg/resource/dhcp_options/hooks.go

nishant221 · 2023-07-17T14:56:49Z

@a-hilaly Thanks for the review. Incorporated the feedbacks and added a follow-up comment on one. Kindly have a look at your convenience.

nishant221 · 2023-07-17T14:58:47Z

/test ec2-kind-e2e

a-hilaly

Very close, happy to approve once these comments are addressed. Thanks again!

pkg/resource/dhcp_options/hooks.go

nishant221 · 2023-07-18T11:02:19Z

/test ec2-kind-e2e

a-hilaly

Happy to merge as soon as our prow jobs are fixed 👍
/retest

LikithaVemulapalli

looks good to me, thanks @nishant221 :)

LikithaVemulapalli · 2023-07-20T13:48:49Z

/lgtm

ack-prow · 2023-07-20T13:48:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [LikithaVemulapalli,a-hilaly]

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

a-hilaly · 2023-07-20T13:53:32Z

@nishant221 Can you make a release PR?

ack-prow bot requested review from LikithaVemulapalli and vijtrip2 July 10, 2023 10:58

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 Jul 10, 2023

LikithaVemulapalli requested changes Jul 13, 2023

View reviewed changes

test/e2e/tests/test_dhcp_options.py Outdated Show resolved Hide resolved

test/e2e/tests/test_dhcp_options.py Outdated Show resolved Hide resolved

test/e2e/tests/test_dhcp_options.py Show resolved Hide resolved

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 13, 2023

a-hilaly reviewed Jul 13, 2023

View reviewed changes

pkg/resource/dhcp_options/hooks.go Outdated Show resolved Hide resolved

nishant221 added 3 commits July 14, 2023 15:46

Adding support for VPC association in DHCPOptions

21ffc8e

Adding support for VPC association in DHCPOptions

be4f71c

Feedbacks

c0f21ab

nishant221 force-pushed the vpcdhcp branch from 609200d to c0f21ab Compare July 14, 2023 10:18

Feedbacks

4f67af5

Feedbacks

9a56413

a-hilaly reviewed Jul 17, 2023

View reviewed changes

Feedbacks

7c31373

a-hilaly reviewed Jul 17, 2023

View reviewed changes

pkg/resource/dhcp_options/hooks.go Show resolved Hide resolved

pkg/resource/dhcp_options/hooks.go Outdated Show resolved Hide resolved

Feedbacks

d606006

nishant221 added 2 commits July 18, 2023 16:47

Feedbacks

d3bfc92

Feedbacks

3ac5f93

a-hilaly approved these changes Jul 18, 2023

View reviewed changes

ack-prow bot added the approved label Jul 18, 2023

Feedbacks

10b2e6f

LikithaVemulapalli approved these changes Jul 18, 2023

View reviewed changes

nishant221 added 4 commits July 20, 2023 09:01

Changing e2e commit sha

0c35dea

Changes

a7a3a67

Changes

752af7f

Changes

39aaa17

ack-prow bot assigned LikithaVemulapalli Jul 20, 2023

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

ack-prow bot merged commit 0436bbe into aws-controllers-k8s:main Jul 20, 2023

Adding support for VPC association in DHCPOptions #143

Adding support for VPC association in DHCPOptions #143

Uh oh!

Conversation

nishant221 commented Jul 10, 2023

Uh oh!

ack-prow bot commented Jul 10, 2023

Uh oh!

LikithaVemulapalli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a-hilaly commented Jul 13, 2023

Uh oh!

Uh oh!

nishant221 commented Jul 14, 2023

Uh oh!

a-hilaly commented Jul 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nishant221 commented Jul 17, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nishant221 commented Jul 17, 2023

Uh oh!

nishant221 commented Jul 17, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nishant221 commented Jul 18, 2023

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

LikithaVemulapalli left a comment

Choose a reason for hiding this comment

Uh oh!

LikithaVemulapalli commented Jul 20, 2023

Uh oh!

ack-prow bot commented Jul 20, 2023

Uh oh!

a-hilaly commented Jul 20, 2023

Uh oh!

Uh oh!

a-hilaly commented Jul 15, 2023 •

edited

Loading

nishant221 commented Jul 17, 2023 •

edited

Loading