Repository: kinvolk/lokomotive
Pull request #889 (2020-09-01 14:49:51)

aws: Add check for multiple worker pools
Add validation check for multiple worker pools, to avoid conflicting
port values in aws_lb_listener. User has to set unique values for
lb_http_port and lb_https_port.

Part of #839

Signed-off-by: knrt10 <kautilya@kinvolk.io>

knrt10: Updated

Patch:
diff --git a/cli/cmd/cluster-apply.go b/cli/cmd/cluster-apply.go
index c69f6fcb..785f086b 100644
--- a/cli/cmd/cluster-apply.go
+++ b/cli/cmd/cluster-apply.go
@@ -121,12 +121,6 @@ func runClusterApply(cmd *cobra.Command, args []string) {
 		}
 	}
 
-	if ph, ok := p.(platform.PlatformWithPostApplyHook); ok {
-		if err := ph.PostApplyHook(kubeconfig); err != nil {
-			ctxLogger.Fatalf("Running platform post install hook failed: %v", err)
-		}
-	}
-
 	if skipComponents {
 		return
 	}
diff --git a/pkg/platform/aks/aks.go b/pkg/platform/aks/aks.go
index 754104ed..240ce7b5 100644
--- a/pkg/platform/aks/aks.go
+++ b/pkg/platform/aks/aks.go
@@ -17,21 +17,15 @@
 package aks
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"path/filepath"
 	"text/template"
-	"time"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/gohcl"
 	"github.com/mitchellh/go-homedir"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
-	v1 "k8s.io/client-go/kubernetes/typed/storage/v1"
 
-	"github.com/kinvolk/lokomotive/pkg/k8sutil"
 	"github.com/kinvolk/lokomotive/pkg/platform"
 	"github.com/kinvolk/lokomotive/pkg/terraform"
 )
@@ -315,48 +309,6 @@ func (c *config) Initialize(ex *terraform.Executor) error {
 	return createTerraformConfigFile(c, terraformRootDir)
 }
 
