registry: report publicDockerImageRepository to image stream if configured #15853

mfojtik · 2017-08-18T12:05:25Z

This PR introduces two new fields for the master config, externalRegistryHostname and internalRegistryHostname.

The first one will be used as a hostname for newly added publicDockerImageRepository field and it will reveal a public Docker pull spec users should use to pull the image if the registry is exposed externally (via route, etc.).

The later is a replacement for OPENSHIFT_DEFAULT_REGISTRY environment variable (but that variable is still picked up to guarantee backward compatibility). If set it will override the OPENSHIFT_DEFAULT_REGISTRY.

mfojtik · 2017-08-18T12:09:38Z

@smarterclayton FYI

mfojtik · 2017-08-18T12:27:08Z

@openshift/api-review the two new master config fields need review... i placed them under image policy as I don't want to invent new top-level struct and we already have allowed registries/etc. there.

mfojtik · 2017-08-18T12:31:55Z

@smarterclayton i guess we should also update the describer to report the public pull over the internal pull spec (if the public pull spec is non-zero), thoughts?

Also I guess the web console wants to pickup the public pull spec to make users happier ;-) (cc @spadgett )

spadgett · 2017-08-18T13:09:09Z

Also I guess the web console wants to pickup the public pull spec to make users happier ;-) (cc @spadgett )

Nice, thanks @mfojtik

cc @petervo @stefwalter @thomasmckay

soltysh

Here's my comments.

soltysh · 2017-08-18T12:56:57Z

pkg/image/apis/image/helper.go

