feat: allow VirtualService HTTPRoute to be inferred if there is single route #1273
Conversation
…e route Signed-off-by: Jesse Suen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1273 +/- ##
==========================================
+ Coverage 81.35% 81.39% +0.03%
==========================================
Files 106 106
Lines 9597 9604 +7
==========================================
+ Hits 7808 7817 +9
+ Misses 1260 1259 -1
+ Partials 529 528 -1
Continue to review full report at Codecov.
|
| vsvcUn, err := client.Resource(istioutil.GetIstioVirtualServiceGVR()).Namespace(ro.Namespace).Get(context.TODO(), "vsvc", metav1.GetOptions{}) | ||
| assert.NoError(t, err) | ||
| routes, _, _ := unstructured.NestedSlice(vsvcUn.Object, "spec", "http") | ||
| route := routes[0].(map[string]interface{}) |
There was a problem hiding this comment.
Shall we add an assert.Equal to verify the route name is correctly inferred?
There was a problem hiding this comment.
Actually the test VirtualService intentionally does not have a route name so there's no name to verify:
kind: VirtualService
metadata:
name: vsvc
namespace: default
spec:
gateways:
- istio-rollout-gateway
hosts:
- istio-rollout.dev.argoproj.io
http:
- route:
- destination:
host: 'stable'
weight: 100
- destination:
host: canary
weight: 0Signed-off-by: Jesse Suen <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
| canarySubset := "" | ||
| stableSubset := "" | ||
| if r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule != nil { | ||
| canarySubset = r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule.CanarySubsetName | ||
| stableSubset = r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule.StableSubsetName | ||
| } | ||
|
|
||
| // err can be ignored because we already called ValidateHTTPRoutes earlier | ||
| routeIndexesToPatch, _ := getRouteIndexesToPatch(r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.VirtualService.Routes, httpRoutes) |
There was a problem hiding this comment.
nitpick: I would panic just in case. If for some reason getRouteIndexesToPatch returns error then routeIndexesToPatch is nil/empty slice. The crash is better than silently do nothing.
There was a problem hiding this comment.
I did consider that but determined it was safe to proceed, even on error because we would only iterate a nil slice, which is safe. Having a panic would be equivalent to allowing a nil pointer dereference.
|
LGTM |
…e route (argoproj#1273) Signed-off-by: Jesse Suen <[email protected]> Signed-off-by: Andrii Perenesenko <[email protected]>
…e route (argoproj#1273) Signed-off-by: Jesse Suen <[email protected]>
…e route (argoproj#1273) Signed-off-by: Jesse Suen <[email protected]>
Currently, the istio configuration in the rollout requires a VirtualService HTTPRoute name to be explicitly specified.
This change makes it possible to infer the HTTP route in the event that there is only a single route specified, simplifying the Rollout config. e.g.:
Signed-off-by: Jesse Suen [email protected]