provider: finalize EnvoyProxy referenced by a managed GatewayClass by cnvergence · Pull Request #1534 · envoyproxy/gateway

cnvergence · 2023-06-15T14:46:19Z

What type of PR is this?

Add support for envoy proxy finalizers managed by GatewayClass

Which issue(s) this PR fixes:

Fixes:

Finalize EnvoyProxy #865

codecov · 2023-06-15T16:07:56Z

Codecov Report

Merging #1534 (7c25ea0) into main (bde72ab) will increase coverage by 0.11%.
The diff coverage is 56.52%.

❗ Current head 7c25ea0 differs from pull request most recent head 5fe8c4e. Consider uploading reports for the commit 5fe8c4e to get more accurate results

@@            Coverage Diff             @@
##             main    #1534      +/-   ##
==========================================
+ Coverage   65.08%   65.20%   +0.11%     
==========================================
  Files          86       86              
  Lines       12476    12518      +42     
==========================================
+ Hits         8120     8162      +42     
- Misses       3835     3837       +2     
+ Partials      521      519       -2

Files Changed	Coverage Δ
internal/provider/kubernetes/controller.go	`48.52% <56.52%> (+1.01%)`	⬆️

... and 4 files with indirect coverage changes

github-actions · 2023-07-21T16:02:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

internal/provider/kubernetes/controller_test.go

internal/provider/kubernetes/controller.go

arkodg · 2023-07-21T16:08:51Z

sorry for the delay in review @cnvergence, added some comments, can you please rebase as well, thanks !

arkodg

LGTM, thanks for hanging in there !

arkodg · 2023-07-27T18:00:06Z

hey @cnvergence trying to understand if this flows has been validated

when a GatewayClass stops referencing a EnvoyProxy resource, but is not deleted, do we remove the EnvoyProxy finalizer ? imo we should

cnvergence · 2023-07-28T08:52:06Z

@arkodg Yes, we should, but currently if GatewayClass stops referencing a EnvoyProxy, the EnvoyProxy resource will remain on the cluster.
I have opened up a follow-up issue for this during the work on this.

Clean up unused Envoy Proxy resource that was referenced by Gateway Class #1575

arkodg · 2023-07-28T18:35:38Z

@cnvergence if we merge this PR, it feels like a regression for this case

user creates EnvoyProxy E-A
user attaches E-A to GatewayClass G-C, which in turn adds a finalizer on E-A
user clears E-A from G-C
since the finalizer is still on E-A, it cannot be deleted by user, which will have a negative impact on the user's experience

cnvergence · 2023-07-31T15:29:59Z

@arkodg you've got the point there
We could rework processing ParamsRef so we would support this case as well.

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

arkodg · 2023-08-30T21:12:58Z

internal/provider/kubernetes/controller.go

