Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

feat(images): use imagePullSecret when pulling a private image #718

Merged
merged 2 commits into from
May 18, 2016

Conversation

helgi
Copy link
Contributor

@helgi helgi commented May 9, 2016

use a k8s Secret to encode a .docker/config.json to completely bypass the Controller when it comes to images if a private registry is being used

Handles the various possible errors coming from k8s and bubble that up through to the end user, similar to how the deis dockerclient can do.
k8s adds more information on top of what docker gives so an error message bubbled up from this compared to when the Controller was handling the Docker interaction

ref #639

@helgi helgi self-assigned this May 9, 2016
@helgi helgi added this to the v2.0-beta4 milestone May 9, 2016
@jchauncey
Copy link
Member

Should we be manually testing this and if so how?

@helgi
Copy link
Contributor Author

helgi commented May 9, 2016

Testing Instructions

Please provide a detailed list for how to test the changes in this PR.

  • Create a Deis Cluster
  • Register an app
  • Create a Private Repo on Quay.io
  • Set user / pw via deis registry:set username=<quay_user> password=<quay_token> for the app
  • Do deis pull referencing an image in a private repo

Also, please provide a description of the desired result after the tester completes the above steps.

  • The app called "abcd" should be deployed using the private image
  • Describe the pod to ensure that image pull secret is set

@helgi
Copy link
Contributor Author

helgi commented May 9, 2016

One e2e caught a scenario I had not accounted for so I need to investigate that better, looks like I am missing a data guard somewhere

@helgi helgi force-pushed the image_secret branch 2 times, most recently from 5e38e49 to 9d8fa91 Compare May 9, 2016 20:51
@codecov-io
Copy link

codecov-io commented May 9, 2016

Current coverage is 84.60%

Merging #718 into master will decrease coverage by 1.05%

@@             master       #718   diff @@
==========================================
  Files            29         29          
  Lines          2585       2649    +64   
  Methods           0          0          
  Messages          0          0          
  Branches        407        421    +14   
==========================================
+ Hits           2214       2241    +27   
- Misses          259        290    +31   
- Partials        112        118     +6   

Powered by Codecov. Last updated by 94323f6...312b64a

@jchauncey jchauncey added the LGTM1 label May 9, 2016
@jchauncey
Copy link
Member

jchauncey commented May 9, 2016

╭─jonathanchauncey at ENG000637 in ~/projects/deis/example-dockerfile-http on master✔ using ‹2.0.0-p645›
╰─± deis pull -a foo quay.io/jchauncey/privateapp
Creating build... done
╭─jonathanchauncey at ENG000637 in ~/projects/deis/example-dockerfile-http on master✔ using ‹2.0.0-p645›
╰─± kgpo --namespace=foo
NAME               READY     STATUS    RESTARTS   AGE
foo-v3-cmd-0tcd7   1/1       Running   0          2m
╭─jonathanchauncey at ENG000637 in ~/projects/deis/example-dockerfile-http on master✔ using ‹2.0.0-p645›
╰─± curl foo.mydomain.io
Powered by Deis

@helgi
Copy link
Contributor Author

helgi commented May 9, 2016

Currently blocked on #721

@jchauncey
Copy link
Member

moving to rc1

@bacongobbler
Copy link
Member

bacongobbler commented May 17, 2016

how is this blocked by #721? Just call registry.publish_release() to pull the image. The image will be pushed to the internal registry, which then you can introspect and bypass any changes necessary to facilitate #721.

@bacongobbler
Copy link
Member

publish_release(source_image, self.image, deis_registry, self.get_registry_auth())
pushes the image into the registry so it should be available for inspection.

@helgi
Copy link
Contributor Author

helgi commented May 17, 2016

@bacongobbler that's not the point, I am aware of how the code works and how I could pull the image into the local registry.

The point here is we are having k8s fully pulling images and dealing with the secrets but then having Controller pull the image into the insecure registry just to inspect a port, causing false sense of security and adding in additional download time and storage requirements. That's what is being discussed in #721

@bacongobbler
Copy link
Member

Right, but that's something we can implement later when we come to a conclusion on how we can resolve it. For now let's just follow what we've been doing since v1. Whether or not it's stored in the registry doesn't matter since the image is in the cluster anyways.

@helgi
Copy link
Contributor Author

helgi commented May 17, 2016

Maybe what's being lost in translation here is that Authenticated External Registry is net new feature functionality as a fully support part of Deis (not a hack) so this scenario doesn't really compare to v1, which allowed for public only images (unless you copied configs over).

Also, by default the port is 5000 and can be overwritten with deis config:set PORT=5001

Doing this post 2.0 wouldn't be very viable since you'd be moving the user from introspection to the above, so we couldn't do this until 3.0

helgi added 2 commits May 17, 2016 19:31
use a k8s Secret to encode a '.docker/config.json' to completely bypass the Controller when it comes to images if a private registry is being used

Handles the various possible errors coming from k8s and bubble that up through to the end user, similar to how the deis dockerclient can do.
k8s adds more information on top of what docker gives so an error message bubbled up from this compared to when the Controller was handling the Docker interaction

ref deis#639
… instead of auto discovering port

Rationale here is to not pull down the image since Kubernetes now owns the downloading and interaction with the Private Registry. Pulling an image down into the Deis Registry will strip away the benefits of the security given.

This change moves the docker port auto-discovery into the docker-client and general port handling into Release model, making the Scheduler unaware of the inherent logic. Ports are still auto-discovered on public images but when Private Registry is used the default port is 5000 and if the user wants to use a different one then they need to do config:set PORT=3000

Logic may change in the future where a user can use Private Registry but pull into the local registry, and get port auto-discovery, at the cost of security.
@helgi helgi merged commit 9979523 into deis:master May 18, 2016
@helgi helgi deleted the image_secret branch May 18, 2016 18:09
@helgi helgi mentioned this pull request May 18, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants