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

Fixes bugs for registry content #14986

Merged

Conversation

@bmcelvee bmcelvee added this to the Future Release milestone May 21, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 21, 2019
@bmcelvee bmcelvee force-pushed the bugs-registry-improvement branch 4 times, most recently from eacdf49 to 7bc78d0 Compare May 21, 2019 19:43
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2019
@@ -16,30 +16,30 @@ Manually secure the registry to serve traffic via TLS:
example:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is currently commented out of the documentation, but I made some requested changes just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we shouldn't recommend to secure the registry manually anymore. Inside the cluster it is secured by default. Outside of the cluster it can be accessed only through routes, which should have documentation about certificates and all this stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @dmage! It sounds like we might want to move the entire Securing and exposing the registry topic: https://docs.openshift.com/container-platform/4.1/registry/securing-exposing-registry.html

Is that correct?

Copy link

Choose a reason for hiding this comment

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

@bmcelvee yes, the whole section of registry-securing-manually.adoc should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll go ahead and remove the entire assembly.

@bmcelvee
Copy link
Contributor Author

@sferich888 and @hongkailiu Please take a look, thanks!

@bmcelvee
Copy link
Contributor Author

bmcelvee commented May 21, 2019

@dmage - if you have time, please take a look too.

@bmcelvee
Copy link
Contributor Author

Going ahead and requesting QE review too so I can get the fixes merged sooner.

Thanks!

@wzheng1 Please take a look, thanks again!

+
[NOTE]
====
Since the Image Registry Operator creates the route, it will likely be the
Copy link

Choose a reason for hiding this comment

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

But operator will not enable default route by default, user should enable it :
$oc patch configs.imageregistry.operator.openshift.io cluster -p '{"spec":{"defaultRoute":true}}' --type='merge' -n openshift-image-registry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @wzheng1! Does the user need to enable the route during install or post install? And would this procedure be part of/an update to the securing the registry procedure in the operator docs? https://docs.openshift.com/container-platform/4.1/registry/configuring-registry-operator.html#registry-operator-default-crd_configuring-registry-operator

Copy link

Choose a reason for hiding this comment

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

User cannot enable it during install, can do it post install.

[NOTE]
====
Since the Image Registry Operator creates the route, it will likely be the
`default-route`.
Copy link

Choose a reason for hiding this comment

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

The default route address will be like: default-route-openshift-image-registry.<cluster_name>, for example default-route-openshift-image-registry.apps.qe-521.qe.devcluster.openshift.com

+
----
$ podman-login -u openshift -p $(oc whoami -t) <registry_ip>:<port>
$ podman login -u openshift -p $(oc whoami -t) $(oc get route -n openshift-image-registry default-route -o jsonpath='{.status.ingress[*].host}{"\n"}')
Copy link

Choose a reason for hiding this comment

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

If use podman to login, I suggest to use internal registry address like before:
sh-4.4# podman login -u kubeadmin -p RDhLYKRgyw3JFpG4-9N9yBbLdGK4CtKeAc5aSjFuk3E image-registry.openshift-image-registry.svc:5000
Login Succeeded!

Copy link

@wzheng1 wzheng1 May 22, 2019

Choose a reason for hiding this comment

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

@dmage could you please decide which way is better about my above comment?

Copy link

Choose a reason for hiding this comment

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

Above steps can be added to accessing-the-registry.adoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

The assumptions of this article are not clear. If it assumes that all actions will be from outside of the cluster, then only external routes can be used. That's a good point that most likely the registry will have untrusted certificate.

Copy link

@wzheng1 wzheng1 May 23, 2019

Choose a reason for hiding this comment

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

@bmcelvee so the conclusion is user has two options to access Openshift internal registry as below:

  1. If use internal routes, user has to be inside cluster like below:
    a. Access into node:
    $oc get nodes --------------this is to get node address
    $oc debug nodes/<node_address>
    b. Use podman command to log into internal route: image-registry.openshift-image-registry.svc:5000:
    sh-4.4# podman login -u kubeadmin -p RDhLYKRgyw3JFpG4-9N9yBbLdGK4CtKeAc5aSjFuk3E image-registry.openshift-image-registry.svc:5000
    Login Succeeded!
    c. Then you can use $podman pull/push;

  2. If use external routes, user need to enable defaultRoute=true first, then create a CA file to copy to local like this:
    a.Get wildcard route ca
    $oc get secret router-certs-default -n openshift-ingress -o yaml
    $echo <tls.crt_content> | base64 -d > ca.crt
    b. Trust ca in your client platform
    $sudo cp ca.crt /etc/pki/ca-trust/source/anchors/<external_route>.crt
    #sudo update-ca-trust enable
    c. Reload docker
    #sudo systemctl daemon-reload
    #sudo systemctl restart docker
    d.the user can use docker to login with this default address locally: default-route-openshift-image-registry.<cluster_name>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much, @wzheng1! Wildcard routes are not supported in 4.1, so I've documented the external route method locally and can put it in the documentation if wildcard routes are offered in the future .

