Added enhanced logging and validations

calicoctl/commands/edit.go

@@ -12,20 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// TODO
-// Better error handling
-// Distinguish between deleted updates and other failures
-// Copyright stuff related to k8s
-// Windows/Linus newlines etc.
-// Need to verify that user is not adding new entry or changing resource identifiers
-// CreateResourcesFromBytes needs to be converted to slice ... this indicates a bug
-//   in the CreateResourcesFromBytes which must be returning structs rather than pointers.
+// TODO:
+// Need to update glide.yaml with the new libcalico-go version once it is merged
 
 package commands
 
 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -107,7 +102,7 @@ Description:
 		ext       string
 		rp        resourcePrinter
 		file      string
-		resources []unversioned.Resource
+		resources []unversioned.ResourceObject
 		nb        []byte
 	)
 	output := parsedArgs["--output"].(string)
@@ -174,7 +169,15 @@ Description:
 
 		// Convert the file into a set of resources.  If this succeeds
 		// then it passes validation, so exit this loop.
-		resources, err = resourcemgr.CreateResourcesFromBytes(b)
+		var rs []unversioned.Resource
+		rs, err = resourcemgr.CreateResourcesFromBytes(b)
+		if err != nil {
+			log.WithError(err).Debug("Failed validation, re-enter editor.")
+			continue
+		}
+
+		resources = convertToSliceOfResourceObjects(rs)
+		err = ValidateIDFieldsSame(results.resources, resources)
 		if err == nil {
 			break
 		}
@@ -189,21 +192,23 @@ Description:
 
 	// Data edited and parsed.  Apply the changes - any errors now and we
 	// exit.
-	resources = convertToSliceOfResources(resources)
-	failed := []unversioned.Resource{}
+	resources = convertToSliceOfResourceObjects(resources)
+	failed := []unversioned.ResourceObject{}
+	var failureErr error
 	for _, resource := range resources {
 		if _, err = executeResourceAction(ac, resource); err != nil {
 			failed = append(failed, resource)
+			failureErr = err
 		}
 	}
 	if len(failed) == 0 {
 		fmt.Printf("Successfully updated %d resources.\n", len(resources))
 	} else {
 		if len(failed) == len(resources) {
-			fmt.Printf("Failed to update any resource, last error: %s\n", err)
+			fmt.Printf("Failed to update any resource, last error: %s\n", failureErr)
 		} else {
 			fmt.Printf("Failed to update %d/%d resources, last error: %s\n",
-				len(failed), len(resources), err)
+				len(failed), len(resources), failureErr)
 		}
 
 		f, err := os.Create(file)
@@ -255,3 +260,21 @@ func addComment(b []byte, err error) []byte {
 	nb.Write(b)
 	return nb.Bytes()
 }
+
+// ValidateIDFieldsSame is used to validate that the Resource Objects
+// supplied in one slice reference the same objects in the other slice
+// by comparing the ID fields.
+func ValidateIDFieldsSame(original, changed []unversioned.ResourceObject) error {
+	if len(original) != len(changed) {
+		return errors.New("Number of resources after editing does not match the number of resources provided. Cannot add or remove resources during edit.")
+	}
+
+	// Assume that that the resources are kept in the same order
+	for i, _ := range original {
+		if original[i].String() != changed[i].String() {
+			return fmt.Errorf("Cannot change identifying metadata fields: %s should match %s", changed[i].String(), original[i].String())
+		}
+	}
+
+	return nil
+}

calicoctl/commands/edit_test.go

