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

Allow using Configs as CredentialSpecs #2771

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

dperny
Copy link
Collaborator

@dperny dperny commented Oct 25, 2018

- What I did

Adds support for passing a secret ID as the CredentialSpec source. Also lays the groundwork for future uses of Secrets in places other than mounted into the container.

- How I did it

Added a new Target type for Secrets. In addition to the existing FileTarget, there is now a RuntimeTarget, which allows using Secrets for the container runtime, but not mounting them into the container. This allows us to send down a Secret to use as a CredentialSpec. Additionally, the same RuntimeTarget could be used, for example, to pass registry credentials, with future work."

Will require an accompanying change in the engine to work.

- How to test it

Includes automated unit tests.

- Description for the changelog

Support distributing Secrets to use as CredentialSpecs.

@dperny
Copy link
Collaborator Author

dperny commented Oct 31, 2018

Gonna rework this PR with Configs instead of secrets, but otherwise it's exactly the same.

@dperny dperny force-pushed the gmsa-support-with-secretreference branch from a66aa6f to d118b04 Compare November 1, 2018 17:55
@dperny dperny changed the title Allow using Secrets as CredentialSpecs Allow using Configs as CredentialSpecs Nov 1, 2018
@dperny
Copy link
Collaborator Author

dperny commented Nov 1, 2018

Changed this PR from Secrets to Configs, because the information in a CredentialSpec isn't secret.

@dperny dperny force-pushed the gmsa-support-with-secretreference branch from d118b04 to 757a237 Compare November 28, 2018 15:52
// container, but is used for some other purpose by the container runtime.
//
// Currently, RuntimeTarget has no fields; it's just a placeholder.
message RuntimeTarget {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the motivation to use a struct and not a bool? Please add that to the comments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Struct because it's part of oneof target in the ConfigReference, where it is contrast with FileTarget. Additionally, if we want to put more information in this field in the future, we can do so; if we use a boolean, we'll have a bad API if we ever need to add info.

@dperny dperny force-pushed the gmsa-support-with-secretreference branch from 757a237 to 6025954 Compare January 3, 2019 19:21
@dperny
Copy link
Collaborator Author

dperny commented Jan 3, 2019

Rebased, gonna see if CI goes green

@dperny dperny force-pushed the gmsa-support-with-secretreference branch from 6025954 to 54f3d5e Compare January 10, 2019 18:24
Adds support for passing a config ID as the CredentialSpec source. Also
lays the groundwork for future uses of secrets and configs in places
other than mounted into the container.

Signed-off-by: Drew Erny <drew.erny@docker.com>
@dperny dperny force-pushed the gmsa-support-with-secretreference branch from 54f3d5e to 5432b8a Compare January 15, 2019 19:38
@dperny
Copy link
Collaborator Author

dperny commented Jan 16, 2019

Continuous disintegration

Signed-off-by: Drew Erny <drew.erny@docker.com>
@dperny dperny force-pushed the gmsa-support-with-secretreference branch from a638d4f to be26111 Compare January 16, 2019 18:44
@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #2771 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2771      +/-   ##
==========================================
+ Coverage   61.92%   61.98%   +0.06%     
==========================================
  Files         137      137              
  Lines       22144    22144              
==========================================
+ Hits        13712    13726      +14     
+ Misses       6956     6940      -16     
- Partials     1476     1478       +2

@dperny
Copy link
Collaborator Author

dperny commented Jan 16, 2019

This is the first time this PR has gone green @anshulpundir

api/api.pb.txt Show resolved Hide resolved
api/api.pb.txt Show resolved Hide resolved
@dperny dperny mentioned this pull request Feb 1, 2019
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Oct 9, 2019
…v18.09)

full diff: moby/swarmkit@142a737...5c86095

- moby/swarmkit#2892 [18.09 backport] Remove hardcoded IPAM config subnet value for ingress network
    - backport of moby/swarmkit#2890 Remove hardcoded IPAM config subnet value for ingress network
    - fixes [ENGORC-2651](https://docker.atlassian.net/browse/ENGORC-2651)
- moby/swarmkit#2836 [18.09 backport] Switch to go 1.11
    - backport of moby/swarmkit#2752 Switch to go 1.11
- moby/swarmkit#2901 [18.09 backport] Bump to golang 1.12.9
    - backport of moby/swarmkit#2880 Bump to golang 1.12.9
- moby/swarmkit#2900 [18.09 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets
    - backport of moby/swarmkit#2762 Increased wait time on test utils WaitForCluster and WatchTaskCreate
    - backport of moby/swarmkit#2771 Allow using Configs as CredentialSpecs
        - **second commit only** (attempt to fix weirdly broken tests)
    - backport of moby/swarmkit#2808 Fix flaky tests
    - backport of moby/swarmkit#2866 Swap gometalinter for golangci-lint
    - backport of moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker
        - related / similar to moby#38103 / docker-archive#102 cluster: set bigger grpc limit for array requests
        - related / similar to moby#39306 Increase max recv gRPC message size for nodes and secrets
        - fixes moby/swarmkit#2733 Error generated when messages size is too big
    - backport of moby/swarmkit#2870 Fix update out of sequence

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Oct 23, 2019
…v18.09)

full diff: moby/swarmkit@142a737...5c86095

- moby/swarmkit#2892 [18.09 backport] Remove hardcoded IPAM config subnet value for ingress network
    - backport of moby/swarmkit#2890 Remove hardcoded IPAM config subnet value for ingress network
    - fixes [ENGORC-2651](https://docker.atlassian.net/browse/ENGORC-2651)
- moby/swarmkit#2836 [18.09 backport] Switch to go 1.11
    - backport of moby/swarmkit#2752 Switch to go 1.11
- moby/swarmkit#2901 [18.09 backport] Bump to golang 1.12.9
    - backport of moby/swarmkit#2880 Bump to golang 1.12.9
- moby/swarmkit#2900 [18.09 backport] Fix update out of sequence and increase max recv gRPC message size for nodes and secrets
    - backport of moby/swarmkit#2762 Increased wait time on test utils WaitForCluster and WatchTaskCreate
    - backport of moby/swarmkit#2771 Allow using Configs as CredentialSpecs
        - **second commit only** (attempt to fix weirdly broken tests)
    - backport of moby/swarmkit#2808 Fix flaky tests
    - backport of moby/swarmkit#2866 Swap gometalinter for golangci-lint
    - backport of moby/swarmkit#2869 Increase max recv gRPC message size to initialize connection broker
        - related / similar to moby/moby#38103 / docker-archive/engine#102 cluster: set bigger grpc limit for array requests
        - related / similar to moby/moby#39306 Increase max recv gRPC message size for nodes and secrets
        - fixes moby/swarmkit#2733 Error generated when messages size is too big
    - backport of moby/swarmkit#2870 Fix update out of sequence

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: e06f07ef337ab890f211397d6b408b75a2512dc5
Component: engine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants