Skip to content
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: add support for N nginx ingresses per service #1

Merged
merged 8 commits into from
Jun 30, 2022
Merged
112 changes: 110 additions & 2 deletions ingress/ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/stretchr/testify/assert"
extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
kubeinformers "k8s.io/client-go/informers"
k8sfake "k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -54,15 +55,29 @@ func newNginxIngress(name string, port int, serviceName string) *extensionsv1bet
}
}

func newFakeIngressControllerMultiIngress(t *testing.T, ing []*extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) {
return underlyingControllerBuilder(t, ing, rollout)
}

func newFakeIngressController(t *testing.T, ing *extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) {
return underlyingControllerBuilder(t, []*extensionsv1beta1.Ingress{ing}, rollout)
}

func underlyingControllerBuilder(t *testing.T, ing []*extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) {
t.Helper()
client := fake.NewSimpleClientset()
if rollout != nil {
client = fake.NewSimpleClientset(rollout)
}
kubeclient := k8sfake.NewSimpleClientset()
if ing != nil {
kubeclient = k8sfake.NewSimpleClientset(ing)
var x []runtime.Object
for _, i := range ing {
if i != nil {
x = append(x, i)
}
}
kubeclient = k8sfake.NewSimpleClientset(x...)
}
i := informers.NewSharedInformerFactory(client, 0)
k8sI := kubeinformers.NewSharedInformerFactory(kubeclient, 0)
Expand Down Expand Up @@ -107,7 +122,11 @@ func newFakeIngressController(t *testing.T, ing *extensionsv1beta1.Ingress, roll
}

if ing != nil {
k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(ing)
for _, i := range ing {
if i != nil {
k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i)
}
}
}
if rollout != nil {
i.Argoproj().V1alpha1().Rollouts().Informer().GetIndexer().Add(rollout)
Expand All @@ -133,6 +152,20 @@ func TestSyncIngressNotReferencedByRollout(t *testing.T) {
assert.Len(t, actions, 0)
}

func TestSyncIngressNotReferencedByRolloutMultiIngress(t *testing.T) {
ings := []*extensionsv1beta1.Ingress{
newNginxIngress("test-stable-ingress", 80, "stable-service"),
newNginxIngress("test-stable-ingress-additional", 80, "stable-service"),
}

ctrl, kubeclient, _ := newFakeIngressControllerMultiIngress(t, ings, nil)

err := ctrl.syncIngress("default/test-stable-ingress")
assert.NoError(t, err)
actions := kubeclient.Actions()
assert.Len(t, actions, 0)
}

func TestSyncIngressReferencedByRollout(t *testing.T) {
ing := newNginxIngress("test-stable-ingress", 80, "stable-service")

Expand Down Expand Up @@ -165,6 +198,42 @@ func TestSyncIngressReferencedByRollout(t *testing.T) {
assert.Equal(t, 1, enqueuedObjects["default/rollout"])
}

func TestSyncIngressReferencedByRolloutMultiIngress(t *testing.T) {
ings := []*extensionsv1beta1.Ingress{
newNginxIngress("test-stable-ingress", 80, "stable-service"),
newNginxIngress("test-stable-ingress-additional", 80, "stable-service"),
}

rollout := &v1alpha1.Rollout{
ObjectMeta: metav1.ObjectMeta{
Name: "rollout",
Namespace: metav1.NamespaceDefault,
},
Spec: v1alpha1.RolloutSpec{
Strategy: v1alpha1.RolloutStrategy{
Canary: &v1alpha1.CanaryStrategy{
StableService: "stable-service",
CanaryService: "canary-service",
TrafficRouting: &v1alpha1.RolloutTrafficRouting{
Nginx: &v1alpha1.NginxTrafficRouting{
StableIngress: "test-stable-ingress",
AdditionalStableIngresses: []string{"test-stable-ingress-additional"},
},
},
},
},
},
}

ctrl, kubeclient, enqueuedObjects := newFakeIngressControllerMultiIngress(t, ings, rollout)

err := ctrl.syncIngress("default/test-stable-ingress")
assert.NoError(t, err)
actions := kubeclient.Actions()
assert.Len(t, actions, 0)
assert.Equal(t, 1, enqueuedObjects["default/rollout"])
}

func TestSkipIngressWithNoAnnotations(t *testing.T) {
ing := newNginxIngress("test-stable-ingress", 80, "stable-service")
ing.Annotations = nil
Expand Down Expand Up @@ -196,3 +265,42 @@ func TestSkipIngressWithNoAnnotations(t *testing.T) {
assert.Len(t, actions, 0)
assert.Len(t, enqueuedObjects, 0)
}

func TestSkipIngressWithNoAnnotationsMultiIngress(t *testing.T) {
ings := []*extensionsv1beta1.Ingress{
newNginxIngress("test-stable-ingress", 80, "stable-service"),
newNginxIngress("test-stable-ingress-additional", 80, "stable-service"),
}
for _, i := range ings {
i.Annotations = nil
}

rollout := &v1alpha1.Rollout{
ObjectMeta: metav1.ObjectMeta{
Name: "rollout",
Namespace: metav1.NamespaceDefault,
},
Spec: v1alpha1.RolloutSpec{
Strategy: v1alpha1.RolloutStrategy{
Canary: &v1alpha1.CanaryStrategy{
StableService: "stable-service",
CanaryService: "canary-service",
TrafficRouting: &v1alpha1.RolloutTrafficRouting{
Nginx: &v1alpha1.NginxTrafficRouting{
StableIngress: "test-stable-ingress",
AdditionalStableIngresses: []string{"test-stable-ingress-additional"},
},
},
},
},
},
}

ctrl, kubeclient, enqueuedObjects := newFakeIngressControllerMultiIngress(t, ings, rollout)

err := ctrl.syncIngress("default/test-stable-ingress")
assert.NoError(t, err)
actions := kubeclient.Actions()
assert.Len(t, actions, 0)
assert.Len(t, enqueuedObjects, 0)
}
4 changes: 4 additions & 0 deletions manifests/crds/rollout-crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -761,6 +761,10 @@ spec:
additionalProperties:
type: string
type: object
additionalStableIngresses:
items:
type: string
type: array
annotationPrefix:
type: string
stableIngress:
Expand Down
4 changes: 4 additions & 0 deletions manifests/install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11729,6 +11729,10 @@ spec:
additionalProperties:
type: string
type: object
additionalStableIngresses:
items:
type: string
type: array
annotationPrefix:
type: string
stableIngress:
Expand Down
4 changes: 4 additions & 0 deletions manifests/namespace-install.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11729,6 +11729,10 @@ spec:
additionalProperties:
type: string
type: object
additionalStableIngresses:
items:
type: string
type: array
annotationPrefix:
type: string
stableIngress:
Expand Down
7 changes: 7 additions & 0 deletions pkg/apiclient/rollout/rollout.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,13 @@
"type": "string"
},
"title": "+optional"
},
"additionalStableIngresses": {
"type": "array",
"items": {
"type": "string"
},
"title": "AdditionalStableIngresses refers to the names of `Ingress` resources in the same namespace as the `Rollout` in a multi ingress scenario\n+optional"
}
},
"title": "NginxTrafficRouting configuration for Nginx ingress controller to control traffic routing"
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/api-rules/violation_exceptions.list
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,IstioVirtualService,TLSRoutes
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,KayentaMetric,Scopes
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,MetricResult,Measurements
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,NginxTrafficRouting,AdditionalStableIngresses
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,Args
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,DryRun
API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,MeasurementRetention
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/rollouts/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,9 @@ type NginxTrafficRouting struct {
StableIngress string `json:"stableIngress" protobuf:"bytes,2,opt,name=stableIngress"`
// +optional
AdditionalIngressAnnotations map[string]string `json:"additionalIngressAnnotations,omitempty" protobuf:"bytes,3,rep,name=additionalIngressAnnotations"`
// AdditionalStableIngresses refers to the names of `Ingress` resources in the same namespace as the `Rollout` in a multi ingress scenario
// +optional
AdditionalStableIngresses []string `json:"additionalStableIngresses,omitempty" protobuf:"bytes,4,rep,name=additionalStableIngresses"`
}

