Skip to content

feat: support attaching EnvoyProxy resource to Gateways#3532

Merged
zirain merged 24 commits intoenvoyproxy:mainfrom
haoqixu:feat-3317
Jun 20, 2024
Merged

feat: support attaching EnvoyProxy resource to Gateways#3532
zirain merged 24 commits intoenvoyproxy:mainfrom
haoqixu:feat-3317

Conversation

@haoqixu
Copy link
Copy Markdown
Contributor

@haoqixu haoqixu commented Jun 4, 2024

What this PR does
Support attaching EnvoyProxy resource to Gateways if MergeGateways is disabled on GatewayClass.

This PR only implements the controller part and does not implement the egctl part. I would like to submit another PR for the egctl part.

Which issue(s) this PR fixes:

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 70.76412% with 88 lines in your changes missing coverage. Please review.

Project coverage is 68.25%. Comparing base (d49337b) to head (f72618f).

Files Patch % Lines
internal/provider/kubernetes/controller.go 26.73% 67 Missing and 7 partials ⚠️
internal/gatewayapi/securitypolicy.go 82.85% 3 Missing and 3 partials ⚠️
internal/gatewayapi/validate.go 82.14% 5 Missing ⚠️
internal/gatewayapi/envoyextensionpolicy.go 92.00% 1 Missing and 1 partial ⚠️
internal/gatewayapi/ext_service.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3532      +/-   ##
==========================================
- Coverage   68.36%   68.25%   -0.12%     
==========================================
  Files         170      170              
  Lines       20711    20760      +49     
==========================================
+ Hits        14159    14169      +10     
- Misses       5530     5570      +40     
+ Partials     1022     1021       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jun 4, 2024

thanks for picking this one up @haoqixu, looking forward to reviewing it once its ready !

@haoqixu haoqixu force-pushed the feat-3317 branch 8 times, most recently from 8d735fb to 19d9764 Compare June 6, 2024 06:39
@haoqixu
Copy link
Copy Markdown
Contributor Author

haoqixu commented Jun 6, 2024

/retest

@haoqixu haoqixu marked this pull request as ready for review June 6, 2024 07:26
@haoqixu haoqixu requested a review from a team as a code owner June 6, 2024 07:26
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there anyway to wrap these up into a helper function ? just like what we did in

func refsEnvoyProxy(gc *gwapiv1.GatewayClass) bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hey is this refactor related to this GH issue, if not, can we seperate it from this PR ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hey still seeing these refactors, so unresolving this one

Copy link
Copy Markdown
Contributor Author

@haoqixu haoqixu Jun 10, 2024

Choose a reason for hiding this comment

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

The updates in GetRouteParentContext and GetParentReferences is necessary. UpgradeParentReferences will do a deepcopy for the pointer fields and makes ParentReferences unable to be used as map keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @shawnh2 who had last updated this lib

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. The updates in GetRouteParentContext and GetParentReferences is indeed necessary, since the ParentRef for xRoute got all upgraded to v1.

So the codes here do need a clean-up, but we can track this in a separate issue.

@haoqixu haoqixu force-pushed the feat-3317 branch 3 times, most recently from b1125c1 to 19b3d24 Compare June 7, 2024 04:45
@haoqixu haoqixu requested review from arkodg and shawnh2 June 7, 2024 05:08
@haoqixu
Copy link
Copy Markdown
Contributor Author

haoqixu commented Jun 7, 2024

/retest

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @zirain since you had moved this code below in a prev commit

@Xunzhuo Xunzhuo changed the title feat: Support attaching EnvoyProxy resource to Gateways feat: support attaching EnvoyProxy resource to Gateways Jun 10, 2024
@haoqixu
Copy link
Copy Markdown
Contributor Author

haoqixu commented Jun 20, 2024

@haoqixu can rebase or merge main to fix the conflict.

done

@zirain zirain merged commit c752b88 into envoyproxy:main Jun 20, 2024
bjlhlin pushed a commit to bjlhlin/gateway that referenced this pull request Jun 24, 2024
)

* refactor: rename EnvoyProxy field

Signed-off-by: haoqixu <[email protected]>

* feat(controller): process gateway.infrastructure.parametersref

Signed-off-by: haoqixu <[email protected]>

* feat(translator): support attaching EnvoyProxy to Gateways

Signed-off-by: haoqixu <[email protected]>

* chore: fix lint

Signed-off-by: haoqixu <[email protected]>

* chore: update generated code

Signed-off-by: haoqixu <[email protected]>

* chore: fix test

Signed-off-by: haoqixu <[email protected]>

