Add independent registry auth support #5128

michaelgugino · 2017-08-18T02:54:21Z

Added the ability to support authentication for independent / 3rd party
registries. This commit will allow users to provide a oreg_auth_user and
oreg_auth_password to dynmically generate a docker config.json file.

The docker config.json file can be used by openshift to authenticate to
independent / 3rd party registries. oreg_host must supply endpoint connection
info in the form of 'hostname.com:port', with (optional) port 443 default.

To update the config.json on a later run, the user can specify
oreg_auth_credentials_replace=False to update the credentials.

These settings must be used in tandem with oreg_url

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1316341

michaelgugino · 2017-08-18T02:56:21Z

I have tested the logic of the new statements to ensure the auth file is placed in the suggested location. I have not yet tested to see if openshift will actually use this auth with a 3rd party registry.

jianlinliu · 2017-08-18T04:03:58Z

roles/openshift_master/tasks/main.yml

+    path: "{{ registry_auth_credentials_path }}"
+  when:
+  - registry_auth_user is defined
+  - openshift_examples_modify_imagestreams | bool


Personally I think openshift_examples_modify_imagestreams have nothing with registry auth here.

You could be right. Perhats registry_auth_user check is enough?

Yeah I agree, only need the first condition.

jianlinliu · 2017-08-18T04:05:33Z

roles/openshift_master/tasks/main.yml

+  register: master_registry_auth_credentials_stat
+
+- name: Create credentials for registry auth
+  command: "docker --config={{ registry_auth_credentials_path }} login -u {{ registry_auth_user }} -p {{ registry_auth_token }} {{ oreg_host }}"


where should user define registry_auth_user and registry_auth_token? In inventory host file?

Personally I would put those values in a vault file in group_vars. You could also put those values in inventory, yes, if you don't mind your service account's username/password living in there. You can also pass those values via extra_vars.

It is better to mention that in inventory/byo/hosts.ose.example, give some example to show user how to define registry_auth_user and registry_auth_token.

jianlinliu

some comments from jialiu

michaelgugino · 2017-08-18T15:53:45Z

so, the code I committed executes as expected. Also somewhat as expected, openshift still does not try to auth or consume those credentials in any way:

Aug 18 11:50:35 ip-172-18-13-138.ec2.internal origin-node[26303]: E0818 11:50:35.665096   26303 kuberuntime_manager.go:619] createPodSandbox for pod "docker-registry-1-deploy_default(579587e8-842b-11e7-bd06-0e81fb6035e4)" failed: rpc error: code = 2 desc = unable to pull sandbox image "registry.reg-aws.openshift.com:443/openshift3/ose-pod:v3.6.0": unauthorized: authentication required

michaelgugino · 2017-08-18T17:02:47Z

Looks like the instructions from bugzilla were close. I'm going to try to update the location of the .docker/config.json to /root to see if that works. On origin /root is the home dir.

https://kubernetes.io/docs/concepts/containers/images/#configuring-nodes-to-authenticate-to-a-private-repository

michaelgugino · 2017-08-21T19:39:10Z

This is currently blocked on: openshift/origin#15880

michaelgugino · 2017-08-21T19:55:56Z

How to test:

# Change the following to "True" to replace existing credentials on master/nodes.
oreg_auth_credentials_replace: False
oreg_auth_user: some_user
oreg_auth_password: some_token

# For OpenShift registry, oreg_auth_password can be generated as follows:
# oc login --config=~/.kube/example.com --username=myuser --password='mypass' https://api.example.com
# oc --config=~/.kube/example.com -t

# port is optional
oreg_host: my.reg.url:443
oreg_url: my.reg.url:443/openshift3/ose-${component}:${version}

openshift_examples_modify_imagestreams: True

Currently blocked on 3.6, but works on 1.5.

sdodson · 2017-08-21T19:48:26Z

roles/openshift_master/defaults/main.yml

