Merge pull request #138 from wallyworld/robust-secret-import

#138

Secrets has fields like "auto-prune", "rotate-policy" which can be empty but which would then cause the import to panic. 
Imports from 3.1 (which didn't have auto-prune) to 3.4 would fail.

This PR adds checks for these fields to avoid the panic, and also defensively does the same for some other similar fields in the attribute map. This will fix the missing "auto-prune" issue and avoid accidental breakage in the future. Even though the checks for the coercion to []interface{} are strictly not needed, being explicit helps readability.

Author: jujubot

secretremoteconsumers.go

@@ -79,7 +79,10 @@ func importSecretRemoteConsumers(source map[string]interface{}, version int) ([]
 	if !ok {
 		return nil, errors.NotValidf("version %d", version)
 	}
-	sourceList := source["remote-consumers"].([]interface{})
+	sourceList, ok := source["remote-consumers"].([]interface{})
+	if !ok {
+		return nil, nil
+	}
 	return importSecretRemoteConsumersList(sourceList, importFunc)
 }
 

secrets.go

@@ -50,14 +50,14 @@ type secret struct {
 	Label_        string            `yaml:"label"`
 	RotatePolicy_ string            `yaml:"rotate-policy,omitempty"`
 	Owner_        string            `yaml:"owner"`
-	AutoPrune_    bool              `yaml:"auto-prune"`
+	AutoPrune_    bool              `yaml:"auto-prune,omitempty"`
 	Created_      time.Time         `yaml:"create-time"`
 	Updated_      time.Time         `yaml:"update-time"`
 	Revisions_    []*secretRevision `yaml:"revisions"`
 
-	ACL_             map[string]*secretAccess `yaml:"acl"`
-	Consumers_       []*secretConsumer        `yaml:"consumers"`
-	RemoteConsumers_ []*secretRemoteConsumer  `yaml:"remote-consumers"`
+	ACL_             map[string]*secretAccess `yaml:"acl,omitempty"`
+	Consumers_       []*secretConsumer        `yaml:"consumers,omitempty"`
+	RemoteConsumers_ []*secretRemoteConsumer  `yaml:"remote-consumers,omitempty"`
 
 	NextRotateTime_ *time.Time `yaml:"next-rotate-time,omitempty"`
 
@@ -352,6 +352,7 @@ func secretV1Fields() (schema.Fields, schema.Defaults) {
 		"rotate-policy":    schema.Omit,
 		"auto-prune":       schema.Omit,
 		"next-rotate-time": schema.Omit,
+		"acl":              schema.Omit,
 		"consumers":        schema.Omit,
 		"remote-consumers": schema.Omit,
 	}
@@ -372,14 +373,21 @@ func importSecret(source map[string]interface{}, importVersion int, fieldFunc fu
 		Version_:        int(valid["secret-version"].(int64)),
 		Description_:    valid["description"].(string),
 		Label_:          valid["label"].(string),
-		RotatePolicy_:   valid["rotate-policy"].(string),
-		AutoPrune_:      valid["auto-prune"].(bool),
 		Owner_:          valid["owner"].(string),
 		Created_:        valid["create-time"].(time.Time).UTC(),
 		Updated_:        valid["update-time"].(time.Time).UTC(),
 		NextRotateTime_: fieldToTimePtr(valid, "next-rotate-time"),
 	}
 
+	if policy, ok := valid["rotate-policy"].(string); ok {
+		secret.RotatePolicy_ = policy
+	}
+
+	// This should be in a v2 schema but it's already also in v1.
+	if autoPrune, ok := valid["auto-prune"].(bool); ok {
+		secret.AutoPrune_ = autoPrune
+	}
+
 	secretACL, err := importSecretAccess(valid, importVersion)
 	if err != nil {
 		return nil, errors.Trace(err)
@@ -456,7 +464,10 @@ func importSecretAccess(source map[string]interface{}, version int) (map[string]
 	if !ok {
 		return nil, errors.NotValidf("version %d", version)
 	}
-	sourceList := source["acl"].(map[interface{}]interface{})
+	sourceList, ok := source["acl"].(map[interface{}]interface{})
+	if !ok {
+		return nil, nil
+	}
 	return importSecretAccessMap(sourceList, importFunc)
 }
 
@@ -571,7 +582,10 @@ func importSecretConsumers(source map[string]interface{}, version int) ([]*secre
 	if !ok {
 		return nil, errors.NotValidf("version %d", version)
 	}
-	sourceList := source["consumers"].([]interface{})
+	sourceList, ok := source["consumers"].([]interface{})
+	if !ok {
+		return nil, nil
+	}
 	return importSecretConsumersList(sourceList, importFunc)
 }
 
@@ -759,7 +773,10 @@ func importSecretRevisions(source map[string]interface{}, version int) ([]*secre
 	if !ok {
 		return nil, errors.NotValidf("version %d", version)
 	}
-	sourceList := source["revisions"].([]interface{})
+	sourceList, ok := source["revisions"].([]interface{})
+	if !ok {
+		return nil, nil
+	}
 	return importSecretRevisionList(sourceList, importFunc)
 }
 

secrets_test.go

@@ -251,3 +251,52 @@ func (s *SecretsSerializationSuite) TestParsingSerializedData(c *gc.C) {
 	secret := s.exportImport(c, original, 1)
 	c.Assert(secret, jc.DeepEquals, original)
 }
+
+type oldSecret struct {
+	ID_          string            `yaml:"id"`
+	Version_     int               `yaml:"secret-version"`
+	Description_ string            `yaml:"description"`
+	Label_       string            `yaml:"label"`
+	Owner_       string            `yaml:"owner"`
+	Created_     time.Time         `yaml:"create-time"`
+	Updated_     time.Time         `yaml:"update-time"`
+	Revisions_   []*secretRevision `yaml:"revisions"`
+}
+
+type oldSecrets struct {
+	Version  int          `yaml:"version"`
+	Secrets_ []*oldSecret `yaml:"secrets"`
+}
+
+func (s *SecretsSerializationSuite) TestParsingSerializedDataNoAutoPrune(c *gc.C) {
+	args := testSecretArgs()
+	original := &oldSecret{
+		ID_:      args.ID,
+		Version_: args.Version,
+		Created_: args.Created.UTC(),
+		Updated_: args.Updated.UTC(),
+	}
+	initial := oldSecrets{
+		Version:  1,
+		Secrets_: []*oldSecret{original},
+	}
+
+	bytes, err := yaml.Marshal(initial)
+	c.Assert(err, jc.ErrorIsNil)
+
+	var source map[string]interface{}
+	err = yaml.Unmarshal(bytes, &source)
+	c.Assert(err, jc.ErrorIsNil)
+
+	secrets, err := importSecrets(source)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(secrets, gc.HasLen, 1)
+
+	c.Assert(secrets[0], jc.DeepEquals, &secret{
+		ID_:      original.ID_,
+		Version_: original.Version_,
+		Owner_:   original.Owner_,
+		Created_: original.Created_,
+		Updated_: original.Updated_,
+	})
+}