Add a "registry" action that creates a private docker registry #97

axinojolais · Jan 5, 2017

Hi,

This adds a action to the kubernetes-worker charm that creates (or deletes) a private, TLS-enabled, authentified (htpasswd) Docker repository in the deployed kubernetes cluster.

Comments welcome, of course !

Thanks

caio1982 · Jan 9, 2017

caio1982 reviewed Jan 9, 2017

View changes

Where is this used in the current diff?

Well it looks like it's not. :| Let me take a look

This is fixed now, by the way.

axinojolais · Jan 12, 2017

axinojolais reviewed Jan 12, 2017

View changes

htpasswd also needs to be base64ified, I'll fix in the next commit

Do we really need to base64 encode it? we're passing around TLS certs which are carriage return sensitive over the relation wires and haven't base64 encoded them. It hasn't seemed to be an issue yet, we might be playing with fire here, but in my experience, base64 encoding the contents is an extra, unnecessary step.

The secret used by the registry image needs to be base64 encoded. We can either do it here, or in the code of the action. This is the same for the certificates by the way. What do you think ?

-edit- i misunderstood the goal/use-case here. What you've got is a great first iteration. We had a bit of back/forth on IRC and I feel that you're on the right path. With some slight tweaks to make the process clear to the user what they will be providing. I am +1 to that effort.

axinojolais · Jan 17, 2017

I still need to validate the last commit

chuckbutler · Jan 17, 2017

chuckbutler reviewed Jan 17, 2017

View changes

I have not tested this branch yet, so i'm holding off on approving this branch to be merged, but I am +1 to this . I'll get this built and tested directly after we complete the 1.5.2 release push we are in now. Thanks for the contributions @axinojolais

+1 to not making the user bae64 encode the values I think this will be a better user experience. Even though its only a single removal of a piped command. 👍

Nice addition here!

Nice. Thanks for pulling this server side instead of client side.

axinojolais · Jan 17, 2017

Ha, I also need to add the ConfigMap mentioned in #100 (comment) - which is actually a required dependency for this PR.

axinojolais · Jan 18, 2017

This is tested, and works fine. However, this PR needs #100 to be merged first.

chuckbutler · Jan 23, 2017

Thanks for the follow up, will give this a look later today / early tomorrow and see if we can get it landed. We're cycling on getting our code upstream so we're not lagging behind in the upstream archive as we are today.

This may bring some concerns, but its not your fault/concern to track and we will ensure we carry this forward into upstream. Thanks for the contribution and patience during the review cycle axino!

axinojolais · Jan 25, 2017

To be complete, this will require 2 more changes : a secret to use the registry, and a modification of the "default" service account to use this secret by default.

chuckbutler · Jan 26, 2017

inline note:

we'll need to pull this in and retarget against kubernetes/kubernetes once we've passed the code review segment of the submission.

axinojolais · Jan 31, 2017

OK this is now complete and ready for review

chuckbutler · Jan 31, 2017

I've left some minor nits around on #100 requesting documentation updates regarding the added feature.

I still need to pull this in and give it a test, but initial tests were good with the configmap addition, I feel that this will be a simple test as well. Thanks for the work done here @axinojolais and your patience. We haven't forgotten about this PR, we're just tasked in 8 different directions at the moment. Will you perchance be at the charmer summit? We can certainly pull aside some time to finish these up and get your PR's landed against github.com/kubernetes/kubernetes then if you're available.

axinojolais · Jan 31, 2017

Nope, a bug got in ! Will fix tomorrow.

chuckbutler · Jan 31, 2017

Glad you're on top of this keeping the PR moving forward while we shuffle things around. Thanks for the update!

axinojolais · Feb 1, 2017

Replaced by kubernetes#40814

Add a "registry" action that creates a private docker registry

e603f35

Make the "registry" action actually use the htpasswd file given as pa…
…rameter

7fd1c6e

encode to base64 in the action, don't ask for already encoded value
This simplifies calling the action a bit.

9fb5bcf

make the "registry" action create an appropriate ConfigMap (or patch …
…the existing one)

c96ff65

create registry-access secret, patch the "default" serviceaccount to …
…use it automatically, lint fixes

57e2bae

axinojolais closed this Feb 1, 2017

axinojolais referenced this pull request in kubernetes/kubernetes Feb 2, 2017
Closed
Add a juju action to create a docker registry inside the cluster #40814

+        +registry:
+        +     description: Create a private Docker registry
+        +     params:
+        +      htpasswd:

+        +# Registry creation
+        +#
+        +# juju run-action kubernetes-worker/0 registry domain=myregistry.internal \
+        +# htpasswd="$(cat htpass)" ingress=true \

@@ -23,17 +23,17 @@ registry:
                  description: An htpasswd file used for authentication.
                tlscert:
                  type: string
-        -        description: base64 encoded TLS certificate for the registry.
+        +        description: TLS certificate for the registry.

                domain:
                  type: string
                  description: The domain name for the registry. Must match the Common Name of the certificate.
                ingress:
                  type: boolean
                  default: false
-        -        description: Create an Ingress object for the registry.
+        +        description: Create an Ingress object for the registry (or delete said object if "delete" is True)

@@ -35,6 +41,10 @@ for param in ('tlscert', 'tlskey', 'domain', 'htpasswd'):
                  error = "failure, parameter {} is required".format(param)
                  action_set({key: error})
                  param_error = True
+        +
+        +    if param in b64_params:
+        +        value = b64encode(value.encode())

juju-solutions/kubernetes forked from kubernetes/kubernetes

Add a "registry" action that creates a private docker registry #97

axinojolais commented Jan 5, 2017

caio1982 reviewed Jan 9, 2017

View changes

axinojolais reviewed Jan 12, 2017

View changes

axinojolais commented Jan 17, 2017

chuckbutler reviewed Jan 17, 2017

View changes

axinojolais commented Jan 17, 2017

axinojolais commented Jan 18, 2017

chuckbutler commented Jan 23, 2017

axinojolais commented Jan 25, 2017

chuckbutler commented Jan 26, 2017

axinojolais commented Jan 31, 2017

chuckbutler commented Jan 31, 2017

axinojolais commented Jan 31, 2017

chuckbutler commented Jan 31, 2017

axinojolais commented Feb 1, 2017

axinojolais closed this Feb 1, 2017

axinojolais referenced this pull request in kubernetes/kubernetes Feb 2, 2017

Add a juju action to create a docker registry inside the cluster #40814

juju-solutions/kubernetes forked from kubernetes/kubernetes

Join GitHub today

Add a "registry" action that creates a private docker registry #97

Conversation

axinojolais commented Jan 5, 2017

caio1982 reviewed Jan 9, 2017 View changes

axinojolais reviewed Jan 12, 2017 View changes

chuckbutler Jan 17, 2017 • edited Edited 1 time chuckbutler Jan 17, 2017

axinojolais commented Jan 17, 2017

chuckbutler reviewed Jan 17, 2017 View changes

axinojolais commented Jan 17, 2017

axinojolais commented Jan 18, 2017

chuckbutler commented Jan 23, 2017

axinojolais commented Jan 25, 2017

chuckbutler commented Jan 26, 2017

axinojolais commented Jan 31, 2017

chuckbutler commented Jan 31, 2017

axinojolais commented Jan 31, 2017

chuckbutler commented Jan 31, 2017

axinojolais commented Feb 1, 2017

axinojolais closed this Feb 1, 2017

axinojolais referenced this pull request in kubernetes/kubernetes Feb 2, 2017

caio1982 reviewed Jan 9, 2017

View changes

axinojolais reviewed Jan 12, 2017

View changes

chuckbutler Jan 17, 2017 •

Edited 1 time

chuckbutler
Jan 17, 2017

chuckbutler reviewed Jan 17, 2017

View changes