From 799bd475fbe940b79ac098bdf5ad2895682be09c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 Apr 2020 13:03:39 +0200 Subject: [PATCH] ServiceCreate/ServiceUpdate: refactor and fix potential NPE - `ContainerSpec` and `PluginSpec` are mutually exclusive, so instead of using two separate if-statements, combine them in a switch. - Use local variables (at cost of some slight duplication) - Fix a potential NPE if image-digest resolution failed for a `PluginSpec`. The code was always using `ContainerSpec.Image` to create a `digestWarning`, but in case we're resoling the digest for a `PluginSpec`, `ContainerSpec` will be `nil` (as they're mutually exclusive). This issue was introduced in 72c3bcf2a533a827402945e3a55872e2db4fb024, where the new `PluginSpec` path was added. Signed-off-by: Sebastiaan van Stijn --- client/service_create.go | 60 +++++++++++++++++++++------------------ client/service_update.go | 61 ++++++++++++++++++++++------------------ 2 files changed, 65 insertions(+), 56 deletions(-) diff --git a/client/service_create.go b/client/service_create.go index 56bfe55b71018..05c1bbd215b57 100644 --- a/client/service_create.go +++ b/client/service_create.go @@ -15,8 +15,7 @@ import ( // ServiceCreate creates a new Service. func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, options types.ServiceCreateOptions) (types.ServiceCreateResponse, error) { - var distErr error - + var response types.ServiceCreateResponse headers := map[string][]string{ "version": {cli.version}, } @@ -31,46 +30,52 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, } if err := validateServiceSpec(service); err != nil { - return types.ServiceCreateResponse{}, err + return response, err } // ensure that the image is tagged - var imgPlatforms []swarm.Platform - if service.TaskTemplate.ContainerSpec != nil { + var resolveWarning string + switch { + case service.TaskTemplate.ContainerSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.ContainerSpec.Image); taggedImg != "" { service.TaskTemplate.ContainerSpec.Image = taggedImg } if options.QueryRegistry { - var img string - img, imgPlatforms, distErr = imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth) - if img != "" { - service.TaskTemplate.ContainerSpec.Image = img + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth); err != nil { + resolveWarning = digestWarning(service.TaskTemplate.ContainerSpec.Image) + } else { + if img != "" { + service.TaskTemplate.ContainerSpec.Image = img + } + if len(imgPlatforms) > 0 { + if service.TaskTemplate.Placement == nil { + service.TaskTemplate.Placement = &swarm.Placement{} + } + service.TaskTemplate.Placement.Platforms = imgPlatforms + } } } - } - - // ensure that the image is tagged - if service.TaskTemplate.PluginSpec != nil { + case service.TaskTemplate.PluginSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.PluginSpec.Remote); taggedImg != "" { service.TaskTemplate.PluginSpec.Remote = taggedImg } if options.QueryRegistry { - var img string - img, imgPlatforms, distErr = imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth) - if img != "" { - service.TaskTemplate.PluginSpec.Remote = img + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth); err != nil { + resolveWarning = digestWarning(service.TaskTemplate.PluginSpec.Remote) + } else { + if img != "" { + service.TaskTemplate.PluginSpec.Remote = img + } + if len(imgPlatforms) > 0 { + if service.TaskTemplate.Placement == nil { + service.TaskTemplate.Placement = &swarm.Placement{} + } + service.TaskTemplate.Placement.Platforms = imgPlatforms + } } } } - if service.TaskTemplate.Placement == nil && len(imgPlatforms) > 0 { - service.TaskTemplate.Placement = &swarm.Placement{} - } - if len(imgPlatforms) > 0 { - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - - var response types.ServiceCreateResponse resp, err := cli.post(ctx, "/services/create", nil, service, headers) defer ensureReaderClosed(resp) if err != nil { @@ -78,9 +83,8 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, } err = json.NewDecoder(resp.body).Decode(&response) - - if distErr != nil { - response.Warnings = append(response.Warnings, digestWarning(service.TaskTemplate.ContainerSpec.Image)) + if resolveWarning != "" { + response.Warnings = append(response.Warnings, resolveWarning) } return response, err diff --git a/client/service_update.go b/client/service_update.go index cd0f59e21338a..4a4d98052ba89 100644 --- a/client/service_update.go +++ b/client/service_update.go @@ -15,8 +15,8 @@ import ( // of swarm.Service, which can be found using ServiceInspectWithRaw. func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version swarm.Version, service swarm.ServiceSpec, options types.ServiceUpdateOptions) (types.ServiceUpdateResponse, error) { var ( - query = url.Values{} - distErr error + query = url.Values{} + response = types.ServiceUpdateResponse{} ) headers := map[string][]string{ @@ -38,46 +38,52 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version query.Set("version", strconv.FormatUint(version.Index, 10)) if err := validateServiceSpec(service); err != nil { - return types.ServiceUpdateResponse{}, err + return response, err } - var imgPlatforms []swarm.Platform // ensure that the image is tagged - if service.TaskTemplate.ContainerSpec != nil { + var resolveWarning string + switch { + case service.TaskTemplate.ContainerSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.ContainerSpec.Image); taggedImg != "" { service.TaskTemplate.ContainerSpec.Image = taggedImg } if options.QueryRegistry { - var img string - img, imgPlatforms, distErr = imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth) - if img != "" { - service.TaskTemplate.ContainerSpec.Image = img + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth); err != nil { + resolveWarning = digestWarning(service.TaskTemplate.ContainerSpec.Image) + } else { + if img != "" { + service.TaskTemplate.ContainerSpec.Image = img + } + if len(imgPlatforms) > 0 { + if service.TaskTemplate.Placement == nil { + service.TaskTemplate.Placement = &swarm.Placement{} + } + service.TaskTemplate.Placement.Platforms = imgPlatforms + } } } - } - - // ensure that the image is tagged - if service.TaskTemplate.PluginSpec != nil { + case service.TaskTemplate.PluginSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.PluginSpec.Remote); taggedImg != "" { service.TaskTemplate.PluginSpec.Remote = taggedImg } if options.QueryRegistry { - var img string - img, imgPlatforms, distErr = imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth) - if img != "" { - service.TaskTemplate.PluginSpec.Remote = img + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth); err != nil { + resolveWarning = digestWarning(service.TaskTemplate.PluginSpec.Remote) + } else { + if img != "" { + service.TaskTemplate.PluginSpec.Remote = img + } + if len(imgPlatforms) > 0 { + if service.TaskTemplate.Placement == nil { + service.TaskTemplate.Placement = &swarm.Placement{} + } + service.TaskTemplate.Placement.Platforms = imgPlatforms + } } } } - if service.TaskTemplate.Placement == nil && len(imgPlatforms) > 0 { - service.TaskTemplate.Placement = &swarm.Placement{} - } - if len(imgPlatforms) > 0 { - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - - var response types.ServiceUpdateResponse resp, err := cli.post(ctx, "/services/"+serviceID+"/update", query, service, headers) defer ensureReaderClosed(resp) if err != nil { @@ -85,9 +91,8 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version } err = json.NewDecoder(resp.body).Decode(&response) - - if distErr != nil { - response.Warnings = append(response.Warnings, digestWarning(service.TaskTemplate.ContainerSpec.Image)) + if resolveWarning != "" { + response.Warnings = append(response.Warnings, resolveWarning) } return response, err