@@ -19,3 +19,6 @@ r_openshift_master_os_firewall_allow:
 - service: etcd embedded
  port: 4001/tcp
  cond: "{{ groups.oo_etcd_to_config | default([]) | length == 0 }}"
+
+registry_auth_credentials_path: "/var/lib/{{ openshift.common.service_type }}/.docker"


registry_auth_credentials_path: "{{ openshift.common.data_dir }}/.docker" instead? This should be pretty stable but that's the var we use elsewhere.

sdodson · 2017-08-21T19:49:23Z

roles/openshift_master/tasks/main.yml

+    path: "{{ registry_auth_credentials_path }}"
+  when:
+  - registry_auth_user is defined
+  - openshift_examples_modify_imagestreams | bool


Yeah I agree, only need the first condition.

sdodson · 2017-08-21T19:57:31Z

inventory/byo/hosts.ose.example

+# If oreg_url points to a registry requiring authentication, provide the following:
+oreg_host: example.com:443
+registry_auth_user: some_user
+registry_auth_token: 'my-token'


Just to ensure commonality with most of our variables what about openshift_registry_auth_host openshift_registry_auth_user and openshift_registry_auth_password instead? Password over token because it's more generic, for our use case where it's an openshift registry it happens to be a token but elsewhere it may just be a password.

how about oreg_* as it matches the bits that we're modifying?

oreg_auth_password seems fine.

That's fine.

Should not we get oreg_host from oreg_url? Seem like oreg_host is a little redundant.

I thought about this. I feel it should actually be the other way around, but that's outside the scope of this PR.

I suppose this would also work in something like this:
oreg_url: somehost.com:443/somedir/registry/openshift3/{{ component }}/{{ version }}
oreg_host: somehost.com:443/somedir/registry

What's the best way to parse oreg_url to derive oreg_host in both situations?
Better situation is this:
oreg_host = somehost.example.com
oreg_url = {{ oreg_host }}/{{ oreg_suffix }}

But that would probably break existing users and would require some refactoring.

There's a few places that parse oreg_url to get registry host.

registry_host: "{{ registry_url.split('/')[0] if '.' in registry_url.split('/')[0] else '' }}"

Yes, and if someone's registry is sitting at example.com/myregistry (ie, it's sitting behind a reverse proxy or something of that nature) then this will break for this purpose. For example: https://docs.docker.com/registry/recipes/nginx/#setting-things-up

I'm happy to change it, but I think we're introducing a bug, and I think it's bad practice to split variables rather than combine them.

sdodson · 2017-08-24T18:21:28Z

inventory/byo/hosts.ose.example

@@ -167,6 +167,13 @@ openshift_release=v3.6
 # modify image streams to point at that registry by setting the following to true
 #openshift_examples_modify_imagestreams=true

+# If oreg_url points to a registry requiring authentication, provide the following:
+oreg_auth_user=some_user
+oreg_auth_password='my-pass'


Lets comment these and make it clear that you must set oreg_url for this to work, otherwise we don't have a registry host to log into. otherwise LGTM.

sdodson · 2017-08-24T19:32:25Z

aos-ci-test

sdodson · 2017-08-24T19:32:28Z

[merge]

sdodson · 2017-08-24T19:37:23Z

Needs a release-3.6 branch backport too

openshift-bot · 2017-08-24T19:48:42Z

[test]ing while waiting on the merge queue

openshift-bot · 2017-08-25T07:12:09Z

error: aos-ci-jenkins/OS_3.6_containerized for de6ccdf (logs)

openshift-bot · 2017-08-25T07:26:01Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for de6ccdf (logs)

sdodson · 2017-08-28T17:47:33Z

aos-ci-test

