From 42e88a7f89194164db47d199913910e01177f4c2 Mon Sep 17 00:00:00 2001 From: Srikanth Date: Fri, 6 Dec 2019 00:27:12 +0530 Subject: [PATCH 1/6] Initial changes for load balancer task Add load balancer fi tasks Add load balancer builder for DO Fix go imports Implement FindIPAddress functionality Add load balancer api ingress status calls Add error checks for FindIPAddress Add delete LB option Update load balancer delete logic Revert make file changes revert utils code changes Revert NewDOCloud changes Remove minor code comments Update with gomod for dependencies --- pkg/commands/BUILD.bazel | 1 + pkg/commands/status_discovery.go | 5 + pkg/model/domodel/BUILD.bazel | 4 + pkg/model/domodel/api_loadbalancer.go | 93 ++++++++ pkg/resources/digitalocean/cloud.go | 40 ++++ pkg/resources/digitalocean/resources.go | 81 ++++++- upup/pkg/fi/cloudup/apply_cluster.go | 9 +- upup/pkg/fi/cloudup/dotasks/BUILD.bazel | 2 + upup/pkg/fi/cloudup/dotasks/loadbalancer.go | 201 ++++++++++++++++++ .../fi/cloudup/dotasks/loadbalancer_fitask.go | 75 +++++++ 10 files changed, 506 insertions(+), 5 deletions(-) create mode 100644 pkg/model/domodel/api_loadbalancer.go create mode 100644 upup/pkg/fi/cloudup/dotasks/loadbalancer.go create mode 100644 upup/pkg/fi/cloudup/dotasks/loadbalancer_fitask.go diff --git a/pkg/commands/BUILD.bazel b/pkg/commands/BUILD.bazel index 6683aa7684f32..053b5e4117d54 100644 --- a/pkg/commands/BUILD.bazel +++ b/pkg/commands/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//pkg/assets:go_default_library", "//pkg/client/simple:go_default_library", "//pkg/featureflag:go_default_library", + "//pkg/resources/digitalocean:go_default_library", "//upup/pkg/fi/cloudup:go_default_library", "//upup/pkg/fi/cloudup/aliup:go_default_library", "//upup/pkg/fi/cloudup/awstasks:go_default_library", diff --git a/pkg/commands/status_discovery.go b/pkg/commands/status_discovery.go index 1d34528802e79..69b412de3d655 100644 --- a/pkg/commands/status_discovery.go +++ b/pkg/commands/status_discovery.go @@ -21,6 +21,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "k8s.io/kops/pkg/apis/kops" + "k8s.io/kops/pkg/resources/digitalocean" "k8s.io/kops/upup/pkg/fi/cloudup" "k8s.io/kops/upup/pkg/fi/cloudup/aliup" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" @@ -77,6 +78,10 @@ func (s *CloudDiscoveryStatusStore) GetApiIngressStatus(cluster *kops.Cluster) ( return osCloud.GetApiIngressStatus(cluster) } + if doCloud, ok := cloud.(*digitalocean.Cloud); ok { + return doCloud.GetApiIngressStatus(cluster) + } + return nil, fmt.Errorf("API Ingress Status not implemented for %T", cloud) } diff --git a/pkg/model/domodel/BUILD.bazel b/pkg/model/domodel/BUILD.bazel index 64cc31bf4e334..c6af73b155117 100644 --- a/pkg/model/domodel/BUILD.bazel +++ b/pkg/model/domodel/BUILD.bazel @@ -3,15 +3,19 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "go_default_library", srcs = [ + "api_loadbalancer.go", "context.go", "droplets.go", ], importpath = "k8s.io/kops/pkg/model/domodel", visibility = ["//visibility:public"], deps = [ + "//pkg/apis/kops:go_default_library", + "//pkg/dns:go_default_library", "//pkg/model:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/do:go_default_library", "//upup/pkg/fi/cloudup/dotasks:go_default_library", + "//upup/pkg/fi/fitasks:go_default_library", ], ) diff --git a/pkg/model/domodel/api_loadbalancer.go b/pkg/model/domodel/api_loadbalancer.go new file mode 100644 index 0000000000000..851a1f09afcbe --- /dev/null +++ b/pkg/model/domodel/api_loadbalancer.go @@ -0,0 +1,93 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package domodel + +import ( + "errors" + "fmt" + "strings" + + "k8s.io/kops/pkg/apis/kops" + "k8s.io/kops/pkg/dns" + "k8s.io/kops/upup/pkg/fi" + "k8s.io/kops/upup/pkg/fi/cloudup/do" + "k8s.io/kops/upup/pkg/fi/cloudup/dotasks" + "k8s.io/kops/upup/pkg/fi/fitasks" +) + +// APILoadBalancerModelBuilder builds a LoadBalancer for accessing the API +type APILoadBalancerModelBuilder struct { + *DOModelContext + Lifecycle *fi.Lifecycle +} + +var _ fi.ModelBuilder = &APILoadBalancerModelBuilder{} + +func (b *APILoadBalancerModelBuilder) Build(c *fi.ModelBuilderContext) error { + // Configuration where a load balancer fronts the API + if !b.UseLoadBalancerForAPI() { + return nil + } + + lbSpec := b.Cluster.Spec.API.LoadBalancer + if lbSpec == nil { + // Skipping API ELB creation; not requested in Spec + return nil + } + + switch lbSpec.Type { + case kops.LoadBalancerTypeInternal: + // OK + case kops.LoadBalancerTypePublic: + // OK + default: + return fmt.Errorf("unhandled LoadBalancer type %q", lbSpec.Type) + } + + loadbalancerName := "api-" + strings.Replace(b.ClusterName(), ".", "-", -1) + clusterMasterTag := do.TagKubernetesClusterMasterPrefix + ":" + strings.Replace(b.ClusterName(), ".", "-", -1) + + // Create LoadBalancer for API ELB + var loadbalancer *dotasks.LoadBalancer + { + + loadbalancer = &dotasks.LoadBalancer{ + Name: fi.String(loadbalancerName), + Region: fi.String(b.Cluster.Spec.Subnets[0].Region), + DropletTag: fi.String(clusterMasterTag), + Lifecycle: b.Lifecycle, + } + + c.AddTask(loadbalancer) + } + + // Temporarily do not know the role of the following function + if dns.IsGossipHostname(b.Cluster.Name) || b.UsePrivateDNS() { + // Ensure the ELB hostname is included in the TLS certificate, + // if we're not going to use an alias for it + // TODO: I don't love this technique for finding the task by name & modifying it + masterKeypairTask, found := c.Tasks["Keypair/master"] + if !found { + return errors.New("keypair/master task not found") + } + masterKeypair := masterKeypairTask.(*fitasks.Keypair) + masterKeypair.AlternateNameTasks = append(masterKeypair.AlternateNameTasks, loadbalancer) + } + + return nil + +} diff --git a/pkg/resources/digitalocean/cloud.go b/pkg/resources/digitalocean/cloud.go index 88e927f786b4d..0c72e81df48cf 100644 --- a/pkg/resources/digitalocean/cloud.go +++ b/pkg/resources/digitalocean/cloud.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "os" + "strings" "github.com/digitalocean/godo" "golang.org/x/oauth2" @@ -128,7 +129,46 @@ func (c *Cloud) Droplets() godo.DropletsService { return c.Client.Droplets } +func (c *Cloud) LoadBalancers() godo.LoadBalancersService { + return c.Client.LoadBalancers +} + // FindVPCInfo is not implemented, it's only here to satisfy the fi.Cloud interface func (c *Cloud) FindVPCInfo(id string) (*fi.VPCInfo, error) { return nil, errors.New("not implemented") } + +func (c *Cloud) GetApiIngressStatus(cluster *kops.Cluster) ([]kops.ApiIngressStatus, error) { + var ingresses []kops.ApiIngressStatus + if cluster.Spec.MasterPublicName != "" { + // Note that this must match Digital Ocean's lb name + klog.V(2).Infof("Querying DO to find Loadbalancers for API (%q)", cluster.Name) + + loadBalaners, _, err := c.LoadBalancers().List(context.TODO(), nil) + + if err != nil { + klog.Errorf("LoadBalancers.List returned error: %v", err) + return nil, err + } + + lbName := "api-" + strings.Replace(cluster.Name, ".", "-", -1) + + for _, lb := range loadBalaners { + if lb.Name == lbName { + klog.V(10).Infof("Matching LB name found for API (%q)", cluster.Name) + + if lb.Status != "active" { + klog.Errorf("load-balancer is not yet active (current status: %s)", lb.Status) + return nil, nil + } + + address := lb.IP + ingresses = append(ingresses, kops.ApiIngressStatus{IP: address}) + + return ingresses, nil + } + } + } + + return nil, nil +} diff --git a/pkg/resources/digitalocean/resources.go b/pkg/resources/digitalocean/resources.go index a144d01223603..47c112bc87d4b 100644 --- a/pkg/resources/digitalocean/resources.go +++ b/pkg/resources/digitalocean/resources.go @@ -33,9 +33,10 @@ import ( ) const ( - resourceTypeDroplet = "droplet" - resourceTypeVolume = "volume" - resourceTypeDNSRecord = "dns-record" + resourceTypeDroplet = "droplet" + resourceTypeVolume = "volume" + resourceTypeDNSRecord = "dns-record" + resourceTypeLoadBalancer = "loadbalancer" ) type listFn func(fi.Cloud, string) ([]*resources.Resource, error) @@ -47,6 +48,7 @@ func ListResources(cloud *Cloud, clusterName string) (map[string]*resources.Reso listVolumes, listDroplets, listDNS, + listLoadBalancers, } for _, fn := range listFunctions { @@ -265,6 +267,67 @@ func getAllRecordsByDomain(cloud *Cloud, domain string) ([]godo.DomainRecord, er return allRecords, nil } +func listLoadBalancers(cloud fi.Cloud, clusterName string) ([]*resources.Resource, error) { + c := cloud.(*Cloud) + var resourceTrackers []*resources.Resource + + clusterTag := "KubernetesCluster-Master:" + strings.Replace(clusterName, ".", "-", -1) + + lbs, err := getAllLoadBalancers(c) + if err != nil { + return nil, fmt.Errorf("failed to list lbs: %v", err) + } + + for _, lb := range lbs { + if strings.Contains(lb.Tag, clusterTag) { + resourceTracker := &resources.Resource{ + Name: lb.Name, + ID: lb.ID, + Type: resourceTypeLoadBalancer, + Deleter: deleteLoadBalancer, + Obj: lb, + } + + var blocks []string + for _, dropletID := range lb.DropletIDs { + blocks = append(blocks, "droplet:"+strconv.Itoa(dropletID)) + } + + resourceTracker.Blocks = blocks + resourceTrackers = append(resourceTrackers, resourceTracker) + } + } + + return resourceTrackers, nil +} + +func getAllLoadBalancers(cloud *Cloud) ([]godo.LoadBalancer, error) { + allLoadBalancers := []godo.LoadBalancer{} + + opt := &godo.ListOptions{} + for { + lbs, resp, err := cloud.LoadBalancers().List(context.TODO(), opt) + if err != nil { + return nil, err + } + + allLoadBalancers = append(allLoadBalancers, lbs...) + + if resp.Links == nil || resp.Links.IsLastPage() { + break + } + + page, err := resp.Links.CurrentPage() + if err != nil { + return nil, err + } + + opt.Page = page + 1 + } + + return allLoadBalancers, nil +} + func deleteDroplet(cloud fi.Cloud, t *resources.Resource) error { c := cloud.(*Cloud) @@ -315,6 +378,18 @@ func deleteRecord(cloud fi.Cloud, domain string, t *resources.Resource) error { return nil } +func deleteLoadBalancer(cloud fi.Cloud, t *resources.Resource) error { + c := cloud.(*Cloud) + lb := t.Obj.(godo.LoadBalancer) + _, err := c.Client.LoadBalancers.Delete(context.TODO(), lb.ID) + + if err != nil { + return fmt.Errorf("failed to delete load balancer with name %s", lb.Name) + } + + return nil +} + func waitForDetach(cloud *Cloud, action *godo.Action) error { timeout := time.After(10 * time.Second) ticker := time.NewTicker(500 * time.Millisecond) diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index eaa995c1602fb..c75c2788d34b5 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -396,8 +396,9 @@ func (c *ApplyClusterCmd) Run() error { modelContext.SSHPublicKeys = sshPublicKeys l.AddTypes(map[string]interface{}{ - "volume": &dotasks.Volume{}, - "droplet": &dotasks.Droplet{}, + "volume": &dotasks.Volume{}, + "droplet": &dotasks.Droplet{}, + "loadbalancer": &dotasks.LoadBalancer{}, }) } case kops.CloudProviderAWS: @@ -643,8 +644,12 @@ func (c *ApplyClusterCmd) Run() error { &model.IAMModelBuilder{KopsModelContext: modelContext, Lifecycle: &securityLifecycle}, ) case kops.CloudProviderDO: + doModelContext := &domodel.DOModelContext{ + KopsModelContext: modelContext, + } l.Builders = append(l.Builders, &model.MasterVolumeBuilder{KopsModelContext: modelContext, Lifecycle: &clusterLifecycle}, + &domodel.APILoadBalancerModelBuilder{DOModelContext: doModelContext, Lifecycle: &securityLifecycle}, ) case kops.CloudProviderGCE: diff --git a/upup/pkg/fi/cloudup/dotasks/BUILD.bazel b/upup/pkg/fi/cloudup/dotasks/BUILD.bazel index c49287836759b..4de9ebf3191c4 100644 --- a/upup/pkg/fi/cloudup/dotasks/BUILD.bazel +++ b/upup/pkg/fi/cloudup/dotasks/BUILD.bazel @@ -5,6 +5,8 @@ go_library( srcs = [ "droplet.go", "droplet_fitask.go", + "loadbalancer.go", + "loadbalancer_fitask.go", "volume.go", "volume_fitask.go", ], diff --git a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go new file mode 100644 index 0000000000000..47480133e041e --- /dev/null +++ b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go @@ -0,0 +1,201 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package dotasks + +import ( + "context" + "errors" + "strconv" + "strings" + "time" + + "github.com/digitalocean/godo" + + "k8s.io/klog" + "k8s.io/kops/pkg/resources/digitalocean" + "k8s.io/kops/upup/pkg/fi" + "k8s.io/kops/upup/pkg/fi/cloudup/do" +) + +//go:generate fitask -type=LoadBalancer +type LoadBalancer struct { + Name *string + ID *string + Lifecycle *fi.Lifecycle + + Region *string + DropletTag *string + IPAddress *string +} + +var _ fi.CompareWithID = &LoadBalancer{} + +func (lb *LoadBalancer) CompareWithID() *string { + return lb.ID +} + +func (lb *LoadBalancer) Find(c *fi.Context) (*LoadBalancer, error) { + cloud := c.Cloud.(*digitalocean.Cloud) + lbService := cloud.LoadBalancers() + + opt := &godo.ListOptions{} + + loadbalancers, _, err := lbService.List(context.TODO(), opt) + + if err != nil { + return nil, err + } + + for _, loadbalancer := range loadbalancers { + if loadbalancer.Name == fi.StringValue(lb.Name) { + return &LoadBalancer{ + Name: fi.String(loadbalancer.Name), + ID: fi.String(loadbalancer.ID), + Lifecycle: lb.Lifecycle, + Region: fi.String(loadbalancer.Region.Slug), + }, nil + } + } + + // Loadbalancer = nil if not found + return nil, nil +} + +func (lb *LoadBalancer) Run(c *fi.Context) error { + return fi.DefaultDeltaRunMethod(lb, c) +} + +func (_ *LoadBalancer) CheckChanges(a, e, changes *LoadBalancer) error { + if a != nil { + if changes.Name != nil { + return fi.CannotChangeField("Name") + } + if changes.ID != nil { + return fi.CannotChangeField("ID") + } + if changes.Region != nil { + return fi.CannotChangeField("Region") + } + } else { + if e.Name == nil { + return fi.RequiredField("Name") + } + if e.Region == nil { + return fi.RequiredField("Region") + } + } + return nil +} + +func (_ *LoadBalancer) RenderDO(t *do.DOAPITarget, a, e, changes *LoadBalancer) error { + + Rules := []godo.ForwardingRule{ + { + EntryProtocol: "https", + EntryPort: 443, + TargetProtocol: "https", + TargetPort: 443, + TlsPassthrough: true, + }, + { + EntryProtocol: "http", + EntryPort: 80, + TargetProtocol: "http", + TargetPort: 80, + }, + } + + HealthCheck := &godo.HealthCheck{ + Protocol: "tcp", + Port: 443, + Path: "", + CheckIntervalSeconds: 60, + ResponseTimeoutSeconds: 5, + UnhealthyThreshold: 3, + HealthyThreshold: 5, + } + + klog.V(10).Infof("Creating load balancer for DO") + + loadBalancerService := t.Cloud.LoadBalancers() + loadbalancer, _, err := loadBalancerService.Create(context.TODO(), &godo.LoadBalancerRequest{ + Name: fi.StringValue(e.Name), + Region: fi.StringValue(e.Region), + Tag: fi.StringValue(e.DropletTag), + ForwardingRules: Rules, + HealthCheck: HealthCheck, + }) + + if err != nil { + klog.Errorf("Error creating load balancer with Name=%s, Error=%v", fi.StringValue(e.Name), err) + return err + } + + e.ID = fi.String(loadbalancer.ID) + e.IPAddress = fi.String(loadbalancer.IP) // This will be empty on create, but will be filled later on FindIPAddress invokation. + + return nil +} + +func (lb *LoadBalancer) FindIPAddress(c *fi.Context) (*string, error) { + cloud := c.Cloud.(*digitalocean.Cloud) + loadBalancerService := cloud.LoadBalancers() + + klog.V(10).Infof("Find IP address for load balancer ID=%s", fi.StringValue(lb.ID)) + loadBalancer, _, err := loadBalancerService.Get(context.TODO(), fi.StringValue(lb.ID)) + if err != nil { + klog.Errorf("Error fetching load balancer with Name=%s", fi.StringValue(lb.Name)) + return nil, err + } + + address := loadBalancer.IP + + if len(address) > 0 && is_ipv4(address) { + klog.V(10).Infof("load balancer address=%s", address) + return &address, nil + } + + klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name)) + time.Sleep(10 * 3) + + klog.V(10).Infof("Sleeping for 30 seconds") + + return nil, errors.New("IP Address is still empty.") +} + +func is_ipv4(host string) bool { + parts := strings.Split(host, ".") + + if len(parts) < 4 { + return false + } + + for _, x := range parts { + if i, err := strconv.Atoi(x); err == nil { + if i < 0 || i > 255 { + return false + } + } else { + return false + } + + } + return true +} + +// terraformVolume represents the digitalocean_volume resource in terraform +// Todo: later. diff --git a/upup/pkg/fi/cloudup/dotasks/loadbalancer_fitask.go b/upup/pkg/fi/cloudup/dotasks/loadbalancer_fitask.go new file mode 100644 index 0000000000000..f1e0ce7b4bcfd --- /dev/null +++ b/upup/pkg/fi/cloudup/dotasks/loadbalancer_fitask.go @@ -0,0 +1,75 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Code generated by ""fitask" -type=LoadBalancer"; DO NOT EDIT + +package dotasks + +import ( + "encoding/json" + + "k8s.io/kops/upup/pkg/fi" +) + +// LoadBalancer + +// JSON marshaling boilerplate +type realLoadBalancer LoadBalancer + +// UnmarshalJSON implements conversion to JSON, supporting an alternate specification of the object as a string +func (o *LoadBalancer) UnmarshalJSON(data []byte) error { + var jsonName string + if err := json.Unmarshal(data, &jsonName); err == nil { + o.Name = &jsonName + return nil + } + + var r realLoadBalancer + if err := json.Unmarshal(data, &r); err != nil { + return err + } + *o = LoadBalancer(r) + return nil +} + +var _ fi.HasLifecycle = &LoadBalancer{} + +// GetLifecycle returns the Lifecycle of the object, implementing fi.HasLifecycle +func (o *LoadBalancer) GetLifecycle() *fi.Lifecycle { + return o.Lifecycle +} + +// SetLifecycle sets the Lifecycle of the object, implementing fi.SetLifecycle +func (o *LoadBalancer) SetLifecycle(lifecycle fi.Lifecycle) { + o.Lifecycle = &lifecycle +} + +var _ fi.HasName = &LoadBalancer{} + +// GetName returns the Name of the object, implementing fi.HasName +func (o *LoadBalancer) GetName() *string { + return o.Name +} + +// SetName sets the Name of the object, implementing fi.SetName +func (o *LoadBalancer) SetName(name string) { + o.Name = &name +} + +// String is the stringer function for the task, producing readable output using fi.TaskAsString +func (o *LoadBalancer) String() string { + return fi.TaskAsString(o) +} From d48bb7aa0bd800dce35b75ce06accd1a1551f66d Mon Sep 17 00:00:00 2001 From: Srikanth Date: Fri, 10 Jan 2020 19:39:03 +0530 Subject: [PATCH 2/6] Fix few review comments --- pkg/model/domodel/api_loadbalancer.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/model/domodel/api_loadbalancer.go b/pkg/model/domodel/api_loadbalancer.go index 851a1f09afcbe..b9447c4ee02da 100644 --- a/pkg/model/domodel/api_loadbalancer.go +++ b/pkg/model/domodel/api_loadbalancer.go @@ -45,7 +45,7 @@ func (b *APILoadBalancerModelBuilder) Build(c *fi.ModelBuilderContext) error { lbSpec := b.Cluster.Spec.API.LoadBalancer if lbSpec == nil { - // Skipping API ELB creation; not requested in Spec + // Skipping API LB creation; not requested in Spec return nil } @@ -58,8 +58,9 @@ func (b *APILoadBalancerModelBuilder) Build(c *fi.ModelBuilderContext) error { return fmt.Errorf("unhandled LoadBalancer type %q", lbSpec.Type) } - loadbalancerName := "api-" + strings.Replace(b.ClusterName(), ".", "-", -1) - clusterMasterTag := do.TagKubernetesClusterMasterPrefix + ":" + strings.Replace(b.ClusterName(), ".", "-", -1) + clusterName := strings.Replace(b.ClusterName(), ".", "-", -1) + loadbalancerName := "api-" + clusterName + clusterMasterTag := do.TagKubernetesClusterMasterPrefix + ":" + clusterName // Create LoadBalancer for API ELB var loadbalancer *dotasks.LoadBalancer From ddf2a90dde0c8e30ab445a9a117a6821d2eae7ea Mon Sep 17 00:00:00 2001 From: Srikanth Date: Sat, 11 Jan 2020 11:02:49 +0530 Subject: [PATCH 3/6] Update more review comments --- pkg/model/domodel/api_loadbalancer.go | 19 +++++++------------ pkg/resources/digitalocean/cloud.go | 10 ++++------ 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/pkg/model/domodel/api_loadbalancer.go b/pkg/model/domodel/api_loadbalancer.go index b9447c4ee02da..6cdd604ac225f 100644 --- a/pkg/model/domodel/api_loadbalancer.go +++ b/pkg/model/domodel/api_loadbalancer.go @@ -62,19 +62,14 @@ func (b *APILoadBalancerModelBuilder) Build(c *fi.ModelBuilderContext) error { loadbalancerName := "api-" + clusterName clusterMasterTag := do.TagKubernetesClusterMasterPrefix + ":" + clusterName - // Create LoadBalancer for API ELB - var loadbalancer *dotasks.LoadBalancer - { - - loadbalancer = &dotasks.LoadBalancer{ - Name: fi.String(loadbalancerName), - Region: fi.String(b.Cluster.Spec.Subnets[0].Region), - DropletTag: fi.String(clusterMasterTag), - Lifecycle: b.Lifecycle, - } - - c.AddTask(loadbalancer) + // Create LoadBalancer for API LB + loadbalancer := &dotasks.LoadBalancer{ + Name: fi.String(loadbalancerName), + Region: fi.String(b.Cluster.Spec.Subnets[0].Region), + DropletTag: fi.String(clusterMasterTag), + Lifecycle: b.Lifecycle, } + c.AddTask(loadbalancer) // Temporarily do not know the role of the following function if dns.IsGossipHostname(b.Cluster.Name) || b.UsePrivateDNS() { diff --git a/pkg/resources/digitalocean/cloud.go b/pkg/resources/digitalocean/cloud.go index 0c72e81df48cf..a59833681c8dc 100644 --- a/pkg/resources/digitalocean/cloud.go +++ b/pkg/resources/digitalocean/cloud.go @@ -144,22 +144,20 @@ func (c *Cloud) GetApiIngressStatus(cluster *kops.Cluster) ([]kops.ApiIngressSta // Note that this must match Digital Ocean's lb name klog.V(2).Infof("Querying DO to find Loadbalancers for API (%q)", cluster.Name) - loadBalaners, _, err := c.LoadBalancers().List(context.TODO(), nil) + loadBalancers, _, err := c.LoadBalancers().List(context.TODO(), nil) if err != nil { - klog.Errorf("LoadBalancers.List returned error: %v", err) - return nil, err + return nil, fmt.Errorf("LoadBalancers.List returned error: %v", err) } lbName := "api-" + strings.Replace(cluster.Name, ".", "-", -1) - for _, lb := range loadBalaners { + for _, lb := range loadBalancers { if lb.Name == lbName { klog.V(10).Infof("Matching LB name found for API (%q)", cluster.Name) if lb.Status != "active" { - klog.Errorf("load-balancer is not yet active (current status: %s)", lb.Status) - return nil, nil + return nil, fmt.Errorf("load-balancer is not yet active (current status: %s)", lb.Status) } address := lb.IP From 38513b3a91a0d89401027a919256ef24a5cf9e52 Mon Sep 17 00:00:00 2001 From: Srikanth Date: Sat, 11 Jan 2020 23:38:07 +0530 Subject: [PATCH 4/6] Address all review comments --- pkg/model/domodel/api_loadbalancer.go | 2 +- pkg/resources/digitalocean/resources.go | 2 +- upup/pkg/fi/cloudup/dotasks/loadbalancer.go | 41 ++++++++------------- 3 files changed, 17 insertions(+), 28 deletions(-) diff --git a/pkg/model/domodel/api_loadbalancer.go b/pkg/model/domodel/api_loadbalancer.go index 6cdd604ac225f..58f578bd33462 100644 --- a/pkg/model/domodel/api_loadbalancer.go +++ b/pkg/model/domodel/api_loadbalancer.go @@ -73,7 +73,7 @@ func (b *APILoadBalancerModelBuilder) Build(c *fi.ModelBuilderContext) error { // Temporarily do not know the role of the following function if dns.IsGossipHostname(b.Cluster.Name) || b.UsePrivateDNS() { - // Ensure the ELB hostname is included in the TLS certificate, + // Ensure the LB hostname is included in the TLS certificate, // if we're not going to use an alias for it // TODO: I don't love this technique for finding the task by name & modifying it masterKeypairTask, found := c.Tasks["Keypair/master"] diff --git a/pkg/resources/digitalocean/resources.go b/pkg/resources/digitalocean/resources.go index 47c112bc87d4b..3954c0032f5be 100644 --- a/pkg/resources/digitalocean/resources.go +++ b/pkg/resources/digitalocean/resources.go @@ -384,7 +384,7 @@ func deleteLoadBalancer(cloud fi.Cloud, t *resources.Resource) error { _, err := c.Client.LoadBalancers.Delete(context.TODO(), lb.ID) if err != nil { - return fmt.Errorf("failed to delete load balancer with name %s", lb.Name) + return fmt.Errorf("failed to delete load balancer with name %s %v", lb.Name, err) } return nil diff --git a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go index 47480133e041e..43124ca97af00 100644 --- a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go +++ b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go @@ -19,8 +19,8 @@ package dotasks import ( "context" "errors" - "strconv" - "strings" + "fmt" + "net" "time" "github.com/digitalocean/godo" @@ -52,15 +52,14 @@ func (lb *LoadBalancer) Find(c *fi.Context) (*LoadBalancer, error) { cloud := c.Cloud.(*digitalocean.Cloud) lbService := cloud.LoadBalancers() - opt := &godo.ListOptions{} + if fi.StringValue(lb.ID) != "" { + loadbalancer, _, err := lbService.Get(context.TODO(), fi.StringValue(lb.ID)) - loadbalancers, _, err := lbService.List(context.TODO(), opt) - - if err != nil { - return nil, err - } + if err != nil { + return nil, fmt.Errorf("load balancer service get request returned error %v", err) + } - for _, loadbalancer := range loadbalancers { + // do another check to double-sure we are talking to the right load balancer. if loadbalancer.Name == fi.StringValue(lb.Name) { return &LoadBalancer{ Name: fi.String(loadbalancer.Name), @@ -164,37 +163,27 @@ func (lb *LoadBalancer) FindIPAddress(c *fi.Context) (*string, error) { address := loadBalancer.IP - if len(address) > 0 && is_ipv4(address) { + if isIPv4(address) { klog.V(10).Infof("load balancer address=%s", address) return &address, nil } klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name)) - time.Sleep(10 * 3) + time.Sleep(10 * time.Second) - klog.V(10).Infof("Sleeping for 30 seconds") + klog.V(10).Infof("Sleeping for 10 seconds") return nil, errors.New("IP Address is still empty.") } -func is_ipv4(host string) bool { - parts := strings.Split(host, ".") +func isIPv4(host string) bool { - if len(parts) < 4 { + ip := net.ParseIP(host) + if ip == nil { return false } - for _, x := range parts { - if i, err := strconv.Atoi(x); err == nil { - if i < 0 || i > 255 { - return false - } - } else { - return false - } - - } - return true + return ip.To4() != nil } // terraformVolume represents the digitalocean_volume resource in terraform From e440397548b74a1c34f8e8bb92d816dd519e7b8d Mon Sep 17 00:00:00 2001 From: Srikanth Date: Tue, 28 Jan 2020 14:22:49 +0530 Subject: [PATCH 5/6] Incorporate review comments --- pkg/resources/digitalocean/cloud.go | 3 +-- upup/pkg/fi/cloudup/dotasks/loadbalancer.go | 14 ++++++-------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/pkg/resources/digitalocean/cloud.go b/pkg/resources/digitalocean/cloud.go index a59833681c8dc..f6e02242c8a89 100644 --- a/pkg/resources/digitalocean/cloud.go +++ b/pkg/resources/digitalocean/cloud.go @@ -144,8 +144,7 @@ func (c *Cloud) GetApiIngressStatus(cluster *kops.Cluster) ([]kops.ApiIngressSta // Note that this must match Digital Ocean's lb name klog.V(2).Infof("Querying DO to find Loadbalancers for API (%q)", cluster.Name) - loadBalancers, _, err := c.LoadBalancers().List(context.TODO(), nil) - + loadBalancers, err := getAllLoadBalancers(c) if err != nil { return nil, fmt.Errorf("LoadBalancers.List returned error: %v", err) } diff --git a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go index 43124ca97af00..80d34937b630c 100644 --- a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go +++ b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go @@ -60,14 +60,12 @@ func (lb *LoadBalancer) Find(c *fi.Context) (*LoadBalancer, error) { } // do another check to double-sure we are talking to the right load balancer. - if loadbalancer.Name == fi.StringValue(lb.Name) { - return &LoadBalancer{ - Name: fi.String(loadbalancer.Name), - ID: fi.String(loadbalancer.ID), - Lifecycle: lb.Lifecycle, - Region: fi.String(loadbalancer.Region.Slug), - }, nil - } + return &LoadBalancer{ + Name: fi.String(loadbalancer.Name), + ID: fi.String(loadbalancer.ID), + Lifecycle: lb.Lifecycle, + Region: fi.String(loadbalancer.Region.Slug), + }, nil } // Loadbalancer = nil if not found From d8a9470aa4531d834bb89c3dce0907a453990cfb Mon Sep 17 00:00:00 2001 From: Srikanth Date: Sun, 2 Feb 2020 20:22:17 +0530 Subject: [PATCH 6/6] Incorporate further review comments --- upup/pkg/fi/cloudup/dotasks/loadbalancer.go | 40 +++++++++------------ 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go index 80d34937b630c..e534fbc59ef14 100644 --- a/upup/pkg/fi/cloudup/dotasks/loadbalancer.go +++ b/upup/pkg/fi/cloudup/dotasks/loadbalancer.go @@ -49,27 +49,25 @@ func (lb *LoadBalancer) CompareWithID() *string { } func (lb *LoadBalancer) Find(c *fi.Context) (*LoadBalancer, error) { + if fi.StringValue(lb.ID) == "" { + // Loadbalancer = nil if not found + return nil, nil + } + cloud := c.Cloud.(*digitalocean.Cloud) lbService := cloud.LoadBalancers() + loadbalancer, _, err := lbService.Get(context.TODO(), fi.StringValue(lb.ID)) - if fi.StringValue(lb.ID) != "" { - loadbalancer, _, err := lbService.Get(context.TODO(), fi.StringValue(lb.ID)) - - if err != nil { - return nil, fmt.Errorf("load balancer service get request returned error %v", err) - } - - // do another check to double-sure we are talking to the right load balancer. - return &LoadBalancer{ - Name: fi.String(loadbalancer.Name), - ID: fi.String(loadbalancer.ID), - Lifecycle: lb.Lifecycle, - Region: fi.String(loadbalancer.Region.Slug), - }, nil + if err != nil { + return nil, fmt.Errorf("load balancer service get request returned error %v", err) } - // Loadbalancer = nil if not found - return nil, nil + return &LoadBalancer{ + Name: fi.String(loadbalancer.Name), + ID: fi.String(loadbalancer.ID), + Lifecycle: lb.Lifecycle, + Region: fi.String(loadbalancer.Region.Slug), + }, nil } func (lb *LoadBalancer) Run(c *fi.Context) error { @@ -166,10 +164,9 @@ func (lb *LoadBalancer) FindIPAddress(c *fi.Context) (*string, error) { return &address, nil } - klog.Errorf("Error fetching IP Address for lb Name=%s", fi.StringValue(lb.Name)) - time.Sleep(10 * time.Second) - - klog.V(10).Infof("Sleeping for 10 seconds") + const lbWaitTime = 10 * time.Second + klog.Warningf("IP address for LB %s not yet available -- sleeping %s", fi.StringValue(lb.Name), lbWaitTime) + time.Sleep(lbWaitTime) return nil, errors.New("IP Address is still empty.") } @@ -183,6 +180,3 @@ func isIPv4(host string) bool { return ip.To4() != nil } - -// terraformVolume represents the digitalocean_volume resource in terraform -// Todo: later.