Skip to content

Commit

Permalink
fix(trafficrouting): Fix setHeaderRoute behavior to affect only ingre…
Browse files Browse the repository at this point in the history 
…ss rules related to rollout

Signed-off-by: Armen Shakhbazian <[email protected]>
  • Loading branch information
@ArenSH
ArenSH committed Aug 19, 2024
1 parent a38748b commit 8ccfbdb
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 30 deletions.
3 changes: 2 additions & 1 deletion rollout/trafficrouting/alb/alb.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ func (r *Reconciler) SetHeaderRoute(headerRoute *v1alpha1.SetHeaderRoute) error
}

func (r *Reconciler) SetHeaderRoutePerIngress(headerRoute *v1alpha1.SetHeaderRoute, ingresses []string) error {
stableService, _ := trafficrouting.GetStableAndCanaryServices(r.cfg.Rollout, true)
for _, ingress := range ingresses {
ctx := context.TODO()
rollout := r.cfg.Rollout
Expand All @@ -164,7 +165,7 @@ func (r *Reconciler) SetHeaderRoutePerIngress(headerRoute *v1alpha1.SetHeaderRou
desiredIngress.RemovePathByServiceName(action)
}
if !hasRule && headerRoute.Match != nil {
desiredIngress.CreateAnnotationBasedPath(action)
desiredIngress.CreateAnnotationBasedPath(action, stableService)
}
desiredIngress.SortHttpPaths(rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes)
patch, modified, err := ingressutil.BuildIngressPatch(ingress.Mode(), ingress, desiredIngress, ingressutil.WithAnnotations(), ingressutil.WithSpec())
Expand Down
111 changes: 91 additions & 20 deletions rollout/trafficrouting/alb/alb_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"k8s.io/utils/pointer"

"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
"github.com/argoproj/argo-rollouts/pkg/client/clientset/versioned/scheme"
"github.com/argoproj/argo-rollouts/utils/aws"
ingressutil "github.com/argoproj/argo-rollouts/utils/ingress"
jsonutil "github.com/argoproj/argo-rollouts/utils/json"
Expand Down Expand Up @@ -125,6 +126,29 @@ func albActionAnnotation(stable string) string {
return fmt.Sprintf("%s%s%s", ingressutil.ALBIngressAnnotation, ingressutil.ALBActionPrefix, stable)
}

func ingressRule(name string) networkingv1.IngressRule {
return networkingv1.IngressRule{
IngressRuleValue: networkingv1.IngressRuleValue{
HTTP: &networkingv1.HTTPIngressRuleValue{
Paths: []networkingv1.HTTPIngressPath{
{
Backend: networkingv1.IngressBackend{
Service: &networkingv1.IngressServiceBackend{
Name: name,
Port: networkingv1.ServiceBackendPort{
Name: "use-annotation",
Number: 0,
},
},
Resource: nil,
},
},
},
},
},
}
}

func ingress(name, stableSvc, canarySvc, actionService string, port, weight int32, managedBy string, includeStickyConfig bool) *networkingv1.Ingress {
managedByValue := ingressutil.ManagedALBAnnotations{
managedBy: ingressutil.ManagedALBAnnotation{albActionAnnotation(actionService)},
Expand All @@ -150,26 +174,7 @@ func ingress(name, stableSvc, canarySvc, actionService string, port, weight int3
},
Spec: networkingv1.IngressSpec{
Rules: []networkingv1.IngressRule{
{
IngressRuleValue: networkingv1.IngressRuleValue{
HTTP: &networkingv1.HTTPIngressRuleValue{
Paths: []networkingv1.HTTPIngressPath{
{
Backend: networkingv1.IngressBackend{
Service: &networkingv1.IngressServiceBackend{
Name: actionService,
Port: networkingv1.ServiceBackendPort{
Name: "use-annotation",
Number: 0,
},
},
Resource: nil,
},
},
},
},
},
},
ingressRule(actionService),
},
},
}
Expand Down Expand Up @@ -2059,6 +2064,72 @@ func TestSetHeaderRouteMultiIngress(t *testing.T) {
assert.Len(t, client.Actions(), 2)
}

func TestSetHeaderAffectsOnlyRelatedRules(t *testing.T) {
ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{
{Name: "header-route"},
}
ingress := ingress("ingress", STABLE_SVC, CANARY_SVC, "stable-svc", 443, 10, ro.Name, false)
// now we have stable-svc, unrelated 1, unrelated 2, stable-svc rules
ingress.Spec.Rules = append(ingress.Spec.Rules, ingressRule("unrelated 1"), ingressRule("unrelated 2"), ingressRule(STABLE_SVC))

client := fake.NewSimpleClientset(ingress)
k8sI := kubeinformers.NewSharedInformerFactory(client, 0)
k8sI.Networking().V1().Ingresses().Informer().GetIndexer().Add(ingress)
ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeNetworking, client, k8sI)
if err != nil {
t.Fatal(err)
}
r, err := NewReconciler(ReconcilerConfig{
Rollout: ro,
Client: client,
Recorder: record.NewFakeEventRecorder(),
ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"},
IngressWrapper: ingressWrapper,
})
assert.NoError(t, err)
err = r.SetHeaderRoute(&v1alpha1.SetHeaderRoute{
Name: "header-route",
Match: []v1alpha1.HeaderRoutingMatch{{
HeaderName: "Agent",
HeaderValue: &v1alpha1.StringMatch{
Prefix: "Chrome",
},
}},
})

assert.Nil(t, err)
actions := client.Actions()
assert.Len(t, client.Actions(), 1)
assert.Equal(t, "patch", actions[0].GetVerb())
if patchAction, ok := actions[0].(k8stesting.PatchAction); ok {
var patchIngress networkingv1.Ingress
_, _, err = scheme.Codecs.UniversalDecoder().Decode(patchAction.GetPatch(), nil, &patchIngress)
assert.Nil(t, err)

assert.Len(t, patchIngress.Spec.Rules, 4)
// Expect first route to have additional path
assert.Len(t, patchIngress.Spec.Rules[0].HTTP.Paths, 2)
assert.Equal(t, patchIngress.Spec.Rules[0].HTTP.Paths[0].Backend.Service.Name, "header-route")
assert.Equal(t, patchIngress.Spec.Rules[0].HTTP.Paths[1].Backend.Service.Name, STABLE_SVC)
// Second one is not affected
assert.Len(t, patchIngress.Spec.Rules[1].HTTP.Paths, 1)
assert.Equal(t, patchIngress.Spec.Rules[1].HTTP.Paths[0].Backend.Service.Name, "unrelated 1")
// Third one is not affected
assert.Len(t, patchIngress.Spec.Rules[2].HTTP.Paths, 1)
assert.Equal(t, patchIngress.Spec.Rules[2].HTTP.Paths[0].Backend.Service.Name, "unrelated 2")
// Last route also should have additional path
assert.Len(t, patchIngress.Spec.Rules[3].HTTP.Paths, 2)
assert.Equal(t, patchIngress.Spec.Rules[3].HTTP.Paths[0].Backend.Service.Name, "header-route")
assert.Equal(t, patchIngress.Spec.Rules[3].HTTP.Paths[1].Backend.Service.Name, STABLE_SVC)
}

// no managed routes, no changes expected
err = r.RemoveManagedRoutes()
assert.Nil(t, err)
assert.Len(t, client.Actions(), 1)
}

func TestRemoveManagedRoutes(t *testing.T) {
ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443)
ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{
Expand Down
19 changes: 14 additions & 5 deletions utils/ingress/wrapper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,9 @@ func (i *Ingress) SetAnnotations(annotations map[string]string) {
}
}

func (i *Ingress) CreateAnnotationBasedPath(actionName string) {
// Add path with [actionName] service to rules
// that already have paths with the [stableService] as Backend
func (i *Ingress) CreateAnnotationBasedPath(actionName string, stableService string) {
i.mux.Lock()
defer i.mux.Unlock()
if HasRuleWithService(i, actionName) {
Expand All @@ -197,8 +199,10 @@ func (i *Ingress) CreateAnnotationBasedPath(actionName string) {
},
}
for _, rule := range i.ingress.Spec.Rules {
rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...)
rule.HTTP.Paths[0] = p
if sIdx := indexPathByService(rule, stableService); sIdx != -1 {
rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...)
rule.HTTP.Paths[0] = p
}
}
case IngressModeExtensions:
t := v1beta1.PathTypeImplementationSpecific
Expand All @@ -211,24 +215,29 @@ func (i *Ingress) CreateAnnotationBasedPath(actionName string) {
},
}
for _, rule := range i.legacyIngress.Spec.Rules {
rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...)
rule.HTTP.Paths[0] = p
if sIdx := indexLegacyPathByService(rule, stableService); sIdx != -1 {
rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...)
rule.HTTP.Paths[0] = p
}
}
}
}

