Support login to the integrated registry via CLI #18930

smarterclayton · 2018-03-10T23:26:04Z

Adds a new oc image login command that allows the user to easily log
in to the integrated registry. By default it tries to use the current
user token (but supports -z for service accounts) and looks up the
public or internal hostname by finding image streams in the current
namespace or the 'openshift' namespace (which is usually populated).

This did highlight that openshift ansible is not setting externalRegistryHostname by default in the install which it should be doing.

smarterclayton · 2018-03-11T22:31:04Z

@bparees for 90% of clusters this will do the right thing once we start forcing registryExternalHostname, since everyone is going to have stuff in openshift. Eventually when david's changes for logging /metrics route publishing to config map land we can also push the registry URL.

bparees · 2018-03-11T22:35:23Z

i don't suppose i could convince you that it should be "oc registry" and not "oc image"?

smarterclayton · 2018-03-11T22:41:13Z

if we have commands that work on both images and registries would we want two top level commands? oc image mirror oc image rebase oc image login oc image import oc image add-layer oc registry mirror oc registry rebase oc registry login oc registry import oc registry add-layer ? Agree registry sounds better for some, but I'd also prefer for a top level command to have more related subcommands (i.e. two almost empty top level commands makes me sad). Agree registry will be more discoverable for some.

…

On Sun, Mar 11, 2018 at 6:35 PM, Ben Parees ***@***.***> wrote: i don't suppose i could convince you that it should be "oc registry" and not "oc image"? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18930 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_r2NuwozFuhaw4ucNUETsnQJSwoks5tdaaugaJpZM4SlfK4> .

bparees · 2018-03-11T22:52:37Z

if we have commands that work on both images and registries would we want
two top level commands?

I would not say we have commands that work on both images+registries, i'd say we have some commands that work on registries, and commands that work on images.

oc image mirror

operates on images

oc image rebase

operates on images

oc image login

operates on registries

oc image import

operates on images

oc image add-layer

operates on images

If we later want to have a registry info command are we going to be happy calling it "oc image info"?

smarterclayton · 2018-03-19T05:15:20Z

/retest

smarterclayton · 2018-03-21T00:20:03Z

/test all

smarterclayton · 2018-03-21T00:20:33Z

this is updated with feedback and adds a registry info command that functions a bit like whoami for scripting interactions with the server.

smarterclayton · 2018-03-22T02:27:19Z

/retest

smarterclayton · 2018-03-22T20:33:26Z

/retest

soltysh

One small nit but overall lgtm.

soltysh · 2018-03-23T08:45:02Z

pkg/oc/cli/cmd/registry/login/login.go

+			if ref, err := imageapi.ParseDockerImageReference(registry); err == nil {
+				o.HostPort = ref.Registry
+				if internal {
+					fmt.Fprintf(o.ErrOut, "info: Using internal registry hostname %s\n", o.HostPort)


If this is info, use o.Out not o.ErrOut, the latter is only for warning and errors.

We should be using stderr for anything that is not directly tied to the output of the command.

Info, warning, and error are all stderr.

Warning and error -> stderr, if info should be possible to hide use glog with different levels. Any other info is info that should put to stdout just like the rest of the output b/c it's relevant to the user.

No, it shouldn’t. Info is not glog. Info is information relevant to the user that is not a warning. It is not normal output. Stdout is data for processing. Glog is not human facing the way info is.

$ grep -R --include=*.go -E "\"info:" pkg/ pkg//oc/admin/migrate/etcd/ttl.go: fmt.Fprintf(o.Out, "info: Lease #%d with TTL %d created\n", lease.ID, lease.TTL) pkg//oc/admin/migrate/etcd/ttl.go: fmt.Fprintf(o.Out, "info: Attaching lease to %d entries\n", len(objectsResp.Kvs)) pkg//oc/admin/migrate/etcd/ttl.go: fmt.Fprintf(o.Out, "info: Lease already attached to %d entries, no change made\n", alreadyAttached) pkg//oc/admin/migrate/migrator.go: fmt.Fprintf(out, "info: to rerun only failing resources, add --include=%s\n", strings.Join(t.resourcesWithErrors.List(), ",")) pkg//oc/admin/router/router.go: fmt.Fprintf(errout, "info: password for stats user %s has been set to %s\n", cfg.StatsUsername, cfg.StatsPassword) pkg//oc/cli/cmd/image/mirror/mirror.go: fmt.Fprintf(o.ErrOut, "info: Filtered all images from %s, skipping\n", src.ref) pkg//oc/cli/cmd/logs.go: o.KubeLogOptions.Out.Write([]byte(fmt.Sprintf("info: Logs available at %s\n", urlString))) pkg//oc/cli/cmd/set/buildhook.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/buildsecret.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/deploymenthook.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/imagelookup.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/probe.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/routebackends.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/triggers.go: fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/volume.go: fmt.Fprintf(v.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name) pkg//oc/cli/cmd/set/volume.go: fmt.Fprintf(v.Err, "info: Generated volume name: %s\n", name) pkg//oc/cli/util/clientcmd/factory.go: glog.V(1).Infof("info: the output version specified is invalid. %s\n", defaultVersionInfo)

All glog output goes to stderr, but info: is effectively v(0) without the prefix (the prefix is not useful for a human interacting with a cli). The user should have the information. The user should not have to filter that out, i.e.:

$ curl https://$( oc registry info )

shouldn't send info: The registry is internal to curl - it should print that on stderr so the shell captures only the url, not the output

The TTL command is outputting to stdout because it prints nothing else.

smarterclayton · 2018-03-27T22:48:18Z

If no other comments marking.

smarterclayton · 2018-03-28T01:20:48Z

/retest

bparees · 2018-03-28T02:42:14Z

nothing from me

bparees

i lied

bparees · 2018-03-28T02:42:50Z

pkg/oc/cli/cmd/registry/info/info.go

+)
+
+var (
+	loginDesc = templates.LongDesc(`


bparees · 2018-03-28T02:42:57Z

pkg/oc/cli/cmd/registry/info/info.go

+
+		Experimental: This command is under active development and may change without notice.`)
+
+	loginExample = templates.Examples(`


infoExample

bparees · 2018-03-28T02:43:13Z

pkg/oc/cli/cmd/registry/info/info.go

+		Use:     "info ",
+		Short:   "Print info about the integrated registry",
+		Long:    loginDesc,
+		Example: fmt.Sprintf(loginExample, name+" login"),


infoExample

bparees · 2018-03-28T02:43:19Z

pkg/oc/cli/cmd/registry/info/info.go

+	cmd := &cobra.Command{
+		Use:     "info ",
+		Short:   "Print info about the integrated registry",
+		Long:    loginDesc,


mfojtik · 2018-03-28T11:02:15Z

/hold

openshift-ci-robot · 2018-03-28T17:26:50Z

New changes are detected. LGTM label has been removed.

smarterclayton · 2018-03-28T18:44:47Z

/retest

smarterclayton · 2018-03-29T15:01:16Z

/retest

smarterclayton · 2018-03-29T19:28:11Z

/retest

smarterclayton · 2018-03-30T00:32:01Z

/retest

…

On Thu, Mar 29, 2018 at 8:07 PM, OpenShift CI Robot < ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_install 5cfdfa5 <5cfdfa5> link <https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18930/test_pull_request_origin_extended_conformance_install/9716/> /test extended_conformance_install Full PR test history <https://openshift-gce-devel.appspot.com/pr/18930>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/smarterclayton>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18930 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p7MiIdTu4vGq5hijC4VC76TghiUNks5tjXcogaJpZM4SlfK4> .

smarterclayton · 2018-03-30T03:34:49Z

/retest

openshift-bot · 2018-03-30T12:51:49Z

/retest

Please review the full test history for this PR and help us cut down flakes.

smarterclayton · 2018-03-30T15:56:22Z

/retest

Adds a new `oc registry login` command that allows the user to easily log in to the integrated registry. By default it tries to use the current user token (but supports `-z` for service accounts) and looks up the public or internal hostname by finding image streams in the current namespace or the 'openshift' namespace (which is usually populated). Also add `oc registry info` for fetching the current registry DNS name.

openshift-ci-robot · 2018-03-30T20:47:13Z

New changes are detected. LGTM label has been removed.

openshift-ci-robot · 2018-03-30T20:47:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [smarterclayton]
~~docs/man/OWNERS~~ [smarterclayton]
~~pkg/oc/OWNERS~~ [smarterclayton]
~~test/cmd/OWNERS~~ [smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2018

openshift-ci-robot requested review from enj and juanvallejo March 10, 2018 23:26

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2018

smarterclayton force-pushed the login_to_registry branch from 9659e75 to ea39c80 Compare March 11, 2018 20:55

smarterclayton force-pushed the login_to_registry branch from ea39c80 to ad685e1 Compare March 18, 2018 20:31

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 18, 2018

smarterclayton force-pushed the login_to_registry branch 5 times, most recently from d63f899 to d79b771 Compare March 20, 2018 15:23

smarterclayton force-pushed the login_to_registry branch from d79b771 to fd7dd78 Compare March 22, 2018 04:46

smarterclayton mentioned this pull request Mar 23, 2018

Parallelize image mirroring and reuse mounted layers #19017

Merged

smarterclayton force-pushed the login_to_registry branch 2 times, most recently from c89cae9 to d2b937c Compare March 23, 2018 02:00

soltysh approved these changes Mar 23, 2018

View reviewed changes

smarterclayton force-pushed the login_to_registry branch from d2b937c to e07916c Compare March 25, 2018 21:56

smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2018

bparees requested changes Mar 28, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2018

smarterclayton force-pushed the login_to_registry branch from e07916c to 5cfdfa5 Compare March 28, 2018 17:26

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2018

smarterclayton added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 28, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2018

smarterclayton force-pushed the login_to_registry branch from 5cfdfa5 to 886132f Compare March 30, 2018 20:47

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2018

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 30, 2018

smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2018

openshift-merge-robot merged commit 0e7ae24 into openshift:master Mar 31, 2018


		Experimental: This command is under active development and may change without notice.`)

		loginExample = templates.Examples(`

Support login to the integrated registry via CLI #18930

Support login to the integrated registry via CLI #18930

Conversation

smarterclayton commented Mar 10, 2018

smarterclayton commented Mar 11, 2018

bparees commented Mar 11, 2018

smarterclayton commented Mar 11, 2018 via email

bparees commented Mar 11, 2018

smarterclayton commented Mar 19, 2018

smarterclayton commented Mar 21, 2018

smarterclayton commented Mar 21, 2018

smarterclayton commented Mar 22, 2018

smarterclayton commented Mar 22, 2018

soltysh left a 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

smarterclayton Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

smarterclayton Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Mar 27, 2018

smarterclayton commented Mar 28, 2018

bparees commented Mar 28, 2018

bparees left a 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

mfojtik commented Mar 28, 2018

openshift-ci-robot commented Mar 28, 2018

smarterclayton commented Mar 28, 2018

smarterclayton commented Mar 29, 2018

smarterclayton commented Mar 29, 2018

smarterclayton commented Mar 30, 2018 via email

smarterclayton commented Mar 30, 2018 via email

openshift-bot commented Mar 30, 2018

smarterclayton commented Mar 30, 2018

openshift-ci-robot commented Mar 30, 2018

openshift-ci-robot commented Mar 30, 2018

smarterclayton Mar 27, 2018 •

edited

Loading

smarterclayton Mar 27, 2018 •

edited

Loading