Copy link

@wzheng1 wzheng1 May 27, 2019

Choose a reason for hiding this comment

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

@bmcelvee By re-reviewing the doc registry-accessing-directly.adoc, I wanna highlight my above 2 methods are separated with each other, one is with internal route address: image-registry.openshift-image-registry.svc:5000 ; another is to login with external route address enabled by user:
$ oc get route -n openshift-image-registry default-route -o jsonpath='{.status.ingress[*].host}{"\n"}'
default-route-openshift-image-registry.apps.qe-wewang-527.qe.devcluster.openshift.com;
If user use external route address, it is OK to use docker command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @wzheng1! So far I've only included the steps for accessing internally since wildcard routing is not supported in 4.1.

I'm a bit confused on the command we should document for the log in step, would it be better as one of the following:

  1. $ podman login -u openshift -p $(oc whoami -t) $(oc get route -n openshift-image-registry default-route -o jsonpath='{.status.ingress[*].host}{"\n"}')

  2. podman login -u kubeadmin -p RDhLYKRgyw3JFpG4-9N9yBbLdGK4CtKeAc5aSjFuk3E image-registry.openshift-image-registry.svc:5000

  3. podman login -u kubeadmin -p $(oc whoami -t) image-registry.openshift-image-registry.svc:5000

Copy link

Choose a reason for hiding this comment

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

I vote for item #3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks again! :) Updated.

----

.. Tag the new image with the form `<registry_ip>:<port>/<project>/<image>`.
The project name must appear in this pull specification for {product-title} to
correctly place and later access the image in the registry:
+
----
$ podman-tag name.io/image 172.30.124.220:5000/openshift/image
$ podman tag name.io/image 172.30.124.220:5000/openshift/image
Copy link

Choose a reason for hiding this comment

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

$ podman tag name.io/image image-registry.openshift-image-registry.svc:5000/openshift/image

to push the image.
====

.. Push the newly-tagged image to your registry:
+
----
$ podman-push 172.30.124.220:5000/openshift/image
$ podman push 172.30.124.220:5000/openshift/image
Copy link

Choose a reason for hiding this comment

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

$ podman tag name.io/image image-registry.openshift-image-registry.svc:5000/openshift/image

+
----
$ oc adm ca create-server-cert \
--signer-cert=/etc/origin/master/ca.crt \
--signer-key=/etc/origin/master/ca.key \
--signer-serial=/etc/origin/master/ca.serial.txt \
--hostnames='podman-registry.default.svc.cluster.local,podman-registry.default.svc,172.30.124.220' \
--hostnames='openshift-image-registry.default.svc.cluster.local,openshift-image-registry.default.svc,172.30.124.220' \
Copy link

Choose a reason for hiding this comment

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

same here

@@ -48,7 +48,7 @@ If the router will be exposed externally, add the public route host name in the
`--hostnames` flag:
+
----
--hostnames='mypodman-registry.example.com,podman-registry.default.svc.cluster.local,172.30.124.220 \
--hostnames='myopenshift-image-registry.example.com,openshift-image-registry.default.svc.cluster.local,172.30.124.220 \
Copy link

Choose a reason for hiding this comment

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

same here

@@ -134,7 +134,7 @@ done on all nodes in the cluster:
----
$ dcertsdir=/etc/podman/certs.d
$ destdir_addr=$dcertsdir/172.30.124.220:5000
$ destdir_name=$dcertsdir/podman-registry.default.svc.cluster.local:5000
$ destdir_name=$dcertsdir/openshift-image-registry.default.svc.cluster.local:5000

$ sudo mkdir -p $destdir_addr $destdir_name
$ sudo cp ca.crt $destdir_addr //<1>
Copy link

