Automatically check and update docker-registry configuration

[legionus]

We made changes in docker-registry configuration, without which the server will
not work correctly. In order not to disrupt the user experience we must change
the old config.

@pweil- @liggitt @mfojtik @miminar

[pweil-]

cc @smarterclayton

Why do we not just check the middleware.registry and middleware.storage fields as a 3.3 fix and push out a more complicated validation scenario until later? It's late and anything going in needs to be very low risk. Generally we should be fully backwards compatible so I'm not sure a migration experience within the registry itself is the way to go. Looking for opinions there.

middleware:
  registry:
    - name: openshift
  repository:
    - name: openshift
      options:
        acceptschema2: false
        pullthrough: true
        enforcequota: false
        projectcachettl: 1m
        blobrepositorycachettl: 10m
  storage:
    - name: openshift

[legionus]

@pweil- This is what I do. I can remove the other checks.

[legionus]

@pweil- Take a look at https://github.com/openshift/origin/pull/10673/files#diff-2a8298ab6ee804cc0d7cae78d6303d8aR97

[liggitt]

fwiw, here's the diff in default config since 1.2.0:

$ git diff v1.2.0 master -- images/dockerregistry/config.yml

diff --git a/images/dockerregistry/config.yml b/images/dockerregistry/config.yml
index 35edd0d..d57e1c7 100644
--- a/images/dockerregistry/config.yml
+++ b/images/dockerregistry/config.yml
@@ -5,7 +5,7 @@ http:
   addr: :5000
 storage:
   cache:
-    layerinfo: inmemory
+    blobdescriptor: inmemory
   filesystem:
     rootdirectory: /registry
   delete:
@@ -13,8 +13,22 @@ storage:
 auth:
   openshift:
     realm: openshift
+    
+    # tokenrealm is a base URL to use for the token-granting registry endpoint.
+    # If unspecified, the scheme and host for the token redirect are determined from the incoming request.
+    # If specified, a scheme and host must be chosen that all registry clients can resolve and access:
+    #
+    # tokenrealm: https://example.com:5000
 middleware:
+  registry:
+    - name: openshift
   repository:
     - name: openshift
       options:
+        acceptschema2: false
         pullthrough: true
+        enforcequota: false
+        projectcachettl: 1m
+        blobrepositorycachettl: 10m
+  storage:
+    - name: openshift

[pweil-]

This is what I do. I can remove the other checks.

yep, saw that. I was more referring to the other checks. They may be great, I just didn't want to make that call right now if we didn't have to.

[pweil-]

this can be separated out to return early and not iterate through all mwType's middlewares or a break could be inserted here.

Also, if we're ok with just checking the two middleware items that were mentioned in the main discussion it looks like none of the middleware pieces need the defaultConfig which means we don't the ResolveConfiguration code right?

Also, tests 😄

[liggitt]

can we do something much simpler for 1.3, purely to enable 1.2 configs to run, like this:

func setDefaultMiddleware(config *configuration.Configuration) error {
    // Default to openshift middleware for relevant types
    // This allows custom configs based on old default configs to continue to work
    if config.Middleware == nil {
        config.Middleware = map[string][]configuration.Middleware{}
    }
    for _, middlewareType := range []string{"registry", "repository", "storage"} {
        if _, exists := config.Middleware[middlewareType]; !exists {
            config.Middleware[middlewareType] = []configuration.Middleware{{Name: "openshift"}}
        }
    }
    return nil
}

[legionus]

@pweil- @liggitt I rewrote PR.

[pweil-]

err is not returned in any case here. How about we record any time we default something on this line and return that so that if we've changed something we can give them a message that says their config is obsolete and here is what we added to get it up to date?

LGTM otherwise

and tests 😄

[liggitt]

Is there any case where replacing the openshift registry, repository, or storage middleware would be what someone wants, or is our registry unable to run unless all three openshift middlewares are configured? If someone does have custom ones registered, does appending openshift after theirs always make sense? (I don't know what impact ordering has here)

[legionus]

or is our registry unable to run unless all three openshift middlewares are configured?

@liggitt Right now docker-registry will not work if not configured all three middlewares.

If someone does have custom ones registered, does appending openshift after theirs always make sense?

Appended to this is the right decision here. If someone created a middleware, it should continue to work.

[liggitt]

add test cases with the 1.0, 1.1, 1.2, and 1.3 default configs. that'll also make sure we don't overwrite an existing middleware stanza

[legionus]

@liggitt I added tests for v1.0.8, v1.2.1, v1.3.0-alpha.3

[pweil-]

I'm good with the latest error reporting to tell folks exactly what we're adding and the tests look fine to me. @liggitt, merge when your appropriate happiness level has been achieved.

[pweil-]

@legionus - once merged let's make #10499 a p2 again since it's tracking an actual upgrade that we still need to address and please create a card to make a backwards compatibility test for the registry so we can catch this in the future.

[liggitt]

LGTM, [merge]

follow up with extended test that spins up the current registry image using the various released default configs and makes sure a push and a pull work

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8499/) (Image: devenv-rhel7_4939)

[openshift-bot]

Evaluated for origin merge up to ed20576

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

Evaluated for origin test up to ed20576

[mfojtik]

@legionus perfect! thank you very much for this last minute fix!

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8499/)