// IstioTrafficRouting configuration for Istio service mesh to enable fine grain configuration
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 27 additions & 11 deletions pkg/apis/rollouts/validation/validation_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,23 +218,39 @@ func setArgValuePlaceHolder(Args []v1alpha1.Argument) {
func ValidateIngress(rollout *v1alpha1.Rollout, ingress *ingressutil.Ingress) field.ErrorList {
allErrs := field.ErrorList{}
fldPath := field.NewPath("spec", "strategy", "canary", "trafficRouting")
canary := rollout.Spec.Strategy.Canary
var ingressName string
var serviceName string
if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil {
if canary.TrafficRouting.Nginx != nil {
// If there are additional stable ingresses
if len(canary.TrafficRouting.Nginx.AdditionalStableIngresses) > 0 {
// validate each ingress as valid
fldPath = fldPath.Child("nginx").Child("additionalStableIngresses")
serviceName = canary.StableService
for _, ing := range canary.TrafficRouting.Nginx.AdditionalStableIngresses {
ingressName = ing
allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs)
}
}
fldPath = fldPath.Child("nginx").Child("stableIngress")
serviceName = rollout.Spec.Strategy.Canary.StableService
ingressName = rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress
} else if rollout.Spec.Strategy.Canary.TrafficRouting.ALB != nil {
serviceName = canary.StableService
ingressName = canary.TrafficRouting.Nginx.StableIngress

allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs)
} else if canary.TrafficRouting.ALB != nil {
fldPath = fldPath.Child("alb").Child("ingress")
ingressName = rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress
serviceName = rollout.Spec.Strategy.Canary.StableService
if rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService != "" {
serviceName = rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService
ingressName = canary.TrafficRouting.ALB.Ingress
serviceName = canary.StableService
if canary.TrafficRouting.ALB.RootService != "" {
serviceName = canary.TrafficRouting.ALB.RootService
}

} else {
return allErrs
allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs)
}

return allErrs
}

func reportErrors(ingress *ingressutil.Ingress, serviceName, ingressName string, fldPath *field.Path, allErrs field.ErrorList) field.ErrorList {
if !ingressutil.HasRuleWithService(ingress, serviceName) {
msg := fmt.Sprintf("ingress `%s` has no rules using service %s backend", ingress.GetName(), serviceName)
allErrs = append(allErrs, field.Invalid(fldPath, ingressName, msg))
Expand Down
13 changes: 13 additions & 0 deletions rollout/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -780,6 +780,19 @@ func (c *rolloutContext) getReferencedIngresses() (*[]ingressutil.Ingress, error
}
ingresses = append(ingresses, *ingress)
} else if canary.TrafficRouting.Nginx != nil {
// If the rollout resource manages more than 1 ingress
if len(canary.TrafficRouting.Nginx.AdditionalStableIngresses) > 0 {
for _, ing := range canary.TrafficRouting.Nginx.AdditionalStableIngresses {
ingress, err := c.ingressWrapper.GetCached(c.rollout.Namespace, ing)
if k8serrors.IsNotFound(err) {
return nil, field.Invalid(fldPath.Child("nginx", "AdditionalStableIngresses"), canary.TrafficRouting.Nginx.StableIngress, err.Error())
}
if err != nil {
return nil, err
}
ingresses = append(ingresses, *ingress)
}
}
ingress, err := c.ingressWrapper.GetCached(c.rollout.Namespace, canary.TrafficRouting.Nginx.StableIngress)
if k8serrors.IsNotFound(err) {
return nil, field.Invalid(fldPath.Child("nginx", "stableIngress"), canary.TrafficRouting.Nginx.StableIngress, err.Error())
Expand Down
Loading