-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(translator): support switching between service/clusterIP routing #3543
Conversation
Codecov ReportAttention: Patch coverage is
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. |
thanks for picking this one up @evacchi ! enabling service routing should be make it easier to integrate with service meshes |
47c0d67
to
a03c362
Compare
internal/gatewayapi/testdata/envoyproxy-endpoint-routing-disabled.in.yaml
Outdated
Show resolved
Hide resolved
0bd31a7
to
bf4a29d
Compare
hey you also want to copy over the value into the translator here
|
internal/gatewayapi/testdata/envoyproxy-endpoint-routing.out.yaml
Outdated
Show resolved
Hide resolved
8e0dab3
to
5e68be5
Compare
internal/gatewayapi/testdata/routingpolicy/envoyproxy-service-routing.out.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
hey @evacchi one more hiccup, hopefully last one, there's a another PR #3532 that is adding support for reconciling multiple I suggest deleting gateway/internal/gatewayapi/route.go Line 193 in e866ec1
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 |
@arkodg wouldn't that flip back the default here to false? gateway/internal/cmd/egctl/translate.go Lines 274 to 286 in 412b1b7
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 |
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
Signed-off-by: Edoardo Vacchi <[email protected]>
26db93d
to
23826e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks !
failed tests are just timing out |
kicked the CI, you can also comment on the GH issue with |
This PR surfaces the internal
EndpointRoutingDisabled
flag to theEnvoyProxy
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 toClusterIPRoutingEnabled
(so that the default is, perhaps less surprisingly, "false")The new config option is called
routingType
and it can be set toService
orCluster
.Fixes #1900