Skip to content
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

⚠️ Only allow bundles with AllNamespaces install mode #924

Closed
Closed
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
3 changes: 2 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ CONTAINER_RUNTIME := docker
else ifneq (, $(shell command -v podman 2>/dev/null))
CONTAINER_RUNTIME := podman
else
$(warning Could not find docker or podman in path! This may result in targets requiring a container runtime failing!)
$(warning Could not find docker or podman in path! This may result in targets requiring a container runtime failing!)
endif

KUSTOMIZE_BUILD_DIR := config/default
Expand Down Expand Up @@ -194,6 +194,7 @@ registry-load-bundles: ## Load selected e2e testdata container images created in
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/prometheus-operator:v1.2.0 prometheus-operator.v1.2.0 prometheus-operator.v1.0.0
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/prometheus-operator:v2.0.0 prometheus-operator.v2.0.0 prometheus-operator.v1.0.0
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/package-with-webhooks:v1.0.0 package-with-webhooks.v1.0.0 package-with-webhooks.v1.0.0
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/package-with-unsupported-install-modes:v1.0.0 package-with-unsupported-install-modes.v1.0.0 package-with-unsupported-install-modes.v1.0.0

#SECTION Build

Expand Down
3 changes: 2 additions & 1 deletion internal/handler/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chartutil"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/yaml"

Expand All @@ -24,7 +25,7 @@ const (
)

func HandleClusterExtension(_ context.Context, fsys fs.FS, ext *ocv1alpha1.ClusterExtension) (*chart.Chart, chartutil.Values, error) {
plainFS, err := convert.RegistryV1ToPlain(fsys, ext.Spec.InstallNamespace, nil)
plainFS, err := convert.RegistryV1ToPlain(fsys, ext.Spec.InstallNamespace, []string{metav1.NamespaceAll})
Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we no longer needs this fix because we no longer have it in operator-controller since #928

if err != nil {
return nil, nil, fmt.Errorf("convert registry+v1 bundle to plain+v0 bundle: %v", err)
}
Expand Down
26 changes: 26 additions & 0 deletions test/e2e/cluster_extension_registryV1_limitations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,29 @@ func TestClusterExtensionPackagesWithWebhooksAreNotAllowed(t *testing.T) {
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "package-with-webhooks.1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle)
}, pollDuration, pollInterval)
}

func TestClusterExtensionPackagesWithUnsupportedInstallModesAreNotAllowed(t *testing.T) {
Copy link
Member

@joelanford joelanford Jun 12, 2024

Choose a reason for hiding this comment

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

Is there a way to put this in a unit test of the ClusterExtension reconciler instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@joelanford I started working on this (and another test above TestClusterExtensionPackagesWithWebhooksAreNotAllowed) when rukpak was a separate thing running in the cluster with unpacker and all that complicated setup.

The idea was that we have an e2e test which helps us verify current limitations. I was hoping that this test will also help with the helm/rukpak integration into operator-controller. But it got blocked by helm/rukpak integration :)

I do not think that we can test it on ClusterExtension reconciler: rukpak stuff is mocked there.

We can probably test it on this level with a fake FS, but at this point I'm not sure how useful it will be.

Now that we use rukpak as a library - we should probably treat it as such: assume that this stuff if well tested on the rukpak side and do not double test on operator controller.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Now that we use rukpak as a library - we should probably treat it as such: assume that this stuff if well tested on the rukpak side and do not double test on operator controller.

I think with @varshaprasad96 's change in #928, where the actual call to convert the bundle happens fully in rukpak, I agree.

ctx := context.Background()
clusterExtension, catalog := testInit(t)
defer testCleanup(t, catalog, clusterExtension)
defer getArtifactsOutput(t)

clusterExtension.Spec = ocv1alpha1.ClusterExtensionSpec{
PackageName: "package-with-unsupported-install-modes",
Version: "1.0.0",
InstallNamespace: "default",
}

require.NoError(t, c.Create(ctx, clusterExtension))
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(ctx, types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
if !assert.NotNil(ct, cond) {
return
}
assert.Equal(ct, metav1.ConditionFalse, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonInstallationFailed, cond.Reason)
assert.Contains(ct, cond.Message, "supported install modes [MultiNamespace OwnNamespace SingleNamespace] do not support target namespaces []")
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "package-with-unsupported-install-modes.1.0.0", Version: "1.0.0"}, clusterExtension.Status.ResolvedBundle)
}, pollDuration, pollInterval)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
FROM scratch

# Core bundle labels.
LABEL operators.operatorframework.io.bundle.mediatype.v1=registry+v1
LABEL operators.operatorframework.io.bundle.manifests.v1=manifests/
LABEL operators.operatorframework.io.bundle.metadata.v1=metadata/
LABEL operators.operatorframework.io.bundle.package.v1=package-with-unsupported-install-modes
LABEL operators.operatorframework.io.bundle.channels.v1=beta
LABEL operators.operatorframework.io.metrics.builder=operator-sdk-v1.28.0
LABEL operators.operatorframework.io.metrics.mediatype.v1=metrics+v1
LABEL operators.operatorframework.io.metrics.project_layout=unknown

