Skip to content

Commit

Permalink
ServiceCreate/ServiceUpdate: refactor and fix potential NPE
Browse files Browse the repository at this point in the history
- `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
  72c3bcf, where the new `PluginSpec` path
  was added.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
  • Loading branch information
thaJeztah committed Apr 16, 2020
1 parent 7d0e939 commit 799bd47
Showing 2 changed files with 65 additions and 56 deletions.
60 changes: 32 additions & 28 deletions client/service_create.go
Original file line number Diff line number Diff line change
@@ -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,56 +30,61 @@ 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 {
return response, err
}

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
61 changes: 33 additions & 28 deletions client/service_update.go
Original file line number Diff line number Diff line change
@@ -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,56 +38,61 @@ 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 {
return response, err
}

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

0 comments on commit 799bd47

Please sign in to comment.