@@ -0,0 +1,62 @@
+// Copyright (c) 2017 Tigera, Inc. All rights reserved.
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package commands_test
+
+import (
+	"github.com/projectcalico/calicoctl/calicoctl/commands"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/projectcalico/libcalico-go/lib/api"
+	"github.com/projectcalico/libcalico-go/lib/api/unversioned"
+)
+
+var _ = Describe("Test ValidateIDFieldsSame", func() {
+	Context("with Profile Metadata", func() {
+		var profile1 *api.Profile
+		var profile2 *api.Profile
+		BeforeEach(func() {
+			profile1 = api.NewProfile()
+			profile1.Metadata.Name = "testProfile1"
+			profile2 = api.NewProfile()
+			profile2.Metadata.Name = "testProfile1"
+		})
+		It("should not error out if the name is not changed", func() {
+			before := []unversioned.ResourceObject{profile1}
+			after := []unversioned.ResourceObject{profile2}
+			err := commands.ValidateIDFieldsSame(before, after)
+			Expect(err).NotTo(HaveOccurred())
+		})
+		It("should return an error if the name is changed", func() {
+			profile2.Metadata.Name = "testName2"
+			before := []unversioned.ResourceObject{profile1}
+			after := []unversioned.ResourceObject{profile2}
+			err := commands.ValidateIDFieldsSame(before, after)
+			Expect(err).To(HaveOccurred())
+		})
+		It("should return an error if a resource is removed", func() {
+			before := []unversioned.ResourceObject{profile1}
+			after := []unversioned.ResourceObject{}
+			err := commands.ValidateIDFieldsSame(before, after)
+			Expect(err).To(HaveOccurred())
+		})
+		It("should return an error if a resource is added", func() {
+			before := []unversioned.ResourceObject{}
+			after := []unversioned.ResourceObject{profile2}
+			err := commands.ValidateIDFieldsSame(before, after)
+			Expect(err).To(HaveOccurred())
+		})
+	})
+})

calicoctl/commands/printer.go

@@ -38,24 +38,25 @@ var (
 	CR = []byte("\n")
 )
 
