Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

fix labels and taints with spaced value for packet, aws, tinkerbell #1291

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix labels and taints with spaced value for packet, aws, tinkerbell #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b476cf
Packet: fix labels and taints with spaced value
knrt10 Dec 28, 2020
a07ebec
aws: fix labels and taints with spaced value
knrt10 Dec 28, 2020
e9fc218
tinkerbell: fix labels && taints with spaced value
knrt10 Dec 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions pkg/platform/aws/aws.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"os"
"path/filepath"
"strings"
"text/template"

"github.com/hashicorp/hcl/v2"
Expand Down Expand Up @@ -261,6 +262,7 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
diagnostics = append(diagnostics, c.checkWorkerPoolNamesUnique()...)
diagnostics = append(diagnostics, c.checkNameSizes()...)
diagnostics = append(diagnostics, c.checkLBPortsUnique()...)
diagnostics = append(diagnostics, c.checkWorkerPoolLabelsAndTaints()...)

if c.ConntrackMaxPerCore < 0 {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Expand Down Expand Up @@ -324,6 +326,37 @@ func (c *config) checkLBPortsUnique() hcl.Diagnostics {
return diagnostics
}

// checkWorkerPoolLabelsAndTaints verifies that all worker pool labels
// and taints are in correct format. Neither key nor value of labels
// and taints map should have empty space in them.
func (c *config) checkWorkerPoolLabelsAndTaints() hcl.Diagnostics {
var diagnostics hcl.Diagnostics

for _, w := range c.WorkerPools {
for k, v := range w.Labels {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools labels map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q label with key %q is incorrect", w.Name, k),
})
}
}

for k, v := range w.Taints {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools taints map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q taints with key %q is incorrect", w.Name, k),
})
}
}
}

return diagnostics
}

// checkNameSizes checks the size of names since AWS has a limit of 32
// characters on resources.
func (c *config) checkNameSizes() hcl.Diagnostics {
Expand Down
28 changes: 28 additions & 0 deletions pkg/platform/aws/aws_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,3 +222,31 @@ func TestConfigurationIsValidWhen(t *testing.T) {
})
}
}

func TestCheckWorkerPoolLabelsWithSpacedValue(t *testing.T) {
c := config{
WorkerPools: []workerPool{
{
Labels: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.checkWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool labels")
}
}

func TestCheckWorkerPoolTaintsWithSpacedValue(t *testing.T) {
c := config{
WorkerPools: []workerPool{
{
Taints: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.checkWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool taints")
}
}
32 changes: 32 additions & 0 deletions pkg/platform/packet/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ func (c *config) checkValidConfig() hcl.Diagnostics {
diagnostics = append(diagnostics, c.checkWorkerPoolNamesUnique()...)
diagnostics = append(diagnostics, c.checkReservationIDs()...)
diagnostics = append(diagnostics, c.validateOSVersion()...)
diagnostics = append(diagnostics, c.checkWorkerPoolLabelsAndTaints()...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to improve commit messages:

  • We do not fix anything here, but we add new validation rule, so commit message should reflect that.
  • Your commit message does not explain why user cannot have whitespace characters in labels or taints.
  • Your commit message only mentiones spaces, while we care about all whitespace characters.
  • Commit message should briefly explain what it adds.
  • Commit message should include issue reference to make it easier to find context for it, otherwise one needs
    to look for associated merge commit later.
-Packet: fix labels and taints with spaced value
+packet: add labels and taints validation against whitespace

-Now user cannot have spaced key or value in labels, taints map for
-worker pools.
-Add tests.
+Currently if user adds whitespace characters to labels or taints, it breaks the cluster
+provisioning, as labels and taints are not allowed to contain whitespace.

+To fail early on such cases, this commit adds validation rules, so error is returned to
+ the user before cluster provisioning starts.

+Refs #X


if c.ConntrackMaxPerCore < 0 {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Expand Down Expand Up @@ -525,6 +526,37 @@ func (c *config) validateOSVersion() hcl.Diagnostics {
return diagnostics
}

// checkWorkerPoolLabelsAndTaints verifies that all worker pool labels
// and taints are in correct format. Neither key nor value of labels
// and taints map should have empty space in them.
func (c *config) checkWorkerPoolLabelsAndTaints() hcl.Diagnostics {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extend platform.WorkerPool interface and add generic validation for it so we don't duplicate code for each platform.

var diagnostics hcl.Diagnostics

for _, w := range c.WorkerPools {
for k, v := range w.Labels {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if I put tab character in key or value? I think we should also validate that.

diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools labels map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q label with key %q is incorrect", w.Name, k),
})
}
}

for k, v := range w.Taints {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools taints map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q taints with key %q is incorrect", w.Name, k),
})
}
}
}

return diagnostics
}

// checkEachReservation checks that hardware reservations are in the correct
// format and, when it will cause problems, that reservation IDs values in this
// pool are not mixed between using "next-available" and specific UUIDs, as this
Expand Down
28 changes: 28 additions & 0 deletions pkg/platform/packet/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,3 +355,31 @@ func TestTerraformAddDeps(t *testing.T) {
})
}
}

