From 799bd475fbe940b79ac098bdf5ad2895682be09c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 Apr 2020 13:03:39 +0200 Subject: [PATCH 1/3] 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 56bfe55b71..05c1bbd215 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 cd0f59e213..4a4d98052b 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 From 10c748cd39377319fbb485ff589b2c74b26ad73b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 Apr 2020 13:20:38 +0200 Subject: [PATCH 2/3] imageWithDigestString: return image unmodified if there are no changes Instead of returning an empty string, return the image unmodified Signed-off-by: Sebastiaan van Stijn --- client/service_create.go | 12 ++++-------- client/service_update.go | 8 ++------ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/client/service_create.go b/client/service_create.go index 05c1bbd215..5dd6a430cb 100644 --- a/client/service_create.go +++ b/client/service_create.go @@ -44,9 +44,7 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, 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 - } + service.TaskTemplate.ContainerSpec.Image = img if len(imgPlatforms) > 0 { if service.TaskTemplate.Placement == nil { service.TaskTemplate.Placement = &swarm.Placement{} @@ -63,9 +61,7 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, 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 - } + service.TaskTemplate.PluginSpec.Remote = img if len(imgPlatforms) > 0 { if service.TaskTemplate.Placement == nil { service.TaskTemplate.Placement = &swarm.Placement{} @@ -123,7 +119,7 @@ func imageDigestAndPlatforms(ctx context.Context, cli DistributionAPIClient, ima // imageWithDigestString takes an image string and a digest, and updates // the image string if it didn't originally contain a digest. It returns -// an empty string if there are no updates. +// image unmodified in other situations. func imageWithDigestString(image string, dgst digest.Digest) string { namedRef, err := reference.ParseNormalizedNamed(image) if err == nil { @@ -135,7 +131,7 @@ func imageWithDigestString(image string, dgst digest.Digest) string { } } } - return "" + return image } // imageWithTagString takes an image string, and returns a tagged image diff --git a/client/service_update.go b/client/service_update.go index 4a4d98052b..4a930eed14 100644 --- a/client/service_update.go +++ b/client/service_update.go @@ -52,9 +52,7 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version 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 - } + service.TaskTemplate.ContainerSpec.Image = img if len(imgPlatforms) > 0 { if service.TaskTemplate.Placement == nil { service.TaskTemplate.Placement = &swarm.Placement{} @@ -71,9 +69,7 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version 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 - } + service.TaskTemplate.PluginSpec.Remote = img if len(imgPlatforms) > 0 { if service.TaskTemplate.Placement == nil { service.TaskTemplate.Placement = &swarm.Placement{} From ed096538e82e8468b4c35690ffe269e66468553b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 Apr 2020 13:42:47 +0200 Subject: [PATCH 3/3] extract logic for resolving image/plugin digest and platform Signed-off-by: Sebastiaan van Stijn --- client/service_create.go | 56 ++++++++++++++++++++++++---------------- client/service_update.go | 24 ++--------------- 2 files changed, 36 insertions(+), 44 deletions(-) diff --git a/client/service_create.go b/client/service_create.go index 5dd6a430cb..e0428bf98b 100644 --- a/client/service_create.go +++ b/client/service_create.go @@ -41,34 +41,14 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, service.TaskTemplate.ContainerSpec.Image = taggedImg } if options.QueryRegistry { - if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth); err != nil { - resolveWarning = digestWarning(service.TaskTemplate.ContainerSpec.Image) - } else { - service.TaskTemplate.ContainerSpec.Image = img - if len(imgPlatforms) > 0 { - if service.TaskTemplate.Placement == nil { - service.TaskTemplate.Placement = &swarm.Placement{} - } - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - } + resolveWarning = resolveContainerSpecImage(ctx, cli, &service.TaskTemplate, options.EncodedRegistryAuth) } case service.TaskTemplate.PluginSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.PluginSpec.Remote); taggedImg != "" { service.TaskTemplate.PluginSpec.Remote = taggedImg } if options.QueryRegistry { - if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth); err != nil { - resolveWarning = digestWarning(service.TaskTemplate.PluginSpec.Remote) - } else { - service.TaskTemplate.PluginSpec.Remote = img - if len(imgPlatforms) > 0 { - if service.TaskTemplate.Placement == nil { - service.TaskTemplate.Placement = &swarm.Placement{} - } - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - } + resolveWarning = resolvePluginSpecRemote(ctx, cli, &service.TaskTemplate, options.EncodedRegistryAuth) } } @@ -86,6 +66,38 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec, return response, err } +func resolveContainerSpecImage(ctx context.Context, cli DistributionAPIClient, taskSpec *swarm.TaskSpec, encodedAuth string) string { + var warning string + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, taskSpec.ContainerSpec.Image, encodedAuth); err != nil { + warning = digestWarning(taskSpec.ContainerSpec.Image) + } else { + taskSpec.ContainerSpec.Image = img + if len(imgPlatforms) > 0 { + if taskSpec.Placement == nil { + taskSpec.Placement = &swarm.Placement{} + } + taskSpec.Placement.Platforms = imgPlatforms + } + } + return warning +} + +func resolvePluginSpecRemote(ctx context.Context, cli DistributionAPIClient, taskSpec *swarm.TaskSpec, encodedAuth string) string { + var warning string + if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, taskSpec.PluginSpec.Remote, encodedAuth); err != nil { + warning = digestWarning(taskSpec.PluginSpec.Remote) + } else { + taskSpec.PluginSpec.Remote = img + if len(imgPlatforms) > 0 { + if taskSpec.Placement == nil { + taskSpec.Placement = &swarm.Placement{} + } + taskSpec.Placement.Platforms = imgPlatforms + } + } + return warning +} + func imageDigestAndPlatforms(ctx context.Context, cli DistributionAPIClient, image, encodedAuth string) (string, []swarm.Platform, error) { distributionInspect, err := cli.DistributionInspect(ctx, image, encodedAuth) var platforms []swarm.Platform diff --git a/client/service_update.go b/client/service_update.go index 4a930eed14..c63895f74f 100644 --- a/client/service_update.go +++ b/client/service_update.go @@ -49,34 +49,14 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version service.TaskTemplate.ContainerSpec.Image = taggedImg } if options.QueryRegistry { - if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth); err != nil { - resolveWarning = digestWarning(service.TaskTemplate.ContainerSpec.Image) - } else { - service.TaskTemplate.ContainerSpec.Image = img - if len(imgPlatforms) > 0 { - if service.TaskTemplate.Placement == nil { - service.TaskTemplate.Placement = &swarm.Placement{} - } - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - } + resolveWarning = resolveContainerSpecImage(ctx, cli, &service.TaskTemplate, options.EncodedRegistryAuth) } case service.TaskTemplate.PluginSpec != nil: if taggedImg := imageWithTagString(service.TaskTemplate.PluginSpec.Remote); taggedImg != "" { service.TaskTemplate.PluginSpec.Remote = taggedImg } if options.QueryRegistry { - if img, imgPlatforms, err := imageDigestAndPlatforms(ctx, cli, service.TaskTemplate.PluginSpec.Remote, options.EncodedRegistryAuth); err != nil { - resolveWarning = digestWarning(service.TaskTemplate.PluginSpec.Remote) - } else { - service.TaskTemplate.PluginSpec.Remote = img - if len(imgPlatforms) > 0 { - if service.TaskTemplate.Placement == nil { - service.TaskTemplate.Placement = &swarm.Placement{} - } - service.TaskTemplate.Placement.Platforms = imgPlatforms - } - } + resolveWarning = resolvePluginSpecRemote(ctx, cli, &service.TaskTemplate, options.EncodedRegistryAuth) } }