Fix: EC2 controller for vpcendpoint does not update status #191

nnbu · 2024-05-21T22:23:43Z

Issue : aws-controllers-k8s/community#2075

Description of changes:
The issue is that vpce CR status is not updated. This happens because when vpce is created, it takes around a minute for it to be created in aws. Due to this, it's status is kept pending. But there is no sync enforced on the reconciler. Due to this, reconciliation happens after 10h default time. At this time, status would be set correctly.

This fix enforces reconciliation to make sure CR is updated correctly when vpce creation in aws is successful.

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

ack-prow · 2024-05-21T22:23:53Z

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.

a-hilaly · 2024-05-21T22:41:04Z

/ok-to-test

a-hilaly

This is great, thank you @nnbu !
Shall we make a release for this?

a-hilaly · 2024-05-21T22:42:22Z

apis/v1alpha1/ack-generate-metadata.yaml

+  build_date: "2024-05-21T22:16:46Z"
+  build_hash: d660ee36fe947607ebea039acd47c35477b4a836
+  go_version: go1.21.1
+  version: v0.28.0-58-gd660ee3


Can please you use the latest version of code-gen?
/hold

a-hilaly · 2024-05-21T23:36:24Z

@nnbu can you also update the e2e tests? i think we might have to wait more than 5x30 seconds
/retest

Issue : aws-controllers-k8s/community#2075 Description of changes: The issue is that vpce CR status is not updated. This happens because when vpce is created, it takes around a minute for it to be created in aws. Due to this, it's status is kept pending. But there is no sync enforced on the reconciler. Due to this, reconciliation happens after 10h default time. At this time, status would be set correctly. This fix enforces reconciliation to make sure CR is updated correctly when vpce creation in aws is successful. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nnbu · 2024-05-22T17:28:53Z

@nnbu can you also update the e2e tests? i think we might have to wait more than 5x30 seconds /retest

@a-hilaly this is done now

a-hilaly

Thank you @nnbu !
/lgtm
/test all

a-hilaly · 2024-05-22T20:13:40Z

/unhold

ack-prow · 2024-05-22T20:14:01Z

[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

…ollers-k8s#191) Issue : aws-controllers-k8s/community#2075 Description of changes: The issue is that vpce CR status is not updated. This happens because when vpce is created, it takes around a minute for it to be created in aws. Due to this, it's status is kept `pending`. But there is no sync enforced on the reconciler. Due to this, reconciliation happens after 10h default time. At this time, status would be set correctly. This fix enforces reconciliation to make sure CR is updated correctly when vpce creation in aws is successful. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

wjf3121 · 2024-06-05T09:25:34Z

@nnbu @a-hilaly

I found a potential bug with this implementation: aws-controllers-k8s/community#1933 (comment)

It seems that we stop syncing the state once the VPCEndpoint becomes available. However an available VPCEndpoint can still later be rejected and the controller will not update the state accordingly.

nnbu · 2024-06-05T09:56:51Z

The fix addresses the issue for the case when the vpc endpoint goes from `pending` to `available`. Your latest comment is about the case when the state changes from `available` to `rejected`. As you mentioned, the trigger for this transition is manual trigger which is not managed/controlled by ack controller. It is not possible to handle such cases without reconciling every single resource at a sufficiently high frequency. E.g. consider that if we delete/change any resource out of the scope of ack controllers, then it won't be reflected in ack immediately. Ack has a default reconciling frequency of 10 hours. So, only at that frequency, ack detects the change of state of the resource.

…

On Wed, Jun 5, 2024, 2:55 PM Junfeng Wu ***@***.***> wrote: @nnbu <https://github.com/nnbu> @a-hilaly <https://github.com/a-hilaly> I found a potential bug with this implementation: aws-controllers-k8s/community#1933 (comment) <aws-controllers-k8s/community#1933 (comment)> It seems that we stop syncing the state once the VPCEndpoint becomes available. However an available VPCEndpoint can still later be rejected and the controller will not update the state accordingly. — Reply to this email directly, view it on GitHub <#191 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASMB42WQGIMFZJF3N3TRNULZF3KSLAVCNFSM6AAAAABICO2JPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBZGMYTOMBYGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

wjf3121 · 2024-06-05T10:56:21Z

templates/hooks/vpc_endpoint/sdk_read_many_post_set_output.go.tpl

+
+	if !vpcEndpointAvailable(&resource{ko}) {
+		// Setting resource synced condition to false will trigger a requeue of
+		// the resource. No need to return a requeue error here.
+		ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil)
+	} else {
+		ackcondition.SetSynced(&resource{ko}, corev1.ConditionTrue, nil, nil)
+	}


@nnbu Thanks for the explanation in this comment, it makes sense to me mostly! I still have one question over this implmenetation.

Here we consider a VPCEndpoint resource not synced as long as it's not in available state. However a VPC endpoint can be in pendingAcceptance state after creation and stay in that state forever/later be in rejected state -- either way it never becomes available

It seems to me that in controller's perspective it should be considered synced in these cases. In earlier versions of the controller that indeed is that case. But with this change, the VPCEndpoint will remain unsynced after its creation unless it becomes available later, which may never happen in some cases.

Does it make sense to only force the resource to be unsynced only when the resource is in pending instead of when it's in a non available state? This seems more inline with the endpoint states definition:

