Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport #23

dmage · 2019-09-17T13:56:51Z

No description provided.

openshift-ci-robot · 2019-09-17T13:56:53Z

@dmage: This pull request references Bugzilla bug 1741391, which is invalid:

expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

[WIP] Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport

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.

dmage · 2019-09-20T12:26:26Z

@sttts this PR is expected eventually to be backported to 4.2.z. Is it ok to change vendor for such PRs?

dmage · 2019-09-24T14:41:58Z

/bugzilla refresh

openshift-ci-robot · 2019-09-24T14:42:04Z

@dmage: This pull request references Bugzilla bug 1741391, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

mtrmac

Overall, sure, this is consistent with how the ImageContentSourcePolicy object only configures accesses by digest.

(I don’t quite understand what the UX is going to look like — will the users be forced to manually update ImageStream* objects with new values all the time? Where will they get the values to use? … This is all very much outside my area of expertise, I just hope that someone is thinking about that.)

mtrmac · 2019-09-24T18:46:24Z

pkg/image/apiserver/registries/registries.go

+// - Implement ImageContentSourcePolicy rules in icspRules.
+// “Whole registries” above means that the configuration applies to everything on that registry, including any possible separately-configured
+// namespaces/repositories within that registry.
+func EditRegistriesConfig(config *sysregistriesv2.V2RegistriesConf, insecureRegistries, blockedRegistries []string, icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy) error {


Please consider vendoring the original (https://github.com/openshift/machine-config-operator/tree/master/pkg/controller/container-runtime-config/registries , or the new repo that is being prepared) so that it is only maintained in one place.

I don't want to vendor any operator into the API server. The repo is not ready yet. But even if this code live in a new repo, I still don't want to vendor it because of its dependency on the all-in-one package sysregistriesv2. It will add dependency on toml, for example, that is not needed for the API server.

/cc @adambkaplan

@mtrmac I'm getting this repo set up. We'll address the refactor in a later PR.

mtrmac · 2019-09-24T18:48:43Z

pkg/image/apiserver/registry/imagestreamimport/rest.go

+		klog.Warningf("failed to merge ImageContentSourcePolicy resources, mirrored images will not be found: %v", err)
+	}
+	for i, reg := range v2regConf.Registries {
+		v2regConf.Registries[i].Prefix = reg.Location


(Strictly speaking, this should happen only if .Prefix == "". It doesn’t make a difference with the data built by EditRegistriesConfig.)

mtrmac · 2019-09-24T18:49:52Z

pkg/image/apiserver/sysregistriesv2/system_registries_v2.go

@@ -0,0 +1,260 @@
+package sysregistriesv2


Why not vendor the original from c/image, and make the relationship and maintenance expectations explicit?

Because the original package also has V1 types that I don't need. And these types need additional dependencies that may create unnecessary complications for backporting this PR (types.SystemContext, for example).

@adambkaplan It'd be good to make packages more granular in the new repo, so we don't need to import dozen packages only because we need to merge two maps.

Fair enough, containers/image#692 really is a pain.

FWIW, after containers/image#716 , c/image v4.0.0 no longer imports the compression implementations. Sure, there is still the TOML decoder, and of course containers/image/types, but I can’t see how merely including that code hurts all that much.

@mtrmac we also need to backport this change to 4.2.z

containers/image does not currently maintain per-OpenShift branches; you can vendor commit df216d7cdc943e3e2ddef7be67a6abdcaf583e61 directly.

(Or use use the later released v4.0.1, but that forces use of Go modules.)

mtrmac · 2019-09-24T18:57:41Z

pkg/image/apiserver/importer/importer.go

+		reg := isi.regConf.Registries[i]
+
+		if bestMatch == nil || len(reg.Prefix) > len(bestMatch.Prefix) {
+			if scopeMatchesRegistry(repoName, reg.Prefix) {


scopeMatchesRegistry is only correct when the second parameter is a registry, while reg.Prefix can be a namespace or a repository name. In particular this will break in the most common case, with ref a repo:tag or repo@digest, and ref.Prefix being repo.

See how c/image/pkg/sysregistriesv2.FindRegistry works (or, ideally, just call it — right now that would require writing isi.regConf.Registries to a temporary file, I suppose that could change).

repoName doesn't contain : nor @ here.

Ah, true. The code is still misusing scopeMatchesRegistry, though.

pkg/image/apiserver/importer/importer.go

mtrmac · 2019-09-24T19:05:04Z

pkg/image/apiserver/importer/importer.go

-	if err != nil {
-		klog.V(5).Infof("unable to access repository %#v: %#v", repository, err)
-		switch {
-		case err == reference.ErrReferenceInvalidFormat:


This detailed error handling seems to have been lost in the move to getManifestByDigest.

mtrmac · 2019-09-24T19:05:43Z

pkg/image/apiserver/importer/importer.go

-	s, err := repo.Manifests(ctx)
-	if err != nil {
-		klog.V(5).Infof("unable to access manifests for repository %#v: %#v", repository, err)
-		switch {


This detailed error handling seems to have been lost in the move to getManifestByDigest.

dmage · 2019-10-04T10:39:12Z

/assign @adambkaplan @sttts

dmage · 2019-10-08T09:13:24Z

@adambkaplan @sttts please take a look

mfojtik · 2019-10-09T14:23:21Z

/hold

This is pending architectural discussion.

I think wiring operator clients and reading CR's inside openshift-apiserver is not the correct way to deal with changing the configuration for the operand. We already have mechanism to do this via operand config (in this case openshift apiserver config) and the operator should have observer loop and sets the configuration using this mechanism.

This is inventing a new "dynamic" config that depends on CRD provided by cluster-config-operator.

Also this config is not changed often, I would say in 99% cases this is "one time setting".

/cc @derekwaynecarr
/cc @smarterclayton
/cc @deads2k

dmage · 2019-10-23T15:58:24Z

/retest

dmage · 2019-10-24T12:38:48Z

@adambkaplan

a bad ICSP shouldn't cause the apiserver to crashloop
I added some debug messages that can help us understand where the image is going to be pulled from

dmage · 2019-10-24T15:00:17Z

Regarding debuggability:

now we have the image reference in all error message (https://github.com/openshift/openshift-apiserver/pull/23/files#diff-37ea299bfbc0e50a72dc84712ffb8076R372)
aggregate errors from all mirrors (https://github.com/openshift/openshift-apiserver/pull/23/files#diff-37ea299bfbc0e50a72dc84712ffb8076R543)
log references that we are trying to access (https://github.com/openshift/openshift-apiserver/pull/23/files#diff-37ea299bfbc0e50a72dc84712ffb8076R522)

adambkaplan

/lgm

ping @mfojtik regarding the hold

adambkaplan · 2019-10-24T15:29:18Z

/lgtm

dmage · 2019-10-25T12:57:02Z

/retest

adambkaplan

/lgtm

dmage · 2019-11-04T12:47:21Z

@sttts @mfojtik can we merge it or is there something left to fix?

mfojtik · 2019-11-04T17:25:19Z

/hold cancel

mfojtik · 2019-11-04T17:25:57Z

@dmage please rename the last commit to bump(*): vendor

mfojtik · 2019-11-04T17:36:36Z

/approve

openshift-bot · 2019-11-04T22:01:55Z

/retest

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

adambkaplan · 2019-11-05T16:51:49Z

/lgtm

Last commit renamed per @mfojtik

openshift-ci-robot · 2019-11-05T16:56:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, dmage, mfojtik

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~~ [mfojtik]

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 · 2019-11-05T18:28:20Z

@dmage: All pull requests linked via external trackers have merged. Bugzilla bug 1741391 has been moved to the MODIFIED state.

In response to this:

Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport

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.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2019

openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2019

openshift-ci-robot requested review from bparees and deads2k September 17, 2019 13:57

dmage force-pushed the import-from-mirrors branch from ae83a7a to 1d3fe8b Compare September 24, 2019 14:34

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 Sep 24, 2019

dmage force-pushed the import-from-mirrors branch from 1d3fe8b to 7b0735c Compare September 24, 2019 14:39

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 24, 2019

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 24, 2019

dmage force-pushed the import-from-mirrors branch from 7b0735c to 4dad4c8 Compare September 24, 2019 15:13

mtrmac reviewed Sep 24, 2019

View reviewed changes

openshift-ci-robot requested a review from adambkaplan September 24, 2019 21:09

dmage force-pushed the import-from-mirrors branch 2 times, most recently from d7eb564 to 4ec0340 Compare October 3, 2019 16:52

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

dmage changed the title ~~[WIP] Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport~~ Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport Oct 3, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2019

openshift-ci-robot assigned adambkaplan and sttts Oct 4, 2019

openshift-ci-robot requested a review from derekwaynecarr October 9, 2019 14:23

dmage force-pushed the import-from-mirrors branch from 2cde0e6 to 693bd90 Compare October 24, 2019 14:54

adambkaplan approved these changes Oct 24, 2019

View reviewed changes

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

dmage force-pushed the import-from-mirrors branch from 693bd90 to bd7ed3c Compare October 31, 2019 15:28

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

adambkaplan approved these changes Oct 31, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 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 Nov 4, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2019

dmage force-pushed the import-from-mirrors branch from bd7ed3c to 138fe24 Compare November 4, 2019 22:26

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

Oleg Bulatov added 2 commits November 5, 2019 17:46

Support ImageContentSourcePolicy by ImageStreamImport

1fc9d22

bump(*): vendor

0e6ca42

dmage force-pushed the import-from-mirrors branch from 138fe24 to 0e6ca42 Compare November 5, 2019 16:47

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

openshift-merge-robot merged commit 36e1300 into openshift:master Nov 5, 2019

mtrmac mentioned this pull request Nov 6, 2019

Use openshift/runtime-utils/pkg/registries and containers/image instead local copies #41

Closed

Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport #23

Bug 1741391: support ImageContentSourcePolicy by ImageStreamImport #23

Conversation

dmage commented Sep 17, 2019

openshift-ci-robot commented Sep 17, 2019

dmage commented Sep 20, 2019

dmage commented Sep 24, 2019

openshift-ci-robot commented Sep 24, 2019

mtrmac 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

mtrmac Sep 24, 2019 • 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

dmage commented Oct 4, 2019

dmage commented Oct 8, 2019

mfojtik commented Oct 9, 2019 • edited Loading

dmage commented Oct 23, 2019

dmage commented Oct 24, 2019

dmage commented Oct 24, 2019

adambkaplan left a comment

Choose a reason for hiding this comment

adambkaplan commented Oct 24, 2019

dmage commented Oct 25, 2019

adambkaplan left a comment

Choose a reason for hiding this comment

dmage commented Nov 4, 2019

mfojtik commented Nov 4, 2019

mfojtik commented Nov 4, 2019

mfojtik commented Nov 4, 2019

openshift-bot commented Nov 4, 2019

adambkaplan commented Nov 5, 2019

openshift-ci-robot commented Nov 5, 2019

openshift-ci-robot commented Nov 5, 2019

mtrmac Sep 24, 2019 •

edited

Loading

mfojtik commented Oct 9, 2019 •

edited

Loading