func TestCheckWorkerPoolLabelsWithSpacedValue(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have TestCheckValidConfig which starts with valid configuration and lets you mutate it to introduce a bad config option. I think we should use this, as eventually it will allow only testing using exported interface.

c := config{
WorkerPools: []workerPool{
{
Labels: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.checkWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool labels")
}
}

func TestCheckWorkerPoolTaintsWithSpacedValue(t *testing.T) {
c := config{
WorkerPools: []workerPool{
{
Taints: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.checkWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool taints")
}
}
32 changes: 32 additions & 0 deletions pkg/platform/tinkerbell/tinkerbell.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ func (c *Config) Validate() hcl.Diagnostics {

d = append(d, platform.WorkerPoolNamesUnique(x)...)
d = append(d, c.validateRequiredFields()...)
d = append(d, c.CheckWorkerPoolLabelsAndTaints()...)

return d
}
Expand Down Expand Up @@ -288,3 +289,34 @@ func (c *Config) validateRequiredFields() hcl.Diagnostics {

return d
}

// CheckWorkerPoolLabelsAndTaints verifies that all worker pool labels
// and taints are in correct format. Neither key nor value of labels
// and taints map should have empty space in them.
func (c *Config) CheckWorkerPoolLabelsAndTaints() hcl.Diagnostics {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should not be exported so we do not pollute the exported API. Config struct already exports generic Validate().

var diagnostics hcl.Diagnostics

for _, w := range c.WorkerPools {
for k, v := range w.Labels {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools labels map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q label with key %q is incorrect", w.PoolName, k),
})
}
}

for k, v := range w.Taints {
if strings.Contains(k, " ") || strings.Contains(v, " ") {
diagnostics = append(diagnostics, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Worker pools taints map should not contain empty spaces",
Detail: fmt.Sprintf("Worker pool %q taints with key %q is incorrect", w.PoolName, k),
})
}
}
}

return diagnostics
}
28 changes: 28 additions & 0 deletions pkg/platform/tinkerbell/tinkerbell_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,3 +244,31 @@ func TestMeta(t *testing.T) {
t.Errorf("Expected %d nodes, got %d", expectedNodes, m.ExpectedNodes)
}
}

func TestCheckWorkerPoolLabelsWithSpacedValue(t *testing.T) {
c := &tinkerbell.Config{
WorkerPools: []tinkerbell.WorkerPool{
{
Labels: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.CheckWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool labels")
}
}

func TestCheckWorkerPoolTaintsWithSpacedValue(t *testing.T) {
c := &tinkerbell.Config{
WorkerPools: []tinkerbell.WorkerPool{
{
Taints: map[string]string{"foo-1": "bar "},
},
},
}

if d := c.CheckWorkerPoolLabelsAndTaints(); !d.HasErrors() {
t.Error("Should fail with space in worker pool taints")
}
}