// remove paths with [actionName] service from the rules
func (i *Ingress) RemovePathByServiceName(actionName string) {
i.mux.Lock()
defer i.mux.Unlock()
switch i.mode {
case IngressModeNetworking:
for _, rule := range i.ingress.Spec.Rules {
// assume action added only to Rules with proper service in the first place
if j := indexPathByService(rule, actionName); j != -1 {
rule.HTTP.Paths = append(rule.HTTP.Paths[:j], rule.HTTP.Paths[j+1:]...)
}
}
case IngressModeExtensions:
for _, rule := range i.legacyIngress.Spec.Rules {
// assume action added only to Rules with proper service in the first place
if j := indexLegacyPathByService(rule, actionName); j != -1 {
rule.HTTP.Paths = append(rule.HTTP.Paths[:j], rule.HTTP.Paths[j+1:]...)
}
Expand Down
8 changes: 4 additions & 4 deletions utils/ingress/wrapper_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,29 +335,29 @@ func TestCreateAnnotationBasedPath(t *testing.T) {
ni, _ := ing.GetNetworkingIngress()

assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths))
ing.CreateAnnotationBasedPath("test-route")
ing.CreateAnnotationBasedPath("test-route", "v1backend")
assert.Equal(t, 2, len(ni.Spec.Rules[0].HTTP.Paths))
})
t.Run("v1 ingress, create existing path", func(t *testing.T) {
ing := networkingIngress()
ni, _ := ing.GetNetworkingIngress()

ing.CreateAnnotationBasedPath("v1backend")
ing.CreateAnnotationBasedPath("v1backend", "v1backend")
assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths))
})
t.Run("v1beta1 ingress, create path", func(t *testing.T) {
ing := extensionsIngress()
ni, _ := ing.GetExtensionsIngress()

assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths))
ing.CreateAnnotationBasedPath("test-route")
ing.CreateAnnotationBasedPath("test-route", "v1beta1backend")
assert.Equal(t, 2, len(ni.Spec.Rules[0].HTTP.Paths))
})
t.Run("v1beta1 ingress, create existing path", func(t *testing.T) {
ing := extensionsIngress()
ni, _ := ing.GetExtensionsIngress()

ing.CreateAnnotationBasedPath("v1beta1backend")
ing.CreateAnnotationBasedPath("v1beta1backend", "v1beta1backend")
assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths))
})
}
Expand Down

0 comments on commit 8ccfbdb

Please sign in to comment.