Pending - The service provider accepted the connection request. This is the initial state if requests are automatically accepted. The VPC endpoint returns to this state if the service consumer modifies the VPC endpoint.

We have such automated pipeline that for a VPCEndpoint that requires manual acceptance, we first wait til the resource to be synced and then accept the connection through API. The change will break the behavior.

@nnbu @a-hilaly , if it makes sense to you, here's the fix I proposed on top of this change: #198

nnbu · 2024-06-05T13:00:34Z

> Does it make sense to only force the resource to be unsynced only when

the resource is in pending instead of when it's in a non available state? This makes sense totally. What are some of the transitionary states for vpc endpoint, similar to pending? We should basically treat all such transitionary states as `notSynced`. @amine will be able to check with the service teams and get such states. Depending on that, we can make the change. Your PR looks good to me if `pending` is the only transitionary state.

…

On Wed, Jun 5, 2024, 4:42 PM Junfeng Wu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In templates/hooks/vpc_endpoint/sdk_read_many_post_set_output.go.tpl <#191 (comment)> : > + + if !vpcEndpointAvailable(&resource{ko}) { + // Setting resource synced condition to false will trigger a requeue of + // the resource. No need to return a requeue error here. + ackcondition.SetSynced(&resource{ko}, corev1.ConditionFalse, nil, nil) + } else { + ackcondition.SetSynced(&resource{ko}, corev1.ConditionTrue, nil, nil) + } @nnbu <https://github.com/nnbu> @a-hilaly <https://github.com/a-hilaly> , if it makes sense to you, here's the fix I proposed on top of this change: #198 <#198> — Reply to this email directly, view it on GitHub <#191 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ASMB42RVPZASY5FXIMXYM53ZF3XC7AVCNFSM6AAAAABICO2JPWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJYHAZDMNJYGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

wjf3121 · 2024-06-05T13:09:13Z

Your PR looks good to me if pending is the only transitionary state.

pending and deleting are the only two transitionary states AFAIK and deleting should already be handled correctly assuming the resource is fully managed by the controller. It would be awesome if you can confirm this internally:)

Context: #191 (comment) A synced VPC endpoint should only be forced to re-sync when it's in `pending` state. For other states like `pendingAcceptance`, `rejected`, `failed`, `expired`, it could be the final state of the endpoint and the sync should be considered completed. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…ollers-k8s#191) Issue : aws-controllers-k8s/community#2075 Description of changes: The issue is that vpce CR status is not updated. This happens because when vpce is created, it takes around a minute for it to be created in aws. Due to this, it's status is kept `pending`. But there is no sync enforced on the reconciler. Due to this, reconciliation happens after 10h default time. At this time, status would be set correctly. This fix enforces reconciliation to make sure CR is updated correctly when vpce creation in aws is successful. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…trollers-k8s#198) Context: aws-controllers-k8s#191 (comment) A synced VPC endpoint should only be forced to re-sync when it's in `pending` state. For other states like `pendingAcceptance`, `rejected`, `failed`, `expired`, it could be the final state of the endpoint and the sync should be considered completed. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

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 May 21, 2024

ack-prow bot requested review from a-hilaly and vijtrip2 May 21, 2024 22:24

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 May 21, 2024

a-hilaly approved these changes May 21, 2024

View reviewed changes

a-hilaly reviewed May 21, 2024

View reviewed changes

ack-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved labels May 21, 2024

nnbu force-pushed the vpce-status branch from c3b51da to 6176672 Compare May 22, 2024 17:00

a-hilaly approved these changes May 22, 2024

View reviewed changes

ack-prow bot assigned a-hilaly May 22, 2024

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2024

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

ack-prow bot merged commit 34439b0 into aws-controllers-k8s:main May 22, 2024
6 checks passed

a-hilaly mentioned this pull request Jun 4, 2024

bug: VPCEndpoint state is not synced until hours later. aws-controllers-k8s/community#1933

Closed

wjf3121 reviewed Jun 5, 2024

View reviewed changes

wjf3121 mentioned this pull request Jun 5, 2024

fix: only force syncing a VPC endpoint when in pending state #198

Merged

Fix: EC2 controller for vpcendpoint does not update status #191

Fix: EC2 controller for vpcendpoint does not update status #191

Uh oh!

Conversation

nnbu commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented May 21, 2024

Uh oh!

a-hilaly commented May 21, 2024

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly May 21, 2024

Choose a reason for hiding this comment

Uh oh!

nnbu May 22, 2024

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented May 21, 2024

Uh oh!

nnbu commented May 22, 2024

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented May 22, 2024

Uh oh!

ack-prow bot commented May 22, 2024

Uh oh!

Uh oh!

wjf3121 commented Jun 5, 2024

Uh oh!

nnbu commented Jun 5, 2024 via email

Uh oh!

wjf3121 Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjf3121 Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

wjf3121 Jun 5, 2024

Choose a reason for hiding this comment

Uh oh!

nnbu commented Jun 5, 2024 via email

Uh oh!

wjf3121 commented Jun 5, 2024

Uh oh!

Uh oh!

nnbu commented May 21, 2024 •

edited

Loading

wjf3121 Jun 5, 2024 •

edited

Loading