Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubernetes: Ignore internal K8S annotations in config_map + use PATCH #12945

Merged
merged 2 commits into from
Mar 27, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
kubernetes: Use JSON patch for updating config_map
  • Loading branch information
radeksimko committed Mar 27, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 4d2e28aecbda1d0ee8c605ccbcd07aca6e249e46
135 changes: 135 additions & 0 deletions builtin/providers/kubernetes/patch_operations.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package kubernetes

import (
"encoding/json"
"reflect"
"sort"
"strings"
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit pick - should this file really be called patch_operations? It seems there are patch and replace operation funcs within :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the struct type is called PatchOperation and the word patch comes from PATCH HTTP method, or more specifically from JSON Patch - so I think it's the right name.

I think eventually we could use these helpers cross providers, but it depends on the exact implementation of the SDK.

func diffStringMap(pathPrefix string, oldV, newV map[string]interface{}) PatchOperations {
ops := make([]PatchOperation, 0, 0)

pathPrefix = strings.TrimRight(pathPrefix, "/")

// This is suboptimal for adding whole new map from scratch
// or deleting the whole map, but it's actually intention.
// There may be some other map items managed outside of TF
// and we don't want to touch these.

for k, _ := range oldV {
if _, ok := newV[k]; ok {
continue
}
ops = append(ops, &RemoveOperation{Path: pathPrefix + "/" + k})
}

for k, v := range newV {
newValue := v.(string)

if oldValue, ok := oldV[k].(string); ok {
if oldValue == newValue {
continue
}

ops = append(ops, &ReplaceOperation{
Path: pathPrefix + "/" + k,
Value: newValue,
})
continue
}

ops = append(ops, &AddOperation{
Path: pathPrefix + "/" + k,
Value: newValue,
})
}

return ops
}

type PatchOperations []PatchOperation

func (po PatchOperations) MarshalJSON() ([]byte, error) {
var v []PatchOperation = po
return json.Marshal(v)
}

func (po PatchOperations) Equal(ops []PatchOperation) bool {
var v []PatchOperation = po

sort.Slice(v, sortByPathAsc(ops))
sort.Slice(ops, sortByPathAsc(ops))

return reflect.DeepEqual(v, ops)
}

func sortByPathAsc(ops []PatchOperation) func(i, j int) bool {
return func(i, j int) bool {
return ops[i].GetPath() < ops[j].GetPath()
}
}

type PatchOperation interface {
MarshalJSON() ([]byte, error)
GetPath() string
}

type ReplaceOperation struct {
Path string `json:"path"`
Value interface{} `json:"value"`
Op string `json:"op"`
}

func (o *ReplaceOperation) GetPath() string {
return o.Path
}

func (o *ReplaceOperation) MarshalJSON() ([]byte, error) {
o.Op = "replace"
return json.Marshal(*o)
}

func (o *ReplaceOperation) String() string {
b, _ := o.MarshalJSON()
return string(b)
}

type AddOperation struct {
Path string `json:"path"`
Value interface{} `json:"value"`
Op string `json:"op"`
}

func (o *AddOperation) GetPath() string {
return o.Path
}

func (o *AddOperation) MarshalJSON() ([]byte, error) {
o.Op = "add"
return json.Marshal(*o)
}

func (o *AddOperation) String() string {
b, _ := o.MarshalJSON()
return string(b)
}

type RemoveOperation struct {
Path string `json:"path"`
Op string `json:"op"`
}

func (o *RemoveOperation) GetPath() string {
return o.Path
}

func (o *RemoveOperation) MarshalJSON() ([]byte, error) {
o.Op = "remove"
return json.Marshal(*o)
}

func (o *RemoveOperation) String() string {
b, _ := o.MarshalJSON()
return string(b)
}
126 changes: 126 additions & 0 deletions builtin/providers/kubernetes/patch_operations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package kubernetes

import (
"fmt"
"testing"
)

func TestDiffStringMap(t *testing.T) {
testCases := []struct {
Path string
Old map[string]interface{}
New map[string]interface{}
ExpectedOps PatchOperations
}{
{
Path: "/parent/",
Old: map[string]interface{}{
"one": "111",
"two": "222",
},
New: map[string]interface{}{
"one": "111",
"two": "222",
"three": "333",
},
ExpectedOps: []PatchOperation{
&AddOperation{
Path: "/parent/three",
Value: "333",
},
},
},
{
Path: "/parent/",
Old: map[string]interface{}{
"one": "111",
"two": "222",
},
New: map[string]interface{}{
"one": "111",
"two": "abcd",
},
ExpectedOps: []PatchOperation{
&ReplaceOperation{
Path: "/parent/two",
Value: "abcd",
},
},
},
{
Path: "/parent/",
Old: map[string]interface{}{
"one": "111",
"two": "222",
},
New: map[string]interface{}{
"two": "abcd",
"three": "333",
},
ExpectedOps: []PatchOperation{
&RemoveOperation{Path: "/parent/one"},
&ReplaceOperation{
Path: "/parent/two",
Value: "abcd",
},
&AddOperation{
Path: "/parent/three",
Value: "333",
},
},
},
{
Path: "/parent/",
Old: map[string]interface{}{
"one": "111",
"two": "222",
},
New: map[string]interface{}{
"two": "222",
},
ExpectedOps: []PatchOperation{
&RemoveOperation{Path: "/parent/one"},
},
},
{
Path: "/parent/",
Old: map[string]interface{}{
"one": "111",
"two": "222",
},
New: map[string]interface{}{},
ExpectedOps: []PatchOperation{
&RemoveOperation{Path: "/parent/one"},
&RemoveOperation{Path: "/parent/two"},
},
},
{
Path: "/parent/",
Old: map[string]interface{}{},
New: map[string]interface{}{
"one": "111",
"two": "222",
},
ExpectedOps: []PatchOperation{
&AddOperation{
Path: "/parent/one",
Value: "111",
},
&AddOperation{
Path: "/parent/two",
Value: "222",
},
},
},
}

for i, tc := range testCases {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
ops := diffStringMap(tc.Path, tc.Old, tc.New)
if !tc.ExpectedOps.Equal(ops) {
t.Fatalf("Operations don't match.\nExpected: %v\nGiven: %v\n", tc.ExpectedOps, ops)
}
})
}

}
23 changes: 14 additions & 9 deletions builtin/providers/kubernetes/resource_kubernetes_config_map.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package kubernetes

