Merge pull request #1365 from kinvolk/imran/restic-fixes

Restic bug fixes; unit tests and docs formatting

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 {

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)
+	}
+}

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.
@@ -44,7 +47,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.
@@ -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
 

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)
+		}
+	}
+}

pkg/components/velero/component_test.go

@@ -170,6 +170,7 @@ component "velero" {
     backup_storage_location {
       bucket   = "foo"
       provider = "aws"
+      region   = "myregion"
     }
   }
 }
@@ -207,14 +208,14 @@ component "velero" {
     backup_storage_location {
       bucket   = "foo"
       provider = "aws"
+      region   = "myregion"
     }
-		
     tolerations {
-      key                = "TestResticToletrationKey"
-      value              = "TestResticToletrationValue"
+      key                = "TestResticTolerationKey"
+      value              = "TestResticTolerationValue"
       operator           = "Equal"
-      effect             = "NoSchedule"
-      toleration_seconds = "1"
+      effect             = "NoExecute"
+      toleration_seconds = 1
     }
   }
 }
@@ -233,9 +234,16 @@ 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")
 
 	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)
 }

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
 }
 

pkg/components/velero/restic/restic_test.go

@@ -0,0 +1,71 @@
+// 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,
+					},
+				},
+			},
+		},
+		{
+			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 {
+		diags := tc.config.Validate()
+		if !tc.wantError && diags.HasErrors() {
+			t.Fatalf("Valid config should not return error, got: %s", diags.Error())
+		}
+	}
+}