Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate security options to use = as separator. #21232

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

calavera
Copy link
Contributor

All other options we have use = as separator, labels,
log configurations, graph configurations and so on.
We should be consistent and use = for the security
options too.

I change the daemon parsing to accept =. It still
supports : but it logs a warning when this separator
is used.

Let's decide if we want to do this and I'll complete it
with tests and documentation changes.

Signed-off-by: David Calavera david.calavera@gmail.com

@jessfraz
Copy link
Contributor

LGTM

@runcom
Copy link
Member

runcom commented Mar 15, 2016

Design LGTM

@thaJeztah
Copy link
Member

👍 thanks!

@thaJeztah
Copy link
Member

Will need to search the docs to update the examples

@calavera calavera force-pushed the consolidate_security_opts_format branch from f412c22 to 377f82b Compare March 15, 2016 23:23
@calavera
Copy link
Contributor Author

@thaJeztah I think I got all of them, but please double check.

@thaJeztah
Copy link
Member

@vdemeester
Copy link
Member

LGTM 🐼
@thaJeztah Given the warning I would say yes, we need a deprecation warning (https://github.com/docker/docker/pull/21232/files#diff-b56258ecab11f7b797306907327cc904R84).
@calavera a question though, shouldn't the warning be also client-side ?

@calavera
Copy link
Contributor Author

we need a deprecation warning

agreed

shouldn't the warning be also client-side ?

we don't do any parsing client side, and we don't show other warnings like this in the client afaik, but I can add it if people want it.

@calavera calavera force-pushed the consolidate_security_opts_format branch 2 times, most recently from 7a96715 to 0d55e0d Compare March 16, 2016 22:32
@rhatdan
Copy link
Contributor

rhatdan commented Mar 17, 2016

The SELinux label ones have multiple ":" in them.

--security-opt label:type:container_log_t --security-opt label:level:s0:c1,c2

I guess becomes

   --security-opt label=type:container_log_t --security-opt label=level:s0:c1,c2

@calavera
Copy link
Contributor Author

I guess becomes

   --security-opt label=type:container_log_t --security-opt label=level:s0:c1,c2

Yes

@justincormack
Copy link
Contributor

We do some parsing client side, as the seccomp config file is parsed client side.

@calavera
Copy link
Contributor Author

All other options we have use `=` as separator, labels,
log configurations, graph configurations and so on.
We should be consistent and use `=` for the security
options too.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the consolidate_security_opts_format branch from 0d55e0d to cb9aeb0 Compare March 17, 2016 17:34
@calavera calavera added this to the 1.11.0 milestone Mar 18, 2016
@jessfraz
Copy link
Contributor

LGTM

jessfraz pushed a commit that referenced this pull request Mar 18, 2016
Consolidate security options to use `=` as separator.
@jessfraz jessfraz merged commit 06e98f0 into moby:master Mar 18, 2016
@@ -1788,17 +1788,17 @@ _docker_run() {
;;
--security-opt)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry David, this change does not work: = cannot be substituted for : here because it is in COMP_WORDBREAKS.
I will create a fix.

Copy link
Member

Choose a reason for hiding this comment

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

note: to be accurate: : us also in $COMP_WORDBREAKS but there is speacial treatment for it in

_get_comp_words_by_ref -n : cur prev words cword

Fix created: #21584

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.

9 participants