-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add support for Kubernetes Pull secrets and Pull policies #1617
Conversation
This implements the client side thing of docker/compose-on-kubernetes#27 option 2 |
Codecov Report
@@ Coverage Diff @@
## master #1617 +/- ##
==========================================
+ Coverage 56.1% 56.17% +0.06%
==========================================
Files 300 300
Lines 20601 20617 +16
==========================================
+ Hits 11559 11581 +22
+ Misses 8209 8203 -6
Partials 833 833 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking until #1615 get merged, LGTM otherwise.
1905a6f
to
7ca3ffd
Compare
7ca3ffd
to
2b49e95
Compare
058e8ed
to
d58defb
Compare
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
d58defb
to
cd51113
Compare
Just rebased on master. @silvin-lubecki @vdemeester PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor changes
01affb8
to
5b93b7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you!
return latest.ServiceConfig{}, err | ||
} | ||
if pullSecret != "" && !capabilities.hasPullSecrets { | ||
return latest.ServiceConfig{}, errors.Errorf("stack API version %s does not support pull secrets (field %q), please use version v1alpha3 or higher", capabilities.apiVersion, PullSecretExtraField) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering; why would I have to manually provide a pull-secret? Would it be possible to automatically resolve credentials for an image and pass them as a pull-secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been discussed with @justincormack here: docker/compose-on-kubernetes#27 and the current Swarm seem not to be a very good practice (using user credentials instead of a custom service account)
Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
5b93b7a
to
d184c09
Compare
@thaJeztah, fields are now unexposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extra information on the background.
We should indeed work on designing something that can be used both for swarm and k8s. (perhaps allow swarm services to use swarm secrets for pulling?)
Pull policy is also something that needs to be improved for swarm services, so we can work on that as well.
We can change those later, as this PR is using extension fields (so does not modify the compose file spec)
LGTM
Based on #1615
- What I did
Added support for referencing pull secrets in Compose Files deployed to Kubernetes, and specifying a Pull Policy
- How I did it
Interpret
x-pull-secret
andx-pull-policy
extra fields on ServiceConfig to be mapped to the PullSecret and PullPolicy fields in Compose on Kubernetes v1alpha3 ServiceConfig.- How to verify it
The PR comes with unit tests covering:
- Description for the changelog
x-pull-secret: some-pull-secret
in compose-files service configs.x-pull-policy: <Never|Always|IfNotPresent>
in compose-files service configs.- A picture of a cute animal (not mandatory but encouraged)