From 71ccd22d5662c99c66d25d9029e5236fd7b8ec90 Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Tue, 21 Jul 2020 02:22:52 -0300 Subject: [PATCH] refactor(pkg/apis): allow allocateContainerPort field not provided --- .../v1alpha1/rpaasinstance_types.go | 2 +- .../v1alpha1/zz_generated.deepcopy.go | 5 + .../rpaasinstance/rpaasinstance_controller.go | 6 +- .../rpaasinstance_controller_test.go | 117 +++++++++++++++--- 4 files changed, 112 insertions(+), 18 deletions(-) diff --git a/pkg/apis/extensions/v1alpha1/rpaasinstance_types.go b/pkg/apis/extensions/v1alpha1/rpaasinstance_types.go index 536c27b17..64304da72 100644 --- a/pkg/apis/extensions/v1alpha1/rpaasinstance_types.go +++ b/pkg/apis/extensions/v1alpha1/rpaasinstance_types.go @@ -68,7 +68,7 @@ type RpaasInstanceSpec struct { // when combined with HostNetwork config to avoid conflicts between // multiple nginx instances. // +optional - AllocateContainerPorts bool `json:"allocateContainerPorts,omitempty"` + AllocateContainerPorts *bool `json:"allocateContainerPorts,omitempty"` // Autoscale holds the infos used to configure the HorizontalPodAutoscaler // for this instance. diff --git a/pkg/apis/extensions/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/extensions/v1alpha1/zz_generated.deepcopy.go index 8add3d713..1c617406a 100644 --- a/pkg/apis/extensions/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/extensions/v1alpha1/zz_generated.deepcopy.go @@ -417,6 +417,11 @@ func (in *RpaasInstanceSpec) DeepCopyInto(out *RpaasInstanceSpec) { **out = **in } in.PodTemplate.DeepCopyInto(&out.PodTemplate) + if in.AllocateContainerPorts != nil { + in, out := &in.AllocateContainerPorts, &out.AllocateContainerPorts + *out = new(bool) + **out = **in + } if in.Autoscale != nil { in, out := &in.Autoscale, &out.Autoscale *out = new(RpaasInstanceAutoscaleSpec) diff --git a/pkg/controller/rpaasinstance/rpaasinstance_controller.go b/pkg/controller/rpaasinstance/rpaasinstance_controller.go index 23d657cf0..49ab9a1c6 100644 --- a/pkg/controller/rpaasinstance/rpaasinstance_controller.go +++ b/pkg/controller/rpaasinstance/rpaasinstance_controller.go @@ -1496,7 +1496,7 @@ func (r *ReconcileRpaasInstance) reconcilePorts(ctx context.Context, instance *e highestPortUsed := portMin - 1 // Loop through all allocated ports and remove ports from removed Nginx - // resources or from resources that have AllocateContainerPorts==false. + // resources or from resources that have AllocateContainerPorts==false (or nil). for _, port := range allocation.Spec.Ports { if port.Port > highestPortUsed { highestPortUsed = port.Port @@ -1513,7 +1513,7 @@ func (r *ReconcileRpaasInstance) reconcilePorts(ctx context.Context, instance *e return nil, err } if portBelongsTo(port, instance) { - if !instance.Spec.AllocateContainerPorts { + if !v1alpha1.BoolValue(instance.Spec.AllocateContainerPorts) { continue } instancePorts = append(instancePorts, int(port.Port)) @@ -1526,7 +1526,7 @@ func (r *ReconcileRpaasInstance) reconcilePorts(ctx context.Context, instance *e // If we should allocate ports and none are allocated yet we have to look // for available ports and allocate them. - if instance != nil && instance.Spec.AllocateContainerPorts { + if instance != nil && v1alpha1.BoolValue(instance.Spec.AllocateContainerPorts) { for port := highestPortUsed + 1; port != highestPortUsed; port++ { if len(instancePorts) >= portCount { break diff --git a/pkg/controller/rpaasinstance/rpaasinstance_controller_test.go b/pkg/controller/rpaasinstance/rpaasinstance_controller_test.go index 26bf78932..5c4cfe140 100644 --- a/pkg/controller/rpaasinstance/rpaasinstance_controller_test.go +++ b/pkg/controller/rpaasinstance/rpaasinstance_controller_test.go @@ -148,6 +148,43 @@ func Test_mergePlans(t *testing.T) { } } +func Test_mergeInstance(t *testing.T) { + tests := []struct { + base v1alpha1.RpaasInstanceSpec + override v1alpha1.RpaasInstanceSpec + expected v1alpha1.RpaasInstanceSpec + }{ + {}, + { + base: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + override: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(true)}, + expected: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(true)}, + }, + { + base: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + override: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + expected: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + }, + { + base: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(true)}, + override: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + expected: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(false)}, + }, + { + base: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(true)}, + expected: v1alpha1.RpaasInstanceSpec{AllocateContainerPorts: v1alpha1.Bool(true)}, + }, + } + + for _, tt := range tests { + t.Run("", func(t *testing.T) { + merged, err := mergeInstance(tt.base, tt.override) + require.NoError(t, err) + assert.Equal(t, tt.expected, merged) + }) + } +} + func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { instance1 := newEmptyRpaasInstance() instance1.Name = "instance1" @@ -200,6 +237,10 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { }, } + instance5 := newEmptyRpaasInstance() + instance5.Name = "instance5" + instance5.Spec.Flavors = []string{"banana"} + mintFlavor := newRpaasFlavor() mintFlavor.Name = "mint" mintFlavor.Spec.InstanceTemplate = &v1alpha1.RpaasInstanceSpec{ @@ -246,10 +287,17 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { }, } + bananaFlavor := newRpaasFlavor() + bananaFlavor.Name = "banana" + bananaFlavor.Spec.InstanceTemplate = &v1alpha1.RpaasInstanceSpec{ + AllocateContainerPorts: v1alpha1.Bool(false), + } + defaultFlavor := newRpaasFlavor() defaultFlavor.Name = "default" defaultFlavor.Spec.Default = true defaultFlavor.Spec.InstanceTemplate = &v1alpha1.RpaasInstanceSpec{ + AllocateContainerPorts: v1alpha1.Bool(true), Service: &nginxv1alpha1.NginxService{ Annotations: map[string]string{ "default-service-annotation": "default", @@ -270,7 +318,7 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { }, } - resources := []runtime.Object{instance1, instance2, instance3, instance4, mintFlavor, mangoFlavor, defaultFlavor} + resources := []runtime.Object{instance1, instance2, instance3, instance4, instance5, mintFlavor, mangoFlavor, bananaFlavor, defaultFlavor} tests := []struct { name string @@ -290,7 +338,8 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { Namespace: instance1.Namespace, }, Spec: v1alpha1.RpaasInstanceSpec{ - PlanName: "my-plan", + PlanName: "my-plan", + AllocateContainerPorts: v1alpha1.Bool(true), Service: &nginxv1alpha1.NginxService{ Annotations: map[string]string{ "default-service-annotation": "default", @@ -325,8 +374,9 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { Namespace: instance2.Namespace, }, Spec: v1alpha1.RpaasInstanceSpec{ - Flavors: []string{"mint"}, - PlanName: "my-plan", + Flavors: []string{"mint"}, + PlanName: "my-plan", + AllocateContainerPorts: v1alpha1.Bool(true), Lifecycle: &nginxv1alpha1.NginxLifecycle{ PostStart: &nginxv1alpha1.NginxLifecycleHandler{ Exec: &corev1.ExecAction{ @@ -381,8 +431,9 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { Namespace: instance3.Namespace, }, Spec: v1alpha1.RpaasInstanceSpec{ - Flavors: []string{"mint", "mango"}, - PlanName: "my-plan", + Flavors: []string{"mint", "mango"}, + PlanName: "my-plan", + AllocateContainerPorts: v1alpha1.Bool(true), Service: &nginxv1alpha1.NginxService{ Annotations: map[string]string{ "default-service-annotation": "default", @@ -434,7 +485,8 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { }, }, Spec: v1alpha1.RpaasInstanceSpec{ - PlanName: "my-plan", + PlanName: "my-plan", + AllocateContainerPorts: v1alpha1.Bool(true), Service: &nginxv1alpha1.NginxService{ Annotations: map[string]string{ "default-service-annotation": "default", @@ -457,6 +509,43 @@ func TestReconcileRpaasInstance_getRpaasInstance(t *testing.T) { }, }, }, + { + name: "when default flavor has container port allocations enabled but flavor turn off it", + objectKey: types.NamespacedName{Name: instance5.Name, Namespace: instance5.Namespace}, + expected: v1alpha1.RpaasInstance{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "extensions.tsuru.io/v1alpha1", + Kind: "RpaasInstance", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: instance5.Name, + Namespace: instance5.Namespace, + }, + Spec: v1alpha1.RpaasInstanceSpec{ + PlanName: "my-plan", + Flavors: []string{"banana"}, + AllocateContainerPorts: v1alpha1.Bool(false), + Service: &nginxv1alpha1.NginxService{ + Annotations: map[string]string{ + "default-service-annotation": "default", + }, + Labels: map[string]string{ + "default-service-label": "default", + "flavored-service-label": "default", + }, + }, + PodTemplate: nginxv1alpha1.NginxPodTemplateSpec{ + Annotations: map[string]string{ + "default-pod-annotation": "default", + }, + Labels: map[string]string{ + "mango-pod-label": "not-a-mango", + "default-pod-label": "default", + }, + }, + }, + }, + }, } for _, tt := range tests { @@ -1018,7 +1107,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1337", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, Status: v1alpha1.RpaasInstanceStatus{}, }, @@ -1056,7 +1145,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1337", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, Status: v1alpha1.RpaasInstanceStatus{}, }, @@ -1144,7 +1233,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1337", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, Status: v1alpha1.RpaasInstanceStatus{}, }, @@ -1301,7 +1390,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1234", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, Status: v1alpha1.RpaasInstanceStatus{}, }, @@ -1313,7 +1402,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1337", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, }, &v1alpha1.RpaasPortAllocation{ @@ -1396,7 +1485,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1234", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, Status: v1alpha1.RpaasInstanceStatus{}, }, @@ -1408,7 +1497,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) { UID: "1337", }, Spec: v1alpha1.RpaasInstanceSpec{ - AllocateContainerPorts: true, + AllocateContainerPorts: v1alpha1.Bool(true), }, }, &v1alpha1.RpaasPortAllocation{