From 5ce3839d0046d649f4bf915ba5d6fb9ed7f29168 Mon Sep 17 00:00:00 2001 From: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> Date: Thu, 23 Sep 2021 21:56:35 +0530 Subject: [PATCH] Add change-group placeholders for granular ordering of resources (#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 --- pkg/kapp/config/default.go | 10 +- pkg/kapp/diffgraph/change.go | 22 +++- pkg/kapp/diffgraph/change_graph_test.go | 118 ++++++++++++++++++ pkg/kapp/diffgraph/change_group_name.go | 72 +++++++++++ .../resourcesmisc/api_extensions_vx_crd.go | 16 +++ 5 files changed, 233 insertions(+), 5 deletions(-) create mode 100644 pkg/kapp/diffgraph/change_group_name.go diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 7255ea4d4..283fa2bf6 100644 --- a/pkg/kapp/config/default.go +++ b/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: diff --git a/pkg/kapp/diffgraph/change.go b/pkg/kapp/diffgraph/change.go index e23bb6b97..1b43eb9c3 100644 --- a/pkg/kapp/diffgraph/change.go +++ b/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) diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index c657330d2..ba6344579 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/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) } diff --git a/pkg/kapp/diffgraph/change_group_name.go b/pkg/kapp/diffgraph/change_group_name.go new file mode 100644 index 000000000..9a19e5f8e --- /dev/null +++ b/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 +} diff --git a/pkg/kapp/resourcesmisc/api_extensions_vx_crd.go b/pkg/kapp/resourcesmisc/api_extensions_vx_crd.go index f7feb289b..bd63f5a83 100644 --- a/pkg/kapp/resourcesmisc/api_extensions_vx_crd.go +++ b/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 +} + /* ---