Merge pull request #204 from keel-hq/bugfix/cache_copy

Bugfix/cache copy

internal/k8s/cache.go

@@ -20,7 +20,10 @@ type GenericResourceCache struct {
 // Values returns a copy of the contents of the cache.
 func (cc *genericResourceCache) Values() []*GenericResource {
 	cc.Lock()
-	r := append([]*GenericResource{}, cc.values...)
+	r := []*GenericResource{}
+	for _, v := range cc.values {
+		r = append(r, v.DeepCopy())
+	}
 	cc.Unlock()
 	return r
 }

internal/k8s/cache_test.go

@@ -0,0 +1,53 @@
+package k8s
+
+import (
+	"testing"
+
+	apps_v1 "k8s.io/api/apps/v1"
+	core_v1 "k8s.io/api/core/v1"
+	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestAddGet(t *testing.T) {
+
+	cc := &GenericResourceCache{}
+
+	d := &apps_v1.Deployment{
+		meta_v1.TypeMeta{},
+		meta_v1.ObjectMeta{
+			Name:        "dep-1",
+			Namespace:   "xxxx",
+			Annotations: map[string]string{},
+			Labels:      map[string]string{},
+		},
+		apps_v1.DeploymentSpec{
+			Template: core_v1.PodTemplateSpec{
+				Spec: core_v1.PodSpec{
+					Containers: []core_v1.Container{
+						{
+							Image: "gcr.io/v2-namespace/hi-world:1.1.1",
+						},
+					},
+				},
+			},
+		},
+		apps_v1.DeploymentStatus{},
+	}
+
+	gr, err := NewGenericResource(d)
+	if err != nil {
+		t.Fatalf("failed to create generic resource: %s", err)
+	}
+
+	cc.Add(gr)
+
+	// updating deployment
+	stored := cc.Values()[0]
+	stored.UpdateContainer(0, "gcr.io/v2-namespace/hi-world:2.2.2.")
+
+	// getting again
+	stored2 := cc.Values()[0]
+	if stored2.Containers()[0].Image != "gcr.io/v2-namespace/hi-world:1.1.1" {
+		t.Errorf("cached entry got modified: %s", stored2.Containers()[0].Image)
+	}
+}

internal/k8s/resource.go

@@ -59,6 +59,28 @@ func (r *GenericResource) String() string {
 	return fmt.Sprintf("%s/%s/%s images: %s", r.Kind(), r.Namespace, r.Name, strings.Join(r.GetImages(), ", "))
 }
 
+// DeepCopy uses an autogenerated deepcopy functions, copying the receiver, creating a new GenericResource
+func (r *GenericResource) DeepCopy() *GenericResource {
+	gr := new(GenericResource)
+	if r.obj == nil {
+		return gr
+	}
+	gr.Identifier = r.Identifier
+	gr.Namespace = r.Namespace
+	gr.Name = r.Name
+
+	switch obj := r.obj.(type) {
+	case *apps_v1.Deployment:
+		gr.obj = obj.DeepCopy()
+	case *apps_v1.StatefulSet:
+		gr.obj = obj.DeepCopy()
+	case *apps_v1.DaemonSet:
+		gr.obj = obj.DeepCopy()
+	}
+
+	return gr
+}
+
 // GetIdentifier returns resource identifier
 func (r *GenericResource) GetIdentifier() string {
 	switch obj := r.obj.(type) {
@@ -146,23 +168,11 @@ func (r *GenericResource) SetLabels(labels map[string]string) {
 func (r *GenericResource) GetSpecAnnotations() (annotations map[string]string) {
 	switch obj := r.obj.(type) {
 	case *apps_v1.Deployment:
-		a := obj.Spec.Template.GetAnnotations()
-		if a == nil {
-			return make(map[string]string)
-		}
-		return a
+		return getOrInitialise(obj.Spec.Template.GetAnnotations())
 	case *apps_v1.StatefulSet:
-		a := obj.Spec.Template.GetAnnotations()
-		if a == nil {
-			return make(map[string]string)
-		}
-		return a
+		return getOrInitialise(obj.Spec.Template.GetAnnotations())
 	case *apps_v1.DaemonSet:
-		a := obj.Spec.Template.GetAnnotations()
-		if a == nil {
-			return make(map[string]string)
-		}
-		return a
+		return getOrInitialise(obj.Spec.Template.GetAnnotations())
 	}
 	return
 }

internal/k8s/translator.go

@@ -18,7 +18,7 @@ func (t *Translator) OnAdd(obj interface{}) {
 		t.Errorf("OnAdd failed to add resource %T: %#v", obj, obj)
 		return
 	}
-	t.Infof("added %s %s", gr.Kind(), gr.Name)
+	t.Debugf("added %s %s", gr.Kind(), gr.Name)
 	t.GenericResourceCache.Add(gr)
 }
 
@@ -28,7 +28,7 @@ func (t *Translator) OnUpdate(oldObj, newObj interface{}) {
 		t.Errorf("OnUpdate failed to update resource %T: %#v", newObj, newObj)
 		return
 	}
-	t.Infof("updated %s %s", gr.Kind(), gr.Name)
+	t.Debugf("updated %s %s", gr.Kind(), gr.Name)
 	t.GenericResourceCache.Add(gr)
 }
 
@@ -38,6 +38,6 @@ func (t *Translator) OnDelete(obj interface{}) {
 		t.Errorf("OnDelete failed to delete resource %T: %#v", obj, obj)
 		return
 	}
-	t.Infof("deleted %s %s", gr.Kind(), gr.Name)
+	t.Debugf("deleted %s %s", gr.Kind(), gr.Name)
 	t.GenericResourceCache.Remove(gr.GetIdentifier())
 }

provider/kubernetes/approvals.go

@@ -11,8 +11,8 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-func getIdentifier(namespace, name, version string) string {
-	return namespace + "/" + name + ":" + version
+func getApprovalIdentifier(resourceIdentifier, version string) string {
+	return resourceIdentifier + ":" + version
 }
 
 // checkForApprovals - filters out deployments and only passes forward approved ones
@@ -64,7 +64,8 @@ func (p *Provider) isApproved(event *types.Event, plan *UpdatePlan) (bool, error
 		}
 	}
 
-	identifier := getIdentifier(plan.Resource.Namespace, plan.Resource.Name, plan.NewVersion)
+	// identifier := getIdentifier(plan.Resource.Namespace, plan.Resource.Name, plan.NewVersion)
+	identifier := getApprovalIdentifier(plan.Resource.Identifier, plan.NewVersion)
 
 	// checking for existing approval
 	existing, err := p.approvalManager.Get(identifier)
@@ -90,7 +91,8 @@ func (p *Provider) isApproved(event *types.Event, plan *UpdatePlan) (bool, error
 				approval.Delta(),
 			)
 
-			fmt.Println("requesting approval, ns: ", plan.Resource.Namespace)
+			// fmt.Println("requesting approval, identifier: ", plan.Resource.Namespace)
+			fmt.Println("requesting approval, identifier: ", identifier)
 
 			return false, p.approvalManager.Create(approval)
 		}

provider/kubernetes/approvals_test.go

@@ -73,7 +73,7 @@ func TestCheckRequestedApproval(t *testing.T) {
 	}
 
 	// checking approvals
-	approval, err := provider.approvalManager.Get("xxxx/dep-1:1.1.2")
+	approval, err := provider.approvalManager.Get("deployment/xxxx/dep-1:1.1.2")
 	if err != nil {
 		t.Fatalf("failed to find approval, err: %s", err)
 	}
@@ -130,7 +130,7 @@ func TestApprovedCheck(t *testing.T) {
 
 	// approving event
 	err = provider.approvalManager.Create(&types.Approval{
-		Identifier:    "xxxx/dep-1:1.1.2",
+		Identifier:    "deployment/xxxx/dep-1:1.1.2",
 		VotesReceived: 2,
 		VotesRequired: 2,
 		Deadline:      time.Now().Add(10 * time.Second),
@@ -139,7 +139,10 @@ func TestApprovedCheck(t *testing.T) {
 		t.Fatalf("failed to create approval: %s", err)
 	}
 
-	appr, _ := provider.approvalManager.Get("xxxx/dep-1:1.1.2")
+	appr, err := provider.approvalManager.Get("deployment/xxxx/dep-1:1.1.2")
+	if err != nil {
+		t.Fatalf("failed to get approval: %s", err)
+	}
 	if appr.Status() != types.ApprovalStatusApproved {
 		t.Fatalf("approval not approved")
 	}

provider/kubernetes/kubernetes.go

@@ -319,7 +319,7 @@ func (p *Provider) createUpdatePlans(repo *types.Repository) ([]*UpdatePlan, err
 		policy := policies.GetPolicy(labels)
 		if policy == types.PolicyTypeNone {
 			// skip
-			log.Infof("no policy defined, skipping: %s, labels: %s", resource.Identifier, labels)
+			log.Debugf("no policy defined, skipping: %s, labels: %s", resource.Identifier, labels)
 			continue
 		}
 

provider/kubernetes/unversioned_updates_test.go

@@ -11,9 +11,8 @@ import (
 	"github.com/keel-hq/keel/types"
 	"github.com/keel-hq/keel/util/timeutil"
 
-	"k8s.io/api/core/v1"
-	// "k8s.io/api/extensions/apps_v1"
 	apps_v1 "k8s.io/api/apps/v1"
+	"k8s.io/api/core/v1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 

provider/kubernetes/versioned_updates.go

@@ -38,11 +38,7 @@ func (p *Provider) checkVersionedDeployment(newVersion *types.Version, policy ty
 
 	shouldUpdateDeployment = false
 
-	// for idx, c := range deployment.Spec.Template.Spec.Containers {
 	for idx, c := range resource.Containers() {
-		// Remove version if any
-		// containerImageName := versionreg.ReplaceAllString(c.Image, "")
-
 		containerImageRef, err := image.Parse(c.Image)
 		if err != nil {
 			log.WithFields(log.Fields{
@@ -141,16 +137,11 @@ func (p *Provider) checkVersionedDeployment(newVersion *types.Version, policy ty
 		}).Info("provider.kubernetes: checked version, deciding whether to update")
 
 		if shouldUpdateContainer {
-
-			// c = updateContainer(c, conatinerImageRef, newVersion.String())
-
 			if containerImageRef.Registry() == image.DefaultRegistryHostname {
 				resource.UpdateContainer(idx, fmt.Sprintf("%s:%s", containerImageRef.ShortName(), newVersion.String()))
 			} else {
 				resource.UpdateContainer(idx, fmt.Sprintf("%s:%s", containerImageRef.Repository(), newVersion.String()))
 			}
-
-			// deployment.Spec.Template.Spec.Containers[idx] = c
 			// marking this deployment for update
 			shouldUpdateDeployment = true