-func printResources(rp resourcePrinter, client *client.Client, resources []unversioned.Resource) error {
+func printResources(rp resourcePrinter, client *client.Client, resources []unversioned.ResourceObject) error {
 	return rp.write(os.Stdout, client, resources)
 }
 
 type resourcePrinter interface {
-	write(w io.Writer, client *client.Client, resources []unversioned.Resource) error
+	write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error
 }
 
 // resourcePrinterJSON implements the resourcePrinter interface and is used to display
 // a slice of resources in JSON format.
 type resourcePrinterJSON struct{}
 
-func (r resourcePrinterJSON) write(w io.Writer, client *client.Client, resources []unversioned.Resource) error {
+func (r resourcePrinterJSON) write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error {
+	// TODO: Remove the comments after testing
 	// The supplied slice of resources may contain actual resource types as well as
 	// resource lists (which themselves contain a slice of actual resources).
 	// For simplicity, expand any resource lists so that we have a flat slice of
 	// real resources.
-	resources = convertToSliceOfResources(resources)
+	// resources = convertToSliceOfResourceObjects(resources)
 	if output, err := json.MarshalIndent(resources, "", "  "); err != nil {
 		return err
 	} else {
@@ -69,12 +70,13 @@ func (r resourcePrinterJSON) write(w io.Writer, client *client.Client, resources
 // a slice of resources in YAML format.
 type resourcePrinterYAML struct{}
 
-func (r resourcePrinterYAML) write(w io.Writer, client *client.Client, resources []unversioned.Resource) error {
+func (r resourcePrinterYAML) write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error {
+	// TODO: Remove the comments after testing
 	// The supplied slice of resources may contain actual resource types as well as
 	// resource lists (which themselves contain a slice of actual resources).
 	// For simplicity, expand any resource lists so that we have a flat slice of
 	// real resources.
-	resources = convertToSliceOfResources(resources)
+	// resources = convertToSliceOfResourceObjects(resources)
 	if output, err := yaml.Marshal(resources); err != nil {
 		return err
 	} else {
@@ -97,7 +99,7 @@ type resourcePrinterTable struct {
 	wide bool
 }
 
-func (r resourcePrinterTable) write(w io.Writer, client *client.Client, resources []unversioned.Resource) error {
+func (r resourcePrinterTable) write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error {
 	log.Infof("Output in table format (wide=%v)", r.wide)
 	for _, resource := range resources {
 		// Get the resource manager for the resource type.
@@ -153,7 +155,7 @@ type resourcePrinterTemplateFile struct {
 	templateFile string
 }
 
-func (r resourcePrinterTemplateFile) write(w io.Writer, client *client.Client, resources []unversioned.Resource) error {
+func (r resourcePrinterTemplateFile) write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error {
 	template, err := ioutil.ReadFile(r.templateFile)
 	if err != nil {
 		return err
@@ -168,7 +170,7 @@ type resourcePrinterTemplate struct {
 	template string
 }
 
-func (r resourcePrinterTemplate) write(w io.Writer, client *client.Client, resources []unversioned.Resource) error {
+func (r resourcePrinterTemplate) write(w io.Writer, client *client.Client, resources []unversioned.ResourceObject) error {
 	// We include a join function in the template as it's useful for multi
 	// value columns.
 	fns := template.FuncMap{

calicoctl/commands/resources.go

@@ -53,12 +53,12 @@ type actionConfig struct {
 	action        action
 }
 
-// Convert loaded resources to a slice of resources for easier processing.
+// Convert loaded resources to a slice of resource objects for easier processing.
 // The loaded resources may be a slice containing resources and resource lists, or
 // may be a single resource or a single resource list.  This function handles the
-// different possible options to convert to a single slice of resources.
-func convertToSliceOfResources(loaded interface{}) []unversioned.Resource {
-	r := []unversioned.Resource{}
+// different possible options to convert to a single slice of resource objects.
+func convertToSliceOfResourceObjects(loaded interface{}) []unversioned.ResourceObject {
+	r := []unversioned.ResourceObject{}
 	log.Infof("Converting resource to slice: %v", loaded)
 
 	switch reflect.TypeOf(loaded).Kind() {
@@ -67,7 +67,7 @@ func convertToSliceOfResources(loaded interface{}) []unversioned.Resource {
 		// return slice.
 		s := reflect.ValueOf(loaded)
 		for i := 0; i < s.Len(); i++ {
-			r = append(r, convertToSliceOfResources(s.Index(i).Interface())...)
+			r = append(r, convertToSliceOfResourceObjects(s.Index(i).Interface())...)
 		}
 	case reflect.Struct:
 		// This is a resource or resource list.  If a resource, add to our return
@@ -76,10 +76,10 @@ func convertToSliceOfResources(loaded interface{}) []unversioned.Resource {
 		if strings.HasSuffix(lr.GetTypeMetadata().Kind, "List") {
 			items := reflect.ValueOf(loaded).Elem().FieldByName("Items")
 			for i := 0; i < items.Len(); i++ {
-				r = append(r, items.Index(i).Interface().(unversioned.Resource))
+				r = append(r, items.Index(i).Interface().(unversioned.ResourceObject))
 			}
 		} else {
-			r = append(r, lr)
+			r = append(r, lr.(unversioned.ResourceObject))
 		}
 	case reflect.Ptr:
 		// This is a resource or resource list.  If a resource, add to our return
@@ -88,10 +88,10 @@ func convertToSliceOfResources(loaded interface{}) []unversioned.Resource {
 		if strings.HasSuffix(lr.GetTypeMetadata().Kind, "List") {
 			items := reflect.ValueOf(loaded).Elem().FieldByName("Items")
 			for i := 0; i < items.Len(); i++ {
-				r = append(r, items.Index(i).Interface().(unversioned.Resource))
+				r = append(r, items.Index(i).Interface().(unversioned.ResourceObject))
 			}
 		} else {
-			r = append(r, lr)
+			r = append(r, lr.(unversioned.ResourceObject))
 		}
 	default:
 		panic(errors.New(fmt.Sprintf("unhandled type %v converting to resource slice",
@@ -102,8 +102,8 @@ func convertToSliceOfResources(loaded interface{}) []unversioned.Resource {
 	return r
 }
 
-// getResourceFromArguments returns a resource instance from the command line arguments.
-func getResourceFromArguments(args map[string]interface{}) (unversioned.Resource, error) {
+// getResourceObjectFromArguments returns a resource instance from the command line arguments.
+func getResourceObjectFromArguments(args map[string]interface{}) (unversioned.ResourceObject, error) {
 	kind := args["<KIND>"].(string)
 	name := argutils.ArgStringOrBlank(args, "<NAME>")
 	node := argutils.ArgStringOrBlank(args, "--node")
@@ -191,7 +191,7 @@ type commandResults struct {
 	singleKind string
 
 	// The results returned from each invocation
-	resources []unversioned.Resource
+	resources []unversioned.ResourceObject
 
 	// The Calico API client used for the requests (useful if required
 	// again).
@@ -209,7 +209,7 @@ type commandResults struct {
 func executeConfigCommand(args map[string]interface{}, action action) commandResults {
 	var r interface{}
 	var err error
-	var resources []unversioned.Resource
+	var resources []unversioned.ResourceObject
 
 	log.Info("Executing config command")
 
@@ -220,8 +220,8 @@ func executeConfigCommand(args map[string]interface{}, action action) commandRes
 			return commandResults{err: err, fileInvalid: true}
 		}
 
-		resources = convertToSliceOfResources(r)
-	} else if r, err := getResourceFromArguments(args); err != nil {
+		resources = convertToSliceOfResourceObjects(r)
+	} else if r, err := getResourceObjectFromArguments(args); err != nil {
 		// Filename is not specific so extract the resource from the arguments.  This
 		// is only useful for delete and get functions - but we don't need to check that
 		// here since the command syntax requires a filename for the other resource
@@ -230,7 +230,7 @@ func executeConfigCommand(args map[string]interface{}, action action) commandRes
 	} else {
 		// We extracted a single resource type with identifiers from the CLI, convert to
 		// a list for simpler handling.
-		resources = []unversioned.Resource{r}
+		resources = []unversioned.ResourceObject{r}
 	}
 
 	if len(resources) == 0 {
@@ -280,12 +280,12 @@ func executeConfigCommand(args map[string]interface{}, action action) commandRes
 	// Now execute the command on each resource in order, exiting as soon as we hit an
 	// error.
 	for _, r := range resources {
-		r, err = executeResourceAction(ac, r)
+		rs, err := executeResourceAction(ac, r)
 		if err != nil {
 			results.err = err
 			break
 		}
-		results.resources = append(results.resources, r)
+		results.resources = append(results.resources, rs...)
 		results.numHandled = results.numHandled + 1
 	}
 
@@ -294,10 +294,11 @@ func executeConfigCommand(args map[string]interface{}, action action) commandRes
 
 // execureResourceAction fans out the specific resource action to the appropriate method
 // on the ResourceManager for the specific resource.
-func executeResourceAction(ac actionConfig, resource unversioned.Resource) (unversioned.Resource, error) {
+func executeResourceAction(ac actionConfig, resource unversioned.ResourceObject) ([]unversioned.ResourceObject, error) {
 	rm := resourcemgr.GetResourceManager(resource)
 	var err error
 	var resourceOut unversioned.Resource
+	var resourceObjects []unversioned.ResourceObject
 
 	switch ac.action {
 	case actionApply:
@@ -322,10 +323,12 @@ func executeResourceAction(ac actionConfig, resource unversioned.Resource) (unve
 			skip = ac.skipNotExists
 		}
 		if skip {
-			resourceOut = resource
+			resourceObjects = []unversioned.ResourceObject{resource}
 			err = nil
 		}
+	} else {
+		resourceObjects = convertToSliceOfResourceObjects(resourceOut)
 	}
 
-	return resourceOut, err
+	return resourceObjects, err
 }