* test: gateway-with-infrastructure-parametersref

Signed-off-by: haoqixu <[email protected]>

* test: add more cases

Signed-off-by: haoqixu <[email protected]>

* fix: EnvoyExtensionPolicy status

Signed-off-by: haoqixu <[email protected]>

* trigger reconciliation for envoyproxies

Signed-off-by: haoqixu <[email protected]>

* chore: rename processGCParamsRef as suggested

Signed-off-by: haoqixu <[email protected]>

* chore: add comments to Resources

Signed-off-by: haoqixu <[email protected]>

* refactor: update as suggested

Signed-off-by: haoqixu <[email protected]>

* refactor: extract resources.GetEnvoyProxy

Signed-off-by: haoqixu <[email protected]>

* refactor: move attachEnvoyProxy into GatewayContext.ResetListeners

Signed-off-by: haoqixu <[email protected]>

* refactor: change ListenerContext.gateway to GatewayContext

Signed-off-by: haoqixu <[email protected]>

* refactor: move getIRKey out of inner loop

Signed-off-by: haoqixu <[email protected]>

* fix: reset *ir.ExtAuth for each gateway

Signed-off-by: haoqixu <[email protected]>

* refactor: continue early to reduce nesting

Signed-off-by: haoqixu <[email protected]>

* refactor: rename envoyProxies and ClassEnvoyProxy

Signed-off-by: haoqixu <[email protected]>

* rename json/yaml name for envoyproxy

Signed-off-by: haoqixu <[email protected]>

* fix: pass the right envoyProxy to t.IsEnvoyServiceRouting

Signed-off-by: haoqixu <[email protected]>

* test: add more cases

Signed-off-by: haoqixu <[email protected]>

* chore: regenerate

Signed-off-by: haoqixu <[email protected]>

---------

Signed-off-by: haoqixu <[email protected]>
Signed-off-by: “bjlhlin” <“[email protected]”>
Signed-off-by: bjlhlin <[email protected]>
bjlhlin pushed a commit to bjlhlin/gateway that referenced this pull request Jun 26, 2024
)

* refactor: rename EnvoyProxy field

Signed-off-by: haoqixu <[email protected]>

* feat(controller): process gateway.infrastructure.parametersref

Signed-off-by: haoqixu <[email protected]>

* feat(translator): support attaching EnvoyProxy to Gateways

Signed-off-by: haoqixu <[email protected]>

* chore: fix lint

Signed-off-by: haoqixu <[email protected]>

* chore: update generated code

Signed-off-by: haoqixu <[email protected]>

* chore: fix test

Signed-off-by: haoqixu <[email protected]>

* test: gateway-with-infrastructure-parametersref

Signed-off-by: haoqixu <[email protected]>

* test: add more cases

Signed-off-by: haoqixu <[email protected]>

* fix: EnvoyExtensionPolicy status

Signed-off-by: haoqixu <[email protected]>

* trigger reconciliation for envoyproxies

Signed-off-by: haoqixu <[email protected]>

* chore: rename processGCParamsRef as suggested

Signed-off-by: haoqixu <[email protected]>

* chore: add comments to Resources

Signed-off-by: haoqixu <[email protected]>

* refactor: update as suggested

Signed-off-by: haoqixu <[email protected]>

* refactor: extract resources.GetEnvoyProxy

Signed-off-by: haoqixu <[email protected]>

* refactor: move attachEnvoyProxy into GatewayContext.ResetListeners

Signed-off-by: haoqixu <[email protected]>

* refactor: change ListenerContext.gateway to GatewayContext

Signed-off-by: haoqixu <[email protected]>

* refactor: move getIRKey out of inner loop

Signed-off-by: haoqixu <[email protected]>

* fix: reset *ir.ExtAuth for each gateway

Signed-off-by: haoqixu <[email protected]>

* refactor: continue early to reduce nesting

Signed-off-by: haoqixu <[email protected]>

* refactor: rename envoyProxies and ClassEnvoyProxy

Signed-off-by: haoqixu <[email protected]>

* rename json/yaml name for envoyproxy

Signed-off-by: haoqixu <[email protected]>

* fix: pass the right envoyProxy to t.IsEnvoyServiceRouting

Signed-off-by: haoqixu <[email protected]>

* test: add more cases

Signed-off-by: haoqixu <[email protected]>

* chore: regenerate

Signed-off-by: haoqixu <[email protected]>

---------

Signed-off-by: haoqixu <[email protected]>
Signed-off-by: bjlhlin <[email protected]>
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.

Support attaching EnvoyProxy resource to Gateways

5 participants