Skip to content

Commit

Permalink
refactor(pkg/apis): allow allocateContainerPort field not provided
Browse files Browse the repository at this point in the history
  • Loading branch information
@nettoclaudio
nettoclaudio committed Jul 21, 2020
1 parent 1ddad63 commit 71ccd22
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 18 deletions.
2 changes: 1 addition & 1 deletion pkg/apis/extensions/v1alpha1/rpaasinstance_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/extensions/v1alpha1/zz_generated.deepcopy.go
View file

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

6 changes: 3 additions & 3 deletions pkg/controller/rpaasinstance/rpaasinstance_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
Expand All @@ -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
Expand Down
117 changes: 103 additions & 14 deletions pkg/controller/rpaasinstance/rpaasinstance_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand All @@ -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
Expand All @@ -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",
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand Down Expand Up @@ -1018,7 +1107,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1337",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
Status: v1alpha1.RpaasInstanceStatus{},
},
Expand Down Expand Up @@ -1056,7 +1145,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1337",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
Status: v1alpha1.RpaasInstanceStatus{},
},
Expand Down Expand Up @@ -1144,7 +1233,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1337",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
Status: v1alpha1.RpaasInstanceStatus{},
},
Expand Down Expand Up @@ -1301,7 +1390,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1234",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
Status: v1alpha1.RpaasInstanceStatus{},
},
Expand All @@ -1313,7 +1402,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1337",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
},
&v1alpha1.RpaasPortAllocation{
Expand Down Expand Up @@ -1396,7 +1485,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1234",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
Status: v1alpha1.RpaasInstanceStatus{},
},
Expand All @@ -1408,7 +1497,7 @@ func TestReconcileNginx_reconcilePorts(t *testing.T) {
UID: "1337",
},
Spec: v1alpha1.RpaasInstanceSpec{
AllocateContainerPorts: true,
AllocateContainerPorts: v1alpha1.Bool(true),
},
},
&v1alpha1.RpaasPortAllocation{
Expand Down

0 comments on commit 71ccd22

Please sign in to comment.