Supermiddleware

[dmage]

The intention of the changes is to move all magic for middlewares into a single package.

[dmage]

flake openshift/origin#17786
/retest

[legionus]

Please move this function to repository.go

[dmage]

I'd like to move as few functions as possible in this PR.

[legionus]

Please use r.app.extraConfig instead of r.enabledMetrics as well.

[dmage]

done

[legionus]

- authcfg := app.extraConfig.Auth
- tokenRealm, err := configuration.TokenRealm(authcfg.TokenRealm)
+ tokenRealm, err := configuration.TokenRealm(app.extraConfig.Auth.TokenRealm)

?

[dmage]

done

[legionus]

Can we rename extraConfig to config ?

[dmage]

It's a good idea for follow ups.

[legionus]

@dmage can you add doc.go for supermiddleware ?

[dmage]

@legionus doc.go - done

[bparees]

What problem is solved by moving them into a single package?

[bparees]

so why is it here? can it be moved into a _test.go file?

[dmage]

It's a huge hack and should be removed, it's here to make a private variable accessible from another module. We gave too much tasks to the type *repository. It implements distribution.Repository and it's responsible for getting data from the image stream. But, for example, pullthroughManifestService wants *repository and uses

• m.repo.namespace
• m.repo.name
• m.repo.getImageOfImageStream
• m.repo.rememberLayersOfManifest
• m.repo.registryOSClient
• m.repo.imageStreamGetter

In other words, it wants to fill cache and to get ImageStreamImage. But right now we can't get imageStreamGetter and cache without constructing entire *repository.

Splitting will be almost mechanical changes, but it will be pretty big and I don't want to mix it with this PR.

[dmage]

@bparees the main problem is BlobDescriptorService (it determines whether the blob exists in the repository). We can register only one implementation, which doesn't get any parameters.

image-registry/pkg/dockerregistry/server/blobdescriptorservice.go

Lines 37 to 42 in c540fa0

	func init() {
	err := middleware.RegisterOptions(storage.BlobDescriptorServiceFactory(&blobDescriptorServiceFactory{}))
	if err != nil {
	logrus.Fatalf("Unable to register BlobDescriptorServiceFactory: %v", err)
	}
	}

However we need to access the image stream from this service. And we can get it from the context.

image-registry/pkg/dockerregistry/server/blobdescriptorservice.go

Lines 62 to 67 in c540fa0

	repo, found := repositoryFrom(ctx)
	if !found || repo == nil {
	err := fmt.Errorf("failed to retrieve repository from context")
	context.GetLogger(ctx).Error(err)
	return distribution.Descriptor{}, err
	}

image-registry/pkg/dockerregistry/server/blobdescriptorservice.go

Line 90 in c540fa0

if !imageStreamHasBlob(repo, dgst) {

It's pretty ugly, but I don't see any better options. But how do we put the repository into the context? Basically we have withRepository(ctx, r.repo) all around the code. And recently I replaced all such calls with my generic wrapper.

https://github.com/openshift/image-registry/blob/c540fa0224d2028b9633a147c283fbf60d371ba9/pkg/dockerregistry/server/withrepository.go

So, the problem is solved, right? No, it isn't. :(

If BlobStore.Create(ctx, ...) is called with the WithMountFrom(src) option, we should be very careful with contexts, the upstream code may call Stat on the source repository with the context from the destination repository. We have a workaround for this

image-registry/pkg/dockerregistry/server/pendingerrors.go

Lines 56 to 59 in c540fa0

	options = append(options, statCrossMountCreateOptions{
	ctx: ctx,
	destRepo: r.repo,
	})

image-registry/pkg/dockerregistry/server/pendingerrors.go

Lines 130 to 151 in c540fa0

	func statSourceRepository(
	ctx context.Context,
	destRepo *repository,
	sourceRepoName reference.Named,
	dgst digest.Digest,
	) (desc distribution.Descriptor, err error) {
	upstreamRepo, err := destRepo.app.registry.Repository(ctx, sourceRepoName)
	if err != nil {
	return distribution.Descriptor{}, err
	}
	namespace, name, err := getNamespaceName(sourceRepoName.Name())
	if err != nil {
	return distribution.Descriptor{}, err
	}
	repo := *destRepo
	repo.namespace = namespace
	repo.name = name
	repo.Repository = upstreamRepo
	return repo.Blobs(ctx).Stat(ctx, dgst)
	}

But, unfortunately, it has a bug. The context isn't modified before calling Stat.

Back to the PR: writing middlewares for the blob descriptor service should be less intriguing and detective, so now we have the entire implementation of the blob descriptor service middleware in the context. And that allowed me to cover it with tests. Though, this PR is just refactoring, bug fixes will be made as follow ups.

[legionus]

What problem is solved by moving them into a single package?

@bparees This is a very important change that corrects the current architecture. Our code will become more maintainable. Also it will allow us to change the caching.

[bparees]

@dmage thanks for the summary, that was very helpful.

[legionus]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, legionus

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/dockerregistry/OWNERS [dmage,legionus]

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

[legionus]

/retest