Skip to content

🐛 Fix operator-controller not using updated registries configuration #1554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions catalogd/internal/source/containers_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/containers/image/v5/oci/layout"
"github.com/containers/image/v5/pkg/blobinfocache/none"
"github.com/containers/image/v5/pkg/compression"
"github.com/containers/image/v5/pkg/sysregistriesv2"
"github.com/containers/image/v5/signature"
"github.com/containers/image/v5/types"
"github.com/go-logr/logr"
Expand Down Expand Up @@ -51,6 +52,9 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
return nil, reconcile.TerminalError(fmt.Errorf("error parsing catalog, catalog %s has a nil image source", catalog.Name))
}

// Reload registries cache in case of configuration update
sysregistriesv2.InvalidateCache()

srcCtx, err := i.SourceContextFunc(l)
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion cmd/operator-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ func main() {
return nil, fmt.Errorf("could not stat auth file, error: %w", err)
}
return srcContext, nil
}}
},
}

clusterExtensionFinalizers := crfinalizer.NewFinalizers()
if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
Expand Down
10 changes: 10 additions & 0 deletions internal/rukpak/source/containers_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/containers/image/v5/oci/layout"
"github.com/containers/image/v5/pkg/blobinfocache/none"
"github.com/containers/image/v5/pkg/compression"
"github.com/containers/image/v5/pkg/sysregistriesv2"
"github.com/containers/image/v5/signature"
"github.com/containers/image/v5/types"
"github.com/go-logr/logr"
Expand All @@ -43,10 +44,14 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
return nil, reconcile.TerminalError(fmt.Errorf("error parsing bundle, bundle %s has a nil image source", bundle.Name))
}

// Reload registries cache in case of configuration update
sysregistriesv2.InvalidateCache()

srcCtx, err := i.SourceContextFunc(l)
if err != nil {
return nil, err
}

//////////////////////////////////////////////////////
//
// Resolve a canonical reference for the image.
Expand Down Expand Up @@ -254,6 +259,11 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
if err != nil {
return fmt.Errorf("error creating image source: %w", err)
}
defer func() {
if err := layoutSrc.Close(); err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a question - imho panicking here seems a little excessive, but I see it in multiple places, so I think it's getting caught

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just following convention here :) Not sure it's the right way to handle it either but I just noticed the source close was missing so I wanted to catch that while I was in here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe let's proceed with the convention for this PR, but create a discussion in upstream slack? I'm also a bit 🤔 about these panics. Maybe we should level set on a tenet for when to use it and why?

}
}()

if err := os.MkdirAll(unpackPath, 0700); err != nil {
return fmt.Errorf("error creating unpack directory: %w", err)
Expand Down
76 changes: 76 additions & 0 deletions test/e2e/cluster_extension_install_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,82 @@ func TestClusterExtensionInstallRegistry(t *testing.T) {
}
}

func TestClusterExtensionInstallRegistryDynamic(t *testing.T) {
// NOTE: Like 'TestClusterExtensionInstallRegistry', this test also requires extra configuration in /etc/containers/registries.conf
packageName := "dynamic"

t.Log("When a cluster extension is installed from a catalog")
t.Log("When the extension bundle format is registry+v1")

clusterExtension, extensionCatalog, sa, ns := testInit(t)
defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns)
defer getArtifactsOutput(t)

clusterExtension.Spec = ocv1.ClusterExtensionSpec{
Source: ocv1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1.CatalogSource{
PackageName: packageName,
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{"olm.operatorframework.io/metadata.name": extensionCatalog.Name},
},
},
},
Namespace: ns.Name,
ServiceAccount: ocv1.ServiceAccountReference{
Name: sa.Name,
},
}
t.Log("It updates the registries.conf file contents")
cm := corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "e2e-registries-conf",
Namespace: "olmv1-system",
},
Data: map[string]string{
"registries.conf": `[[registry]]
prefix = "dynamic-registry.operator-controller-e2e.svc.cluster.local:5000"
location = "docker-registry.operator-controller-e2e.svc.cluster.local:5000"`,
},
}
require.NoError(t, c.Update(context.Background(), &cm))

t.Log("It resolves the specified package with correct bundle path")
t.Log("By creating the ClusterExtension resource")
require.NoError(t, c.Create(context.Background(), clusterExtension))

t.Log("By eventually reporting a successful resolution and bundle path")
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
}, 2*time.Minute, pollInterval)

// Give the check 2 minutes instead of the typical 1 for the pod's
// files to update from the configmap change.
// The theoretical max time is the kubelet sync period of 1 minute +
// ConfigMap cache TTL of 1 minute = 2 minutes
t.Log("By eventually reporting progressing as True")
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing)
if assert.NotNil(ct, cond) {
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1.ReasonSucceeded, cond.Reason)
}
}, 2*time.Minute, pollInterval)

t.Log("By eventually installing the package successfully")
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
if assert.NotNil(ct, cond) {
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1.ReasonSucceeded, cond.Reason)
assert.Contains(ct, cond.Message, "Installed bundle")
assert.NotEmpty(ct, clusterExtension.Status.Install.Bundle)
}
}, pollDuration, pollInterval)
}

func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) {
t.Log("When a cluster extension is installed from a catalog")

Expand Down
20 changes: 20 additions & 0 deletions testdata/images/catalogs/test-catalog/v1/configs/catalog.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,23 @@ properties:
value:
packageName: test-mirrored
version: 1.2.0
---
schema: olm.package
name: dynamic
defaultChannel: beta
---
schema: olm.channel
name: beta
package: dynamic
entries:
- name: dynamic-operator.1.2.0
---
schema: olm.bundle
name: dynamic-operator.1.2.0
package: dynamic
image: dynamic-registry.operator-controller-e2e.svc.cluster.local:5000/bundles/registry-v1/test-operator:v1.0.0
properties:
- type: olm.package
value:
packageName: dynamic
version: 1.2.0
Loading