From b4f67ea54e23e4a8221db088f2e036ddb803d636 Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Wed, 10 Feb 2021 17:06:57 +0530 Subject: [PATCH 1/6] typo: Toletration to Toleration Signed-off-by: Imran Pochi --- pkg/components/velero/component_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/components/velero/component_test.go b/pkg/components/velero/component_test.go index a928da68c..3f7702026 100644 --- a/pkg/components/velero/component_test.go +++ b/pkg/components/velero/component_test.go @@ -210,8 +210,8 @@ component "velero" { } tolerations { - key = "TestResticToletrationKey" - value = "TestResticToletrationValue" + key = "TestResticTolerationKey" + value = "TestResticTolerationValue" operator = "Equal" effect = "NoSchedule" toleration_seconds = "1" @@ -233,7 +233,7 @@ component "velero" { m := testutil.RenderManifests(t, component, Name, configHCL) jsonPath := "{.spec.template.spec.tolerations[0].key}" - expected := "TestResticToletrationKey" + expected := "TestResticTolerationKey" gotConfig := testutil.ConfigFromMap(t, m, "velero/templates/restic-daemonset.yaml") From 4281ca82d06ea0ea169c82dec3f0330463310e35 Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Mon, 8 Feb 2021 18:00:26 +0530 Subject: [PATCH 2/6] velero: update type of Tolerations.TolerationSeconds This commit updates the TolerationSeconds from string to int64. Also update the json tag from `toleration_seconds` to `tolerationSeconds`. JSON tag is updated because, Tolerations struct is directly marshalled to JSON and stored as a string TolerationsRaw which is then passed to the helm templates. This causes the Kubernetes API Server to complain during validation that no such field regarding `toleration_seconds` since API server was expected `tolerationSeconds`. Data type is changed to int64, becuase Kubernetes API server expects the TolerationSeconds to be int64 instead of string. Thereby causing validation to fail at the API server end. Updates the unit test to reflect the same. Signed-off-by: Imran Pochi --- pkg/components/internal/testutil/jsonpath.go | 15 +++++++++++++++ pkg/components/util/types.go | 2 +- pkg/components/velero/component_test.go | 10 ++++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/pkg/components/internal/testutil/jsonpath.go b/pkg/components/internal/testutil/jsonpath.go index f72c4eb64..9e3fa2c83 100644 --- a/pkg/components/internal/testutil/jsonpath.go +++ b/pkg/components/internal/testutil/jsonpath.go @@ -89,3 +89,18 @@ func MatchJSONPathStringValue(t *testing.T, yamlConfig string, jsonPath string, t.Fatalf("Expected: %s, Got: %s", expected, got) } } + +// MatchJSONPathInt64Value is a helper function for component unit tests. It compares the integer at +// a JSON path in a YAML config to the expected integer. +func MatchJSONPathInt64Value(t *testing.T, yamlConfig string, jsonPath string, expected int64) { + obj := jsonPathValue(t, yamlConfig, jsonPath) + + got, ok := obj.(int64) + if !ok { + t.Fatalf("Value is not an integer: %#v", obj) + } + + if got != expected { + t.Fatalf("Expected: %d, Got: %d", expected, got) + } +} diff --git a/pkg/components/util/types.go b/pkg/components/util/types.go index bfb201341..971aad738 100644 --- a/pkg/components/util/types.go +++ b/pkg/components/util/types.go @@ -44,7 +44,7 @@ type Toleration struct { Effect string `hcl:"effect,optional" json:"effect,omitempty"` Operator string `hcl:"operator,optional" json:"operator,omitempty"` Value string `hcl:"value,optional" json:"value,omitempty"` - TolerationSeconds string `hcl:"toleration_seconds,optional" json:"toleration_seconds,omitempty"` + TolerationSeconds int64 `hcl:"toleration_seconds,optional" json:"tolerationSeconds,omitempty"` } // RenderTolerations takes a list of tolerations. diff --git a/pkg/components/velero/component_test.go b/pkg/components/velero/component_test.go index 3f7702026..e426978bf 100644 --- a/pkg/components/velero/component_test.go +++ b/pkg/components/velero/component_test.go @@ -208,13 +208,12 @@ component "velero" { bucket = "foo" provider = "aws" } - tolerations { key = "TestResticTolerationKey" value = "TestResticTolerationValue" operator = "Equal" effect = "NoSchedule" - toleration_seconds = "1" + toleration_seconds = 1 } } } @@ -238,4 +237,11 @@ component "velero" { gotConfig := testutil.ConfigFromMap(t, m, "velero/templates/restic-daemonset.yaml") testutil.MatchJSONPathStringValue(t, gotConfig, jsonPath, expected) + + jsonPath = "{.spec.template.spec.tolerations[0].tolerationSeconds}" + expectedNumber := int64(1) + + gotConfig = testutil.ConfigFromMap(t, m, "velero/templates/restic-daemonset.yaml") + + testutil.MatchJSONPathInt64Value(t, gotConfig, jsonPath, expectedNumber) } From 9b8bb5ee1449b7fb4b818fe11b30688f17a58a3d Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Thu, 11 Feb 2021 11:58:47 +0530 Subject: [PATCH 3/6] util: add validation to Tolerations This commit adds validation to the Tolerations type. This is done so because Kubernetes API server expects that if `TolerationSeconds` is set then `Effect` must be set to `NoExecute`. This condition checks for the same and reports and error to the user if this condition is not met. Adds unit test for the same. Signed-off-by: Imran Pochi --- pkg/components/util/types.go | 20 ++++++++++++- pkg/components/util/types_test.go | 47 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 pkg/components/util/types_test.go diff --git a/pkg/components/util/types.go b/pkg/components/util/types.go index 971aad738..0ea5a78f4 100644 --- a/pkg/components/util/types.go +++ b/pkg/components/util/types.go @@ -14,7 +14,10 @@ package util -import "encoding/json" +import ( + "encoding/json" + "fmt" +) // NodeAffinity is a struct that other components can use to define the HCL format of NodeAffinity // in Kubernetes PodSpec. @@ -54,6 +57,12 @@ func RenderTolerations(t []Toleration) (string, error) { return "", nil } + for _, toleration := range t { + if err := validateToleration(toleration); err != nil { + return "", fmt.Errorf("toleration validation failed: %v", err) + } + } + b, err := json.Marshal(t) if err != nil { return "", err @@ -62,6 +71,15 @@ func RenderTolerations(t []Toleration) (string, error) { return string(b), nil } +func validateToleration(t Toleration) error { + // If TolerationSeconds is set then; Effect must be `NoExecute` + if t.TolerationSeconds != 0 && t.Effect != "NoExecute" { + return fmt.Errorf("`effect` must be `NoExecute` as `toleration_seconds` is set: got %s", t.Effect) + } + + return nil +} + // NodeSelector is a type used when defining node selector for the pod spec. type NodeSelector map[string]string diff --git a/pkg/components/util/types_test.go b/pkg/components/util/types_test.go new file mode 100644 index 000000000..1cce161b1 --- /dev/null +++ b/pkg/components/util/types_test.go @@ -0,0 +1,47 @@ +// Copyright 2021 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 util_test + +import ( + "testing" + + "github.com/kinvolk/lokomotive/pkg/components/util" +) + +func TestTolerationValidation(t *testing.T) { + testCases := []struct { + desc string + toleration util.Toleration + wantErr bool + }{ + { + desc: "TolerationSeconds set; Effect must be `NoExecute`", + toleration: util.Toleration{ + Key: "testkey", + Value: "testvalue", + Effect: "NoSchedule", + TolerationSeconds: 3, + }, + wantErr: true, + }, + } + + for _, tc := range testCases { + _, err := util.RenderTolerations(append([]util.Toleration{}, tc.toleration)) + if !tc.wantErr && err != nil { + t.Fatalf("Valid Toleration should not return error, got: %s", err) + } + } +} From d0958401f7459d88e79a8ee74fd7a25388ba73a8 Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Thu, 11 Feb 2021 12:00:24 +0530 Subject: [PATCH 4/6] velero: add check for restic.tolerations This commit adds a check to check if restic.tolerations.toleration_seconds is set then the value of restic.tolerations.effect must be `NoExecute` Signed-off-by: Imran Pochi --- pkg/components/velero/component_test.go | 2 +- pkg/components/velero/restic/restic_test.go | 60 +++++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 pkg/components/velero/restic/restic_test.go diff --git a/pkg/components/velero/component_test.go b/pkg/components/velero/component_test.go index e426978bf..5f9164b63 100644 --- a/pkg/components/velero/component_test.go +++ b/pkg/components/velero/component_test.go @@ -212,7 +212,7 @@ component "velero" { key = "TestResticTolerationKey" value = "TestResticTolerationValue" operator = "Equal" - effect = "NoSchedule" + effect = "NoExecute" toleration_seconds = 1 } } diff --git a/pkg/components/velero/restic/restic_test.go b/pkg/components/velero/restic/restic_test.go new file mode 100644 index 000000000..ecf91d5ef --- /dev/null +++ b/pkg/components/velero/restic/restic_test.go @@ -0,0 +1,60 @@ +// Copyright 2021 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 restic_test + +import ( + "testing" + + "github.com/kinvolk/lokomotive/pkg/components/util" + "github.com/kinvolk/lokomotive/pkg/components/velero/restic" +) + +//nolint:funlen +func TestResticConfig(t *testing.T) { + tests := []struct { + desc string + config *restic.Configuration + wantError bool + }{ + { + desc: "effect should be NoExecute if TolerationSeconds is Set", + wantError: true, + config: &restic.Configuration{ + Credentials: "foo", + BackupStorageLocation: &restic.BackupStorageLocation{ + Bucket: "mybucket", + Provider: "aws", + Region: "myregion", + }, + Tolerations: []util.Toleration{ + { + Key: "key", + Value: "value", + Effect: "NoSchedule", + Operator: "Equal", + TolerationSeconds: 3, + }, + }, + }, + }, + } + + for _, tc := range tests { + diags := tc.config.Validate() + if !tc.wantError && diags.HasErrors() { + t.Fatalf("Valid config should not return error, got: %s", diags.Error()) + } + } +} From ccb6177811ba17a6002a60af888965e02bebd41b Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Mon, 8 Feb 2021 17:55:48 +0530 Subject: [PATCH 5/6] velero: add check for restic.region This commit adds a conditional check for restic.region is to be set if restic.provider is `aws`. This change is done to reflect the addtional configuration of region that is required when provider is `aws`. Without this additional check lokoctl throws an error message that is caught few levels below in the stack in the velero. Catching this error allows a consistent error message reporting to the user. If any other provider is used, then region value is optional. Updates the unit test to reflect the same. Signed-off-by: Imran Pochi --- pkg/components/velero/component_test.go | 2 ++ pkg/components/velero/restic/restic.go | 8 ++++++++ pkg/components/velero/restic/restic_test.go | 11 +++++++++++ 3 files changed, 21 insertions(+) diff --git a/pkg/components/velero/component_test.go b/pkg/components/velero/component_test.go index 5f9164b63..47e8ca85f 100644 --- a/pkg/components/velero/component_test.go +++ b/pkg/components/velero/component_test.go @@ -170,6 +170,7 @@ component "velero" { backup_storage_location { bucket = "foo" provider = "aws" + region = "myregion" } } } @@ -207,6 +208,7 @@ component "velero" { backup_storage_location { bucket = "foo" provider = "aws" + region = "myregion" } tolerations { key = "TestResticTolerationKey" diff --git a/pkg/components/velero/restic/restic.go b/pkg/components/velero/restic/restic.go index 38491d112..b84afdba9 100644 --- a/pkg/components/velero/restic/restic.go +++ b/pkg/components/velero/restic/restic.go @@ -111,6 +111,14 @@ func (c *Configuration) Validate() hcl.Diagnostics { }) } + if c.BackupStorageLocation.Provider == "aws" && c.BackupStorageLocation.Region == "" { + diagnostics = append(diagnostics, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Make sure `restic.backup_storage_location.region` value is set", + Detail: "restic.backup_storage_location.region must be provided for `aws` provider", + }) + } + return diagnostics } diff --git a/pkg/components/velero/restic/restic_test.go b/pkg/components/velero/restic/restic_test.go index ecf91d5ef..6695dbb5f 100644 --- a/pkg/components/velero/restic/restic_test.go +++ b/pkg/components/velero/restic/restic_test.go @@ -49,6 +49,17 @@ func TestResticConfig(t *testing.T) { }, }, }, + { + desc: "region should be present if provider is aws", + wantError: true, + config: &restic.Configuration{ + Credentials: "foo", + BackupStorageLocation: &restic.BackupStorageLocation{ + Bucket: "mybucket", + Provider: "aws", + }, + }, + }, } for _, tc := range tests { From 09bf03969e8dc891cab78fd257345ea114f928db Mon Sep 17 00:00:00 2001 From: Imran Pochi Date: Mon, 8 Feb 2021 18:26:32 +0530 Subject: [PATCH 6/6] docs: fix formatting for Velero component This commit fixes the formatting for the configuration reference documentation of Velero component. Signed-off-by: Imran Pochi --- .../components/velero.md | 102 +++++++++--------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/docs/configuration-reference/components/velero.md b/docs/configuration-reference/components/velero.md index ad6fc27b6..d078dfd0c 100644 --- a/docs/configuration-reference/components/velero.md +++ b/docs/configuration-reference/components/velero.md @@ -31,64 +31,64 @@ Velero component configuration example: # velero.lokocfg component "velero" { - # provider = "azure/openebs/restic" - # azure { - # # Required arguments. - # subscription_id = "9e5ac23c-6df8-44c4-9790-6f6decf96268" - # tenant_id = "78bdc534-b34f-4bda-a6ca-6df52915b0b5" - # client_id = "d44117a8-b69d-437b-9073-e4e3b25e164a" - # client_secret = "c26f9698-a563-409e-87ee-4dcf96007b73" - # resource_group = "my-resource-group" + # provider = "azure/openebs/restic" + # azure { + # # Required arguments. + # subscription_id = "9e5ac23c-6df8-44c4-9790-6f6decf96268" + # tenant_id = "78bdc534-b34f-4bda-a6ca-6df52915b0b5" + # client_id = "d44117a8-b69d-437b-9073-e4e3b25e164a" + # client_secret = "c26f9698-a563-409e-87ee-4dcf96007b73" + # resource_group = "my-resource-group" # - # backup_storage_location { - # resource_group = "my-resource-group" - # storage_account = "mybackupstorageaccount" - # bucket = "backupscontainer" - # } + # backup_storage_location { + # resource_group = "my-resource-group" + # storage_account = "mybackupstorageaccount" + # bucket = "backupscontainer" + # } # - # # Optional parameters - # volume_snapshot_location { - # resource_group = "my-resource-group" - # api_timeout = "10m" - # } - # } - - # openebs { - # credentials = file("cloud-credentails-file") - # provider = "aws" - # - # backup_storage_location { - # provider = "aws" - # region = "my-region" - # bucket = "my-bucket" - # name = "my-backup-location" - # } + # # Optional parameters + # volume_snapshot_location { + # resource_group = "my-resource-group" + # api_timeout = "10m" + # } + # } # - # volume_snapshot_location { - # bucket = "my-bucket" - # region = "my-region" - # provider = "aws" - # name = "my-snapshot-location" - # prefix = "backup-prefix" - # local = false + # openebs { + # credentials = file("cloud-credentails-file") + # provider = "aws" # - # openebs_namespace = "openebs" + # backup_storage_location { + # provider = "aws" + # region = "my-region" + # bucket = "my-bucket" + # name = "my-backup-location" + # } # - # s3_url = "mybucket.example.com" - # } - # } - - # restic { - # credentials = file("cloud-credentials-file") + # volume_snapshot_location { + # bucket = "my-bucket" + # region = "my-region" + # provider = "aws" + # name = "my-snapshot-location" + # prefix = "backup-prefix" + # local = false + # + # openebs_namespace = "openebs" + # + # s3_url = "mybucket.example.com" + # } + # } + # + # restic { + # credentials = file("cloud-credentials-file") # - # require_volume_annotation = true + # require_volume_annotation = true # - # backup_storage_location { - # provider = "aws" - # bucket = "my-bucket" - # name = "my-backup-location" - # } - # } + # backup_storage_location { + # provider = "aws" + # bucket = "my-bucket" + # name = "my-backup-location" + # } + # } # Optional. metrics {