generating ingress virtualservice to enable cross-cluster communication with passthrough gateway #324
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #324 +/- ##
==========================================
- Coverage 71.49% 71.45% -0.05%
==========================================
Files 65 65
Lines 8845 8990 +145
==========================================
+ Hits 6324 6424 +100
- Misses 2191 2227 +36
- Partials 330 339 +9 ☔ View full report in Codecov by Sentry. |
| @@ -260,6 +262,17 @@ func modifyServiceEntryForNewServiceOrPod( | |||
| util.LogElapsedTimeSinceForModifySE(ctxLogger, "AdmiralCacheCreateServiceEntryForDeployment", | |||
| deploymentOrRolloutName, deploymentOrRolloutNS, rc.ClusterID, "", start) | |||
| modifySEerr = common.AppendError(modifySEerr, errCreateSE) | |||
|
|
|||
| if common.IsVSBasedRoutingEnabled() { | |||
There was a problem hiding this comment.
Coud we start introducing interface and decouple the interaction of service entry? This would help test units in isolation and not have integration style unit tests. we would be benefit from injecting mock implementations in the unit tests.
WDYT?
There was a problem hiding this comment.
thats a valid point and it has been discussed several times in the past to refactor the modifySE code. Some parts of the code does employ interface driven mocking strategy but it is specifically lacking in this piece of code.
There was a problem hiding this comment.
Where are we adding the sni value in DestinationRules?
There was a problem hiding this comment.
@nirvanagit , with your fix in istio we would probably won't need to add that. So this implementation currently works with istio versions that contain your fix.
We could make it backwards compatible and I'll be doing that later in upcoming PRs.
There was a problem hiding this comment.
Got it.
So the plan is to keep the VirtualServices ready, and then test out by modifying Gateway to use PASSTHROUGH in a dev setup?
There was a problem hiding this comment.
@nirvanagit Yes we did test changing the to passthrough and rolling back to auto_passthrough, it works without errors. Once the VS is in place
shriramsharma
force-pushed
the
generate-vs-cross-cluster-routing
branch
3 times, most recently
from
79c05fe to
3656a1c
Compare
| if fqdn == "" { | ||
| return "", fmt.Errorf("fqdn is empty") | ||
| } | ||
| return fmt.Sprintf("outbound_.80_._.%s", fqdn), nil |
There was a problem hiding this comment.
We already talked about making this port configurable.
| } | ||
|
|
||
| vs := networkingV1Alpha3.VirtualService{ | ||
| Hosts: []string{host}, |
There was a problem hiding this comment.
We spoke about this as well, the host needs to match the SNI host
There was a problem hiding this comment.
Not sure if this is a bug in the istio implementation
shriramsharma
force-pushed
the
generate-vs-cross-cluster-routing
branch
4 times, most recently
from
8689a3c to
9d0ca04
Compare
| // We are using the SNI host in hosts field as they need to match | ||
| Hosts: []string{sniHost}, | ||
| Gateways: gateways, | ||
| ExportTo: []string{"istio-system"}, |
…on with passthrough gateway Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
Signed-off-by: Shriram Sharma <[email protected]>
shriramsharma
force-pushed
the
generate-vs-cross-cluster-routing
branch
from
5cc8608 to
d4c9629
Compare
Signed-off-by: Shriram Sharma <[email protected]>
Checklist
🚨 Please review this repository's contribution guidelines.
Description
What does this change do and why?
This change would generate VS exported to ingress running in passthrough mode.
[Link to related ISSUE]
Thank you!