-			}
-			r.log.Error(err, "failed to process parametersRef for gatewayclass", "name", acceptedGC.Name)
-			return reconcile.Result{}, err
+	if err := r.processParamsRef(ctx, acceptedGC, resourceTree); err != nil {


if acceptedGC.Spec.ParametersRef != nil is removed from here, can we add it back ?

looks like we can't, since this is the only code flow which can remove a finalizer from an unreferenced envoyproxy resource

arkodg

LGTM thanks @cnvergence !

…Class (envoyproxy#1534)" This reverts commit e158ef1. Signed-off-by: Arko Dasgupta <arko@tetrate.io>

…Class(#1534) (#2000) Revert "provider: finalize EnvoyProxy referenced by a managed GatewayClass (#1534)" This reverts commit e158ef1. Signed-off-by: Arko Dasgupta <arko@tetrate.io>

cnvergence changed the title ~~Add/remove finalizers for envoy proxies~~ provider: finalize EnvoyProxy referenced by a managed GatewayClass Jun 15, 2023

cnvergence force-pushed the envoy-proxy-finalizers branch 2 times, most recently from 4c8ff65 to 535b66a Compare June 15, 2023 15:54

cnvergence force-pushed the envoy-proxy-finalizers branch 3 times, most recently from 49674be to 9d4cfa1 Compare June 18, 2023 19:30

cnvergence marked this pull request as ready for review June 19, 2023 08:21

cnvergence requested a review from a team as a code owner June 19, 2023 08:21

cnvergence force-pushed the envoy-proxy-finalizers branch from 5cd8828 to 75672f2 Compare June 21, 2023 12:54

github-actions bot added the stale label Jul 21, 2023

arkodg reviewed Jul 21, 2023

View reviewed changes

internal/provider/kubernetes/controller_test.go Outdated Show resolved Hide resolved

arkodg reviewed Jul 21, 2023

View reviewed changes

internal/provider/kubernetes/controller.go Outdated Show resolved Hide resolved

github-actions bot removed the stale label Jul 21, 2023

cnvergence force-pushed the envoy-proxy-finalizers branch 3 times, most recently from 9d8523d to add74e8 Compare July 26, 2023 14:59

arkodg modified the milestones: 0.6.0, 0.6.0-rc1 Jul 27, 2023

arkodg previously approved these changes Jul 27, 2023

View reviewed changes

arkodg self-requested a review July 27, 2023 17:58

cnvergence dismissed arkodg’s stale review via cefe3bb July 31, 2023 15:22

cnvergence force-pushed the envoy-proxy-finalizers branch from 318ae8a to cefe3bb Compare July 31, 2023 15:22

cnvergence added 11 commits August 30, 2023 21:47

Add/remove finalizers for envoy proxies

d11b0e0

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Refactor finalizers funcs

4fb41f6

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

add finalizer check to the tests

7713603

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Add rbac role and process finalizers

30a6ab1

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

improve logging

17aa923

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Update test case remove finalizer

34174e9

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Rework processing ParamsRef

e1197da

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Remove roles.yaml

0ceae24

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Add patch to rbac.tpl

55bc00a

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Add integration test for envoyproxy finalizers

5cf95f4

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

Restore envoyproxy status checks

5fe8c4e

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

cnvergence force-pushed the envoy-proxy-finalizers branch from 7c25ea0 to 5fe8c4e Compare August 30, 2023 19:47

arkodg reviewed Aug 30, 2023

View reviewed changes

arkodg approved these changes Aug 30, 2023

View reviewed changes

arkodg requested review from a team, Xunzhuo, qicz and zhaohuabing and removed request for a team September 12, 2023 19:24

arkodg mentioned this pull request Sep 12, 2023

feat: watch resource with selectors #1661

Merged

Alice-Lilith approved these changes Sep 12, 2023

View reviewed changes

Alice-Lilith merged commit e158ef1 into envoyproxy:main Sep 12, 2023

cnvergence deleted the envoy-proxy-finalizers branch September 13, 2023 12:51

Alice-Lilith mentioned this pull request Sep 14, 2023

Finalize EnvoyProxy #865

Open

cnvergence mentioned this pull request Oct 18, 2023

fix: wrong gatewayclass status without paramsRef #1999

Closed

arkodg pushed a commit to arkodg/gateway that referenced this pull request Oct 18, 2023

Revert "provider: finalize EnvoyProxy referenced by a managed Gateway…

88096e3

…Class (envoyproxy#1534)" This reverts commit e158ef1. Signed-off-by: Arko Dasgupta <arko@tetrate.io>

cnvergence mentioned this pull request Jul 22, 2024

feat: finalize envoyproxy for gatewayclass #3930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider: finalize EnvoyProxy referenced by a managed GatewayClass#1534

provider: finalize EnvoyProxy referenced by a managed GatewayClass#1534
Alice-Lilith merged 11 commits intoenvoyproxy:mainfrom
cnvergence:envoy-proxy-finalizers

cnvergence commented Jun 15, 2023 •

edited

Loading

Uh oh!

codecov bot commented Jun 15, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Jul 21, 2023

Uh oh!

Uh oh!

Uh oh!

arkodg commented Jul 21, 2023

Uh oh!

arkodg left a comment •

edited

Loading

Uh oh!

arkodg commented Jul 27, 2023

Uh oh!

cnvergence commented Jul 28, 2023

Uh oh!

arkodg commented Jul 28, 2023

Uh oh!

cnvergence commented Jul 31, 2023

Uh oh!

arkodg Aug 30, 2023

Uh oh!

arkodg Aug 30, 2023

Uh oh!

arkodg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cnvergence commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jul 21, 2023

Uh oh!

Uh oh!

Uh oh!

arkodg commented Jul 21, 2023

Uh oh!

arkodg left a comment • 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!

arkodg commented Jul 27, 2023

Uh oh!

cnvergence commented Jul 28, 2023

Uh oh!

arkodg commented Jul 28, 2023

Uh oh!

cnvergence commented Jul 31, 2023

Uh oh!

arkodg Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cnvergence commented Jun 15, 2023 •

edited

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading

arkodg left a comment •

edited

Loading