Skip to content

feat(translator): support switching between service/clusterIP routing#3543

Merged
arkodg merged 21 commits intoenvoyproxy:mainfrom
evacchi:disable-endpoint-routing
Jun 14, 2024
Merged

feat(translator): support switching between service/clusterIP routing#3543
arkodg merged 21 commits intoenvoyproxy:mainfrom
evacchi:disable-endpoint-routing

Conversation

@evacchi
Copy link
Copy Markdown
Contributor

@evacchi evacchi commented Jun 5, 2024 •
edited
Loading

This PR surfaces the internal EndpointRoutingDisabled flag to the EnvoyProxy spec. This is still missing an e2e test, opening as draft to double-check.

The flag defaults to true as it is currently set internally. It might make sense to flip the name to ClusterIPRoutingEnabled (so that the default is, perhaps less surprisingly, "false")

The new config option is called routingType and it can be set to Service or Cluster.

Fixes #1900

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 68.19%. Comparing base (eb29549) to head (1d18050).

Files Patch % Lines
internal/gatewayapi/translator.go 66.66% 3 Missing and 1 partial ⚠️
internal/gatewayapi/ext_service.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3543      +/-   ##
==========================================
+ Coverage   68.17%   68.19%   +0.02%     
==========================================
  Files         168      168              
  Lines       20484    20496      +12     
==========================================
+ Hits        13964    13977      +13     
+ Misses       5511     5510       -1     
  Partials     1009     1009

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

arkodg
arkodg reviewed Jun 5, 2024
View reviewed changes
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jun 5, 2024

thanks for picking this one up @evacchi ! enabling service routing should be make it easier to integrate with service meshes

@evacchi evacchi force-pushed the disable-endpoint-routing branch from 47c0d67 to a03c362 Compare June 6, 2024 17:39
arkodg
arkodg reviewed Jun 6, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 6, 2024
View reviewed changes
@evacchi evacchi force-pushed the disable-endpoint-routing branch from 0bd31a7 to bf4a29d Compare June 7, 2024 10:03
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 7, 2024
View reviewed changes
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jun 7, 2024

hey you also want to copy over the value into the translator here

gateway/internal/gatewayapi/runner/runner.go

Line 83 in 33fceb0

t := &gatewayapi.Translator{

arkodg
arkodg reviewed Jun 10, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 10, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 10, 2024
View reviewed changes
arkodg
arkodg reviewed Jun 10, 2024
View reviewed changes
@evacchi evacchi force-pushed the disable-endpoint-routing branch 2 times, most recently from 8e0dab3 to 5e68be5 Compare June 11, 2024 14:19
@evacchi evacchi marked this pull request as ready for review June 11, 2024 16:25
@evacchi evacchi requested a review from a team as a code owner June 11, 2024 16:25
arkodg
arkodg previously approved these changes Jun 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team June 11, 2024 16:37
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jun 11, 2024

hey @evacchi one more hiccup, hopefully last one, there's a another PR #3532 that is adding support for reconciling multiple EnvoyProxies that can attach to Gateway, and that impacts this PR.

I suggest deleting IsEndpointRoutingDisabled from the translator struct and instead recomputing it using the helper func that youve added in this PR, e.g. in here

gateway/internal/gatewayapi/route.go

Line 193 in e866ec1

if !t.EndpointRoutingDisabled && ds != nil && len(ds.Endpoints) > 0 && ds.AddressType != nil {

this will also eliminate the need to have a separate test function, and you'll be able to reuse the same one

Once this PR is in, #3532 can modify and pass the right EnvoyProxy into the helper func cc @haoqixu

@evacchi
Copy link
Copy Markdown
Contributor Author

evacchi commented Jun 12, 2024 •
edited
Loading

@arkodg wouldn't that flip back the default here to false?

https://github.com/envoyproxy/gateway/blob/412b1b787dc30fbbff02056cc56e502ae3b0419c/internal/cmd/egctl/translate.go#L274-L286

I will inject an empty EnvoyProxy (not a default Envoy proxy) and propagate the changes, let me know if that makes sense.

I have also renamed isEndpointRoutingDisabled() to IsEnvoyServiceRouting() (to avoid any confusion) and made it a method of *Resources.

evacchi added 18 commits June 13, 2024 09:35
@evacchi
feat(translator): support switching between service/clusterIP routing
0127b3a 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
use enum instead of bool
04b5a0d 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
try adding e2e test
f924c72 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
724db42 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
update-tests
a9b5f4d 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
incorrect default
e7cae30 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
fix formatting
91bc7c2 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
Apply more suggestions
d1fb677 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
bffceb0 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
1c9cc50 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
977ed38 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
fix test
9e2ce62 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
add translator test case
96fb91a 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
3d3cf34 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
testcase
09354da 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
IsEnvoyServiceRouting
bb87bc3 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
Restore explicit disable flag in Translator
5123f2e 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
formatting
23826e1 
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi evacchi force-pushed the disable-endpoint-routing branch from 26db93d to 23826e1 Compare June 13, 2024 07:35
arkodg
arkodg approved these changes Jun 13, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !

@arkodg arkodg requested review from a team June 13, 2024 23:42
@evacchi
Copy link
Copy Markdown
Contributor Author

evacchi commented Jun 14, 2024

failed tests are just timing out

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jun 14, 2024

failed tests are just timing out

kicked the CI, you can also comment on the GH issue with /retest which retriggers the CI

@arkodg arkodg merged commit 59614bd into envoyproxy:main Jun 14, 2024
@kflynn kflynn mentioned this pull request Jun 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg approved these changes

@cnvergence cnvergence cnvergence approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support routing to Service Cluster IP

3 participants

@evacchi @arkodg @cnvergence