Choose a reason for hiding this comment

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

@dmage whether above steps works in 4.x now?

@wzheng1
Copy link

wzheng1 commented May 22, 2019

@bmcelvee comments added, some of them need @dmage double confirm.

. Log in to the registry directly:

.. Ensure you are logged in to {product-title} as a *regular user*:
. Ensure you are logged in to {product-title} as a cluster administrator:
+
----
$ oc login
Copy link
Contributor

Choose a reason for hiding this comment

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

We should show a user how to validate they are an admin here!

. Log in to the registry directly:

.. Ensure you are logged in to {product-title} as a *regular user*:
. Ensure you are logged in to {product-title} as a cluster administrator:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need to be an administrator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmage What level of user do you need to be in order to access the registry? (I originally brought this content forward from 3.x, and am trying to clarify/update it.)

Copy link

Choose a reason for hiding this comment

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

A regular user is fine.

+
----
$ podman-login -u openshift -p $(oc whoami -t) <registry_ip>:<port>
$ podman login -u openshift -p $(oc whoami -t) $(oc get route -n openshift-image-registry default-route -o jsonpath='{.status.ingress[*].host}{"\n"}')
Copy link
Contributor

Choose a reason for hiding this comment

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

The assumptions of this article are not clear. If it assumes that all actions will be from outside of the cluster, then only external routes can be used. That's a good point that most likely the registry will have untrusted certificate.

@@ -16,30 +16,30 @@ Manually secure the registry to serve traffic via TLS:
example:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we shouldn't recommend to secure the registry manually anymore. Inside the cluster it is secured by default. Outside of the cluster it can be accessed only through routes, which should have documentation about certificates and all this stuff.

@bmcelvee bmcelvee force-pushed the bugs-registry-improvement branch 5 times, most recently from 0f38a72 to 1dcd584 Compare May 29, 2019 20:31
@bmcelvee bmcelvee force-pushed the bugs-registry-improvement branch from 1dcd584 to 4d98c73 Compare May 30, 2019 13:15
@bmcelvee
Copy link
Contributor Author

Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

LGTM, just some nits.

* For pulling images, for example when using the `podman-pull` command,
the user must have the *registry-viewer* role. To add this role:
. Access the node:
.. Get the node's address:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a single step w/o a substep?

+
----
$ oc policy add-role-to-user registry-editor <user_name>
$ oc login -u kubeadmin -p <PASSWORD_FROM_INSTALL_LOG>
Copy link
Contributor

Choose a reason for hiding this comment

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

Replaceable could be lowercase.

modules/registry-accessing-directly.adoc Show resolved Hide resolved
+
----
$ podman-login -u openshift -p $(oc whoami -t) <registry_ip>:<port>
Login Succeeded!
----
+
[NOTE]
====
You can pass any value for the username, the token contains all necessary
Copy link
Contributor

Choose a reason for hiding this comment

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

s/username/user name/

Copy link
Contributor

Choose a reason for hiding this comment

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

x2 in this para.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also maybe semicolon instead: "You can pass any value for the user name; the token contains all necessary information."

@adellape adellape added the peer-review-done Signifies that the peer review team has reviewed this PR label May 30, 2019
@bmcelvee bmcelvee force-pushed the bugs-registry-improvement branch from 4d98c73 to 975cc47 Compare May 30, 2019 17:43
...
cf2616975b4a: Image successfully pushed
Digest: sha256:3662dd821983bc4326bee12caec61367e7fb6f6a3ee547cbaff98f77403cab55
$ podman push name.io/image image-registry.openshift-image-registry.svc:5000/openshift/image
Copy link

Choose a reason for hiding this comment

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

This command should be:
$ podman push image-registry.openshift-image-registry.svc:5000/openshift/image

Copy link

@wzheng1 wzheng1 May 31, 2019

Choose a reason for hiding this comment

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

Sorry for missing it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much, @wzheng1!

@bmcelvee bmcelvee force-pushed the bugs-registry-improvement branch from 975cc47 to 18ee0ef Compare May 31, 2019 16:49
@bmcelvee
Copy link
Contributor Author

All bugs moved to verified, and I pushed the final request. Merging the PR now, and we can make any additional edits in new PRs. Thanks!

@bmcelvee bmcelvee merged commit c1ea042 into openshift:enterprise-4.1 May 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.1 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants