Add change-group placeholders for granular ordering of resources (car…

…vel-dev#310)

* Update default change groups and rules for granular ordering(POC)
* Wrote PlaceholderParser which can be instantiated with a resource to parse changeGroup names
* Making PlaceholderParser more specific as ChangeGroupName. Resolving nits
* Optimisation: Using regex based string replace instead of looping over placeholders
* Add change graph test for change group placeholders
* Add more resources to TestChangeGraphWithNamespaceAndCRDs
* Add back the namespace matcher

Co-authored-by: Soumik Majumder <soumikm@vmware.com>

pkg/kapp/config/default.go

@@ -386,10 +386,16 @@ changeGroupBindings:
   resourceMatchers: &crdMatchers
   - apiGroupKindMatcher: {kind: CustomResourceDefinition, apiGroup: apiextensions.k8s.io}
 
+- name: change-groups.kapp.k14s.io/crds-{crd-group}-{crd-kind}
+  resourceMatchers: *crdMatchers
+
 - name: change-groups.kapp.k14s.io/namespaces
   resourceMatchers: &namespaceMatchers
   - apiGroupKindMatcher: {kind: Namespace, apiGroup: ""}
 
+- name: change-groups.kapp.k14s.io/namespaces-{name}
+  resourceMatchers: *namespaceMatchers
+
 - name: change-groups.kapp.k14s.io/storage-class
   resourceMatchers: &storageClassMatchers
   - apiVersionKindMatcher: {kind: StorageClass, apiVersion: storage/v1}
@@ -439,7 +445,7 @@ changeGroupBindings:
 changeRuleBindings:
 # Insert CRDs before all CRs
 - rules:
-  - "upsert after upserting change-groups.kapp.k14s.io/crds"
+  - "upsert after upserting change-groups.kapp.k14s.io/crds-{api-group}-{kind}"
   resourceMatchers:
   - andMatcher:
       matchers:
@@ -482,7 +488,7 @@ changeRuleBindings:
 
 # Insert namespaces before all namespaced resources
 - rules:
-  - "upsert after upserting change-groups.kapp.k14s.io/namespaces"
+  - "upsert after upserting change-groups.kapp.k14s.io/namespaces-{namespace}"
   resourceMatchers:
   - andMatcher:
       matchers:

pkg/kapp/diffgraph/change.go

@@ -104,7 +104,11 @@ func (c *Change) Groups() ([]ChangeGroup, error) {
 
 	for k, v := range res.Annotations() {
 		if k == changeGroupAnnKey || strings.HasPrefix(k, changeGroupAnnPrefixKey) {
-			groupKey, err := NewChangeGroupFromAnnString(v)
+			name, err := NewChangeGroupNameForResource(v, c.Change.Resource()).AsString()
+			if err != nil {
+				return nil, err
+			}
+			groupKey, err := NewChangeGroupFromAnnString(name)
 			if err != nil {
 				return nil, err
 			}
@@ -116,7 +120,11 @@ func (c *Change) Groups() ([]ChangeGroup, error) {
 		rms := ctlconf.ResourceMatchers(groupConfig.ResourceMatchers).AsResourceMatchers()
 
 		if (ctlres.AnyMatcher{rms}).Matches(res) {
-			groupKey, err := NewChangeGroupFromAnnString(groupConfig.Name)
+			name, err := NewChangeGroupNameForResource(groupConfig.Name, c.Change.Resource()).AsString()
+			if err != nil {
+				return nil, err
+			}
+			groupKey, err := NewChangeGroupFromAnnString(name)
 			if err != nil {
 				return nil, err
 			}
@@ -139,7 +147,11 @@ func (c *Change) AllRules() ([]ChangeRule, error) {
 
 	for k, v := range res.Annotations() {
 		if k == changeRuleAnnKey || strings.HasPrefix(k, changeRuleAnnPrefixKey) {
-			rule, err := NewChangeRuleFromAnnString(v)
+			ruleStr, err := NewChangeGroupNameForResource(v, c.Change.Resource()).AsString()
+			if err != nil {
+				return nil, err
+			}
+			rule, err := NewChangeRuleFromAnnString(ruleStr)
 			if err != nil {
 				return nil, fmt.Errorf("Resource %s: %s", res.Description(), err)
 			}
@@ -152,6 +164,10 @@ func (c *Change) AllRules() ([]ChangeRule, error) {
 
 		if (ctlres.AnyMatcher{rms}).Matches(res) {
 			for _, ruleStr := range ruleConfig.Rules {
+				ruleStr, err := NewChangeGroupNameForResource(ruleStr, c.Change.Resource()).AsString()
+				if err != nil {
+					return nil, err
+				}
 				rule, err := NewChangeRuleFromAnnString(ruleStr)
 				if err != nil {
 					return nil, fmt.Errorf("Resource %s: %s", res.Description(), err)

pkg/kapp/diffgraph/change_graph_test.go

@@ -95,6 +95,10 @@ kind: CustomResourceDefinition
 apiVersion: apiextensions.k8s.io/v1
 metadata:
   name: app-config
+spec:
+  group: app-group
+  names:
+    kind: app-kind
 ---
 kind: Namespace
 apiVersion: v1
@@ -465,6 +469,120 @@ metadata:
 	}
 }
 
+func TestChangeGraphWithNamespaceAndCRDs(t *testing.T) {
+	configYAML := `
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: kapp-namespace-1
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: kapp-namespace-2
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: app
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: kapp-secret-1
+  namespace: kapp-namespace-2
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: kapp-secret-2
+  namespace: kapp-namespace-2
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: kapp-secret-3
+  namespace: default
+---
+kind: CustomResourceDefinition
+apiVersion: apiextensions.k8s.io/v1
+metadata:
+  name: kapp-crd-1
+spec:
+  group: appGroup
+  names:
+    kind: KappCRD1
+---
+kind: CustomResourceDefinition
+apiVersion: apiextensions.k8s.io/v1
+metadata:
+  name: kapp-crd-2
+spec:
+  group: appGroup
+  names:
+    kind: KappCRD2
+---
+kind: CustomResourceDefinition
+apiVersion: apiextensions.k8s.io/v1
+metadata:
+  name: kapp-crd-3
+spec:
+  group: appGroup
+  names:
+    kind: KappCRD3
+---
+kind: KappCRD1
+apiVersion: appGroup/v1
+metadata:
+  name: kapp-cr-1
+---
+kind: KappCRD2
+apiVersion: appGroup/v1
+metadata:
+  name: kapp-cr-2
+`
+
+	_, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil)
+	if err != nil {
+		t.Fatalf("Error parsing conf defaults")
+	}
+
+	opts := buildGraphOpts{
+		resourcesBs:         configYAML,
+		op:                  ctldgraph.ActualChangeOpUpsert,
+		changeGroupBindings: conf.ChangeGroupBindings(),
+		changeRuleBindings:  conf.ChangeRuleBindings(),
+	}
+
+	graph, err := buildChangeGraphWithOpts(opts, t)
+	if err != nil {
+		t.Fatalf("Expected graph to build")
+	}
+
+	output := strings.TrimSpace(graph.PrintStr())
+	expectedOutput := strings.TrimSpace(`
+(upsert) namespace/kapp-namespace-1 (v1) cluster
+(upsert) namespace/kapp-namespace-2 (v1) cluster
+(upsert) namespace/app (v1) cluster
+(upsert) secret/kapp-secret-1 (v1) namespace: kapp-namespace-2
+  (upsert) namespace/kapp-namespace-2 (v1) cluster
+(upsert) secret/kapp-secret-2 (v1) namespace: kapp-namespace-2
+  (upsert) namespace/kapp-namespace-2 (v1) cluster
+(upsert) secret/kapp-secret-3 (v1) namespace: default
+(upsert) customresourcedefinition/kapp-crd-1 (apiextensions.k8s.io/v1) cluster
+(upsert) customresourcedefinition/kapp-crd-2 (apiextensions.k8s.io/v1) cluster
+(upsert) customresourcedefinition/kapp-crd-3 (apiextensions.k8s.io/v1) cluster
+(upsert) kappcrd1/kapp-cr-1 (appGroup/v1) cluster
+  (upsert) customresourcedefinition/kapp-crd-1 (apiextensions.k8s.io/v1) cluster
+(upsert) kappcrd2/kapp-cr-2 (appGroup/v1) cluster
+  (upsert) customresourcedefinition/kapp-crd-2 (apiextensions.k8s.io/v1) cluster
+`)
+
+	if output != expectedOutput {
+		t.Fatalf("Expected output to be >>>%s<<< but was >>>%s<<<", expectedOutput, output)
+	}
+}
+
 func buildChangeGraph(resourcesBs string, op ctldgraph.ActualChangeOp, t *testing.T) (*ctldgraph.ChangeGraph, error) {
 	return buildChangeGraphWithOpts(buildGraphOpts{resourcesBs: resourcesBs, op: op}, t)
 }

pkg/kapp/diffgraph/change_group_name.go

@@ -0,0 +1,72 @@
+// Copyright 2020 VMware, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package diffgraph
+
+import (
+	"fmt"
+	"regexp"
+
+	ctlres "github.com/k14s/kapp/pkg/kapp/resources"
+	ctlcrd "github.com/k14s/kapp/pkg/kapp/resourcesmisc"
+)
+
+type ChangeGroupName struct {
+	name     string
+	resource ctlres.Resource
+}
+
+func NewChangeGroupNameForResource(name string, resource ctlres.Resource) ChangeGroupName {
+	return ChangeGroupName{name, resource}
+}
+
+var (
+	placeholderMatcher = regexp.MustCompile("{.+?}")
+)
+
+// Placeholders have the format {placeholder-name}
+// Other patterns like ${placeholder-name} are commonly used by other operators/tools
+func (c ChangeGroupName) AsString() (string, error) {
+	var crdKind, crdGroup string
+	var err error
+	crd := ctlcrd.NewAPIExtensionsVxCRD(c.resource)
+	if crd != nil {
+		crdKind, err = crd.Kind()
+		if err != nil {
+			return c.name, err
+		}
+		crdGroup, err = crd.Group()
+		if err != nil {
+			return c.name, err
+		}
+	}
+
+	values := map[string]string{
+		"{api-group}": c.resource.APIGroup(),
+		"{kind}":      c.resource.Kind(),
+		"{name}":      c.resource.Name(),
+		"{namespace}": c.resource.Namespace(),
+		"{crd-kind}":  crdKind,
+		"{crd-group}": crdGroup,
+	}
+
+	replaced := placeholderMatcher.ReplaceAllStringFunc(c.name, func(placeholder string) string {
+		value, found := values[placeholder]
+		if !found {
+			err = fmt.Errorf("Expected placeholder to be one of these: %s but was %s", c.placeholders(values), placeholder)
+		}
+		if value == "" {
+			err = fmt.Errorf("Placeholder %s does not have a value for target resource (hint: placeholders with the 'crd-' prefix can only be used with CRDs)", placeholder)
+		}
+		return value
+	})
+
+	return replaced, err
+}
+
+func (c ChangeGroupName) placeholders(values map[string]string) (placeholders []string) {
+	for k := range values {
+		placeholders = append(placeholders, k)
+	}
+	return placeholders
+}

pkg/kapp/resourcesmisc/api_extensions_vx_crd.go

@@ -81,6 +81,22 @@ func (o crdObj) Versions() []string {
 	return result
 }
 
+func (s APIExtensionsVxCRD) Group() (crdGroup string, err error) {
+	crdObj, err := s.contents()
+	if err != nil {
+		return crdGroup, err
+	}
+	return crdObj.Spec.Group, err
+}
+
+func (s APIExtensionsVxCRD) Kind() (crdName string, err error) {
+	crdObj, err := s.contents()
+	if err != nil {
+		return crdName, err
+	}
+	return crdObj.Spec.Names.Kind, err
+}
+
 /*
 
 ---