From 17c911bcd895e1904547911a68c17ab5a7d9b1de Mon Sep 17 00:00:00 2001 From: Claudio Netto Date: Wed, 25 Mar 2020 17:40:38 -0300 Subject: [PATCH] fix(api): prevent nil dereferencing in GetInstanceInfo method --- internal/pkg/rpaas/k8s.go | 77 +++++++--------- internal/pkg/rpaas/k8s_test.go | 157 +++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+), 44 deletions(-) diff --git a/internal/pkg/rpaas/k8s.go b/internal/pkg/rpaas/k8s.go index 10dd02954..32fbee2e5 100644 --- a/internal/pkg/rpaas/k8s.go +++ b/internal/pkg/rpaas/k8s.go @@ -1378,35 +1378,36 @@ func setTeamOwner(instance *v1alpha1.RpaasInstance, team string) { instance.Spec.PodTemplate.Labels = mergeMap(instance.Spec.PodTemplate.Labels, newLabels) } -func newInstanceInfo(instance *v1alpha1.RpaasInstance, ingresses []corev1.LoadBalancerIngress) *clientTypes.InstanceInfo { +func newInstanceInfo(instance *v1alpha1.RpaasInstance, routes []Route, ingresses []corev1.LoadBalancerIngress) *clientTypes.InstanceInfo { info := &clientTypes.InstanceInfo{ - Replicas: instance.Spec.Replicas, - Plan: instance.Spec.PlanName, - Autoscale: &clientTypes.Autoscale{ - MinReplicas: instance.Spec.Autoscale.MinReplicas, - MaxReplicas: &instance.Spec.Autoscale.MaxReplicas, - CPU: instance.Spec.Autoscale.TargetCPUUtilizationPercentage, - Memory: instance.Spec.Autoscale.TargetMemoryUtilizationPercentage, - }, - Binds: instance.Spec.Binds, - Name: instance.ObjectMeta.Name, + Name: instance.ObjectMeta.Name, + Description: instance.Annotations[labelKey("description")], + Team: instance.Annotations[labelKey("team-owner")], + Tags: strings.Split(instance.Annotations[labelKey("tags")], ","), + Replicas: instance.Spec.Replicas, + Plan: instance.Spec.PlanName, + Binds: instance.Spec.Binds, } - for _, route := range instance.Spec.Locations { - info.Routes = append(info.Routes, clientTypes.Route{ - Path: route.Path, - Destination: route.Destination, - }) + autoscale := instance.Spec.Autoscale + if autoscale != nil { + info.Autoscale = &clientTypes.Autoscale{ + MinReplicas: autoscale.MinReplicas, + MaxReplicas: &autoscale.MaxReplicas, + CPU: autoscale.TargetCPUUtilizationPercentage, + Memory: autoscale.TargetMemoryUtilizationPercentage, + } } - if desc, ok := instance.ObjectMeta.Annotations["description"]; ok { - info.Description = desc + for _, r := range routes { + info.Routes = append(info.Routes, clientTypes.Route{ + Path: r.Path, + Destination: r.Destination, + Content: r.Content, + HTTPSOnly: r.HTTPSOnly, + }) } - info.Tags = strings.Split(instance.ObjectMeta.Annotations["tags"], ",") - - setInfoTeam(instance, info) - for _, ingress := range ingresses { info.Addresses = append(info.Addresses, clientTypes.InstanceAddress{ Hostname: ingress.Hostname, @@ -1420,6 +1421,10 @@ func newInstanceInfo(instance *v1alpha1.RpaasInstance, ingresses []corev1.LoadBa func (m *k8sRpaasManager) getLoadBalancerIngress(ctx context.Context, instance *v1alpha1.RpaasInstance) ([]corev1.LoadBalancerIngress, error) { var nginx nginxv1alpha1.Nginx err := m.cli.Get(ctx, types.NamespacedName{Name: instance.Name, Namespace: instance.Namespace}, &nginx) + if err != nil && k8sErrors.IsNotFound(err) { + return nil, nil + } + if err != nil { return nil, err } @@ -1438,37 +1443,21 @@ func (m *k8sRpaasManager) getLoadBalancerIngress(ctx context.Context, instance * return svc.Status.LoadBalancer.Ingress, nil } -func setInfoTeam(instance *v1alpha1.RpaasInstance, infoPayload *clientTypes.InstanceInfo) { - teamLabelKey := labelKey("team-owner") - team, ok := instance.ObjectMeta.Annotations[teamLabelKey] - if ok { - infoPayload.Team = team - return - } - - team, ok = instance.Labels[teamLabelKey] - if ok { - infoPayload.Team = team - return - } - - team, ok = instance.Spec.PodTemplate.Labels[teamLabelKey] - if ok { - infoPayload.Team = team - return - } -} - func (m *k8sRpaasManager) GetInstanceInfo(ctx context.Context, instanceName string) (*clientTypes.InstanceInfo, error) { instance, err := m.GetInstance(ctx, instanceName) if err != nil { return nil, err } + routes, err := m.GetRoutes(ctx, instanceName) + if err != nil { + return nil, err + } + ingresses, err := m.getLoadBalancerIngress(ctx, instance) if err != nil { return nil, err } - return newInstanceInfo(instance, ingresses), nil + return newInstanceInfo(instance, routes, ingresses), nil } diff --git a/internal/pkg/rpaas/k8s_test.go b/internal/pkg/rpaas/k8s_test.go index 99bc4c207..80fb14248 100644 --- a/internal/pkg/rpaas/k8s_test.go +++ b/internal/pkg/rpaas/k8s_test.go @@ -3571,3 +3571,160 @@ func Test_k8sRpaasManager_DeleteAutoscale(t *testing.T) { }) } } + +func Test_k8sRpaasManager_GetInstanceInfo(t *testing.T) { + instance1 := newEmptyRpaasInstance() + instance1.Name = "instance1" + instance1.Annotations = map[string]string{ + "rpaas.extensions.tsuru.io/description": "Some description about this instance", + "rpaas.extensions.tsuru.io/tags": "tag1,tag2,tag3", + "rpaas.extensions.tsuru.io/team-owner": "tsuru", + } + instance1.Spec.PlanName = "huge" + + instance2 := instance1.DeepCopy() + instance2.Name = "instance2" + instance2.Spec.Replicas = pointerToInt(3) + instance2.Spec.Autoscale = &v1alpha1.RpaasInstanceAutoscaleSpec{ + MaxReplicas: 100, + MinReplicas: pointerToInt(1), + TargetCPUUtilizationPercentage: pointerToInt(90), + } + + instance3 := instance1.DeepCopy() + instance3.Name = "instance3" + instance3.Spec.Locations = []v1alpha1.Location{ + { + Path: "/custom/path/1", + Destination: "app1.tsuru.example.com", + }, + { + Path: "/custom/path/2", + Content: &v1alpha1.Value{ + Value: "# some nginx configuration", + }, + }, + { + Path: "/custom/path/3", + Destination: "app3.tsuru.example.com", + ForceHTTPS: true, + }, + } + + instance4 := instance1.DeepCopy() + instance4.Name = "instance4" + + service4 := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: instance4.Name + "-service", + Namespace: instance4.Namespace, + }, + Status: corev1.ServiceStatus{ + LoadBalancer: corev1.LoadBalancerStatus{ + Ingress: []corev1.LoadBalancerIngress{ + { + IP: "192.168.10.10", + Hostname: "instance4-service.rpaasv2.tsuru.example.com", + }, + }, + }, + }, + } + + nginx4 := &nginxv1alpha1.Nginx{ + ObjectMeta: metav1.ObjectMeta{ + Name: instance4.Name, + Namespace: instance4.Namespace, + }, + Status: nginxv1alpha1.NginxStatus{ + Services: []nginxv1alpha1.ServiceStatus{ + {Name: service4.Name}, + }, + }, + } + + resources := []runtime.Object{instance1, instance2, instance3, instance4, nginx4, service4} + + testCases := []struct { + instance string + expected clientTypes.InstanceInfo + }{ + { + instance: "instance1", + expected: clientTypes.InstanceInfo{ + Name: "instance1", + Description: "Some description about this instance", + Team: "tsuru", + Tags: []string{"tag1", "tag2", "tag3"}, + Plan: "huge", + }, + }, + { + instance: "instance2", + expected: clientTypes.InstanceInfo{ + Name: "instance2", + Description: "Some description about this instance", + Team: "tsuru", + Tags: []string{"tag1", "tag2", "tag3"}, + Plan: "huge", + Replicas: pointerToInt(3), + Autoscale: &clientTypes.Autoscale{ + MaxReplicas: pointerToInt(100), + MinReplicas: pointerToInt(1), + CPU: pointerToInt(90), + }, + }, + }, + { + instance: "instance3", + expected: clientTypes.InstanceInfo{ + Name: "instance3", + Description: "Some description about this instance", + Team: "tsuru", + Tags: []string{"tag1", "tag2", "tag3"}, + Plan: "huge", + Routes: []clientTypes.Route{ + { + Path: "/custom/path/1", + Destination: "app1.tsuru.example.com", + }, + { + Path: "/custom/path/2", + Content: "# some nginx configuration", + }, + { + Path: "/custom/path/3", + Destination: "app3.tsuru.example.com", + HTTPSOnly: true, + }, + }, + }, + }, + { + instance: "instance4", + expected: clientTypes.InstanceInfo{ + Name: "instance4", + Description: "Some description about this instance", + Team: "tsuru", + Tags: []string{"tag1", "tag2", "tag3"}, + Plan: "huge", + Addresses: []clientTypes.InstanceAddress{ + { + IP: "192.168.10.10", + Hostname: "instance4-service.rpaasv2.tsuru.example.com", + }, + }, + }, + }, + } + + for _, tt := range testCases { + t.Run("", func(t *testing.T) { + manager := &k8sRpaasManager{cli: fake.NewFakeClientWithScheme(newScheme(), resources...)} + got, err := manager.GetInstanceInfo(context.Background(), tt.instance) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, tt.expected, *got) + }) + } +}