-const (
-	retryInterval = 5 * time.Second
-	timeout       = 10 * time.Minute
-)
-
-// PostApplyHook implements platform.PlatformWithPostApplyHook interface and defines hooks
-// which should be executed after AKS cluster is created.
-func (c *config) PostApplyHook(kubeconfig []byte) error {
-	client, err := k8sutil.NewClientset(kubeconfig)
-	if err != nil {
-		return fmt.Errorf("creating clientset from kubeconfig: %w", err)
-	}
-
-	return waitForDefaultStorageClass(client.StorageV1().StorageClasses())
-}
-
-// waitForDefaultStorageClass waits until the default storage class appears on a given cluster.
-// If it doesn't appear within a defined time range, an error is returned.
-func waitForDefaultStorageClass(sci v1.StorageClassInterface) error {
-	// AKS still uses annotation with .beta.
-	defaultStorageClassAnnotation := "storageclass.beta.kubernetes.io/is-default-class"
-
-	if err := wait.PollImmediate(retryInterval, timeout, func() (done bool, err error) {
-		scs, err := sci.List(context.TODO(), metav1.ListOptions{})
-		if err != nil {
-			return false, fmt.Errorf("getting storage classes: %w", err)
-		}
-
-		for _, sc := range scs.Items {
-			if v, ok := sc.ObjectMeta.Annotations[defaultStorageClassAnnotation]; ok && v == "true" {
-				return true, nil
-			}
-		}
-
-		return false, nil
-	}); err != nil {
-		return fmt.Errorf("waiting for the default storage class to be configured: %w", err)
-	}
-
-	return nil
-}
-
 // createTerraformConfigFiles create Terraform config files in given directory.
 func createTerraformConfigFile(cfg *config, terraformRootDir string) error {
 	t := template.Must(template.New("t").Parse(terraformConfigTmpl))
diff --git a/pkg/platform/aws/aws.go b/pkg/platform/aws/aws.go
index 7a5f6e8f..0ff7a554 100644
--- a/pkg/platform/aws/aws.go
+++ b/pkg/platform/aws/aws.go
@@ -278,6 +278,7 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
 	diagnostics = append(diagnostics, c.checkNotEmptyWorkers()...)
 	diagnostics = append(diagnostics, c.checkWorkerPoolNamesUnique()...)
 	diagnostics = append(diagnostics, c.checkNameSizes()...)
+	diagnostics = append(diagnostics, c.checkLBPortsUnique()...)
 
 	if c.OIDC != nil {
 		_, diags := c.OIDC.ToKubeAPIServerFlags(c.clusterDomain())
@@ -287,6 +288,52 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
 	return diagnostics
 }
 
+// checkLBPortsUnique checks that the lb_http_port and lb_https_port
+// flags have different values if the user is using multiple worker
+// pools.
+func (c *config) checkLBPortsUnique() hcl.Diagnostics {
+	var diagnostics hcl.Diagnostics
+
+	portMap := make(map[int]struct {
+		field    string
+		poolName string
+	})
+
+	for _, wp := range c.WorkerPools {
+		httpPort := wp.LBHTTPPort
+		if httpPort == 0 {
+			httpPort = 80
+		}
+
+		httpsPort := wp.LBHTTPSPort
+		if httpsPort == 0 {
+			httpsPort = 443
+		}
+
+		ports := map[int]string{
+			httpPort:  "lb_http_port",
+			httpsPort: "lb_https_port",
+		}
+
+		for p, field := range ports {
+			if v, ok := portMap[p]; ok {
+				diagnostics = append(diagnostics, &hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Unique ports required",
+					Detail:   fmt.Sprintf("'worker_pool.%s.%s' collides with 'worker_pool.%s.%s'", wp.Name, field, v.poolName, v.field), //nolint:lll
+				})
+			}
+
+			portMap[p] = struct {
+				field    string
+				poolName string
+			}{field, wp.Name}
+		}
+	}
+
+	return diagnostics
+}
+
 // checkNameSizes checks the size of names since AWS has a limit of 32
 // characters on resources.
 func (c *config) checkNameSizes() hcl.Diagnostics {
diff --git a/pkg/platform/platform.go b/pkg/platform/platform.go
index 6c58369b..dceaad5d 100644
--- a/pkg/platform/platform.go
+++ b/pkg/platform/platform.go
@@ -74,13 +74,6 @@ type Platform interface {
 	Meta() Meta
 }
 
-// PlatformWithPostApplyHook runs code after Terraform finishes applying. This allows
-// running sanity checks on the newly created cluster. Implementing this
-// interface is optional for platforms.
-type PlatformWithPostApplyHook interface { // nolint:golint
-	PostApplyHook(kubeconfig []byte) error
-}
-
 // Meta is a generic information format about the platform.
 type Meta struct {
 	AssetDir      string


Test patch:
diff --git a/pkg/platform/aws/aws_internal_test.go b/pkg/platform/aws/aws_internal_test.go
new file mode 100644
index 00000000..df34f886
--- /dev/null
+++ b/pkg/platform/aws/aws_internal_test.go
@@ -0,0 +1,169 @@
+// Copyright 2020 The Lokomotive 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 aws
+
+import (
+	"testing"
+
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/hcl/v2/gohcl"
+	"github.com/hashicorp/hcl/v2/hclparse"
+
+	lokoconfig "github.com/kinvolk/lokomotive/pkg/config"
+)
+
+// loadConfigFromString loads config from string.
+func loadConfigFromString(t *testing.T, c string) hcl.Diagnostics {
+	p := hclparse.NewParser()
+
+	f, d := p.ParseHCL([]byte(c), "x.lokocfg")
+	if d.HasErrors() {
+		t.Fatalf("parsing HCL should succeed, got: %v", d)
+	}
+
+	configBody := hcl.MergeFiles([]*hcl.File{f})
+
+	var rootConfig lokoconfig.RootConfig
+
+	if d := gohcl.DecodeBody(configBody, nil, &rootConfig); d.HasErrors() {
+		t.Fatalf("decoding root config should succeed, got: %v", d)
+	}
+
+	cc := &config{}
+
+	return cc.LoadConfig(&rootConfig.Cluster.Config, &hcl.EvalContext{})
+}
+
+func TestLoadConfig(t *testing.T) {
+	c := `
+cluster "aws" {
+  asset_dir           = "/fooo"
+  cluster_name        = "mycluster"
+  dns_zone            = "testzone"
+  dns_zone_id         = "testzoneID"
+  ssh_pubkeys         = ["testkey"]
+  worker_pool "foo" {
+    count   = 1
+    ssh_pubkeys = ["testkey"]
+  }
+}
+`
+
+	if d := loadConfigFromString(t, c); d.HasErrors() {
+		t.Fatalf("valid config should not return error, got: %v", d)
+	}
+}
+
+//nolint: funlen
+func TestWorkerPoolPort(t *testing.T) {
+	type testCase struct {
+		// Config to test.
+		cfg config
+		// Expected output after running test.
+		hasError bool
+	}
+
+	cases := []testCase{
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name: "pool-1",
+					},
+				},
+			},
+			hasError: false,
+		},
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name:        "pool-1",
+						LBHTTPPort:  80,
+						LBHTTPSPort: 443,
+					},
+				},
+			},
+			hasError: false,
+		},
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name: "pool-1",
+					},
+					{
+						Name: "pool-2",
+					},
+				},
+			},
+			hasError: true,
+		},
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name: "pool-1",
+					},
+					{
+						Name:        "pool-2",
+						LBHTTPPort:  80,
+						LBHTTPSPort: 443,
+					},
+				},
+			},
+			hasError: true,
+		},
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name:        "pool-1",
+						LBHTTPPort:  8080,
+						LBHTTPSPort: 8443,
+					},
+					{
+						Name:        "pool-2",
+						LBHTTPPort:  8080,
+						LBHTTPSPort: 8443,
+					},
+				},
+			},
+			hasError: true,
+		},
+		{
+			cfg: config{
+				WorkerPools: []workerPool{
+					{
+						Name: "pool-1",
+					},
+					{
+						Name:        "pool-2",
+						LBHTTPPort:  8080,
+						LBHTTPSPort: 8443,
+					},
+				},
+			},
+			hasError: false,
+		},
+	}
+
+	for tcIdx, tc := range cases {
+		output := tc.cfg.checkLBPortsUnique()
+		if output.HasErrors() != tc.hasError {
+			t.Errorf("In test %v, expected %v, got %v", tcIdx+1, tc.hasError, output.HasErrors())
+		}
+	}
+}