Skip to content

Validate stack-names for empty values - #1088

Merged
vdemeester merged 1 commit into
docker:masterfrom
thaJeztah:fix_empty_stackname_validation
May 28, 2018
Merged

vdemeester merged 1 commit into
docker:masterfrom
thaJeztah:fix_empty_stackname_validation

Conversation

@thaJeztah

@thaJeztah thaJeztah commented May 28, 2018 •

Copy link
Copy Markdown
Member

Fixes moby/moby#36776

Add validation for stack names to prevent an empty name resulting in all
stacks to be returned after filtering, which can result in removal of services
for all stacks if --prune, or docker stack rm is used.

Before this change;

docker stack deploy -c docker-compose.yml one
docker stack deploy -c docker-compose.yml two
docker stack deploy -c docker-compose.yml three

docker stack deploy -c docker-compose.yml --prune ''
Removing service one_web
Removing service two_web
Removing service three_web

After this change:

docker stack deploy -c docker-compose.yml one
docker stack deploy -c docker-compose.yml two
docker stack deploy -c docker-compose.yml three

docker stack deploy -c docker-compose.yml --prune ''
invalid stack name: ""

Other stack commands were updated as well:

Before this change;

docker stack deploy -c docker-compose.yml ''
Creating network _default
failed to create network _default: Error response from daemon: rpc error: code = InvalidArgument desc = name must be valid as a DNS name component

docker stack ps ''
nothing found in stack:

docker stack rm ''
Removing service one_web
Removing service three_web
Removing service two_web

After this change:

docker stack deploy -c docker-compose.yml ''
invalid stack name: ""

docker stack ps ''
invalid stack name: ""

docker stack rm ''
invalid stack name: ""

Signed-off-by: Sebastiaan van Stijn github@gone.nl

- Description for the changelog

* Fix `docker stack deploy --prune` with empty name removing all services [docker/cli#1088](https://github.com/docker/cli/pull/1088)

Comment thread cli/command/stack/swarm/common.go Outdated
case '"', '\'':
return true
}
return false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:Β wouldn't this be more readable

    return unicode.IsSpace(r) || r == '"' || r == '\''

?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(which might optionally then be inlined into validateStackName)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; I initially had more stuff in the switch, but now it's not really needed anymore. I'll update

@silvin-lubecki silvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vdemeester vdemeester left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🐯

Add validation for stack names to prevent an empty name resulting in _all_
stacks to be returned after filtering, which can result in removal of services
for all stacks if `--prune`, or `docker stack rm` is used.

Before this change;

    docker stack deploy -c docker-compose.yml one
    docker stack deploy -c docker-compose.yml two
    docker stack deploy -c docker-compose.yml three

    docker stack deploy -c docker-compose.yml --prune ''
    Removing service one_web
    Removing service two_web
    Removing service three_web

After this change:

    docker stack deploy -c docker-compose.yml one
    docker stack deploy -c docker-compose.yml two
    docker stack deploy -c docker-compose.yml three

    docker stack deploy -c docker-compose.yml --prune ''
    invalid stack name: ""

Other stack commands were updated as well:

Before this change;

    docker stack deploy -c docker-compose.yml ''
    Creating network _default
    failed to create network _default: Error response from daemon: rpc error: code = InvalidArgument desc = name must be valid as a DNS name component

    docker stack ps ''
    nothing found in stack:

    docker stack rm ''
    Removing service one_web
    Removing service three_web
    Removing service two_web

After this change:

    docker stack deploy -c docker-compose.yml ''
    invalid stack name: ""

    docker stack ps ''
    invalid stack name: ""

    docker stack rm ''
    invalid stack name: ""

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
thaJeztah force-pushed the fix_empty_stackname_validation branch from f354695 to d38f397 Compare May 28, 2018 15:03
@thaJeztah

Copy link
Copy Markdown
Member Author

Updated πŸ‘

which might optionally then be inlined into validateStackName

I decided not to inline, as I thought having a quotesOrWhitespace() was slightly more readable, but let me know if you think I should πŸ‘

@vdemeester
vdemeester merged commit 9b3bba8 into docker:master May 28, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone May 28, 2018
@thaJeztah
thaJeztah deleted the fix_empty_stackname_validation branch May 30, 2018 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docker stack deploy --prune with empty name removes all the swarm services

5 participants