(feature): add direct image registry client Unpacker implementation
#145
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| package main | ||
|
There was a problem hiding this comment. Not sure this really matters, but I'd suggest (for a follow-up) moving the GC function into a separate package so that we can avoid a unit test in the |
||
|
|
||
| import ( | ||
| "context" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/client-go/metadata/fake" | ||
|
|
||
| "github.com/operator-framework/catalogd/api/core/v1alpha1" | ||
| ) | ||
|
|
||
| func TestUnpackStartupGarbageCollection(t *testing.T) { | ||
| for _, tt := range []struct { | ||
| name string | ||
| existCatalogs []*metav1.PartialObjectMetadata | ||
| notExistCatalogs []*metav1.PartialObjectMetadata | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "successful garbage collection", | ||
| existCatalogs: []*metav1.PartialObjectMetadata{ | ||
| { | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "Catalog", | ||
| APIVersion: v1alpha1.GroupVersion.String(), | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "one", | ||
| }, | ||
| }, | ||
| { | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "Catalog", | ||
| APIVersion: v1alpha1.GroupVersion.String(), | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "two", | ||
| }, | ||
| }, | ||
| }, | ||
| notExistCatalogs: []*metav1.PartialObjectMetadata{ | ||
| { | ||
| TypeMeta: metav1.TypeMeta{ | ||
| Kind: "Catalog", | ||
| APIVersion: v1alpha1.GroupVersion.String(), | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "three", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx := context.Background() | ||
| cachePath := t.TempDir() | ||
| scheme := runtime.NewScheme() | ||
| require.NoError(t, metav1.AddMetaToScheme(scheme)) | ||
|
|
||
| allCatalogs := append(tt.existCatalogs, tt.notExistCatalogs...) | ||
| for _, catalog := range allCatalogs { | ||
| require.NoError(t, os.MkdirAll(filepath.Join(cachePath, catalog.Name, "fakedigest"), os.ModePerm)) | ||
| } | ||
|
|
||
| runtimeObjs := []runtime.Object{} | ||
| for _, catalog := range tt.existCatalogs { | ||
| runtimeObjs = append(runtimeObjs, catalog) | ||
| } | ||
|
|
||
| metaClient := fake.NewSimpleMetadataClient(scheme, runtimeObjs...) | ||
|
|
||
| err := unpackStartupGarbageCollection(ctx, cachePath, logr.Discard(), metaClient) | ||
| if !tt.wantErr { | ||
| assert.NoError(t, err) | ||
| entries, err := os.ReadDir(cachePath) | ||
| require.NoError(t, err) | ||
| assert.Len(t, entries, len(tt.existCatalogs)) | ||
| for _, catalog := range tt.existCatalogs { | ||
| assert.DirExists(t, filepath.Join(cachePath, catalog.Name)) | ||
| } | ||
|
|
||
| for _, catalog := range tt.notExistCatalogs { | ||
| assert.NoDirExists(t, filepath.Join(cachePath, catalog.Name)) | ||
| } | ||
| } else { | ||
| assert.Error(t, err) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| apiVersion: kustomize.config.k8s.io/v1beta1 | ||
| kind: Kustomization | ||
| resources: | ||
| - ../default | ||
|
|
||
| patches: | ||
| - patch: |- | ||
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: controller-manager | ||
| namespace: system | ||
| spec: | ||
| template: | ||
| spec: | ||
| containers: | ||
| - name: manager | ||
| volumeMounts: | ||
| - mountPath: /etc/ssl/certs/ | ||
|
There was a problem hiding this comment. Are we going to overwrite any existing certs by mounting this config map at this location? Ideally, we are augmenting the existing CA trust of the container. There was a problem hiding this comment. As far as I can tell, yes. I'm not confident this really matters though as this only happens for e2e testing. We made the decision to not rely on external registries for e2e testing so our e2e tests should only ever target our e2e registry and therefore shouldn't require any of the other CA certs. That being said, I could totally be missing something but it seems to hold true for now since the e2e tests run and pass with this overwriting. There was a problem hiding this comment. Seems okay for now. Maybe something to capture as a follow-up to see if there's an easy way to copy our certs in. There was a problem hiding this comment. I'd prefer to have this context captured as a comment here somewhere somehow (I'm not sure what the mechanics for using comments in a kustomization files are). Issues aren't available to a reader of the code, comments are, even if the comment is just pointing to the GitHub issue. |
||
| name: ca-certs | ||
| readOnly: true | ||
| volumes: | ||
| - name: ca-certs | ||
| configMap: | ||
| name: docker-registry.catalogd-e2e.svc | ||
| defaultMode: 0644 | ||
| optional: false | ||
| items: | ||
| - key: ca-certificates.crt | ||
| path: ca-certificates.crt | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you seen https://github.com/tilt-dev/ctlptl?
Since we're already tilt-enabled, perhaps this is a reasonable tool to evaluate for our cluster setup purposes, especially since it seems to support setting up image registries that are
simultaneously accessible to the local dev environment and the in-cluster processes.
Can we spend 30 minutes now (but time boxed) evaluating this to see if it would be a drop in replacement?
If it is a good candidate, it would be fine to integrate in a follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen that. I played around with doing it the kind way and wasn't able to get that working well (there seemed to be resolution issues from the direct client and it couldn't find the registry when using both the names mentioned in the docs). My understanding is that ctlptl pretty much does the steps mentioned in the kind docs for you and would, in theory, have the same results so i didn't give it a shot.
That being said, I'm happy to time box an evaluation of it. I'll circle back around to investigating this after addressing other things since this would be a follow-up anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a shot and ran into the same issues I had with the approach documented by kind. It just can't seem to resolve the registry URL. Maybe this is just an issue on my machine though? I haven't tested it on my Mac, but I've had kind related troubles in the past on my work laptop running Fedora so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds good. Maybe make a tracker for this, even if it's just a way to mentally bookmark this apparent gap in the ecosystem for cluster+registry tooling. It seems like progress is being made, so perhaps we can revisit again in 3-6 months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#192