Added the ability to support authentication for independent / 3rd party registries. This commit will allow users to provide a `oreg_auth_user` and `oreg_auth_password` to dynmically generate a docker config.json file. The docker config.json file can be used by openshift to authenticate to independent / 3rd party registries. `oreg_host` must supply endpoint connection info in the form of 'hostname.com:port', with (optional) port 443 default. To update the config.json on a later run, the user can specify `oreg_auth_credentials_replace=False` to update the credentials. These settings must be used in tandem with `oreg_url` Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1316341

openshift-bot · 2017-08-28T17:55:42Z

Evaluated for openshift ansible merge up to 5815311

openshift-bot · 2017-08-28T17:55:43Z

Evaluated for openshift ansible test up to 5815311

openshift-bot · 2017-08-28T19:57:57Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_openshift_ansible/549/) (Base Commit: ca5ebcb) (PR Branch Commit: 5815311)

openshift-bot · 2017-08-28T23:04:10Z

success: "aos-ci-jenkins/OS_3.6_NOT_containerized, aos-ci-jenkins/OS_3.6_NOT_containerized_e2e_tests" for 5815311 (logs)

openshift-bot · 2017-08-28T23:06:58Z

success: "aos-ci-jenkins/OS_3.6_containerized, aos-ci-jenkins/OS_3.6_containerized_e2e_tests" for 5815311 (logs)

openshift-bot · 2017-08-29T14:07:41Z

continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/940/) (Base Commit: 60d5448) (PR Branch Commit: 5815311)

michaelgugino requested a review from sdodson August 18, 2017 02:54

michaelgugino force-pushed the reg-auth branch from b765139 to e6c0953 Compare August 18, 2017 02:57

jianlinliu reviewed Aug 18, 2017

View reviewed changes

michaelgugino force-pushed the reg-auth branch from e6c0953 to a6b8b24 Compare August 21, 2017 19:53

sdodson reviewed Aug 21, 2017

View reviewed changes

michaelgugino force-pushed the reg-auth branch 2 times, most recently from e3338f3 to b0efbf6 Compare August 23, 2017 14:25

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2017

michaelgugino force-pushed the reg-auth branch from b0efbf6 to b5a7c96 Compare August 23, 2017 20:24

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2017

sdodson reviewed Aug 24, 2017

View reviewed changes

michaelgugino force-pushed the reg-auth branch from b5a7c96 to de6ccdf Compare August 24, 2017 18:26

sdodson approved these changes Aug 24, 2017

View reviewed changes

This was referenced Aug 24, 2017

[1.4] Add independent registry auth support #5164

Merged

[1.5] Add independent registry auth support #5163

Merged

sdodson added affects_3.4 affects_3.5 affects_3.6 backported_3.4 backported_3.5 labels Aug 24, 2017

michaelgugino force-pushed the reg-auth branch from de6ccdf to 5815311 Compare August 28, 2017 17:49

sdodson merged commit 5e32de3 into openshift:master Aug 29, 2017

Add independent registry auth support #5128

Add independent registry auth support #5128

Conversation

michaelgugino commented Aug 18, 2017 • edited Loading

michaelgugino commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianlinliu left a comment

Choose a reason for hiding this comment

michaelgugino commented Aug 18, 2017

michaelgugino commented Aug 18, 2017 • edited Loading

michaelgugino commented Aug 21, 2017

michaelgugino commented Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdodson commented Aug 24, 2017

sdodson commented Aug 24, 2017

sdodson commented Aug 24, 2017

openshift-bot commented Aug 24, 2017

openshift-bot commented Aug 25, 2017

openshift-bot commented Aug 25, 2017

sdodson commented Aug 28, 2017

openshift-bot commented Aug 28, 2017

openshift-bot commented Aug 28, 2017

openshift-bot commented Aug 28, 2017

openshift-bot commented Aug 28, 2017

openshift-bot commented Aug 28, 2017

openshift-bot commented Aug 29, 2017

michaelgugino commented Aug 18, 2017 •

edited

Loading

michaelgugino commented Aug 18, 2017 •

edited

Loading

michaelgugino commented Aug 21, 2017 •

edited

Loading