import (
"fmt"
"log"

"github.com/hashicorp/terraform/helper/schema"
pkgApi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/errors"
api "k8s.io/kubernetes/pkg/api/v1"
kubernetes "k8s.io/kubernetes/pkg/client/clientset_generated/release_1_5"
@@ -73,19 +75,22 @@ func resourceKubernetesConfigMapRead(d *schema.ResourceData, meta interface{}) e
func resourceKubernetesConfigMapUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*kubernetes.Clientset)

metadata := expandMetadata(d.Get("metadata").([]interface{}))
namespace, name := idParts(d.Id())
// This is necessary in case the name is generated
metadata.Name = name

cfgMap := api.ConfigMap{
ObjectMeta: metadata,
Data: expandStringMap(d.Get("data").(map[string]interface{})),
ops := patchMetadata("metadata.0.", "/metadata/", d)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we guaranteed to have metadata.0. ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I believe we are:

&schema.Schema{
		Type:        schema.TypeList,
		Required:    true,
		MaxItems:    1,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, LGTM

if d.HasChange("data") {
oldV, newV := d.GetChange("data")
diffOps := diffStringMap("/data/", oldV.(map[string]interface{}), newV.(map[string]interface{}))
ops = append(ops, diffOps...)
}
log.Printf("[INFO] Updating config map: %#v", cfgMap)
out, err := conn.CoreV1().ConfigMaps(namespace).Update(&cfgMap)
data, err := ops.MarshalJSON()
if err != nil {
return err
return fmt.Errorf("Failed to marshal update operations: %s", err)
}
log.Printf("[INFO] Updating config map %q: %v", name, string(data))
out, err := conn.CoreV1().ConfigMaps(namespace).Patch(name, pkgApi.JSONPatchType, data)
if err != nil {
return fmt.Errorf("Failed to update Config Map: %s", err)
}
log.Printf("[INFO] Submitted updated config map: %#v", out)
d.SetId(buildId(out.ObjectMeta))
16 changes: 16 additions & 0 deletions builtin/providers/kubernetes/structures.go
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ import (
"fmt"
"strings"

"github.com/hashicorp/terraform/helper/schema"
api "k8s.io/kubernetes/pkg/api/v1"
)

@@ -39,6 +40,21 @@ func expandMetadata(in []interface{}) api.ObjectMeta {
return meta
}

func patchMetadata(keyPrefix, pathPrefix string, d *schema.ResourceData) PatchOperations {
ops := make([]PatchOperation, 0, 0)
if d.HasChange(keyPrefix + "annotations") {
oldV, newV := d.GetChange(keyPrefix + "annotations")
diffOps := diffStringMap(pathPrefix+"annotations", oldV.(map[string]interface{}), newV.(map[string]interface{}))
ops = append(ops, diffOps...)
}
if d.HasChange(keyPrefix + "labels") {
oldV, newV := d.GetChange(keyPrefix + "labels")
diffOps := diffStringMap(pathPrefix+"labels", oldV.(map[string]interface{}), newV.(map[string]interface{}))
ops = append(ops, diffOps...)
}
return ops
}

func expandStringMap(m map[string]interface{}) map[string]string {
result := make(map[string]string)
for k, v := range m {