From 33caed3531ecc79769781e26771e290ad3df735e Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Sat, 14 Jul 2018 00:25:26 -0400 Subject: [PATCH 01/22] Completed filters and most_recent processing using OpenStack imageservice API --- builder/openstack/image_query.go | 147 ++++++++++++++++++++ builder/openstack/run_config.go | 2 +- builder/openstack/step_run_source_server.go | 6 + builder/openstack/step_source_image_info.go | 87 ++++++++++++ 4 files changed, 241 insertions(+), 1 deletion(-) create mode 100644 builder/openstack/image_query.go create mode 100644 builder/openstack/step_source_image_info.go diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go new file mode 100644 index 000000000..5b2db0655 --- /dev/null +++ b/builder/openstack/image_query.go @@ -0,0 +1,147 @@ +package openstack + +import ( + "fmt" + "reflect" + "strings" + "time" + "strconv" + + "github.com/hashicorp/packer/packer" + "github.com/hashicorp/go-multierror" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" +) + +const ( + descendingSort = "desc" + createdAtKey = "created_at" +) + +// Retrieve the specific ImageDateFilter using the exported const from images +func getDateFilter(s string) (images.ImageDateFilter, error) { + filters := [...]images.ImageDateFilter{ + images.FilterGT, + images.FilterGTE, + images.FilterLT, + images.FilterLTE, + images.FilterNEQ, + images.FilterEQ, + } + + for _, filter := range filters { + if string(filter) == s { + return filter, nil + } + } + + return images.ImageDateFilter(nil), fmt.Errorf("No ImageDateFilter found for %s", s) +} + +// Allows construction of all fields from ListOpts using the "q" tags and +// type detection to set all fields within a provided ListOpts struct +func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *packer.MultiError { + + // fill each field in the ListOpts based on tag/type + metaOpts := reflect.ValueOf(listOpts).Elem() + + multiErr := packer.MultiError{} + + for i := 0; i < metaOpts.Type().NumField(); i++ { + vField := metaOpts.Field(i) + tField := metaOpts.Type().Field(i) + fieldName := tField.Name + key := metaOpts.Type().Field(i).Tag.Get("q") + + // get key from the map and set values if they exist + if val, exists := input[key]; exists && vField.CanSet() { + switch vField.Kind() { + + case reflect.Int64: + iVal, err := strconv.Atoi(val) + if err != nil { + multierror.Append(err, multiErr.Errors...) + } else { + vField.Set(reflect.ValueOf(iVal)) + } + + case reflect.String: + vField.Set(reflect.ValueOf(val)) + + case reflect.Slice: + typeOfSlice := reflect.TypeOf(vField).Elem() + fieldArray := reflect.MakeSlice(reflect.SliceOf(typeOfSlice), 0, 0) + for _, s := range strings.Split(val, ",") { + if len(s) > 0 { + fieldArray = reflect.Append(fieldArray, reflect.ValueOf(s)) + } + } + vField.Set(fieldArray) + + default: + multierror.Append( + fmt.Errorf("Unsupported struct type %s", vField.Type().Name), + multiErr.Errors...) + } + + } else if fieldName == reflect.TypeOf(images.ListOpts{}.CreatedAtQuery).Name() || + fieldName == reflect.TypeOf(images.ListOpts{}.UpdatedAtQuery).Name() { + // get ImageDateQuery from string and set to this field + query, err := dateToImageDateQuery(&key, &val) + if err != nil { + multierror.Append(err, multiErr.Errors...) + continue + } + vField.Set(reflect.ValueOf(query)) + } + } + + return &multiErr +} + +// Apply most recent filtering logic to ListOpts where user has filled fields. +// This does not check whether both are filled. Allow OpenStack to determine which to use. +// It is suggested that users use the newest sort field +// See https://developer.openstack.org/api-ref/image/v2/ +func applyMostRecent(listOpts *images.ListOpts) { + // apply to old sorting properties if user used them. This overwrites previous values? + if listOpts.SortDir == "" && listOpts.SortKey != "" { + listOpts.SortDir = descendingSort + listOpts.SortKey = createdAtKey + } + + // apply to new sorting property + if listOpts.Sort != "" { + listOpts.Sort = fmt.Sprintf("%s:%s,%s", createdAtKey, descendingSort, listOpts.Sort) + } else { + listOpts.Sort = fmt.Sprintf("%s:%s", createdAtKey, descendingSort) + } + + return +} + +// Converts a given date entry to ImageDateQuery for use in ListOpts +func dateToImageDateQuery(val *string, key *string) (*images.ImageDateQuery, error) { + q := images.ImageDateQuery{} + sep := ":" + entries := strings.Split(*val, sep) + + if len(entries) > 3 { + filter, err := getDateFilter(entries[0]) + if err != nil { + return nil, fmt.Errorf("Failed to parse date filter for %s", key) + } else { + q.Filter = filter + } + + date, err := time.Parse((*val)[len(entries[0]):], time.RFC3339) + if err != nil { + return nil, fmt.Errorf("Failed to parse date format for %s", key) + } else { + q.Date = date + } + + return &q, nil + } + + return nil, fmt.Errorf("Incorrect date query format for %s", key) +} diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index b98b65ea8..c96eb46bb 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -76,7 +76,7 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } if c.SourceImage == "" && c.SourceImageName == "" { - errs = append(errs, errors.New("Either a source_image or a source_image_name must be specified")) + errs = append(errs, errors.New("Either a source_image or a source_image_`name must be specified")) } else if len(c.SourceImage) > 0 && len(c.SourceImageName) > 0 { errs = append(errs, errors.New("Only a source_image or a source_image_name can be specified, not both.")) } diff --git a/builder/openstack/step_run_source_server.go b/builder/openstack/step_run_source_server.go index e56218467..c14061efc 100644 --- a/builder/openstack/step_run_source_server.go +++ b/builder/openstack/step_run_source_server.go @@ -76,6 +76,12 @@ func (s *StepRunSourceServer) Run(_ context.Context, state multistep.StateBag) m ServiceClient: computeClient, Metadata: s.InstanceMetadata, } + + // check if image filter returned a source image ID and replace + if imageID := state.Get("source_image").(string); imageID != "" { + serverOpts.ImageRef = imageID + } + var serverOptsExt servers.CreateOptsBuilder // Create root volume in the Block Storage service if required. diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go new file mode 100644 index 000000000..c3bf4486d --- /dev/null +++ b/builder/openstack/step_source_image_info.go @@ -0,0 +1,87 @@ +package openstack + +import ( + "log" + "fmt" + "context" + + + "github.com/hashicorp/packer/packer" + "github.com/hashicorp/packer/helper/multistep" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" + "github.com/gophercloud/gophercloud/pagination" +) + +type StepSourceImageInfo struct { + SourceImage string + SourceImageName string + ImageFilters ImageFilterOptions +} + +type ImageFilterOptions struct { + Filters map[string]string + MostRecent bool `mapstructure:"most_recent"` +} + +func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { + config := state.Get("config").(Config) + ui := state.Get("ui").(packer.Ui) + + client, err := config.computeV2Client() + + // if an ID is provided we skip the filter since that will return a single image + if s.SourceImage != "" { + state.Put("source_image", s.SourceImage) + return multistep.ActionContinue + } + + params := &images.ListOpts{} + + // build ListOpts from filters + if len(s.ImageFilters.Filters) > 0 { + err = buildImageFilters(s.ImageFilters.Filters, params) + if err != nil { + err := fmt.Errorf("Errors encountered in filter parsing.\n%s" + err.Error()) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + } + + if s.ImageFilters.MostRecent { + applyMostRecent(params) + } + + log.Printf("Using Image Filters %v", params) + image := &images.Image{} + err = images.List(client, params).EachPage(func (page pagination.Page) (bool, error) { + i, err := images.ExtractImages(page) + if err != nil { + return false, err + } + + switch len(i) { + case 0: + return false, fmt.Errorf("No image was found matching filters: %v", ) + case 1: + *image = i[0] + return true, nil + default: + return false, fmt.Errorf("Your query returned more than one result. Please try a more specific search, or set most_recent to true.") + } + + return true, nil + }) + + if err != nil { + err := fmt.Errorf("Error querying image: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + + ui.Message(fmt.Sprintf("Found Image ID: %s", image.ID)) + + state.Put("source_image", image) + return multistep.ActionContinue +} \ No newline at end of file From 810a602a1bbcb09029d26a283dd5dc8ecabaf1b1 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Sat, 14 Jul 2018 22:35:05 -0400 Subject: [PATCH 02/22] Added testing on query helper functions --- builder/openstack/image_query_test.go | 100 ++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 builder/openstack/image_query_test.go diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go new file mode 100644 index 000000000..90808bdaf --- /dev/null +++ b/builder/openstack/image_query_test.go @@ -0,0 +1,100 @@ +package openstack + +import ( + "testing" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" +) + +func TestGetImageFilter(t *testing.T) { + passedExpectedMap := map[string]images.ImageDateFilter{ + "gt": images.FilterGT, + "gte": images.FilterGTE, + "lt": images.FilterLT, + "lte": images.FilterLTE, + "neq": images.FilterNEQ, + "ne": images.FilterEQ, + } + + for passed, expected := range passedExpectedMap { + filter, err := getDateFilter(passed) + if err != nil { + t.Errorf("Passed %s, received error: %s", passed, err.Error()) + } else if filter != expected { + t.Errorf("Expected %s, got %s", string(expected), filter) + } + } +} + +func TestBuildImageFilter(t *testing.T) { + testOpts := images.ListOpts{} + + filters := map[string]string{ + "limit": "3", + "name": "Ubuntu 16.04", + "visibility": "public", + "image_status": "active", + "size_min": "0", + "sort": "created_at:desc", + } + + multiErr := buildImageFilters(filters, &testOpts) + + if multiErr != nil { + for _, err := range multiErr.Errors { + t.Error(err) + } + } + + if testOpts.Limit != 3 { + t.Errorf("Limit did not parse correctly") + } + + if testOpts.Name != filters["name"] { + t.Errorf("Name did not parse correctly") + } + + if testOpts.Visibility != images.ImageVisibility(filters["visibility"]) { + t.Errorf("Visibility did not parse correctly") + } + + if testOpts.Status != images.ImageStatus(filters["image_status"]) { + t.Errorf("Image status did not parse correctly") + } + + if testOpts.SizeMin != 0 { + t.Errorf("Size min did not parse correctly") + } + + if testOpts.Sort != filters["sort"] { + t.Errorf("Limit did not parse correctly") + } +} + +func TestApplyMostRecent(t *testing.T) { + testOpts := images.ListOpts{ + Name: "RHEL 7.0", + SizeMin: 0, + } + + applyMostRecent(&testOpts) + + if testOpts.Sort != "created_at:desc" { + t.Errorf("Error applying most recent filter: sort") + } + + if testOpts.SortDir != "desc" || testOpts.SortKey != "created_at" { + t.Errorf("Error applying most recent filter: sort_dir/sort_key") + } +} + +func TestDateToImageDateQuery(t *testing.T) { + tests := [][2]string{ + {"2006-01-02T15:04:05Z07:00", "created_at"}, + } + + for _, test := range tests { + if _, err := dateToImageDateQuery(&test[0], &test[1]); err != nil { + t.Error(err) + } + } +} \ No newline at end of file From a87c8fec389b9b04695d4c4b1fc114232f58357e Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Sat, 14 Jul 2018 23:03:04 -0400 Subject: [PATCH 03/22] Fixing up reflection issues on ListOpts builder --- builder/openstack/image_query.go | 9 +++++---- builder/openstack/image_query_test.go | 10 ++++++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index 5b2db0655..b86599801 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -19,7 +19,7 @@ const ( // Retrieve the specific ImageDateFilter using the exported const from images func getDateFilter(s string) (images.ImageDateFilter, error) { - filters := [...]images.ImageDateFilter{ + filters := []images.ImageDateFilter{ images.FilterGT, images.FilterGTE, images.FilterLT, @@ -34,7 +34,8 @@ func getDateFilter(s string) (images.ImageDateFilter, error) { } } - return images.ImageDateFilter(nil), fmt.Errorf("No ImageDateFilter found for %s", s) + var badFilter images.ImageDateFilter + return badFilter, fmt.Errorf("No ImageDateFilter found for %s", s) } // Allows construction of all fields from ListOpts using the "q" tags and @@ -121,7 +122,7 @@ func applyMostRecent(listOpts *images.ListOpts) { // Converts a given date entry to ImageDateQuery for use in ListOpts func dateToImageDateQuery(val *string, key *string) (*images.ImageDateQuery, error) { - q := images.ImageDateQuery{} + q := new(images.ImageDateQuery) sep := ":" entries := strings.Split(*val, sep) @@ -140,7 +141,7 @@ func dateToImageDateQuery(val *string, key *string) (*images.ImageDateQuery, err q.Date = date } - return &q, nil + return q, nil } return nil, fmt.Errorf("Incorrect date query format for %s", key) diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 90808bdaf..32f605a91 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -12,7 +12,7 @@ func TestGetImageFilter(t *testing.T) { "lt": images.FilterLT, "lte": images.FilterLTE, "neq": images.FilterNEQ, - "ne": images.FilterEQ, + "eq": images.FilterEQ, } for passed, expected := range passedExpectedMap { @@ -20,7 +20,7 @@ func TestGetImageFilter(t *testing.T) { if err != nil { t.Errorf("Passed %s, received error: %s", passed, err.Error()) } else if filter != expected { - t.Errorf("Expected %s, got %s", string(expected), filter) + t.Errorf("Expected %s, got %s", expected, filter) } } } @@ -53,11 +53,13 @@ func TestBuildImageFilter(t *testing.T) { t.Errorf("Name did not parse correctly") } - if testOpts.Visibility != images.ImageVisibility(filters["visibility"]) { + var visibility images.ImageVisibility = "public" + if testOpts.Visibility != visibility { t.Errorf("Visibility did not parse correctly") } - if testOpts.Status != images.ImageStatus(filters["image_status"]) { + var imageStatus images.ImageStatus = "active" + if testOpts.Status != imageStatus { t.Errorf("Image status did not parse correctly") } From 94018c691cced9f004c4ec8b9004c1b5ac7b8e5e Mon Sep 17 00:00:00 2001 From: tcarrio Date: Sun, 15 Jul 2018 23:35:02 -0400 Subject: [PATCH 04/22] Fixed step interface, added step to builder, passing all tests for new image_query.go --- builder/openstack/builder.go | 5 + builder/openstack/image_query.go | 119 ++++++++++++++++---- builder/openstack/image_query_test.go | 82 ++++++++++---- builder/openstack/run_config.go | 35 +++--- builder/openstack/step_source_image_info.go | 33 +++--- 5 files changed, 198 insertions(+), 76 deletions(-) diff --git a/builder/openstack/builder.go b/builder/openstack/builder.go index 2938d67c0..a2b34ddfa 100644 --- a/builder/openstack/builder.go +++ b/builder/openstack/builder.go @@ -86,6 +86,11 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe PrivateKeyFile: b.config.RunConfig.Comm.SSHPrivateKey, SSHAgentAuth: b.config.RunConfig.Comm.SSHAgentAuth, }, + &StepSourceImageInfo{ + SourceImage: b.config.SourceImage, + SourceImageName: b.config.SourceImageName, + ImageFilters: b.config.SourceImageFilters, + }, &StepCreateVolume{ UseBlockStorageVolume: b.config.UseBlockStorageVolume, SourceImage: b.config.SourceImage, diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index b86599801..2eb54eeb3 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -3,13 +3,13 @@ package openstack import ( "fmt" "reflect" + "strconv" "strings" "time" - "strconv" - "github.com/hashicorp/packer/packer" - "github.com/hashicorp/go-multierror" "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" + "github.com/hashicorp/go-multierror" + "github.com/hashicorp/packer/packer" ) const ( @@ -35,7 +35,47 @@ func getDateFilter(s string) (images.ImageDateFilter, error) { } var badFilter images.ImageDateFilter - return badFilter, fmt.Errorf("No ImageDateFilter found for %s", s) + return badFilter, fmt.Errorf("No valid ImageDateFilter found for %s", s) +} + +// Retrieve the specific ImageVisibility using the exported const from images +func getImageVisibility(s string) (images.ImageVisibility, error) { + visibilities := [...]images.ImageVisibility{ + images.ImageVisibilityPublic, + images.ImageVisibilityPrivate, + images.ImageVisibilityCommunity, + images.ImageVisibilityShared, + } + + for _, visibility := range visibilities { + if string(visibility) == s { + return visibility, nil + } + } + + var nilVisibility images.ImageVisibility + return nilVisibility, fmt.Errorf("No valid ImageVisilibility found for %s", s) +} + +// Retrieve the specific ImageVisibility using the exported const from images +func getImageStatus(s string) (images.ImageStatus, error) { + statuses := [...]images.ImageStatus{ + images.ImageStatusActive, + images.ImageStatusDeactivated, + images.ImageStatusDeleted, + images.ImageStatusPendingDelete, + images.ImageStatusQueued, + images.ImageStatusSaving, + } + + for _, status := range statuses { + if string(status) == s { + return status, nil + } + } + + var nilStatus images.ImageStatus + return nilStatus, fmt.Errorf("No valid ImageVisilibility found for %s", s) } // Allows construction of all fields from ListOpts using the "q" tags and @@ -43,7 +83,7 @@ func getDateFilter(s string) (images.ImageDateFilter, error) { func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *packer.MultiError { // fill each field in the ListOpts based on tag/type - metaOpts := reflect.ValueOf(listOpts).Elem() + metaOpts := reflect.Indirect(reflect.ValueOf(listOpts)) multiErr := packer.MultiError{} @@ -57,17 +97,46 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack if val, exists := input[key]; exists && vField.CanSet() { switch vField.Kind() { - case reflect.Int64: + // Handles integer types used in ListOpts + case reflect.Int64, reflect.Int: iVal, err := strconv.Atoi(val) if err != nil { multierror.Append(err, multiErr.Errors...) - } else { + continue + } + + if vField.Kind() == reflect.Int { vField.Set(reflect.ValueOf(iVal)) + } else { + var i64Val int64 + i64Val = int64(iVal) + vField.Set(reflect.ValueOf(i64Val)) } + // Handles string and types using string case reflect.String: - vField.Set(reflect.ValueOf(val)) + switch vField.Type() { + default: + vField.Set(reflect.ValueOf(val)) + + case reflect.TypeOf(images.ImageVisibility("")): + iv, err := getImageVisibility(val) + if err != nil { + multierror.Append(err, multiErr.Errors...) + continue + } + vField.Set(reflect.ValueOf(iv)) + + case reflect.TypeOf(images.ImageStatus("")): + is, err := getImageStatus(val) + if err != nil { + multierror.Append(err, multiErr.Errors...) + continue + } + vField.Set(reflect.ValueOf(is)) + } + // Generates slice of strings for Tags case reflect.Slice: typeOfSlice := reflect.TypeOf(vField).Elem() fieldArray := reflect.MakeSlice(reflect.SliceOf(typeOfSlice), 0, 0) @@ -80,18 +149,21 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack default: multierror.Append( - fmt.Errorf("Unsupported struct type %s", vField.Type().Name), + fmt.Errorf("Unsupported kind %s", vField.Kind()), multiErr.Errors...) } - } else if fieldName == reflect.TypeOf(images.ListOpts{}.CreatedAtQuery).Name() || - fieldName == reflect.TypeOf(images.ListOpts{}.UpdatedAtQuery).Name() { + // Handles ImageDateQuery types + } else if fieldName == reflect.TypeOf(listOpts.CreatedAtQuery).Name() || + fieldName == reflect.TypeOf(listOpts.UpdatedAtQuery).Name() { + // get ImageDateQuery from string and set to this field - query, err := dateToImageDateQuery(&key, &val) + query, err := dateToImageDateQuery(key, val) if err != nil { multierror.Append(err, multiErr.Errors...) continue } + vField.Set(reflect.ValueOf(query)) } } @@ -104,13 +176,12 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack // It is suggested that users use the newest sort field // See https://developer.openstack.org/api-ref/image/v2/ func applyMostRecent(listOpts *images.ListOpts) { - // apply to old sorting properties if user used them. This overwrites previous values? - if listOpts.SortDir == "" && listOpts.SortKey != "" { - listOpts.SortDir = descendingSort - listOpts.SortKey = createdAtKey - } + // Apply to old sorting properties if user used them. This overwrites previous values. + // The docs don't seem to mention more than one field being allowed here and how they would be + listOpts.SortDir = descendingSort + listOpts.SortKey = createdAtKey - // apply to new sorting property + // Apply to new sorting property. if listOpts.Sort != "" { listOpts.Sort = fmt.Sprintf("%s:%s,%s", createdAtKey, descendingSort, listOpts.Sort) } else { @@ -121,10 +192,10 @@ func applyMostRecent(listOpts *images.ListOpts) { } // Converts a given date entry to ImageDateQuery for use in ListOpts -func dateToImageDateQuery(val *string, key *string) (*images.ImageDateQuery, error) { +func dateToImageDateQuery(val string, key string) (*images.ImageDateQuery, error) { q := new(images.ImageDateQuery) sep := ":" - entries := strings.Split(*val, sep) + entries := strings.Split(val, sep) if len(entries) > 3 { filter, err := getDateFilter(entries[0]) @@ -134,9 +205,13 @@ func dateToImageDateQuery(val *string, key *string) (*images.ImageDateQuery, err q.Filter = filter } - date, err := time.Parse((*val)[len(entries[0]):], time.RFC3339) + dateSubstr := val[len(entries[0])+1:] + date, err := time.Parse(time.RFC3339, dateSubstr) if err != nil { - return nil, fmt.Errorf("Failed to parse date format for %s", key) + return nil, fmt.Errorf("Failed to parse date format for %s.\nDate: %s.\nError: %s", + key, + dateSubstr, + err.Error()) } else { q.Date = date } diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 32f605a91..0876ccf09 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -2,17 +2,19 @@ package openstack import ( "testing" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" + "github.com/mitchellh/mapstructure" ) func TestGetImageFilter(t *testing.T) { passedExpectedMap := map[string]images.ImageDateFilter{ - "gt": images.FilterGT, - "gte": images.FilterGTE, - "lt": images.FilterLT, - "lte": images.FilterLTE, - "neq": images.FilterNEQ, - "eq": images.FilterEQ, + "gt": images.FilterGT, + "gte": images.FilterGTE, + "lt": images.FilterLT, + "lte": images.FilterLTE, + "neq": images.FilterNEQ, + "eq": images.FilterEQ, } for passed, expected := range passedExpectedMap { @@ -29,12 +31,12 @@ func TestBuildImageFilter(t *testing.T) { testOpts := images.ListOpts{} filters := map[string]string{ - "limit": "3", - "name": "Ubuntu 16.04", + "limit": "3", + "name": "Ubuntu 16.04", "visibility": "public", - "image_status": "active", - "size_min": "0", - "sort": "created_at:desc", + "status": "active", + "size_min": "0", + "sort": "created_at:desc", } multiErr := buildImageFilters(filters, &testOpts) @@ -46,11 +48,11 @@ func TestBuildImageFilter(t *testing.T) { } if testOpts.Limit != 3 { - t.Errorf("Limit did not parse correctly") + t.Errorf("Limit did not parse correctly: %d", testOpts.Limit) } if testOpts.Name != filters["name"] { - t.Errorf("Name did not parse correctly") + t.Errorf("Name did not parse correctly: %") } var visibility images.ImageVisibility = "public" @@ -60,7 +62,7 @@ func TestBuildImageFilter(t *testing.T) { var imageStatus images.ImageStatus = "active" if testOpts.Status != imageStatus { - t.Errorf("Image status did not parse correctly") + t.Errorf("Image status did not parse correctly: %s", testOpts.Status) } if testOpts.SizeMin != 0 { @@ -73,30 +75,64 @@ func TestBuildImageFilter(t *testing.T) { } func TestApplyMostRecent(t *testing.T) { - testOpts := images.ListOpts{ - Name: "RHEL 7.0", + testSortEmptyOpts := images.ListOpts{ + Name: "RHEL 7.0", + SizeMin: 0, + } + + testSortFilledOpts := images.ListOpts{ + Name: "Ubuntu 16.04", SizeMin: 0, + Sort: "tags:ubuntu", + } + + applyMostRecent(&testSortEmptyOpts) + + if testSortEmptyOpts.Sort != "created_at:desc" { + t.Errorf("Error applying most recent filter: sort") + } + + if testSortEmptyOpts.SortDir != "desc" || testSortEmptyOpts.SortKey != "created_at" { + t.Errorf("Error applying most recent filter: sort_dir/sort_key:\n{sort_dir: %s, sort_key: %s}", + testSortEmptyOpts.SortDir, testSortEmptyOpts.SortKey) } - applyMostRecent(&testOpts) + applyMostRecent(&testSortFilledOpts) - if testOpts.Sort != "created_at:desc" { + if testSortFilledOpts.Sort != "created_at:desc,tags:ubuntu" { t.Errorf("Error applying most recent filter: sort") } - if testOpts.SortDir != "desc" || testOpts.SortKey != "created_at" { - t.Errorf("Error applying most recent filter: sort_dir/sort_key") + if testSortFilledOpts.SortDir != "desc" || testSortFilledOpts.SortKey != "created_at" { + t.Errorf("Error applying most recent filter: sort_dir/sort_key:\n{sort_dir: %s, sort_key: %s}", + testSortFilledOpts.SortDir, testSortFilledOpts.SortKey) } } func TestDateToImageDateQuery(t *testing.T) { tests := [][2]string{ - {"2006-01-02T15:04:05Z07:00", "created_at"}, + {"gt:2012-11-01T22:08:41+00:00", "created_at"}, } for _, test := range tests { - if _, err := dateToImageDateQuery(&test[0], &test[1]); err != nil { + if _, err := dateToImageDateQuery(test[0], test[1]); err != nil { t.Error(err) } } -} \ No newline at end of file +} + +func TestImageFilterOptionsDecode(t *testing.T) { + opts := ImageFilterOptions{} + input := map[string]interface{}{ + "most_recent": true, + "filters": map[string]interface{}{ + "visibility": "protected", + "tag": "prod", + "name": "ubuntu 16.04", + }, + } + err := mapstructure.Decode(input, &opts) + if err != nil { + t.Error("Did not successfully generate ImageFilterOptions from %v. Contains %v", input, opts) + } +} diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index c96eb46bb..0db1110bd 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -18,21 +18,22 @@ type RunConfig struct { SSHInterface string `mapstructure:"ssh_interface"` SSHIPVersion string `mapstructure:"ssh_ip_version"` - SourceImage string `mapstructure:"source_image"` - SourceImageName string `mapstructure:"source_image_name"` - Flavor string `mapstructure:"flavor"` - AvailabilityZone string `mapstructure:"availability_zone"` - RackconnectWait bool `mapstructure:"rackconnect_wait"` - FloatingIPNetwork string `mapstructure:"floating_ip_network"` - FloatingIP string `mapstructure:"floating_ip"` - ReuseIPs bool `mapstructure:"reuse_ips"` - SecurityGroups []string `mapstructure:"security_groups"` - Networks []string `mapstructure:"networks"` - Ports []string `mapstructure:"ports"` - UserData string `mapstructure:"user_data"` - UserDataFile string `mapstructure:"user_data_file"` - InstanceName string `mapstructure:"instance_name"` - InstanceMetadata map[string]string `mapstructure:"instance_metadata"` + SourceImage string `mapstructure:"source_image"` + SourceImageName string `mapstructure:"source_image_name"` + SourceImageFilters ImageFilterOptions `mapstructure:"source_image_filter"` + Flavor string `mapstructure:"flavor"` + AvailabilityZone string `mapstructure:"availability_zone"` + RackconnectWait bool `mapstructure:"rackconnect_wait"` + FloatingIPNetwork string `mapstructure:"floating_ip_network"` + FloatingIP string `mapstructure:"floating_ip"` + ReuseIPs bool `mapstructure:"reuse_ips"` + SecurityGroups []string `mapstructure:"security_groups"` + Networks []string `mapstructure:"networks"` + Ports []string `mapstructure:"ports"` + UserData string `mapstructure:"user_data"` + UserDataFile string `mapstructure:"user_data_file"` + InstanceName string `mapstructure:"instance_name"` + InstanceMetadata map[string]string `mapstructure:"instance_metadata"` ConfigDrive bool `mapstructure:"config_drive"` @@ -75,8 +76,8 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } } - if c.SourceImage == "" && c.SourceImageName == "" { - errs = append(errs, errors.New("Either a source_image or a source_image_`name must be specified")) + if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters != nil { + errs = append(errs, errors.New("Either a source_image, a source_image_name, or must be specified")) } else if len(c.SourceImage) > 0 && len(c.SourceImageName) > 0 { errs = append(errs, errors.New("Only a source_image or a source_image_name can be specified, not both.")) } diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index c3bf4486d..116bdfa67 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -1,26 +1,25 @@ package openstack import ( - "log" - "fmt" "context" + "fmt" + "log" - - "github.com/hashicorp/packer/packer" - "github.com/hashicorp/packer/helper/multistep" "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/gophercloud/gophercloud/pagination" + "github.com/hashicorp/packer/helper/multistep" + "github.com/hashicorp/packer/packer" ) type StepSourceImageInfo struct { - SourceImage string - SourceImageName string - ImageFilters ImageFilterOptions + SourceImage string + SourceImageName string + ImageFilters ImageFilterOptions } type ImageFilterOptions struct { - Filters map[string]string - MostRecent bool `mapstructure:"most_recent"` + Filters map[string]string `mapstructure:"filters"` + MostRecent bool `mapstructure:"most_recent"` } func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { @@ -54,7 +53,7 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m log.Printf("Using Image Filters %v", params) image := &images.Image{} - err = images.List(client, params).EachPage(func (page pagination.Page) (bool, error) { + err = images.List(client, params).EachPage(func(page pagination.Page) (bool, error) { i, err := images.ExtractImages(page) if err != nil { return false, err @@ -62,12 +61,14 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m switch len(i) { case 0: - return false, fmt.Errorf("No image was found matching filters: %v", ) + return false, fmt.Errorf("No image was found matching filters: %v", params) case 1: *image = i[0] return true, nil default: - return false, fmt.Errorf("Your query returned more than one result. Please try a more specific search, or set most_recent to true.") + return false, fmt.Errorf( + "Your query returned more than one result. Please try a more specific search, or set most_recent to true. Search filters: %v", + params) } return true, nil @@ -84,4 +85,8 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m state.Put("source_image", image) return multistep.ActionContinue -} \ No newline at end of file +} + +func (s *StepSourceImageInfo) Cleanup(state multistep.StateBag) { + // No cleanup required for backout +} From 6dc71590eb16e97e76ccfec5c6f2e65d857fc3b9 Mon Sep 17 00:00:00 2001 From: tcarrio Date: Mon, 16 Jul 2018 01:29:28 -0400 Subject: [PATCH 05/22] Updated OpenStack Builder docs and tests --- builder/openstack/image_query_test.go | 6 +-- .../source/docs/builders/openstack.html.md | 37 ++++++++++++++++++- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 0876ccf09..c9c9c92e1 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -52,7 +52,7 @@ func TestBuildImageFilter(t *testing.T) { } if testOpts.Name != filters["name"] { - t.Errorf("Name did not parse correctly: %") + t.Errorf("Name did not parse correctly: %s", filters["name"]) } var visibility images.ImageVisibility = "public" @@ -66,11 +66,11 @@ func TestBuildImageFilter(t *testing.T) { } if testOpts.SizeMin != 0 { - t.Errorf("Size min did not parse correctly") + t.Errorf("Size min did not parse correctly: %s", filters["size_min"]) } if testOpts.Sort != filters["sort"] { - t.Errorf("Limit did not parse correctly") + t.Errorf("Sort did not parse correctly: %s", filters["sort"]) } } diff --git a/website/source/docs/builders/openstack.html.md b/website/source/docs/builders/openstack.html.md index a05ce3d67..4163d90b5 100644 --- a/website/source/docs/builders/openstack.html.md +++ b/website/source/docs/builders/openstack.html.md @@ -153,7 +153,7 @@ builder. Defaults to false. - `region` (string) - The name of the region, such as "DFW", in which to - launch the server to create the AMI. If not specified, Packer will use the + launch the server to create the image. If not specified, Packer will use the environment variable `OS_REGION_NAME`, if set. - `reuse_ips` (boolean) - Whether or not to attempt to reuse existing @@ -166,6 +166,41 @@ builder. - `security_groups` (array of strings) - A list of security groups by name to add to this instance. +- `source_image_filter` (object) - Filters used to populate filter options. + Example: + + ``` json + { + "source_image_filter": { + "filters": { + "name": "ubuntu-16.04*", + "visibility": "protected", + "owner": "d1a588cf4b0743344508dc145649372d1", + "tag": "prod" + }, + "most_recent": true + } + } + ``` + + This selects the most recent production Ubuntu 16.04 shared to you by the given owner. + NOTE: This will fail unless *exactly* one image is returned. In the above + example, `most_recent` will cause this to succeed by selecting the newest image. + + - `filters` (map of strings) - filters used to select a `source_image`. + NOTE: This will fail unless *exactly* one image is returned. + Any filter described in the docs for [ImageService](https://developer.openstack.org/api-ref/image/v2/) + is valid. + + - `most_recent` (boolean) - Selects the newest created image when true. + This is most useful for selecting a daily distro build. + + You may set this in place of `source_image` or in conjunction with it. If you + set this in conjunction with `source_image`, the `source_image` will be added to + the filter. The provided `source_image` must meet all of the filtering criteria + provided in `source_image_filter`; this pins the image returned by the filter, + but will cause Packer to fail if the `source_image` does not exist. + - `ssh_interface` (string) - The type of interface to connect via SSH. Values useful for Rackspace are "public" or "private", and the default behavior is to connect via whichever is returned first from the OpenStack API. From 3a6ab0fc0ed5de502cc333ccc5c0ab5317512b2a Mon Sep 17 00:00:00 2001 From: tcarrio Date: Mon, 16 Jul 2018 01:51:14 -0400 Subject: [PATCH 06/22] Updated test logic and Error->Errof for formatted output. --- builder/openstack/image_query_test.go | 2 +- builder/openstack/run_config.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index c9c9c92e1..1263669cf 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -133,6 +133,6 @@ func TestImageFilterOptionsDecode(t *testing.T) { } err := mapstructure.Decode(input, &opts) if err != nil { - t.Error("Did not successfully generate ImageFilterOptions from %v. Contains %v", input, opts) + t.Errorf("Did not successfully generate ImageFilterOptions from %v. Contains %v", input, opts) } } diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index 0db1110bd..1520369f5 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -76,7 +76,7 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } } - if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters != nil { + if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters == nil { errs = append(errs, errors.New("Either a source_image, a source_image_name, or must be specified")) } else if len(c.SourceImage) > 0 && len(c.SourceImageName) > 0 { errs = append(errs, errors.New("Only a source_image or a source_image_name can be specified, not both.")) From e776ad51a97150ed40087fe8b78b49e69df2d4e0 Mon Sep 17 00:00:00 2001 From: tcarrio Date: Mon, 16 Jul 2018 01:56:49 -0400 Subject: [PATCH 07/22] Removing statement from old image erroring logic --- builder/openstack/step_source_image_info.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 116bdfa67..682bdacde 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -70,8 +70,6 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m "Your query returned more than one result. Please try a more specific search, or set most_recent to true. Search filters: %v", params) } - - return true, nil }) if err != nil { From de9999ecb9ab61ccdfdc9278c4f753ff844f3e9e Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Mon, 16 Jul 2018 13:00:02 -0400 Subject: [PATCH 08/22] Updated tag to slice, docs, comments, only active images, source_image_name supercedes filter name --- builder/openstack/image_query.go | 35 +++++-------------- builder/openstack/image_query_test.go | 8 +++-- builder/openstack/run_config.go | 2 +- builder/openstack/step_source_image_info.go | 10 ++++-- .../source/docs/builders/openstack.html.md | 6 ++-- 5 files changed, 26 insertions(+), 35 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index 2eb54eeb3..91cfb612e 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -59,19 +59,9 @@ func getImageVisibility(s string) (images.ImageVisibility, error) { // Retrieve the specific ImageVisibility using the exported const from images func getImageStatus(s string) (images.ImageStatus, error) { - statuses := [...]images.ImageStatus{ - images.ImageStatusActive, - images.ImageStatusDeactivated, - images.ImageStatusDeleted, - images.ImageStatusPendingDelete, - images.ImageStatusQueued, - images.ImageStatusSaving, - } - - for _, status := range statuses { - if string(status) == s { - return status, nil - } + activeStatus := images.ImageStatusActive + if string(activeStatus) == s { + return activeStatus, nil } var nilStatus images.ImageStatus @@ -80,7 +70,7 @@ func getImageStatus(s string) (images.ImageStatus, error) { // Allows construction of all fields from ListOpts using the "q" tags and // type detection to set all fields within a provided ListOpts struct -func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *packer.MultiError { +func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) *packer.MultiError { // fill each field in the ListOpts based on tag/type metaOpts := reflect.Indirect(reflect.ValueOf(listOpts)) @@ -99,7 +89,7 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack // Handles integer types used in ListOpts case reflect.Int64, reflect.Int: - iVal, err := strconv.Atoi(val) + iVal, err := strconv.Atoi(val.(string)) if err != nil { multierror.Append(err, multiErr.Errors...) continue @@ -120,7 +110,7 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack vField.Set(reflect.ValueOf(val)) case reflect.TypeOf(images.ImageVisibility("")): - iv, err := getImageVisibility(val) + iv, err := getImageVisibility(val.(string)) if err != nil { multierror.Append(err, multiErr.Errors...) continue @@ -128,7 +118,7 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack vField.Set(reflect.ValueOf(iv)) case reflect.TypeOf(images.ImageStatus("")): - is, err := getImageStatus(val) + is, err := getImageStatus(val.(string)) if err != nil { multierror.Append(err, multiErr.Errors...) continue @@ -138,14 +128,7 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack // Generates slice of strings for Tags case reflect.Slice: - typeOfSlice := reflect.TypeOf(vField).Elem() - fieldArray := reflect.MakeSlice(reflect.SliceOf(typeOfSlice), 0, 0) - for _, s := range strings.Split(val, ",") { - if len(s) > 0 { - fieldArray = reflect.Append(fieldArray, reflect.ValueOf(s)) - } - } - vField.Set(fieldArray) + vField.Set(reflect.ValueOf(val)) default: multierror.Append( @@ -158,7 +141,7 @@ func buildImageFilters(input map[string]string, listOpts *images.ListOpts) *pack fieldName == reflect.TypeOf(listOpts.UpdatedAtQuery).Name() { // get ImageDateQuery from string and set to this field - query, err := dateToImageDateQuery(key, val) + query, err := dateToImageDateQuery(key, val.(string)) if err != nil { multierror.Append(err, multiErr.Errors...) continue diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 1263669cf..d8b24feca 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -30,7 +30,7 @@ func TestGetImageFilter(t *testing.T) { func TestBuildImageFilter(t *testing.T) { testOpts := images.ListOpts{} - filters := map[string]string{ + filters := map[string]interface{}{ "limit": "3", "name": "Ubuntu 16.04", "visibility": "public", @@ -127,12 +127,14 @@ func TestImageFilterOptionsDecode(t *testing.T) { "most_recent": true, "filters": map[string]interface{}{ "visibility": "protected", - "tag": "prod", + "tag": []string{"prod","ready"}, "name": "ubuntu 16.04", }, } err := mapstructure.Decode(input, &opts) if err != nil { - t.Errorf("Did not successfully generate ImageFilterOptions from %v. Contains %v", input, opts) + t.Errorf("Did not successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) + } else { + t.Logf("Successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) } } diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index 1520369f5..b7d773743 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -77,7 +77,7 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters == nil { - errs = append(errs, errors.New("Either a source_image, a source_image_name, or must be specified")) + errs = append(errs, errors.New("Either a source_image, a source_image_name, or source_image_filter must be specified")) } else if len(c.SourceImage) > 0 && len(c.SourceImageName) > 0 { errs = append(errs, errors.New("Only a source_image or a source_image_name can be specified, not both.")) } diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 682bdacde..b5f6e20f9 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -18,7 +18,7 @@ type StepSourceImageInfo struct { } type ImageFilterOptions struct { - Filters map[string]string `mapstructure:"filters"` + Filters map[string]interface{} `mapstructure:"filters"` MostRecent bool `mapstructure:"most_recent"` } @@ -28,7 +28,7 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m client, err := config.computeV2Client() - // if an ID is provided we skip the filter since that will return a single image + // if an ID is provided we skip the filter since that will return a single or no image if s.SourceImage != "" { state.Put("source_image", s.SourceImage) return multistep.ActionContinue @@ -47,6 +47,12 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m } } + // override the "name" provided in filters if "s.SourceImageName" was filled + if s.SourceImageName != "" { + params.Name = s.SourceImageName + } + + // apply "most_recent" logic to the sort fields and allow OpenStack to return the latest qualified image if s.ImageFilters.MostRecent { applyMostRecent(params) } diff --git a/website/source/docs/builders/openstack.html.md b/website/source/docs/builders/openstack.html.md index 4163d90b5..bdc70f34f 100644 --- a/website/source/docs/builders/openstack.html.md +++ b/website/source/docs/builders/openstack.html.md @@ -184,11 +184,11 @@ builder. ``` This selects the most recent production Ubuntu 16.04 shared to you by the given owner. - NOTE: This will fail unless *exactly* one image is returned. In the above - example, `most_recent` will cause this to succeed by selecting the newest image. + NOTE: This will fail unless *exactly* one image is returned, or `most_recent` is set to true. + In the example, `most_recent` will cause this to succeed by selecting the newest image. - `filters` (map of strings) - filters used to select a `source_image`. - NOTE: This will fail unless *exactly* one image is returned. + NOTE: This will fail unless *exactly* one image is returned, or `most_recent` is set to true. Any filter described in the docs for [ImageService](https://developer.openstack.org/api-ref/image/v2/) is valid. From c8fd53d60b9f69096d5208fce7d7452661c23b63 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Mon, 16 Jul 2018 13:26:09 -0400 Subject: [PATCH 09/22] make fmt --- builder/openstack/image_query_test.go | 2 +- builder/openstack/step_source_image_info.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index d8b24feca..f7eb0fa5e 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -127,7 +127,7 @@ func TestImageFilterOptionsDecode(t *testing.T) { "most_recent": true, "filters": map[string]interface{}{ "visibility": "protected", - "tag": []string{"prod","ready"}, + "tag": []string{"prod", "ready"}, "name": "ubuntu 16.04", }, } diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index b5f6e20f9..2f51b0aab 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -19,7 +19,7 @@ type StepSourceImageInfo struct { type ImageFilterOptions struct { Filters map[string]interface{} `mapstructure:"filters"` - MostRecent bool `mapstructure:"most_recent"` + MostRecent bool `mapstructure:"most_recent"` } func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { From e9e58e6b2b18f0909c1ec748e21cee3adb30c253 Mon Sep 17 00:00:00 2001 From: tcarrio Date: Mon, 16 Jul 2018 19:12:21 -0400 Subject: [PATCH 10/22] Tags field parsed from "tags" and updated test case --- builder/openstack/image_query.go | 14 ++++++++------ builder/openstack/image_query_test.go | 1 + 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index 91cfb612e..ff3a9c249 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -126,21 +126,16 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) vField.Set(reflect.ValueOf(is)) } - // Generates slice of strings for Tags - case reflect.Slice: - vField.Set(reflect.ValueOf(val)) - default: multierror.Append( fmt.Errorf("Unsupported kind %s", vField.Kind()), multiErr.Errors...) } - // Handles ImageDateQuery types } else if fieldName == reflect.TypeOf(listOpts.CreatedAtQuery).Name() || fieldName == reflect.TypeOf(listOpts.UpdatedAtQuery).Name() { + // Handles ImageDateQuery types - // get ImageDateQuery from string and set to this field query, err := dateToImageDateQuery(key, val.(string)) if err != nil { multierror.Append(err, multiErr.Errors...) @@ -148,6 +143,13 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) } vField.Set(reflect.ValueOf(query)) + + } else if fieldName == reflect.TypeOf(listOpts.Tags).Name() { + // Handles "tags" case and processes as slice of string + + if val, exists := input["tags"]; exists && vField.CanSet() { + vField.Set(reflect.ValueOf(val)) + } } } diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index f7eb0fa5e..accdf7e3d 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -37,6 +37,7 @@ func TestBuildImageFilter(t *testing.T) { "status": "active", "size_min": "0", "sort": "created_at:desc", + "tags": []string{"prod", "ready"}, } multiErr := buildImageFilters(filters, &testOpts) From db3d2682b54ee0c11bdca26da8188c91341512a6 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Wed, 18 Jul 2018 11:43:42 -0400 Subject: [PATCH 11/22] Updated allowed filters to tags, visibility, owner, and name. Test cases updated and passed --- builder/openstack/image_query.go | 189 ++++-------------- builder/openstack/image_query_test.go | 121 +++-------- .../source/docs/builders/openstack.html.md | 14 +- 3 files changed, 86 insertions(+), 238 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index ff3a9c249..134aa51b8 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -2,40 +2,20 @@ package openstack import ( "fmt" - "reflect" - "strconv" - "strings" - "time" - "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/packer/packer" + "reflect" ) const ( - descendingSort = "desc" - createdAtKey = "created_at" + mostRecentSort = "created_at:desc" ) -// Retrieve the specific ImageDateFilter using the exported const from images -func getDateFilter(s string) (images.ImageDateFilter, error) { - filters := []images.ImageDateFilter{ - images.FilterGT, - images.FilterGTE, - images.FilterLT, - images.FilterLTE, - images.FilterNEQ, - images.FilterEQ, - } - - for _, filter := range filters { - if string(filter) == s { - return filter, nil - } - } - - var badFilter images.ImageDateFilter - return badFilter, fmt.Errorf("No valid ImageDateFilter found for %s", s) +var validFields = map[string]string{ + "Name": "name", + "Visibility": "visibility", + "Owner": "owner", + "Tags": "tags", } // Retrieve the specific ImageVisibility using the exported const from images @@ -54,155 +34,72 @@ func getImageVisibility(s string) (images.ImageVisibility, error) { } var nilVisibility images.ImageVisibility - return nilVisibility, fmt.Errorf("No valid ImageVisilibility found for %s", s) -} - -// Retrieve the specific ImageVisibility using the exported const from images -func getImageStatus(s string) (images.ImageStatus, error) { - activeStatus := images.ImageStatusActive - if string(activeStatus) == s { - return activeStatus, nil - } - - var nilStatus images.ImageStatus - return nilStatus, fmt.Errorf("No valid ImageVisilibility found for %s", s) + return nilVisibility, fmt.Errorf("No valid ImageVisibility found for %s", s) } -// Allows construction of all fields from ListOpts using the "q" tags and -// type detection to set all fields within a provided ListOpts struct +// Allows construction of all supported fields from ListOpts func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) *packer.MultiError { // fill each field in the ListOpts based on tag/type metaOpts := reflect.Indirect(reflect.ValueOf(listOpts)) - multiErr := packer.MultiError{} for i := 0; i < metaOpts.Type().NumField(); i++ { vField := metaOpts.Field(i) - tField := metaOpts.Type().Field(i) - fieldName := tField.Name - key := metaOpts.Type().Field(i).Tag.Get("q") - - // get key from the map and set values if they exist - if val, exists := input[key]; exists && vField.CanSet() { - switch vField.Kind() { - - // Handles integer types used in ListOpts - case reflect.Int64, reflect.Int: - iVal, err := strconv.Atoi(val.(string)) - if err != nil { - multierror.Append(err, multiErr.Errors...) - continue - } + fieldName := metaOpts.Type().Field(i).Name - if vField.Kind() == reflect.Int { - vField.Set(reflect.ValueOf(iVal)) - } else { - var i64Val int64 - i64Val = int64(iVal) - vField.Set(reflect.ValueOf(i64Val)) - } + // 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 + } - // Handles string and types using string - case reflect.String: - switch vField.Type() { - default: - vField.Set(reflect.ValueOf(val)) + // check that this key was provided by the user, then set the field and have compatible types + if val, exists := input[key]; exists { - case reflect.TypeOf(images.ImageVisibility("")): - iv, err := getImageVisibility(val.(string)) - if err != nil { - multierror.Append(err, multiErr.Errors...) + 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", + valType, + fieldName, + )) continue } - vField.Set(reflect.ValueOf(iv)) + vField.Set(reflect.ValueOf(val)) - case reflect.TypeOf(images.ImageStatus("")): - is, err := getImageStatus(val.(string)) + case "visibility": + visibility, err := getImageVisibility(val.(string)) if err != nil { - multierror.Append(err, multiErr.Errors...) + multiErr.Errors = append(multiErr.Errors, err) continue } - vField.Set(reflect.ValueOf(is)) - } - - default: - multierror.Append( - fmt.Errorf("Unsupported kind %s", vField.Kind()), - multiErr.Errors...) - } - - } else if fieldName == reflect.TypeOf(listOpts.CreatedAtQuery).Name() || - fieldName == reflect.TypeOf(listOpts.UpdatedAtQuery).Name() { - // Handles ImageDateQuery types + vField.Set(reflect.ValueOf(visibility)) - query, err := dateToImageDateQuery(key, val.(string)) - if err != nil { - multierror.Append(err, multiErr.Errors...) - continue - } - - vField.Set(reflect.ValueOf(query)) - - } else if fieldName == reflect.TypeOf(listOpts.Tags).Name() { - // Handles "tags" case and processes as slice of string - - if val, exists := input["tags"]; exists && vField.CanSet() { - vField.Set(reflect.ValueOf(val)) + default: + multiErr.Errors = append(multiErr.Errors, + fmt.Errorf("Unsupported filter key provided: %s", key)) + } } } } + // Set defaults for status and member_status + listOpts.Status = images.ImageStatusActive + listOpts.MemberStatus = images.ImageMemberStatusAccepted + return &multiErr } // Apply most recent filtering logic to ListOpts where user has filled fields. -// This does not check whether both are filled. Allow OpenStack to determine which to use. -// It is suggested that users use the newest sort field // See https://developer.openstack.org/api-ref/image/v2/ func applyMostRecent(listOpts *images.ListOpts) { - // Apply to old sorting properties if user used them. This overwrites previous values. - // The docs don't seem to mention more than one field being allowed here and how they would be - listOpts.SortDir = descendingSort - listOpts.SortKey = createdAtKey - - // Apply to new sorting property. - if listOpts.Sort != "" { - listOpts.Sort = fmt.Sprintf("%s:%s,%s", createdAtKey, descendingSort, listOpts.Sort) - } else { - listOpts.Sort = fmt.Sprintf("%s:%s", createdAtKey, descendingSort) - } + // Sort isn't supported through our API so there should be no existing values. + // Overwriting .Sort is okay. + listOpts.Sort = mostRecentSort return } - -// Converts a given date entry to ImageDateQuery for use in ListOpts -func dateToImageDateQuery(val string, key string) (*images.ImageDateQuery, error) { - q := new(images.ImageDateQuery) - sep := ":" - entries := strings.Split(val, sep) - - if len(entries) > 3 { - filter, err := getDateFilter(entries[0]) - if err != nil { - return nil, fmt.Errorf("Failed to parse date filter for %s", key) - } else { - q.Filter = filter - } - - dateSubstr := val[len(entries[0])+1:] - date, err := time.Parse(time.RFC3339, dateSubstr) - if err != nil { - return nil, fmt.Errorf("Failed to parse date format for %s.\nDate: %s.\nError: %s", - key, - dateSubstr, - err.Error()) - } else { - q.Date = date - } - - return q, nil - } - - return nil, fmt.Errorf("Incorrect date query format for %s", key) -} diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index accdf7e3d..fd2251b6b 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -7,23 +7,22 @@ import ( "github.com/mitchellh/mapstructure" ) -func TestGetImageFilter(t *testing.T) { - passedExpectedMap := map[string]images.ImageDateFilter{ - "gt": images.FilterGT, - "gte": images.FilterGTE, - "lt": images.FilterLT, - "lte": images.FilterLTE, - "neq": images.FilterNEQ, - "eq": images.FilterEQ, +func TestImageFilterOptionsDecode(t *testing.T) { + opts := ImageFilterOptions{} + input := map[string]interface{}{ + "most_recent": true, + "filters": map[string]interface{}{ + "visibility": "protected", + "tag": []string{"prod", "ready"}, + "name": "ubuntu 16.04", + "owner": "tcarrio", + }, } - - for passed, expected := range passedExpectedMap { - filter, err := getDateFilter(passed) - if err != nil { - t.Errorf("Passed %s, received error: %s", passed, err.Error()) - } else if filter != expected { - t.Errorf("Expected %s, got %s", expected, filter) - } + err := mapstructure.Decode(input, &opts) + if err != nil { + t.Errorf("Did not successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) + } else { + t.Log("Successfully generate ImageFilterOptions.") } } @@ -35,107 +34,51 @@ func TestBuildImageFilter(t *testing.T) { "name": "Ubuntu 16.04", "visibility": "public", "status": "active", - "size_min": "0", + "size_min": "25", "sort": "created_at:desc", "tags": []string{"prod", "ready"}, } multiErr := buildImageFilters(filters, &testOpts) - if multiErr != nil { - for _, err := range multiErr.Errors { - t.Error(err) - } + if len(multiErr.Errors) > 0 { + t.Error(multiErr.Error()) } - if testOpts.Limit != 3 { - t.Errorf("Limit did not parse correctly: %d", testOpts.Limit) + if testOpts.Limit != 0 { + t.Errorf("Limit was parsed: %d", testOpts.Limit) } if testOpts.Name != filters["name"] { - t.Errorf("Name did not parse correctly: %s", filters["name"]) + t.Errorf("Name did not parse correctly: %s", testOpts.Name) } - var visibility images.ImageVisibility = "public" - if testOpts.Visibility != visibility { - t.Errorf("Visibility did not parse correctly") + if testOpts.Visibility != images.ImageVisibilityPublic { + t.Errorf("Visibility did not parse correctly: %v", testOpts.Visibility) } - var imageStatus images.ImageStatus = "active" - if testOpts.Status != imageStatus { + if testOpts.Status != images.ImageStatusActive { t.Errorf("Image status did not parse correctly: %s", testOpts.Status) } if testOpts.SizeMin != 0 { - t.Errorf("Size min did not parse correctly: %s", filters["size_min"]) + t.Errorf("Size min was parsed: %d", testOpts.SizeMin) } - if testOpts.Sort != filters["sort"] { - t.Errorf("Sort did not parse correctly: %s", filters["sort"]) + if len(testOpts.Sort) > 0 { + t.Errorf("Sort was parsed: %s", testOpts.Sort) } } func TestApplyMostRecent(t *testing.T) { - testSortEmptyOpts := images.ListOpts{ - Name: "RHEL 7.0", - SizeMin: 0, - } - - testSortFilledOpts := images.ListOpts{ - Name: "Ubuntu 16.04", - SizeMin: 0, - Sort: "tags:ubuntu", + testSortOpts := images.ListOpts{ + Name: "RHEL 7.0", + Tags: []string{"prod", "ready"}, } - applyMostRecent(&testSortEmptyOpts) + applyMostRecent(&testSortOpts) - if testSortEmptyOpts.Sort != "created_at:desc" { + if testSortOpts.Sort != "created_at:desc" { t.Errorf("Error applying most recent filter: sort") } - - if testSortEmptyOpts.SortDir != "desc" || testSortEmptyOpts.SortKey != "created_at" { - t.Errorf("Error applying most recent filter: sort_dir/sort_key:\n{sort_dir: %s, sort_key: %s}", - testSortEmptyOpts.SortDir, testSortEmptyOpts.SortKey) - } - - applyMostRecent(&testSortFilledOpts) - - if testSortFilledOpts.Sort != "created_at:desc,tags:ubuntu" { - t.Errorf("Error applying most recent filter: sort") - } - - if testSortFilledOpts.SortDir != "desc" || testSortFilledOpts.SortKey != "created_at" { - t.Errorf("Error applying most recent filter: sort_dir/sort_key:\n{sort_dir: %s, sort_key: %s}", - testSortFilledOpts.SortDir, testSortFilledOpts.SortKey) - } -} - -func TestDateToImageDateQuery(t *testing.T) { - tests := [][2]string{ - {"gt:2012-11-01T22:08:41+00:00", "created_at"}, - } - - for _, test := range tests { - if _, err := dateToImageDateQuery(test[0], test[1]); err != nil { - t.Error(err) - } - } -} - -func TestImageFilterOptionsDecode(t *testing.T) { - opts := ImageFilterOptions{} - input := map[string]interface{}{ - "most_recent": true, - "filters": map[string]interface{}{ - "visibility": "protected", - "tag": []string{"prod", "ready"}, - "name": "ubuntu 16.04", - }, - } - err := mapstructure.Decode(input, &opts) - if err != nil { - t.Errorf("Did not successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) - } else { - t.Logf("Successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) - } } diff --git a/website/source/docs/builders/openstack.html.md b/website/source/docs/builders/openstack.html.md index bdc70f34f..44f7effb8 100644 --- a/website/source/docs/builders/openstack.html.md +++ b/website/source/docs/builders/openstack.html.md @@ -176,7 +176,7 @@ builder. "name": "ubuntu-16.04*", "visibility": "protected", "owner": "d1a588cf4b0743344508dc145649372d1", - "tag": "prod" + "tag": ["prod", "ready"] }, "most_recent": true } @@ -189,8 +189,16 @@ builder. - `filters` (map of strings) - filters used to select a `source_image`. NOTE: This will fail unless *exactly* one image is returned, or `most_recent` is set to true. - Any filter described in the docs for [ImageService](https://developer.openstack.org/api-ref/image/v2/) - is valid. + Of the filters described in [ImageService](https://developer.openstack.org/api-ref/image/v2/), the following + are valid: + + - name (string) + + - owner (string) + + - tags (slice of strings) + + - visibility (string) - `most_recent` (boolean) - Selects the newest created image when true. This is most useful for selecting a daily distro build. From 036918b81bbba1b150b0cfe6ca002b12ce7c85b1 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Fri, 20 Jul 2018 07:51:00 -0400 Subject: [PATCH 12/22] Updated to comments in PR#6490 - Thanks @haxorof --- builder/openstack/image_query.go | 1 + builder/openstack/step_source_image_info.go | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index 134aa51b8..c6772a1aa 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -100,6 +100,7 @@ func applyMostRecent(listOpts *images.ListOpts) { // Sort isn't supported through our API so there should be no existing values. // Overwriting .Sort is okay. listOpts.Sort = mostRecentSort + listOpts.Limit = 1 return } diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 2f51b0aab..641eab1d9 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -26,7 +26,7 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m config := state.Get("config").(Config) ui := state.Get("ui").(packer.Ui) - client, err := config.computeV2Client() + client, err := config.imageV2Client() // if an ID is provided we skip the filter since that will return a single or no image if s.SourceImage != "" { @@ -38,11 +38,11 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m // build ListOpts from filters if len(s.ImageFilters.Filters) > 0 { - err = buildImageFilters(s.ImageFilters.Filters, params) - if err != nil { - err := fmt.Errorf("Errors encountered in filter parsing.\n%s" + err.Error()) + errs := buildImageFilters(s.ImageFilters.Filters, params) + if len(errs.Errors) > 0 { + err := fmt.Errorf("Errors encountered in filter parsing.\n%s" + errs.Error()) state.Put("error", err) - ui.Error(err.Error()) + ui.Error(errs.Error()) return multistep.ActionHalt } } @@ -87,7 +87,7 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m ui.Message(fmt.Sprintf("Found Image ID: %s", image.ID)) - state.Put("source_image", image) + state.Put("source_image", image.ID) return multistep.ActionContinue } From 8d98237a1573082d22d48a06e00ae42199e87ee8 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Fri, 20 Jul 2018 08:10:43 -0400 Subject: [PATCH 13/22] I should have a git-status alias to make fmt --- builder/openstack/image_query_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index fd2251b6b..d1731ecf2 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -41,7 +41,7 @@ func TestBuildImageFilter(t *testing.T) { multiErr := buildImageFilters(filters, &testOpts) - if len(multiErr.Errors) > 0 { + if len(multiErr.Errors) > 0 { t.Error(multiErr.Error()) } From 2d5d1890d4ac5b445f197d723c5eda3a0e5beaff Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Fri, 20 Jul 2018 08:29:53 -0400 Subject: [PATCH 14/22] Switching sort method --- builder/openstack/image_query.go | 6 ++++-- builder/openstack/image_query_test.go | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index c6772a1aa..b07e77510 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -8,7 +8,8 @@ import ( ) const ( - mostRecentSort = "created_at:desc" + mostRecentSortDir = "desc" + mostRecentSortKey = "created_at" ) var validFields = map[string]string{ @@ -99,7 +100,8 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) func applyMostRecent(listOpts *images.ListOpts) { // Sort isn't supported through our API so there should be no existing values. // Overwriting .Sort is okay. - listOpts.Sort = mostRecentSort + listOpts.SortKey = mostRecentSortKey + listOpts.SortDir = mostRecentSortDir listOpts.Limit = 1 return diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index d1731ecf2..58382750f 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -78,7 +78,7 @@ func TestApplyMostRecent(t *testing.T) { applyMostRecent(&testSortOpts) - if testSortOpts.Sort != "created_at:desc" { + if testSortOpts.SortKey != "created_at" || testSortOpts.SortDir != "desc" { t.Errorf("Error applying most recent filter: sort") } } From 41470628024fe9ffd0bac3344e8334b2c2f74b6b Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Fri, 20 Jul 2018 18:09:48 -0400 Subject: [PATCH 15/22] Internally handling most_recent logic. --- builder/openstack/image_query.go | 8 +++----- builder/openstack/step_source_image_info.go | 4 ++++ 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index b07e77510..1d16f6cc3 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -8,8 +8,7 @@ import ( ) const ( - mostRecentSortDir = "desc" - mostRecentSortKey = "created_at" + mostRecentSort = "created_at:desc" ) var validFields = map[string]string{ @@ -99,9 +98,8 @@ func buildImageFilters(input map[string]interface{}, listOpts *images.ListOpts) // See https://developer.openstack.org/api-ref/image/v2/ func applyMostRecent(listOpts *images.ListOpts) { // Sort isn't supported through our API so there should be no existing values. - // Overwriting .Sort is okay. - listOpts.SortKey = mostRecentSortKey - listOpts.SortDir = mostRecentSortDir + // Overwriting ListOpts.Sort is okay. + listOpts.Sort = mostRecentSort listOpts.Limit = 1 return diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 641eab1d9..029993e6c 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -72,6 +72,10 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m *image = i[0] return true, nil default: + if s.ImageFilters.MostRecent { + *image = i[0] + return true, nil + } return false, fmt.Errorf( "Your query returned more than one result. Please try a more specific search, or set most_recent to true. Search filters: %v", params) From fc19cd8d286e3acd7523c341e2d3857663ae7a70 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Sat, 21 Jul 2018 11:52:05 -0400 Subject: [PATCH 16/22] Updated limit logic, removed limiter, updated test cases and sort field used --- builder/openstack/image_query.go | 1 - builder/openstack/image_query_test.go | 12 ++++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go index 1d16f6cc3..a3c516695 100644 --- a/builder/openstack/image_query.go +++ b/builder/openstack/image_query.go @@ -100,7 +100,6 @@ func applyMostRecent(listOpts *images.ListOpts) { // Sort isn't supported through our API so there should be no existing values. // Overwriting ListOpts.Sort is okay. listOpts.Sort = mostRecentSort - listOpts.Limit = 1 return } diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go index 58382750f..63854a1c5 100644 --- a/builder/openstack/image_query_test.go +++ b/builder/openstack/image_query_test.go @@ -21,11 +21,11 @@ func TestImageFilterOptionsDecode(t *testing.T) { err := mapstructure.Decode(input, &opts) if err != nil { t.Errorf("Did not successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) - } else { - t.Log("Successfully generate ImageFilterOptions.") } } +// This test case confirms that only allowed fields will be set to values +// The checked values are non-nil for their target type func TestBuildImageFilter(t *testing.T) { testOpts := images.ListOpts{} @@ -39,11 +39,7 @@ func TestBuildImageFilter(t *testing.T) { "tags": []string{"prod", "ready"}, } - multiErr := buildImageFilters(filters, &testOpts) - - if len(multiErr.Errors) > 0 { - t.Error(multiErr.Error()) - } + buildImageFilters(filters, &testOpts) if testOpts.Limit != 0 { t.Errorf("Limit was parsed: %d", testOpts.Limit) @@ -78,7 +74,7 @@ func TestApplyMostRecent(t *testing.T) { applyMostRecent(&testSortOpts) - if testSortOpts.SortKey != "created_at" || testSortOpts.SortDir != "desc" { + if testSortOpts.Sort != "created_at:desc" { t.Errorf("Error applying most recent filter: sort") } } From 5ca5c037eb628f61067d0f0b24fb3c5ed6a358c8 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Wed, 1 Aug 2018 11:27:19 -0400 Subject: [PATCH 17/22] Updated filter parser to log all invalid values/fields provided by user --- builder/openstack/image_query.go | 37 +++++++++++++++++++-------- builder/openstack/image_query_test.go | 30 +++++++++++++++++++++- 2 files changed, 55 insertions(+), 12 deletions(-) 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", From c5fe1e9e34d55d110d975a2c31b649b8fa7265e8 Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Sun, 19 Aug 2018 18:21:32 -0400 Subject: [PATCH 18/22] Updated to @rickard-von-essen's code review suggestions, including: - filter build and error checking in Prepare stage (multiErr created in the original function will be returned to Prepare and appended, so all errors show). - source_image overrides source_image_filter. - Doc edit --- builder/openstack/builder.go | 7 +-- builder/openstack/run_config.go | 20 ++++++++ builder/openstack/step_source_image_info.go | 46 ++++--------------- .../source/docs/builders/openstack.html.md | 20 ++++---- 4 files changed, 45 insertions(+), 48 deletions(-) diff --git a/builder/openstack/builder.go b/builder/openstack/builder.go index a2b34ddfa..8e4c97ba6 100644 --- a/builder/openstack/builder.go +++ b/builder/openstack/builder.go @@ -87,9 +87,10 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe SSHAgentAuth: b.config.RunConfig.Comm.SSHAgentAuth, }, &StepSourceImageInfo{ - SourceImage: b.config.SourceImage, - SourceImageName: b.config.SourceImageName, - ImageFilters: b.config.SourceImageFilters, + SourceImage: b.config.SourceImage, + SourceImageName: b.config.SourceImageName, + SourceImageOpts: b.config.SourceImageOpts, + SourceMostRecent: b.config.SourceImageFilters.MostRecent, }, &StepCreateVolume{ UseBlockStorageVolume: b.config.UseBlockStorageVolume, diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index b7d773743..7df9a65d8 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/hashicorp/packer/common/uuid" "github.com/hashicorp/packer/helper/communicator" "github.com/hashicorp/packer/template/interpolate" @@ -21,6 +22,7 @@ type RunConfig struct { SourceImage string `mapstructure:"source_image"` SourceImageName string `mapstructure:"source_image_name"` SourceImageFilters ImageFilterOptions `mapstructure:"source_image_filter"` + SourceImageOpts images.ListOpts `mapstructure:""` Flavor string `mapstructure:"flavor"` AvailabilityZone string `mapstructure:"availability_zone"` RackconnectWait bool `mapstructure:"rackconnect_wait"` @@ -112,5 +114,23 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } } + // if neither ID or image name is provided outside the filter, build the filter + if len(c.SourceImage) == 0 && len(c.SourceImageName) == 0 { + params := &images.ListOpts{} + + if len(c.SourceImageFilters.Filters) > 0 { + filterErrs := buildImageFilters(c.SourceImageFilters.Filters, params) + if len(filterErrs.Errors) > 0 { + errs = append(errs, filterErrs.Errors...) + } + } + + if c.SourceImageFilters.MostRecent { + applyMostRecent(params) + } + + c.SourceImageOpts = *params + } + return errs } diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 029993e6c..035ef9d00 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -12,9 +12,10 @@ import ( ) type StepSourceImageInfo struct { - SourceImage string - SourceImageName string - ImageFilters ImageFilterOptions + SourceImage string + SourceImageName string + SourceImageOpts images.ListOpts + SourceMostRecent bool } type ImageFilterOptions struct { @@ -28,38 +29,9 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m client, err := config.imageV2Client() - // if an ID is provided we skip the filter since that will return a single or no image - if s.SourceImage != "" { - state.Put("source_image", s.SourceImage) - return multistep.ActionContinue - } - - params := &images.ListOpts{} - - // build ListOpts from filters - if len(s.ImageFilters.Filters) > 0 { - errs := buildImageFilters(s.ImageFilters.Filters, params) - if len(errs.Errors) > 0 { - err := fmt.Errorf("Errors encountered in filter parsing.\n%s" + errs.Error()) - state.Put("error", err) - ui.Error(errs.Error()) - return multistep.ActionHalt - } - } - - // override the "name" provided in filters if "s.SourceImageName" was filled - if s.SourceImageName != "" { - params.Name = s.SourceImageName - } - - // apply "most_recent" logic to the sort fields and allow OpenStack to return the latest qualified image - if s.ImageFilters.MostRecent { - applyMostRecent(params) - } - - log.Printf("Using Image Filters %v", params) + log.Printf("Using Image Filters %v", s.SourceImageOpts) image := &images.Image{} - err = images.List(client, params).EachPage(func(page pagination.Page) (bool, error) { + err = images.List(client, s.SourceImageOpts).EachPage(func(page pagination.Page) (bool, error) { i, err := images.ExtractImages(page) if err != nil { return false, err @@ -67,18 +39,18 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m switch len(i) { case 0: - return false, fmt.Errorf("No image was found matching filters: %v", params) + return false, fmt.Errorf("No image was found matching filters: %v", s.SourceImageOpts) case 1: *image = i[0] return true, nil default: - if s.ImageFilters.MostRecent { + if s.SourceMostRecent { *image = i[0] return true, nil } return false, fmt.Errorf( "Your query returned more than one result. Please try a more specific search, or set most_recent to true. Search filters: %v", - params) + s.SourceImageOpts) } }) diff --git a/website/source/docs/builders/openstack.html.md b/website/source/docs/builders/openstack.html.md index 44f7effb8..15aec123b 100644 --- a/website/source/docs/builders/openstack.html.md +++ b/website/source/docs/builders/openstack.html.md @@ -70,6 +70,11 @@ builder. is an alternative way of providing `source_image` and only either of them can be specified. +- `source_image_filter` (map) - The search filters for determining the base + image to use. This is an alternative way of providing `source_image` and + only one of these methods can be used. `source_image` will override the + filters. + - `username` or `user_id` (string) - The username or id used to connect to the OpenStack service. If not specified, Packer will use the environment variable `OS_USERNAME` or `OS_USERID`, if set. This is not required if @@ -176,7 +181,7 @@ builder. "name": "ubuntu-16.04*", "visibility": "protected", "owner": "d1a588cf4b0743344508dc145649372d1", - "tag": ["prod", "ready"] + "tags": ["prod", "ready"] }, "most_recent": true } @@ -185,7 +190,8 @@ builder. This selects the most recent production Ubuntu 16.04 shared to you by the given owner. NOTE: This will fail unless *exactly* one image is returned, or `most_recent` is set to true. - In the example, `most_recent` will cause this to succeed by selecting the newest image. + In the example of multiple returned images, `most_recent` will cause this to succeed by selecting + the newest image of the returned images. - `filters` (map of strings) - filters used to select a `source_image`. NOTE: This will fail unless *exactly* one image is returned, or `most_recent` is set to true. @@ -196,18 +202,16 @@ builder. - owner (string) - - tags (slice of strings) + - tags (array of strings) - visibility (string) - `most_recent` (boolean) - Selects the newest created image when true. This is most useful for selecting a daily distro build. - You may set this in place of `source_image` or in conjunction with it. If you - set this in conjunction with `source_image`, the `source_image` will be added to - the filter. The provided `source_image` must meet all of the filtering criteria - provided in `source_image_filter`; this pins the image returned by the filter, - but will cause Packer to fail if the `source_image` does not exist. + You may set use this in place of `source_image` If `source_image_filter` is provided + alongside `source_image`, the `source_image` will override the filter. The filter + will not be used in this case. - `ssh_interface` (string) - The type of interface to connect via SSH. Values useful for Rackspace are "public" or "private", and the default behavior is From bd1961b927804ba0b24bc7884cb2020cef639ed5 Mon Sep 17 00:00:00 2001 From: Rickard von Essen Date: Tue, 21 Aug 2018 12:46:42 +0200 Subject: [PATCH 19/22] Correctly fail if no Image matches filter --- builder/openstack/step_source_image_info.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index 035ef9d00..bdd0b25b5 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -38,15 +38,13 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m } switch len(i) { - case 0: - return false, fmt.Errorf("No image was found matching filters: %v", s.SourceImageOpts) case 1: *image = i[0] - return true, nil + return false, nil default: if s.SourceMostRecent { *image = i[0] - return true, nil + return false, nil } return false, fmt.Errorf( "Your query returned more than one result. Please try a more specific search, or set most_recent to true. Search filters: %v", @@ -54,6 +52,13 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m } }) + if image.ID == "" { + err := fmt.Errorf("No image was found matching filters: %v", s.SourceImageOpts) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + if err != nil { err := fmt.Errorf("Error querying image: %s", err) state.Put("error", err) From 3eb4151599f22b97de087a1dd06c1d9922c23d6d Mon Sep 17 00:00:00 2001 From: tcarrio Date: Tue, 21 Aug 2018 17:36:30 -0400 Subject: [PATCH 20/22] Fixed wildcard usage in docs --- website/source/docs/builders/openstack.html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/docs/builders/openstack.html.md b/website/source/docs/builders/openstack.html.md index 15aec123b..28566bd0d 100644 --- a/website/source/docs/builders/openstack.html.md +++ b/website/source/docs/builders/openstack.html.md @@ -178,7 +178,7 @@ builder. { "source_image_filter": { "filters": { - "name": "ubuntu-16.04*", + "name": "ubuntu-16.04", "visibility": "protected", "owner": "d1a588cf4b0743344508dc145649372d1", "tags": ["prod", "ready"] From 3b49caaf406755fcccad665c844984e469bcce62 Mon Sep 17 00:00:00 2001 From: Rickard von Essen Date: Wed, 22 Aug 2018 13:37:43 +0200 Subject: [PATCH 21/22] OpenStack: refactored how source_image_filter is handled to remove reflection --- builder/openstack/builder.go | 6 +- builder/openstack/image_query.go | 120 -------------------- builder/openstack/image_query_test.go | 108 ------------------ builder/openstack/run_config.go | 114 ++++++++++++++----- builder/openstack/run_config_test.go | 30 +++++ builder/openstack/step_run_source_server.go | 4 +- builder/openstack/step_source_image_info.go | 17 ++- 7 files changed, 128 insertions(+), 271 deletions(-) delete mode 100644 builder/openstack/image_query.go delete mode 100644 builder/openstack/image_query_test.go diff --git a/builder/openstack/builder.go b/builder/openstack/builder.go index 8e4c97ba6..638dcc8ba 100644 --- a/builder/openstack/builder.go +++ b/builder/openstack/builder.go @@ -87,9 +87,9 @@ func (b *Builder) Run(ui packer.Ui, hook packer.Hook, cache packer.Cache) (packe SSHAgentAuth: b.config.RunConfig.Comm.SSHAgentAuth, }, &StepSourceImageInfo{ - SourceImage: b.config.SourceImage, - SourceImageName: b.config.SourceImageName, - SourceImageOpts: b.config.SourceImageOpts, + SourceImage: b.config.RunConfig.SourceImage, + SourceImageName: b.config.RunConfig.SourceImageName, + SourceImageOpts: b.config.RunConfig.sourceImageOpts, SourceMostRecent: b.config.SourceImageFilters.MostRecent, }, &StepCreateVolume{ diff --git a/builder/openstack/image_query.go b/builder/openstack/image_query.go deleted file mode 100644 index 1d1ef0d27..000000000 --- a/builder/openstack/image_query.go +++ /dev/null @@ -1,120 +0,0 @@ -package openstack - -import ( - "fmt" - "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" - "github.com/hashicorp/packer/packer" - "reflect" -) - -const ( - mostRecentSort = "created_at:desc" -) - -var validFields = map[string]string{ - "Name": "name", - "Visibility": "visibility", - "Owner": "owner", - "Tags": "tags", -} - -// Retrieve the specific ImageVisibility using the exported const from images -func getImageVisibility(s string) (images.ImageVisibility, error) { - visibilities := [...]images.ImageVisibility{ - images.ImageVisibilityPublic, - images.ImageVisibilityPrivate, - images.ImageVisibilityCommunity, - images.ImageVisibilityShared, - } - - for _, visibility := range visibilities { - if string(visibility) == s { - return visibility, nil - } - } - - var nilVisibility images.ImageVisibility - return nilVisibility, fmt.Errorf("No valid ImageVisibility found for %s", s) -} - -// 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 - metaOpts := reflect.Indirect(reflect.ValueOf(listOpts)) - multiErr := packer.MultiError{} - - for i := 0; i < metaOpts.Type().NumField(); i++ { - vField := metaOpts.Field(i) - fieldName := metaOpts.Type().Field(i).Name - - // check the valid fields map and whether we can set this field - if key, exists := validFields[fieldName]; exists { - - // 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 (%s)", - valType, - key, - fieldName, - )) - break - } - vField.Set(reflect.ValueOf(val)) - - case "visibility": - visibility, err := getImageVisibility(val.(string)) - if err != nil { - multiErr.Errors = append(multiErr.Errors, err) - break - } - vField.Set(reflect.ValueOf(visibility)) - - } - - // 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 - - return &multiErr -} - -// Apply most recent filtering logic to ListOpts where user has filled fields. -// See https://developer.openstack.org/api-ref/image/v2/ -func applyMostRecent(listOpts *images.ListOpts) { - // Sort isn't supported through our API so there should be no existing values. - // Overwriting ListOpts.Sort is okay. - listOpts.Sort = mostRecentSort - - return -} diff --git a/builder/openstack/image_query_test.go b/builder/openstack/image_query_test.go deleted file mode 100644 index 71ffe3b37..000000000 --- a/builder/openstack/image_query_test.go +++ /dev/null @@ -1,108 +0,0 @@ -package openstack - -import ( - "testing" - - "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" - "github.com/mitchellh/mapstructure" -) - -func TestImageFilterOptionsDecode(t *testing.T) { - opts := ImageFilterOptions{} - input := map[string]interface{}{ - "most_recent": true, - "filters": map[string]interface{}{ - "visibility": "protected", - "tag": []string{"prod", "ready"}, - "name": "ubuntu 16.04", - "owner": "tcarrio", - }, - } - err := mapstructure.Decode(input, &opts) - if err != nil { - t.Errorf("Did not successfully generate ImageFilterOptions from %v.\nContains %v", input, opts) - } -} - -// This test case confirms that only allowed fields will be set to values -// The checked values are non-nil for their target type -func TestBuildImageFilter(t *testing.T) { - testOpts := images.ListOpts{} - - filters := map[string]interface{}{ - "limit": "3", - "name": "Ubuntu 16.04", - "visibility": "public", - "status": "active", - "size_min": "25", - "sort": "created_at:desc", - "tags": []string{"prod", "ready"}, - } - - // 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) - } - - if testOpts.Name != filters["name"] { - t.Errorf("Name did not parse correctly: %s", testOpts.Name) - } - - if testOpts.Visibility != images.ImageVisibilityPublic { - t.Errorf("Visibility did not parse correctly: %v", testOpts.Visibility) - } - - if testOpts.Status != images.ImageStatusActive { - t.Errorf("Image status did not parse correctly: %s", testOpts.Status) - } - - if testOpts.SizeMin != 0 { - t.Errorf("Size min was parsed: %d", testOpts.SizeMin) - } - - if len(testOpts.Sort) > 0 { - t.Errorf("Sort was parsed: %s", testOpts.Sort) - } -} - -// 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", - Tags: []string{"prod", "ready"}, - } - - applyMostRecent(&testSortOpts) - - if testSortOpts.Sort != "created_at:desc" { - t.Errorf("Error applying most recent filter: sort") - } -} diff --git a/builder/openstack/run_config.go b/builder/openstack/run_config.go index 7df9a65d8..ccc87dffc 100644 --- a/builder/openstack/run_config.go +++ b/builder/openstack/run_config.go @@ -19,23 +19,22 @@ type RunConfig struct { SSHInterface string `mapstructure:"ssh_interface"` SSHIPVersion string `mapstructure:"ssh_ip_version"` - SourceImage string `mapstructure:"source_image"` - SourceImageName string `mapstructure:"source_image_name"` - SourceImageFilters ImageFilterOptions `mapstructure:"source_image_filter"` - SourceImageOpts images.ListOpts `mapstructure:""` - Flavor string `mapstructure:"flavor"` - AvailabilityZone string `mapstructure:"availability_zone"` - RackconnectWait bool `mapstructure:"rackconnect_wait"` - FloatingIPNetwork string `mapstructure:"floating_ip_network"` - FloatingIP string `mapstructure:"floating_ip"` - ReuseIPs bool `mapstructure:"reuse_ips"` - SecurityGroups []string `mapstructure:"security_groups"` - Networks []string `mapstructure:"networks"` - Ports []string `mapstructure:"ports"` - UserData string `mapstructure:"user_data"` - UserDataFile string `mapstructure:"user_data_file"` - InstanceName string `mapstructure:"instance_name"` - InstanceMetadata map[string]string `mapstructure:"instance_metadata"` + SourceImage string `mapstructure:"source_image"` + SourceImageName string `mapstructure:"source_image_name"` + SourceImageFilters ImageFilter `mapstructure:"source_image_filter"` + Flavor string `mapstructure:"flavor"` + AvailabilityZone string `mapstructure:"availability_zone"` + RackconnectWait bool `mapstructure:"rackconnect_wait"` + FloatingIPNetwork string `mapstructure:"floating_ip_network"` + FloatingIP string `mapstructure:"floating_ip"` + ReuseIPs bool `mapstructure:"reuse_ips"` + SecurityGroups []string `mapstructure:"security_groups"` + Networks []string `mapstructure:"networks"` + Ports []string `mapstructure:"ports"` + UserData string `mapstructure:"user_data"` + UserDataFile string `mapstructure:"user_data_file"` + InstanceName string `mapstructure:"instance_name"` + InstanceMetadata map[string]string `mapstructure:"instance_metadata"` ConfigDrive bool `mapstructure:"config_drive"` @@ -50,6 +49,52 @@ type RunConfig struct { // Not really used, but here for BC OpenstackProvider string `mapstructure:"openstack_provider"` UseFloatingIp bool `mapstructure:"use_floating_ip"` + + sourceImageOpts images.ListOpts +} + +type ImageFilter struct { + Filters ImageFilterOptions `mapstructure:"filters"` + MostRecent bool `mapstructure:"most_recent"` +} + +type ImageFilterOptions struct { + Name string `mapstructure:"name"` + Owner string `mapstructure:"owner"` + Tags []string `mapstructure:"tags"` + Visibility string `mapstructure:"visibility"` +} + +func (f *ImageFilterOptions) Empty() bool { + return f.Name == "" && f.Owner == "" && len(f.Tags) == 0 && f.Visibility == "" +} + +func (f *ImageFilterOptions) Build() (*images.ListOpts, error) { + opts := images.ListOpts{} + // Set defaults for status, member_status, and sort + opts.Status = images.ImageStatusActive + opts.MemberStatus = images.ImageMemberStatusAccepted + opts.Sort = "created_at:desc" + + var err error + + if f.Name != "" { + opts.Name = f.Name + } + if f.Owner != "" { + opts.Owner = f.Owner + } + if len(f.Tags) > 0 { + opts.Tags = f.Tags + } + if f.Visibility != "" { + v, err := getImageVisibility(f.Visibility) + if err == nil { + opts.Visibility = *v + } + } + + return &opts, err } func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { @@ -78,7 +123,7 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { } } - if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters == nil { + if c.SourceImage == "" && c.SourceImageName == "" && c.SourceImageFilters.Filters.Empty() { errs = append(errs, errors.New("Either a source_image, a source_image_name, or source_image_filter must be specified")) } else if len(c.SourceImage) > 0 && len(c.SourceImageName) > 0 { errs = append(errs, errors.New("Only a source_image or a source_image_name can be specified, not both.")) @@ -116,21 +161,32 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { // if neither ID or image name is provided outside the filter, build the filter if len(c.SourceImage) == 0 && len(c.SourceImageName) == 0 { - params := &images.ListOpts{} - if len(c.SourceImageFilters.Filters) > 0 { - filterErrs := buildImageFilters(c.SourceImageFilters.Filters, params) - if len(filterErrs.Errors) > 0 { - errs = append(errs, filterErrs.Errors...) - } - } + listOpts, filterErr := c.SourceImageFilters.Filters.Build() - if c.SourceImageFilters.MostRecent { - applyMostRecent(params) + if filterErr != nil { + errs = append(errs, filterErr) } - - c.SourceImageOpts = *params + c.sourceImageOpts = *listOpts } return errs } + +// Retrieve the specific ImageVisibility using the exported const from images +func getImageVisibility(visibility string) (*images.ImageVisibility, error) { + visibilities := [...]images.ImageVisibility{ + images.ImageVisibilityPublic, + images.ImageVisibilityPrivate, + images.ImageVisibilityCommunity, + images.ImageVisibilityShared, + } + + for _, v := range visibilities { + if string(v) == visibility { + return &v, nil + } + } + + return nil, fmt.Errorf("Not a valid visibility: %s", visibility) +} diff --git a/builder/openstack/run_config_test.go b/builder/openstack/run_config_test.go index 6ce0cf602..36b9e5716 100644 --- a/builder/openstack/run_config_test.go +++ b/builder/openstack/run_config_test.go @@ -4,6 +4,7 @@ import ( "os" "testing" + "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/hashicorp/packer/helper/communicator" ) @@ -127,3 +128,32 @@ func TestRunConfigPrepare_FloatingIPPoolCompat(t *testing.T) { t.Fatalf("invalid value: %s", c.FloatingIPNetwork) } } + +// This test case confirms that only allowed fields will be set to values +// The checked values are non-nil for their target type +func TestBuildImageFilter(t *testing.T) { + + filters := ImageFilterOptions{ + Name: "Ubuntu 16.04", + Visibility: "public", + Owner: "1234567890", + Tags: []string{"prod", "ready"}, + } + + listOpts, err := filters.Build() + if err != nil { + t.Errorf("Building filter failed with: %s", err) + } + + if listOpts.Name != "Ubuntu 16.04" { + t.Errorf("Name did not build correctly: %s", listOpts.Name) + } + + if listOpts.Visibility != images.ImageVisibilityPublic { + t.Errorf("Visibility did not build correctly: %s", listOpts.Visibility) + } + + if listOpts.Owner != "1234567890" { + t.Errorf("Owner did not build correctly: %s", listOpts.Owner) + } +} diff --git a/builder/openstack/step_run_source_server.go b/builder/openstack/step_run_source_server.go index c14061efc..6bbb40eba 100644 --- a/builder/openstack/step_run_source_server.go +++ b/builder/openstack/step_run_source_server.go @@ -78,8 +78,8 @@ func (s *StepRunSourceServer) Run(_ context.Context, state multistep.StateBag) m } // check if image filter returned a source image ID and replace - if imageID := state.Get("source_image").(string); imageID != "" { - serverOpts.ImageRef = imageID + if imageID, ok := state.GetOk("source_image"); ok { + serverOpts.ImageRef = imageID.(string) } var serverOptsExt servers.CreateOptsBuilder diff --git a/builder/openstack/step_source_image_info.go b/builder/openstack/step_source_image_info.go index bdd0b25b5..6cf3500ae 100644 --- a/builder/openstack/step_source_image_info.go +++ b/builder/openstack/step_source_image_info.go @@ -18,15 +18,14 @@ type StepSourceImageInfo struct { SourceMostRecent bool } -type ImageFilterOptions struct { - Filters map[string]interface{} `mapstructure:"filters"` - MostRecent bool `mapstructure:"most_recent"` -} - func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) multistep.StepAction { config := state.Get("config").(Config) ui := state.Get("ui").(packer.Ui) + if s.SourceImage != "" || s.SourceImageName != "" { + return multistep.ActionContinue + } + client, err := config.imageV2Client() log.Printf("Using Image Filters %v", s.SourceImageOpts) @@ -52,15 +51,15 @@ func (s *StepSourceImageInfo) Run(_ context.Context, state multistep.StateBag) m } }) - if image.ID == "" { - err := fmt.Errorf("No image was found matching filters: %v", s.SourceImageOpts) + if err != nil { + err := fmt.Errorf("Error querying image: %s", err) state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt } - if err != nil { - err := fmt.Errorf("Error querying image: %s", err) + if image.ID == "" { + err := fmt.Errorf("No image was found matching filters: %v", s.SourceImageOpts) state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt From e2fe5cd7758d57bcb86e0415769a8d0aa600edfb Mon Sep 17 00:00:00 2001 From: Tom Carrio Date: Wed, 22 Aug 2018 18:23:12 -0400 Subject: [PATCH 22/22] Updated test cases to cover bad filters and empty() --- builder/openstack/run_config_test.go | 53 ++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/builder/openstack/run_config_test.go b/builder/openstack/run_config_test.go index 36b9e5716..f660a4e82 100644 --- a/builder/openstack/run_config_test.go +++ b/builder/openstack/run_config_test.go @@ -6,6 +6,7 @@ import ( "github.com/gophercloud/gophercloud/openstack/imageservice/v2/images" "github.com/hashicorp/packer/helper/communicator" + "github.com/mitchellh/mapstructure" ) func init() { @@ -157,3 +158,55 @@ func TestBuildImageFilter(t *testing.T) { t.Errorf("Owner did not build correctly: %s", listOpts.Owner) } } + +func TestBuildBadImageFilter(t *testing.T) { + filterMap := map[string]interface{}{ + "limit": "3", + "size_min": "25", + } + + filters := ImageFilterOptions{} + mapstructure.Decode(filterMap, &filters) + listOpts, err := filters.Build() + + if err != nil { + t.Errorf("Error returned processing image filter: %s", err.Error()) + return // we cannot trust listOpts to not cause unexpected behaviour + } + + if listOpts.Limit == filterMap["limit"] { + t.Errorf("Limit was parsed into ListOpts: %d", listOpts.Limit) + } + + if listOpts.SizeMin != 0 { + t.Errorf("SizeMin was parsed into ListOpts: %d", listOpts.SizeMin) + } + + if listOpts.Sort != "created_at:desc" { + t.Errorf("Sort was not applied: %s", listOpts.Sort) + } + + if !filters.Empty() { + t.Errorf("The filters should be empty due to lack of input") + } +} + +// Tests that the Empty method on ImageFilterOptions works as expected +func TestImageFiltersEmpty(t *testing.T) { + filledFilters := ImageFilterOptions{ + Name: "Ubuntu 16.04", + Visibility: "public", + Owner: "1234567890", + Tags: []string{"prod", "ready"}, + } + + if filledFilters.Empty() { + t.Errorf("Expected filled filters to be non-empty: %v", filledFilters) + } + + emptyFilters := ImageFilterOptions{} + + if !emptyFilters.Empty() { + t.Errorf("Expected default filter to be empty: %v", emptyFilters) + } +}