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.

Sign up for GitHub

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

helper/schema: validate subresources more effectively #997

Merged
merged 1 commit into from
Feb 18, 2015
Merged

helper/schema: validate subresources more effectively #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 6 additions & 12 deletions helper/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,19 +1007,13 @@ func (m schemaMap) validateObject(
}

// Detect any extra/unknown keys and report those as errors.
prefix := k + "."
for configK, _ := range c.Raw {
if k != "" {
if !strings.HasPrefix(configK, prefix) {
continue
raw, _ := c.Get(k)
if m, ok := raw.(map[string]interface{}); ok {
for subk, _ := range m {
if _, ok := schema[subk]; !ok {
es = append(es, fmt.Errorf(
"%s: invalid or unknown key: %s", k, subk))
}

configK = configK[len(prefix):]
}

if _, ok := schema[configK]; !ok {
es = append(es, fmt.Errorf(
"%s: invalid or unknown key: %s", k, configK))
}
}

Expand Down
93 changes: 78 additions & 15 deletions helper/schema/schema_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2484,7 +2484,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Warn bool
Err bool
}{
// Good
// #0 Good
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2500,7 +2500,7 @@ func TestSchemaMap_Validate(t *testing.T) {
},
},

// Good, because the var is not set and that error will come elsewhere
// #1 Good, because the var is not set and that error will come elsewhere
Copy link
Contributor

Choose a reason for hiding this comment

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

Noooooo shoulda just made them K/V with test names if you were already touching everything. 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha it was a lot easier to just do this.

{
Schema: map[string]*Schema{
"size": &Schema{
Expand All @@ -2518,7 +2518,7 @@ func TestSchemaMap_Validate(t *testing.T) {
},
},

// Required field not set
// #2 Required field not set
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2532,7 +2532,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Invalid type
// #3 Invalid type
{
Schema: map[string]*Schema{
"port": &Schema{
Expand All @@ -2548,6 +2548,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// #4
{
Schema: map[string]*Schema{
"user_data": &Schema{
Expand All @@ -2567,7 +2568,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Bad type, interpolated
// #5 Bad type, interpolated
{
Schema: map[string]*Schema{
"size": &Schema{
Expand All @@ -2587,7 +2588,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Required but has DefaultFunc
// #6 Required but has DefaultFunc
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2602,7 +2603,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Config: nil,
},

// Required but has DefaultFunc return nil
// #7 Required but has DefaultFunc return nil
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2619,7 +2620,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Optional sub-resource
// #8 Optional sub-resource
{
Schema: map[string]*Schema{
"ingress": &Schema{
Expand All @@ -2640,7 +2641,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: false,
},

// Not a list
// #9 Not a list
{
Schema: map[string]*Schema{
"ingress": &Schema{
Expand All @@ -2663,7 +2664,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Required sub-resource field
// #10 Required sub-resource field
{
Schema: map[string]*Schema{
"ingress": &Schema{
Expand All @@ -2688,7 +2689,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Good sub-resource
// #11 Good sub-resource
{
Schema: map[string]*Schema{
"ingress": &Schema{
Expand Down Expand Up @@ -2716,7 +2717,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: false,
},

// Invalid/unknown field
// #12 Invalid/unknown field
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2734,7 +2735,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Computed field set
// #13 Computed field set
{
Schema: map[string]*Schema{
"availability_zone": &Schema{
Expand All @@ -2750,7 +2751,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Not a set
// #14 Not a set
{
Schema: map[string]*Schema{
"ports": &Schema{
Expand All @@ -2770,7 +2771,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// Maps
// #15 Maps
{
Schema: map[string]*Schema{
"user_data": &Schema{
Expand All @@ -2786,6 +2787,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// #16
{
Schema: map[string]*Schema{
"user_data": &Schema{
Expand All @@ -2803,6 +2805,7 @@ func TestSchemaMap_Validate(t *testing.T) {
},
},

// #17
{
Schema: map[string]*Schema{
"user_data": &Schema{
Expand All @@ -2818,6 +2821,7 @@ func TestSchemaMap_Validate(t *testing.T) {
},
},

// #18
{
Schema: map[string]*Schema{
"user_data": &Schema{
Expand All @@ -2835,6 +2839,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: true,
},

// #19
{
Schema: map[string]*Schema{
"security_groups": &Schema{
Expand All @@ -2856,6 +2861,7 @@ func TestSchemaMap_Validate(t *testing.T) {
Err: false,
},

// #20
{
Schema: map[string]*Schema{
"security_groups": &Schema{
Expand All @@ -2873,6 +2879,63 @@ func TestSchemaMap_Validate(t *testing.T) {

Err: true,
},

// #21 Bad, subresource should not allow unknown elements
{
Schema: map[string]*Schema{
"ingress": &Schema{
Type: TypeList,
Optional: true,
Elem: &Resource{
Schema: map[string]*Schema{
"port": &Schema{
Type: TypeInt,
Required: true,
},
},
},
},
},

Config: map[string]interface{}{
"ingress": []interface{}{
map[string]interface{}{
"port": 80,
"other": "yes",
},
},
},

Err: true,
},

// #22 Bad, subresource should not allow invalid types
{
Schema: map[string]*Schema{
"ingress": &Schema{
Type: TypeList,
Optional: true,
Elem: &Resource{
Schema: map[string]*Schema{
"port": &Schema{
Type: TypeInt,
Required: true,
},
},
},
},
},

Config: map[string]interface{}{
"ingress": []interface{}{
map[string]interface{}{
"port": "bad",
},
},
},

Err: true,
},
}

for i, tc := range cases {
Expand Down
3 changes: 3 additions & 0 deletions terraform/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,9 @@ func (c *ResourceConfig) IsSet(k string) bool {
func (c *ResourceConfig) get(
k string, raw map[string]interface{}) (interface{}, bool) {
parts := strings.Split(k, ".")
if len(parts) == 1 && parts[0] == "" {
parts = nil
}

var current interface{} = raw
for _, part := range parts {
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL range nil is AOK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) That's why even the simplest PRs are useful.

Expand Down