# Copy files to locations specified by labels.
COPY manifests /manifests/
COPY metadata /metadata/
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
apiVersion: operators.coreos.com/v1alpha1
kind: ClusterServiceVersion
metadata:
name: package-with-unsupported-install-modes.v1.0.0
namespace: placeholder
spec:
webhookdefinitions:
- admissionReviewVersions:
- v1alpha1
- v1beta1
containerPort: 443
conversionCRDs:
- testcds.package-with-unsupported-install-modes.io
deploymentName: test-operator-controller-manager
generateName: testcrds
sideEffects: None
targetPort: 9443
type: ConversionWebhook
webhookPath: /convert
description: Test package with webhooks
displayName: Package with webhooks
icon:
- base64data: PHN2ZyB3aWR0aD0iMjQ5MCIgaGVpZ2h0PSIyNTAwIiB2aWV3Qm94PSIwIDAgMjU2IDI1NyIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIiBwcmVzZXJ2ZUFzcGVjdFJhdGlvPSJ4TWlkWU1pZCI+PHBhdGggZD0iTTEyOC4wMDEuNjY3QzU3LjMxMS42NjcgMCA1Ny45NzEgMCAxMjguNjY0YzAgNzAuNjkgNTcuMzExIDEyNy45OTggMTI4LjAwMSAxMjcuOTk4UzI1NiAxOTkuMzU0IDI1NiAxMjguNjY0QzI1NiA1Ny45NyAxOTguNjg5LjY2NyAxMjguMDAxLjY2N3ptMCAyMzkuNTZjLTIwLjExMiAwLTM2LjQxOS0xMy40MzUtMzYuNDE5LTMwLjAwNGg3Mi44MzhjMCAxNi41NjYtMTYuMzA2IDMwLjAwNC0zNi40MTkgMzAuMDA0em02MC4xNTMtMzkuOTRINjcuODQyVjE3OC40N2gxMjAuMzE0djIxLjgxNmgtLjAwMnptLS40MzItMzMuMDQ1SDY4LjE4NWMtLjM5OC0uNDU4LS44MDQtLjkxLTEuMTg4LTEuMzc1LTEyLjMxNS0xNC45NTQtMTUuMjE2LTIyLjc2LTE4LjAzMi0zMC43MTYtLjA0OC0uMjYyIDE0LjkzMyAzLjA2IDI1LjU1NiA1LjQ1IDAgMCA1LjQ2NiAxLjI2NSAxMy40NTggMi43MjItNy42NzMtOC45OTQtMTIuMjMtMjAuNDI4LTEyLjIzLTMyLjExNiAwLTI1LjY1OCAxOS42OC00OC4wNzkgMTIuNTgtNjYuMjAxIDYuOTEuNTYyIDE0LjMgMTQuNTgzIDE0LjggMzYuNTA1IDcuMzQ2LTEwLjE1MiAxMC40Mi0yOC42OSAxMC40Mi00MC4wNTYgMC0xMS43NjkgNy43NTUtMjUuNDQgMTUuNTEyLTI1LjkwNy02LjkxNSAxMS4zOTYgMS43OSAyMS4xNjUgOS41MyA0NS40IDIuOTAyIDkuMTAzIDIuNTMyIDI0LjQyMyA0Ljc3MiAzNC4xMzguNzQ0LTIwLjE3OCA0LjIxMy00OS42MiAxNy4wMTQtNTkuNzg0LTUuNjQ3IDEyLjguODM2IDI4LjgxOCA1LjI3IDM2LjUxOCA3LjE1NCAxMi40MjQgMTEuNDkgMjEuODM2IDExLjQ5IDM5LjYzOCAwIDExLjkzNi00LjQwNyAyMy4xNzMtMTEuODQgMzEuOTU4IDguNDUyLTEuNTg2IDE0LjI4OS0zLjAxNiAxNC4yODktMy4wMTZsMjcuNDUtNS4zNTVjLjAwMi0uMDAyLTMuOTg3IDE2LjQwMS0xOS4zMTQgMzIuMTk3eiIgZmlsbD0iI0RBNEUzMSIvPjwvc3ZnPg==
mediatype: image/svg+xml
installModes:
- supported: true
type: OwnNamespace
- supported: true
type: SingleNamespace
- supported: true
type: MultiNamespace
- supported: false
type: AllNamespaces
version: 1.0.0
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
annotations:
# Core bundle annotations.
operators.operatorframework.io.bundle.mediatype.v1: registry+v1
operators.operatorframework.io.bundle.manifests.v1: manifests/
operators.operatorframework.io.bundle.metadata.v1: metadata/
operators.operatorframework.io.bundle.package.v1: package-with-unsupported-install-modes
operators.operatorframework.io.bundle.channels.v1: beta
operators.operatorframework.io.metrics.builder: operator-sdk-v1.28.0
operators.operatorframework.io.metrics.mediatype.v1: metrics+v1
operators.operatorframework.io.metrics.project_layout: unknown
21 changes: 21 additions & 0 deletions testdata/catalogs/test-catalog/catalog.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,24 @@ properties:
value:
packageName: package-with-webhooks
version: 1.0.0

---
schema: olm.package
name: package-with-unsupported-install-modes
defaultChannel: beta
---
schema: olm.channel
name: beta
package: package-with-unsupported-install-modes
entries:
- name: package-with-unsupported-install-modes.1.0.0
---
schema: olm.bundle
name: package-with-unsupported-install-modes.1.0.0
package: package-with-unsupported-install-modes
image: docker-registry.operator-controller-e2e.svc.cluster.local:5000/bundles/registry-v1/package-with-unsupported-install-modes:v1.0.0
properties:
- type: olm.package
value:
packageName: package-with-unsupported-install-modes
version: 1.0.0
Loading