diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index a3c516695..1d1ef0d27 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -38,6 +38,7 @@ func getImageVisibility(s string) (images.ImageVisibility, error) { } // Allows construction of all supported fields from ListOpts +// The `input` map will be modified but is not reused further in the builder func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) *packer.MultiError { // fill each field in the ListOpts based on tag/type @@ -50,24 +51,29 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) // check the valid fields map and whether we can set this field if key, exists := validFields[fieldName]; exists { - if !vField.CanSet() { - multiErr.Errors = append(multiErr.Errors, fmt.Errorf("Unsettable field: %s", fieldName)) - continue - } // check that this key was provided by the user, then set the field and have compatible types if val, exists := input[key]; exists { + // non-settable field + if !vField.CanSet() { + multiErr.Errors = append(multiErr.Errors, fmt.Errorf("Unsettable field: %s", fieldName)) + + // remove key from input filters so we can go over them after + delete(input, key) + continue + } + switch key { case "owner", "name", "tags": - if valType := reflect.TypeOf(val); valType != vField.Type() { multiErr.Errors = append(multiErr.Errors, - fmt.Errorf("Invalid type '%v' for field %s", + fmt.Errorf("Invalid type '%v' for field %s (%s)", valType, + key, fieldName, )) - continue + break } vField.Set(reflect.ValueOf(val)) @@ -75,18 +81,27 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) visibility, err := getImageVisibility(val.(string)) if err != nil { multiErr.Errors = append(multiErr.Errors, err) - continue + break } vField.Set(reflect.ValueOf(visibility)) - default: - multiErr.Errors = append(multiErr.Errors, - fmt.Errorf("Unsupported filter key provided: %s", key)) } + + // remove key from input filters so we can go over them after + delete(input, key) } } } + // error any invalid filters + for key, value := range input { + multiErr.Errors = append(multiErr.Errors, fmt.Errorf("Invalid filter: %s: %v (type: %v)", + key, + value, + reflect.TypeOf(value), + )) + } + // Set defaults for status and member_status listOpts.Status = images.ImageStatusActive listOpts.MemberStatus = images.ImageMemberStatusAccepted diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 63854a1c5..71ffe3b37 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -39,7 +39,13 @@ func TestBuildImageFilter(t *testing.T) { "tags": []string{"prod", "ready"}, } - buildImageFilters(filters, &testOpts) + // copy of original filters to pass to build function + passedFilters := make(map[string]interface{}) + for k, v := range filters { + passedFilters[k] = v + } + + buildImageFilters(passedFilters, &testOpts) if testOpts.Limit != 0 { t.Errorf("Limit was parsed: %d", testOpts.Limit) @@ -66,6 +72,28 @@ func TestBuildImageFilter(t *testing.T) { } } +// This test case confirms that invalid filter input are caught and do not result in a panic +func TestInvalidFilterInput(t *testing.T) { + + testOpts := images.ListOpts{} + + filters := map[string]interface{}{ + "tags": "prod", // supposed to be a []string + "owner": 12345, // supposed to be a string + "invalid_field": 0, // not a valid field in ListOpts + } + + numFields := len(filters) + + multiErr := buildImageFilters(filters, &testOpts) + if len(multiErr.Errors) != numFields { + t.Errorf("Failed to catch all %d invalid types/fields in filters", numFields) + for _, err := range multiErr.Errors { + t.Log(err.Error()) + } + } +} + func TestApplyMostRecent(t *testing.T) { testSortOpts := images.ListOpts{ Name: "RHEL 7.0",