Remove importance of order of rule in RestoreSession (#795)

Tasks:
- [x] Remove importance of rule order
- [x] Add validator to ensure followings,
	- [x] There is at most one rule with empty targetHosts field.
	- [x] No two rule with non-empty targetHosts matches for a host.
	- [x] If snapshot is specified in a rule then paths is not specified.
- [x] Update concept doc to make rules behavior clear

Fixes: #790

apis/stash/v1beta1/validator.go

@@ -1,11 +1,69 @@
 package v1beta1
 
+import "fmt"
+
 // TODO: complete
 func (r BackupSession) IsValid() error {
 	return nil
 }
 
 // TODO: complete
 func (r RestoreSession) IsValid() error {
+	// ========== spec.Rules validation================
+	// We must ensure following:
+	// 1. There is at most one rule with empty targetHosts field.
+	// 2. No two rules with non-emtpy targetHosts matches for a host.
+	// 3. If snapshot field is specified in a rule then paths is not specified.
+
+	// ensure the there is at most one rule with source
+	var ruleIdx []int
+	for i, rule := range r.Spec.Rules {
+		if len(rule.TargetHosts) == 0 {
+			ruleIdx = append(ruleIdx, i)
+		}
+	}
+	if len(ruleIdx) > 1 {
+		return fmt.Errorf("\n\t"+
+			"Error: Invalid RestoreSession specification.\n\t"+
+			"Reason: %s.\n\t"+
+			"Hints: There can be at most one rule with empty targetHosts.", multipleRuleWithEmptyTargetHostError(ruleIdx))
+	}
+
+	// ensure that no two rules with non-emtpy targetHosts matches for a host
+	res := make(map[string]int, 0)
+	for i, rule := range r.Spec.Rules {
+		for _, host := range rule.TargetHosts {
+			v, ok := res[host]
+			if ok {
+				return fmt.Errorf("\n\t"+
+					"Error: Invalid RestoreSession specification.\n\t"+
+					"Reason: Multiple rules (rule[%d] and rule[%d]) match for host %q.\n\t"+
+					"Hints: There could be only one matching rule for a host.", v, i, host)
+			} else {
+				res[host] = i
+			}
+		}
+	}
+
+	// ensure that path is not specified in a rule if snapshot field is specified
+	for i, rule := range r.Spec.Rules {
+		if len(rule.Snapshots) != 0 && len(rule.Paths) != 0 {
+			return fmt.Errorf("\n\t"+
+				"Error: Invalid RestoreSession specification.\n\t"+
+				"Reason: Both 'snapshots' and 'paths' fileds are specified in rule[%d].\n\t"+
+				"Hints: A snpashot contains backup data of only one directory. So, you can't specify 'paths' if you specify snapshot field.", i)
+		}
+	}
 	return nil
 }
+
+func multipleRuleWithEmptyTargetHostError(ruleIndexes []int) string {
+	ids := ""
+	for i, idx := range ruleIndexes {
+		ids += fmt.Sprintf("rule[%d]", idx)
+		if i < len(ruleIndexes)-1 {
+			ids += ", "
+		}
+	}
+	return fmt.Sprintf("%d rules found with empty targetHosts (Rules: %s)", len(ruleIndexes), ids)
+}

chart/stash/templates/apiregistration.yaml

@@ -22,6 +22,22 @@ spec:
 ---
 apiVersion: apiregistration.k8s.io/v1beta1
 kind: APIService
+metadata:
+  name: v1beta1.admission.stash.appscode.com
+  labels:
+    {{- include "stash.labels" . | nindent 4 }}
+spec:
+  group: admission.stash.appscode.com
+  version: v1beta1
+  service:
+    namespace: {{ .Release.Namespace }}
+    name: {{ template "stash.fullname" . }}
+  caBundle: {{ b64enc $ca.Cert }}
+  groupPriorityMinimum: {{ .Values.apiserver.groupPriorityMinimum }}
+  versionPriority: {{ .Values.apiserver.versionPriority }}
+---
+apiVersion: apiregistration.k8s.io/v1beta1
+kind: APIService
 metadata:
   name: v1alpha1.repositories.stash.appscode.com
   labels:

chart/stash/templates/validating-webhook.yaml

@@ -75,4 +75,25 @@ webhooks:
 {{- if and (ge $major 1) (ge $minor 12) }}
   sideEffects: None
 {{- end }}
+- name: restoresession.admission.stash.appscode.com
+  clientConfig:
+    service:
+      namespace: default
+      name: kubernetes
+      path: /apis/admission.stash.appscode.com/v1beta1/restoresessionvalidators
+    caBundle: {{ b64enc .Values.apiserver.ca }}
+  rules:
+  - operations:
+    - CREATE
+    - UPDATE
+    apiGroups:
+    - stash.appscode.com
+    apiVersions:
+    - "*"
+    resources:
+    - restoresessions
+  failurePolicy: Fail
+{{- if and (ge $major 1) (ge $minor 12) }}
+  sideEffects: None
+{{- end }}
 {{ end }}

docs/concepts/crds/restoresession.md

@@ -157,13 +157,15 @@ Each restore rule has the following fields:
 - **targetHosts :** `targetHosts` field contains a list of host names which are subject to this rule. If `targetHosts` field is empty, this rule applies to all hosts for which there is no specific rule. In the sample `RestoreSession` given above, the first rule applies to only `host-3` and `host-4` and the second rule is applicable to all hosts.
 - **sourceHost :** `sourceHost` specifies the name of host whose backed up data will be restored by this rule. In the sample `RestoreSession`, the first rule specify that backed up data of `host-0` (i.e. `pod-0` of old StatefulSet) will be restored into `host-3` and `host-4` (i.e. `pod-3` and `pod-4` of new StatefulSet). If you keep `sourceHost` field empty as the second rule of the above example, data from a similar backup host will be restored on the respective restore host. That means, backed up data of `host-0` will be restored into `host-0`, backed up data of `host-1` will be restored into `host-1` and so on.
 - **paths :** `paths` specifies a list of directories that will be restored into the hosts who are subject to this rule.
-- **snapshots :** `snapshots` specifies the list of snapshots that will be restored into the hosts who are subject to this rule.
+- **snapshots :** `snapshots` specifies the list of snapshots that will be restored into the hosts who are subject to this rule. If you don't specify snapshot field, latest snasphot of the directories specified in `paths` section will be restored.
 
-  >Note that, if you specify `snapshots` section, you don't have to specify `paths` section because each snapshot contains backup data of only one directory. Thus, if you specify both `paths` and `snapshots` and if a path does not exist in the specified snapshot then restore process will fail.
+Restore rules comply with the following conditions:
 
-When the restore process runs in a host, it starts matching rules from the beginning. When a rule is matched, it immediately takes backup according to that rule and completes it's backup process. No further rules are checked. So, if your rule section has multiple matching rules for a host, only the first matching rule will be applied to restore for this host.
-
-> If no rule matches for a host, no data will be restored on that host.
+- There could be at most one rule with empty `targetHosts` field.
+- No two rules with non-emtpy `targetHosts` can't be matched for a single host.
+- Stash backup only one directory in a single snapshot. So, if you specify `snapshots` field in a rule, you can't specify `paths` field as it may cause restore failure if a directory wasn't backed up in a snapshot specified in the `snapshots` field.
+- If no rule matches for a host, no data will be restored on that host.
+- The order of the rules does not have any effect on the restore process.
 
 #### spec.runtimeSettings
 

hack/deploy/apiservices.yaml

@@ -18,6 +18,23 @@ spec:
 # register as aggregated apiserver
 apiVersion: apiregistration.k8s.io/v1beta1
 kind: APIService
+metadata:
+  name: v1beta1.admission.stash.appscode.com
+  labels:
+    app: stash
+spec:
+  caBundle: ${SERVICE_SERVING_CERT_CA}
+  group: admission.stash.appscode.com
+  groupPriorityMinimum: 1000
+  versionPriority: 15
+  service:
+    name: stash-operator
+    namespace: ${STASH_NAMESPACE}
+  version: v1beta1
+---
+# register as aggregated apiserver
+apiVersion: apiregistration.k8s.io/v1beta1
+kind: APIService
 metadata:
   name: v1alpha1.repositories.stash.appscode.com
   labels:

hack/deploy/stash.sh

@@ -1,7 +1,7 @@
 #!/bin/bash
 set -eou pipefail
 
-crds=(restics repositories recoveries backupconfigurations backupsessions)
+crds=(restics repositories recoveries backupconfigurations backupsessions backupconfigurationtemplates functions restoresessions tasks)
 
 echo "checking kubeconfig context"
 kubectl config current-context || {

hack/deploy/validating-webhook.yaml

@@ -62,3 +62,22 @@ webhooks:
     - repositories
   failurePolicy: Fail
   ${STASH_WEBHOOK_SIDE_EFFECTS}
+- name: restoresession.admission.stash.appscode.com
+  clientConfig:
+    service:
+      namespace: default
+      name: kubernetes
+      path: /apis/admission.stash.appscode.com/v1beta1/restoresessionvalidators
+    caBundle: ${KUBE_CA}
+  rules:
+  - operations:
+    - CREATE
+    - UPDATE
+    apiGroups:
+    - stash.appscode.com
+    apiVersions:
+    - "*"
+    resources:
+    - restoresessions
+  failurePolicy: Fail
+  ${STASH_WEBHOOK_SIDE_EFFECTS}

pkg/cmds/server/start.go

@@ -86,6 +86,7 @@ func (o StashOptions) Config() (*server.StashConfig, error) {
 		"/apis/admission.stash.appscode.com/v1alpha1/replicationcontrollermutators",
 		"/apis/admission.stash.appscode.com/v1alpha1/replicasetmutators",
 		"/apis/admission.stash.appscode.com/v1alpha1/deploymentconfigmutators",
+		"/apis/admission.stash.appscode.com/v1beta1/restoresessionvalidators",
 	}
 
 	extraConfig := controller.NewConfig(serverConfig.ClientConfig)

pkg/server/server.go

@@ -117,7 +117,7 @@ func (c completedConfig) New() (*StashServer, error) {
 			ctrl.NewRecoveryWebhook(),
 			ctrl.NewRepositoryWebhook(),
 			// ctrl.NewBackupSessionWebhook(),
-			// ctrl.NewRestoreSessionWebhook(),
+			ctrl.NewRestoreSessionWebhook(),
 		)
 	}
 	if c.ExtraConfig.EnableMutatingWebhook {

pkg/util/options.go

@@ -31,26 +31,35 @@ func RestoreOptionForRestoreSession(restoreSession api.RestoreSession, extraOpt
 	return RestoreOptionsForHost(extraOpt.Host, restoreSession.Spec.Rules)
 }
 
-// return first matching rule
-// if hosts is empty for any rule, it will match any hostname
+// return the matching rule
+// if targetHosts is empty for a rule, it will match any hostname
 func RestoreOptionsForHost(hostname string, rules []api.Rule) restic.RestoreOptions {
+	var matchedRule restic.RestoreOptions
+	// first check for rules non-empty targetHost
 	for _, rule := range rules {
-		// if host is specified in rule then use it. otherwise use workload itself as host
+		// if sourceHost is specified in the rule then use it. otherwise use workload itself as host
 		sourceHost := hostname
 		if rule.SourceHost != "" {
 			sourceHost = rule.SourceHost
 		}
 
 		if len(rule.TargetHosts) == 0 || go_str.Contains(rule.TargetHosts, hostname) {
-			return restic.RestoreOptions{
+			matchedRule = restic.RestoreOptions{
 				Host:        hostname,
 				SourceHost:  sourceHost,
 				RestoreDirs: rule.Paths,
 				Snapshots:   rule.Snapshots,
 			}
+			// if rule has empty targetHost then check further rules to see if any other rule with non-empty targetHost matches
+			if len(rule.TargetHosts) == 0 {
+				continue
+			} else {
+				return matchedRule
+			}
 		}
 	}
-	return restic.RestoreOptions{}
+	// matchedRule is either emtpy or contains restore option for the rules with empty targetHost field.
+	return matchedRule
 }
 
 func SetupOptionsForRepository(repository api_v1alpha1.Repository, extraOpt ExtraOptions) (restic.SetupOptions, error) {