+// RegistryHostnameRetriever.
+// The first argument is a function that lazy-loads the value of
+// OPENSHIFT_DEFAULT_REGISTRY environment variable.
+func DefaultRegistryHostnameRetriever(defaultFn func() (string, bool), external, internal string) RegistryHostnameRetriever {


With this function type being repeated over and over again, I'd suggest to leave the old DefaultRegistryFunc type definition and use that instead of copy&pasting this.

DefaultRegistryFunc sounds misleading and you will have to repeat that instead of this so it does not fix the problem. I like DefaultRegistryHostnameRetriever better because it matches the interface name.

soltysh · 2017-08-18T12:59:04Z

pkg/image/apis/image/helper.go

+}
+
+type defaultRegistryHostnameRetriever struct {
+	defaultFn          func() (string, bool)


I'd suggest naming this env/backwards/else type of function rather than default. Because it's not default.

soltysh · 2017-08-18T13:02:05Z

pkg/image/registry/imagestreammapping/strategy.go

@@ -16,16 +16,16 @@ type Strategy struct {
 	runtime.ObjectTyper
 	names.NameGenerator

-	defaultRegistry imageapi.DefaultRegistry
+	registry imageapi.RegistryHostnameRetriever


registryHostnameRetriever

soltysh · 2017-08-18T13:02:49Z

pkg/image/registry/imagestreamimport/strategy.go

 	return &strategy{
 		ObjectTyper:       kapi.Scheme,
 		allowedRegistries: registries,
-		registryFn:        registryFn,
+		registry:          registry,


registryHostnameRetriever, please. registry in this context will be misleading.

soltysh · 2017-08-18T13:07:19Z

pkg/image/apis/image/helper.go

+// the internal Docker Registry hostname. If the master configuration propertly
+// InternalRegistryHostname is set, it will prefer that over the lazy-loaded
+// environment variable 'OPENSHIFT_DEFAULT_REGISTRY'.
+func (r *defaultRegistryHostnameRetriever) InternalRegistryHostnameFn() func() (string, bool) {


Can we just return the result of the invocation rather than the function itself? It looks like an overkill to me.

we still need to lazy-load the env variable from the cache, so we can't just return the result of invocation here.

That's just a detail of the implementation - the point of returning false was "not resolved yet"

@smarterclayton i don't get this... if you don't set the env var when openshift process is starting, how will this succeed when lazy loaded? more thinking about this I guess this should just read the var when we start and be done with it :-) Makes the interface/usage much easier.

soltysh · 2017-08-18T13:11:45Z

pkg/image/registry/imagestreamimport/strategy.go

@@ -46,7 +46,7 @@ func (s *strategy) ValidateAllowedRegistries(isi *imageapi.ImageStreamImport) fi
 	allowedRegistries := *s.allowedRegistries
 	// FIXME: The registryFn won't return the registry location until the registry service


That FIXME is for removal with the static configs.

soltysh · 2017-08-18T13:15:35Z

pkg/cmd/server/api/v1/types.go

+	// InternalRegistryHostname sets the hostname for the default internal Docker
+	// Registry. This can be overriden by using OPENSHIFT_DEFAULT_REGISTRY
+	// environment variable.
+	InternalRegistryHostname string `json:"internalRegistryHostname"`


omitempty for both fields, since we allow empty values.

mfojtik · 2017-08-18T14:28:53Z

@soltysh comments addressed, thx!

smarterclayton · 2017-08-22T19:03:45Z

pkg/cmd/server/api/types.go

+	// Registry. The external hostname should be set only when the registry is
+	// exposed externally. The value is used in 'publicDockerImageRepository'
+	// field in ImageStreams.
+	ExternalRegistryHostname string


Clarify in godoc that this is host and host:port

smarterclayton · 2017-08-22T20:10:13Z

pkg/image/apis/image/helper.go

+// RegistryHostnameRetriever represents an interface for retrieving the hostname
+// of internal and external registry.
+type RegistryHostnameRetriever interface {
+	InternalRegistryHostnameFn() func() (string, bool)


I don't like this, it should be an interface still. You can pass separate small interfaces down.

smarterclayton

Get rid of the fn() stuff

mfojtik · 2017-08-24T10:50:41Z

(5) Provide public pull URL for images

mfojtik · 2017-08-25T08:59:39Z

@smarterclayton reworked the interface and implementation (i realized that I can pass the function without wrapping it up in another function), it is much cleaner now.,

mfojtik · 2017-08-25T12:59:38Z

/test extended_conformance_gce

smarterclayton · 2017-08-27T05:16:55Z

pkg/image/apis/image/helper.go

+}
+
+// InternalRegistryHostnameFn returns a function that can be used to lazy-load
+// the internal Docker Registry hostname. If the master configuration propertly


smarterclayton · 2017-08-27T05:17:16Z

pkg/cmd/server/api/v1/types.go

@@ -372,6 +372,15 @@ type ImagePolicyConfig struct {
 	// this policy - typically only administrators or system integrations will have those
 	// permissions.
 	AllowedRegistriesForImport *AllowedRegistries `json:"allowedRegistriesForImport,omitempty"`
+	// InternalRegistryHostname sets the hostname for the default internal Docker


image registry

smarterclayton · 2017-08-27T05:17:32Z

pkg/cmd/server/api/v1/types.go

@@ -372,6 +372,15 @@ type ImagePolicyConfig struct {
 	// this policy - typically only administrators or system integrations will have those
 	// permissions.
 	AllowedRegistriesForImport *AllowedRegistries `json:"allowedRegistriesForImport,omitempty"`
+	// InternalRegistryHostname sets the hostname for the default internal Docker
+	// Registry. This can be overriden by using OPENSHIFT_DEFAULT_REGISTRY


Can it? I thought you say down below it is not overriden.

smarterclayton · 2017-08-27T05:17:59Z

Pretty close, a couple of comments.

mfojtik · 2017-08-28T08:33:29Z

@smarterclayton typos fixed and OPENSHIFT_DEFAULT_REGISTRY clarified in godoc.

…gured

smarterclayton · 2017-08-28T19:57:00Z

/lgtm

openshift-merge-robot · 2017-08-28T19:57:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, smarterclayton

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [mfojtik,smarterclayton]
~~pkg/image/OWNERS~~ [mfojtik,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-08-29T05:53:39Z

/retest

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

mfojtik · 2017-08-29T07:36:50Z

flake: #16025

/test extended_conformance_gce

mfojtik · 2017-08-29T12:34:17Z

another deployment flake: #16025

/test extended_conformance_install_update

openshift-bot · 2017-08-29T17:59:46Z

/retest

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

LalatenduMohanty · 2017-08-29T20:01:24Z

@csrwng I am trying to understand how this PR will effect cluster up. As it will help me understand if it effects Minishift. Any pointers?

openshift-merge-robot · 2017-08-29T23:48:07Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-08-30T01:37:08Z

@mfojtik: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`10fcce5`	link	`/test extended_conformance_install_update`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2017-08-30T02:50:40Z

Automatic merge from submit-queue (batch tested with PRs 15853, 15916, 16017, 16027, 16043)

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2017

openshift-merge-robot assigned soltysh and fabianofranz Aug 18, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Aug 18, 2017

mfojtik force-pushed the public-pull branch from 7f30537 to 3179a2a Compare August 18, 2017 12:23

mfojtik requested review from smarterclayton and soltysh August 18, 2017 12:25

mfojtik force-pushed the public-pull branch from 3179a2a to 07f3130 Compare August 18, 2017 12:52

mfojtik changed the title ~~WIP: registry: report publicDockerImageRepository to image stream if configured~~ registry: report publicDockerImageRepository to image stream if configured Aug 18, 2017

mfojtik force-pushed the public-pull branch from 07f3130 to d3fe3c1 Compare August 18, 2017 12:57

soltysh suggested changes Aug 18, 2017

View reviewed changes

mfojtik force-pushed the public-pull branch from d3fe3c1 to 1c58af1 Compare August 18, 2017 13:33

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

mfojtik force-pushed the public-pull branch from 1c58af1 to 0ffb71e Compare August 18, 2017 15:35

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 18, 2017

smarterclayton reviewed Aug 22, 2017

View reviewed changes

smarterclayton suggested changes Aug 22, 2017

View reviewed changes

mfojtik force-pushed the public-pull branch from 0ffb71e to 28371e9 Compare August 25, 2017 08:55

smarterclayton reviewed Aug 27, 2017

View reviewed changes

mfojtik force-pushed the public-pull branch from 28371e9 to 0f5febf Compare August 28, 2017 08:33

registry: report publicDockerImageRepository to image stream if confi…

10fcce5

…gured

mfojtik force-pushed the public-pull branch from 0f5febf to 10fcce5 Compare August 28, 2017 09:37

openshift-ci-robot assigned smarterclayton Aug 28, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

soltysh approved these changes Aug 29, 2017

View reviewed changes

openshift-merge-robot merged commit 4d89e57 into openshift:master Aug 30, 2017

soltysh mentioned this pull request Sep 28, 2017

BZ 1446644 Adding more detail on image streams openshift/openshift-docs#5322

Merged

mfojtik deleted the public-pull branch September 5, 2018 21:07

		@@ -46,7 +46,7 @@ func (s strategy) ValidateAllowedRegistries(isi imageapi.ImageStreamImport) fi
		allowedRegistries := *s.allowedRegistries
		// FIXME: The registryFn won't return the registry location until the registry service

registry: report publicDockerImageRepository to image stream if configured #15853

registry: report publicDockerImageRepository to image stream if configured #15853

Conversation

mfojtik commented Aug 18, 2017 • edited Loading

mfojtik commented Aug 18, 2017

mfojtik commented Aug 18, 2017

mfojtik commented Aug 18, 2017

spadgett commented Aug 18, 2017

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

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

mfojtik commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton left a comment

Choose a reason for hiding this comment

mfojtik commented Aug 24, 2017

mfojtik commented Aug 25, 2017

mfojtik commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Aug 27, 2017

mfojtik commented Aug 28, 2017

smarterclayton commented Aug 28, 2017

openshift-merge-robot commented Aug 28, 2017

openshift-bot commented Aug 29, 2017

mfojtik commented Aug 29, 2017

mfojtik commented Aug 29, 2017

openshift-bot commented Aug 29, 2017

LalatenduMohanty commented Aug 29, 2017

openshift-merge-robot commented Aug 29, 2017

openshift-ci-robot commented Aug 30, 2017

openshift-merge-robot commented Aug 30, 2017

mfojtik commented Aug 18, 2017 •

edited

Loading