Registry Configuration for Builds #144

adambkaplan · 2018-12-11T15:05:54Z

Enable support for configuring container registries for builds. See docker day-two tasks

adambkaplan · 2018-12-11T15:07:01Z

bparees · 2018-12-11T15:12:26Z

config/v1/types_build.go

+	// RegistriesConfig is a reference to a ConfigMap containing the registry configuration
+	// for image search, allowed insecure registries, and blocked registries.
+	// +optional
+	RegistriesConfig ConfigMapReference `json:"registriesConfig,omitempty"`


i'm torn over whether this should be top level, or put in BuildDefaults so that we can later allow end-users to provide their own value if they want to.... i'm leaning towards the latter for flexibility, even if we decide we never want to expose it.

Similarly, while you're in here, can you add HTTP/HTTPS/NOPROXY settings to BuildDefaults? (Same logic.. we may or may not want to let end-users specify their own proxy settings on a per build basis in the future).

(we may also want to revisit moving additionalTrustedCA into BuildDefaults for the same reason, but for now i'm content to leave it while we get things working)

I had similar thoughts about the HTTP proxies - these should be first-class in some capacity, rather than buried in an ENV var. I'll add those.

Adding to build defaults does give us flexibility. In the long run I'd be worried about security concerns/pushback for such a feature. At present, though, I don't think there is harm going with this approach.

yes, the security concerns are why i'm not sure we'd ever allow end-users to set these, but again i don't want to paint ourselves into a corner in case we do decide it is an ok thing to do.

for now it'll be a "default" that is effectively the enforced setting.

When you add the proxies please make a struct, so we don't have 3 new top-level fields.

(gitproxy should have been done that way too. if we get time we might want to fix that for 4.0)

I think there is also merit in-lining the registries.conf file, rather than referencing a ConfigMap. Thoughts?

I'm fine with it, though i think @deads2k has been against inlining content like that historically, i can't remember the reasoning though. It certainly would make the consuming logic a lot simpler.

config/v1/types_build.go

bparees · 2018-12-11T16:34:49Z

nits, otherwise lgtm

adambkaplan · 2018-12-14T20:08:39Z

config/v1/types_build.go

+type RegistriesConfig struct {
+	// SearchRegistries lists the registries to search for images if an image repository is not specified in an image pull spec
+	// +optional
+	SearchRegistries []string `json:"searchRegistries,omitempty"`


@bparees I feel giving RegistriesConfig first-class support will be more consumer-friendly and resilient to change in the long run.

seems reasonable to me, but it means you're going to have to reconstruct the registries.conf file in the build controller

correct, but that isn't hard and isolates tooling changes from cluster admins (YAML yesterday, TOML today, tomorrow who knows...)

I think SearchRegistries should have a default value of:

registry.redhat.io, quay.io, registry.access.redhat.com, docker.io

so that it "just works" out of the box.

you'll need to make it a pointer so users can differentiate between "i didn't provide a value and so i get the default" and "i provided an empty array, i don't want any search behavior"

(I want to stop embedding a registries.conf in our builder image)

smarterclayton · 2018-12-17T17:14:09Z

config/v1/types_build.go

+type RegistriesList []string
+
+var (
+	DefaultSearchRegistries RegistriesList = []string{"registry.redhat.io", "quay.io", "registry.access.redhat.com", "docker.io"}


We disabled these defaults in 3.x - why would we need to have them here?

yes, just say no to registry search lists. embrace the fully qualified image pull spec. we painted ourselves into a corner by doing this in the past.

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

Nm, this is search.

Search is pretty fraught. I'm not sure I want to support that in the future. We agreed in 3.10 / 3.11 that search is counter to our goals with Kubernetes and so endorsing and future tolerating global search is counter to our goals.

If this has been (and would continue to be) a pain point, how big of a pain would we create by removing this as a confg option?

I think we're in agreement that the sane default is nil, but I'm hesitant to remove search as a config option.

+1 to what @adambkaplan said. our main concern here is people who already have buildconfigs with non-qualified pullspecs. Those people are either going to have to go fix those buildconfigs, or (if we have this field) the admin can go setup an appropriate search to make things work.

If we are comfortable telling them that what worked in 3.11 (because we did have search, or at least configurable search by tweaking the docker config on the nodes) won't work in 4.0, i'm happy to ditch this option...

Remember that build pull is not identical to kubernetes pull behavior, though (wasn't in 3.11, won't be in 4.0).

@smarterclayton so is the the final decision? People using un-qualified image specs in 4.0 get broken and told to fix it?

adambkaplan · 2018-12-17T20:42:41Z

Another item that we need to address is the "block all registries but X, Y, Z" that was available in 3.x: https://docs.okd.io/3.11/day_two_guide/docker_tasks.html#docker-external-registries-whitelist-and-blacklist

I don't think we have this capability with the containers/image library that drives buildah. @rhatdan may be able to weigh in. The old YAML syntax seemed to transform the "search" registries into "allowed" registries in the case of "block all".

IMO I think setting up an HTTP proxy that whitelists the allowed registries would be a better approach if someone wants to only allow pulls from specific repositories.

adambkaplan · 2018-12-18T15:42:36Z

/hold

Scoping this PR back down to the registry config. Waiting on containers/image#548 to see best course of action to enable white-list behavior.

adambkaplan · 2019-01-07T19:43:58Z

/hold cancel

bparees · 2019-01-07T20:07:21Z

/approve

bparees · 2019-01-07T20:10:31Z

config/v1/types_build.go

+type RegistriesConfig struct {
+	// SearchRegistries lists the registries to search for images if an image repository is not specified in an image pull spec
+	// +optional
+	SearchRegistries RegistriesList `json:"searchRegistries,omitempty"`


This discussion doesn't seem to be resolved:
#144 (comment)

My understanding is that we shouldn't have default search repos - if we're in agreement on that then I think we need to doc the behavior change (alongside the new process for updating the search registries).

There will be no default search behavior:
https://coreos.slack.com/archives/CE4L0F143/p1547071004002600

bparees · 2019-01-09T22:14:45Z

/hold

we need a way to default to docker.io. So either this goes in as is, and we treat an empty array as "default to docker.io", or the searchregistry field needs to be a pointer so users can explicitly set the search list as empty if they so choose. My preference is the latter.

adambkaplan

@bparees @smarterclayton updated to use pointers for the registry list.

adambkaplan · 2019-01-09T22:45:23Z

config/v1/types_build.go

+type RegistriesConfig struct {
+	// SearchRegistries lists the registries to search for images if an image repository is not specified in an image pull spec
+	// +optional
+	SearchRegistries *RegistriesList `json:"searchRegistries,omitempty"`


Builds will default to using docker.io for the search registry. Pointer here allows cluster admins to override this behavior. See openshift/builder#36

Others use pointers for API consistency.

I don't think making the rest pointers for consistency(when we don't otherwise have a good reason to make them pointers) is appropriate but i'll let @openshift/api-reviewers make that call.

Builds will default to using docker.io for the search registry. Pointer here allows cluster admins to override this behavior. See openshift/builder#36

This implies that nil and empty slice have different behavior. Please document the difference.

I don't think making the rest pointers for consistency(when we don't otherwise have a good reason to make them pointers) is appropriate but i'll let @openshift/api-reviewers make that call.

Correct. Only things that must be pointers should be pointers and the difference in behavior should be clearly documented

config/v1/types_build.go

deads2k · 2019-01-10T21:19:06Z

config/v1/types_build.go

@@ -72,6 +76,27 @@ type ImageLabel struct {
 	Value string `json:"value,omitempty"`
 }

+type RegistriesList []string


RegistryList (a list of rats is a RatList, not a RatsList)

deads2k · 2019-01-10T21:19:48Z

config/v1/types_build.go

@@ -72,6 +76,27 @@ type ImageLabel struct {
 	Value string `json:"value,omitempty"`
 }

+type RegistriesList []string


Also, are you sure you want this? No reason you can't have it, but it may mean a bunch of casts.

you mean vs just using a string type directly? I assume this was following a pattern we've established elsewhere, i don't have a preference.

you mean vs just using a string type directly? I assume this was following a pattern we've established elsewhere, i don't have a preference.

We've done it both ways and unless we're enumerating valid values, I've found []string to be easier to deal with.

I thought it would be easier to work with, but truthfully there isn't much difference. Ranging over the slice will still require the dereference.

config/v1/types_build.go

bparees · 2019-01-10T23:31:29Z

looks like all comments have been addressed, @adambkaplan please squash and we'll get this merged.

* Enable support for configuring container registries for builds. See docker day-two tasks. * Document default search behavior. JIRA-ID: DEVEXP-154

adambkaplan · 2019-01-11T04:18:06Z

@openshift/api-reviewers squashed & ready to merge.

bparees · 2019-01-11T04:28:01Z

/lgtm

bparees · 2019-01-11T04:28:11Z

/hold cancel

bparees · 2019-01-11T04:29:44Z

/unassign @jwforres
/assign @deads2k @derekwaynecarr @eparis

adambkaplan · 2019-01-11T18:57:56Z

/retest

openshift-ci-robot · 2019-01-11T19:25:21Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: adambkaplan, bparees

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:

OWNERS

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 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 11, 2018

openshift-ci-robot requested review from abhinavdahiya and sjenning December 11, 2018 15:06

openshift-ci-robot assigned jwforres Dec 11, 2018

openshift-ci-robot requested a review from bparees December 11, 2018 15:07

bparees reviewed Dec 11, 2018

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 11, 2018

adambkaplan changed the title ~~Add Registries Config for Builds~~ Build Config Updates Dec 11, 2018

bparees reviewed Dec 11, 2018

View reviewed changes

config/v1/types_build.go Outdated Show resolved Hide resolved

adambkaplan force-pushed the build-reg-conf branch from 423a18a to 5fec18e Compare December 11, 2018 19:08

adambkaplan commented Dec 14, 2018

View reviewed changes

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 Dec 17, 2018

smarterclayton reviewed Dec 17, 2018

View reviewed changes

adambkaplan changed the title ~~Build Config Updates~~ Registry Configuration for Builds Dec 18, 2018

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

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2019

adambkaplan force-pushed the build-reg-conf branch from a268694 to 0453796 Compare January 7, 2019 19:42

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 7, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2019

bparees reviewed Jan 7, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2019

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

adambkaplan force-pushed the build-reg-conf branch from 57e8758 to 43f5071 Compare January 9, 2019 22:43

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2019

adambkaplan commented Jan 9, 2019

View reviewed changes

deads2k reviewed Jan 10, 2019

View reviewed changes

config/v1/types_build.go Show resolved Hide resolved

deads2k reviewed Jan 10, 2019

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2019

derekwaynecarr reviewed Jan 10, 2019

View reviewed changes

config/v1/types_build.go Show resolved Hide resolved

Registry Configuration for Builds

ec43dc9

* Enable support for configuring container registries for builds. See docker day-two tasks. * Document default search behavior. JIRA-ID: DEVEXP-154

adambkaplan force-pushed the build-reg-conf branch from 0fc5341 to ec43dc9 Compare January 11, 2019 04:16

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2019

openshift-ci-robot assigned deads2k, derekwaynecarr and eparis and unassigned jwforres Jan 11, 2019

bparees removed their assignment Jan 11, 2019

bparees added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2019

openshift-merge-robot merged commit 6c7a691 into openshift:master Jan 11, 2019

Registry Configuration for Builds #144

Registry Configuration for Builds #144

Conversation

adambkaplan commented Dec 11, 2018 • edited

adambkaplan commented Dec 11, 2018

bparees Dec 11, 2018 • edited

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

bparees commented Dec 11, 2018

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 Dec 17, 2018 • edited

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

adambkaplan commented Dec 17, 2018

adambkaplan commented Dec 18, 2018

adambkaplan commented Jan 7, 2019

bparees commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Jan 9, 2019 • edited

adambkaplan 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

bparees commented Jan 10, 2019

adambkaplan commented Jan 11, 2019

bparees commented Jan 11, 2019

bparees commented Jan 11, 2019

bparees commented Jan 11, 2019

adambkaplan commented Jan 11, 2019

openshift-ci-robot commented Jan 11, 2019

adambkaplan commented Dec 11, 2018 •

edited

bparees Dec 11, 2018 •

edited

smarterclayton Dec 17, 2018 •

edited

bparees commented Jan 9, 2019 •

edited