Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
74 changes: 9 additions & 65 deletions internal/operator-controller/applier/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,17 @@ package applier
import (
"crypto/sha256"
"encoding/json"
"errors"
"fmt"
"io/fs"
"strings"

"helm.sh/helm/v3/pkg/chart"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/yaml"

"github.com/operator-framework/api/pkg/operators/v1alpha1"

ocv1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
)
Expand All @@ -35,9 +32,6 @@ type RegistryV1ManifestProvider struct {
IsWebhookSupportEnabled bool
IsSingleOwnNamespaceEnabled bool
}
type registryV1Config struct {
WatchNamespace string `json:"watchNamespace"`
}

func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtension) ([]client.Object, error) {
rv1, err := source.FromFS(bundleFS).GetBundle()
Expand Down Expand Up @@ -74,44 +68,18 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
render.WithCertificateProvider(r.CertificateProvider),
}

watchNamespace, err := r.getWatchNamespace(ext)
if err != nil {
return nil, fmt.Errorf("invalid bundle configuration: %w", err)
}

if watchNamespace != "" {
opts = append(opts, render.WithTargetNamespaces(watchNamespace))
}

return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
}

// getWatchNamespace determines the watch namespace the ClusterExtension should use based on the
// configuration in .spec.config.Inline. Only active if SingleOwnNamespace support is enabled.
func (r *RegistryV1ManifestProvider) getWatchNamespace(ext *ocv1.ClusterExtension) (string, error) {
if !r.IsSingleOwnNamespaceEnabled {
return "", nil
}

var watchNamespace string
if ext.Spec.Config != nil && ext.Spec.Config.Inline != nil {
cfg := &registryV1Config{}
// Using k8s.io/yaml package as that is able to handle both json and yaml
// In most cases, at this point we should have a valid JSON/YAML object in the byte slice and failures will
// be related to object structure (e.g. additional fields).
if err := yaml.UnmarshalStrict(ext.Spec.Config.Inline.Raw, cfg); err != nil {
return "", fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshallError(err))
if r.IsSingleOwnNamespaceEnabled && ext.Spec.Config != nil && ext.Spec.Config.ConfigType == ocv1.ClusterExtensionConfigTypeInline {
bundleConfig, err := bundle.UnmarshallConfig(ext.Spec.Config.Inline.Raw, &rv1, ext.Spec.Namespace)
if err != nil {
return nil, fmt.Errorf("invalid bundle configuration: %w", err)
}
watchNamespace = cfg.WatchNamespace
} else {
return "", nil
}

if errs := validation.IsDNS1123Subdomain(watchNamespace); len(errs) > 0 {
return "", fmt.Errorf("invalid watch namespace '%s': namespace must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", watchNamespace)
if bundleConfig != nil && bundleConfig.WatchNamespace != "" {
opts = append(opts, render.WithTargetNamespaces(bundleConfig.WatchNamespace))
}
}

return watchNamespace, nil
return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
}

// RegistryV1HelmChartProvider creates a Helm-Chart from a registry+v1 bundle and its associated ClusterExtension
Expand Down Expand Up @@ -160,27 +128,3 @@ func (r *RegistryV1HelmChartProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExten

return chrt, nil
}

func formatUnmarshallError(err error) error {
var unmarshalErr *json.UnmarshalTypeError
if errors.As(err, &unmarshalErr) {
if unmarshalErr.Field == "" {
return errors.New("input is not a valid JSON object")
} else {
return fmt.Errorf("invalid value type for field %q: expected %q but got %q", unmarshalErr.Field, unmarshalErr.Type.String(), unmarshalErr.Value)
}
}

// unwrap error until the core and process it
for {
unwrapped := errors.Unwrap(err)
if unwrapped == nil {
// usually the errors present in the form json: <message> or yaml: <message>
// we want to extract <message> if we can
errMessageComponents := strings.Split(err.Error(), ":")
coreErrMessage := strings.TrimSpace(errMessageComponents[len(errMessageComponents)-1])
return errors.New(coreErrMessage)
}
err = unwrapped
}
}
155 changes: 84 additions & 71 deletions internal/operator-controller/applier/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,42 @@ func Test_RegistryV1ManifestProvider_Integration(t *testing.T) {
require.Contains(t, err.Error(), "some error")
})

t.Run("surfaces bundle config unmarshall errors", func(t *testing.T) {
provider := applier.RegistryV1ManifestProvider{
BundleRenderer: render.BundleRenderer{
ResourceGenerators: []render.ResourceGenerator{
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
return nil, nil
},
},
},
// must be true for now as we only unmarshal configuration when this feature is on
// once we go GA and remove IsSingleOwnNamespaceEnabled it's ok to just delete this
IsSingleOwnNamespaceEnabled: true,
}

// The contents of the bundle are not important for this tesy, only that it be a valid bundle
// to avoid errors in the deserialization process
bundleFS := bundlefs.Builder().WithPackageName("test").
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()

ext := &ocv1.ClusterExtension{
Spec: ocv1.ClusterExtensionSpec{
Namespace: "install-namespace",
Config: &ocv1.ClusterExtensionConfig{
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
Inline: &apiextensionsv1.JSON{
Raw: []byte(`{"watchNamespace": "install-namespace"}`),
},
},
},
}

_, err := provider.Get(bundleFS, ext)
require.Error(t, err)
require.Contains(t, err.Error(), "invalid bundle configuration")
})

t.Run("returns rendered manifests", func(t *testing.T) {
provider := applier.RegistryV1ManifestProvider{
BundleRenderer: registryv1.Renderer,
Expand Down Expand Up @@ -188,77 +224,6 @@ func Test_RegistryV1ManifestProvider_WebhookSupport(t *testing.T) {
})
}

func Test_RegistryV1ManifestProvider_ConfigUnmarshalling(t *testing.T) {
for _, tc := range []struct {
name string
configBytes []byte
expectedErrMessage string
}{
{
name: "accepts json config",
configBytes: []byte(`{"watchNamespace": "some-namespace"}`),
},
{
name: "accepts yaml config",
configBytes: []byte(`watchNamespace: some-namespace`),
},
{
name: "rejects invalid json",
configBytes: []byte(`{"hello`),
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: found unexpected end of stream`,
},
{
name: "rejects valid json that isn't of object type",
configBytes: []byte(`true`),
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: input is not a valid JSON object`,
},
{
name: "rejects additional fields",
configBytes: []byte(`somekey: somevalue`),
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: unknown field "somekey"`,
},
{
name: "rejects valid json but invalid registry+v1",
configBytes: []byte(`{"watchNamespace": {"hello": "there"}}`),
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: invalid value type for field "watchNamespace": expected "string" but got "object"`,
},
} {
t.Run(tc.name, func(t *testing.T) {
provider := applier.RegistryV1ManifestProvider{
BundleRenderer: render.BundleRenderer{
ResourceGenerators: []render.ResourceGenerator{
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
return nil, nil
},
},
},
IsSingleOwnNamespaceEnabled: true,
}

bundleFS := bundlefs.Builder().WithPackageName("test").
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()

_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
Spec: ocv1.ClusterExtensionSpec{
Namespace: "install-namespace",
Config: &ocv1.ClusterExtensionConfig{
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
Inline: &apiextensionsv1.JSON{
Raw: tc.configBytes,
},
},
},
})
if tc.expectedErrMessage != "" {
require.Error(t, err)
require.Contains(t, err.Error(), tc.expectedErrMessage)
} else {
require.NoError(t, err)
}
})
}
}

func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
t.Run("rejects bundles without AllNamespaces install mode when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
provider := applier.RegistryV1ManifestProvider{
Expand All @@ -276,6 +241,54 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
require.Equal(t, "unsupported bundle: bundle does not support AllNamespaces install mode", err.Error())
})

t.Run("rejects bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
expectedWatchNamespace := "some-namespace"
provider := applier.RegistryV1ManifestProvider{
BundleRenderer: render.BundleRenderer{
ResourceGenerators: []render.ResourceGenerator{
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
t.Log("ensure watch namespace is appropriately configured")
require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces)
return nil, nil
},
},
},
IsSingleOwnNamespaceEnabled: false,
}

bundleFS := bundlefs.Builder().WithPackageName("test").
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()

_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
Spec: ocv1.ClusterExtensionSpec{
Namespace: "install-namespace",
Config: &ocv1.ClusterExtensionConfig{
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
Inline: &apiextensionsv1.JSON{
Raw: []byte(`{"watchNamespace": "` + expectedWatchNamespace + `"}`),
},
},
},
})
require.Error(t, err)
require.Contains(t, err.Error(), "unsupported bundle")
})

t.Run("rejects bundles without AllNamespaces install mode and with OwnNamespace support when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
provider := applier.RegistryV1ManifestProvider{
IsSingleOwnNamespaceEnabled: false,
}
bundleFS := bundlefs.Builder().WithPackageName("test").
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace).Build()).Build()
_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
Spec: ocv1.ClusterExtensionSpec{
Namespace: "install-namespace",
},
})
require.Error(t, err)
require.Contains(t, err.Error(), "unsupported bundle")
})

t.Run("accepts bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
expectedWatchNamespace := "some-namespace"
provider := applier.RegistryV1ManifestProvider{
Expand Down
108 changes: 108 additions & 0 deletions internal/operator-controller/rukpak/bundle/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package bundle

import (
"encoding/json"
"errors"
"fmt"
"strings"

"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation"
"sigs.k8s.io/yaml"

"github.com/operator-framework/api/pkg/operators/v1alpha1"
)

type Config struct {
WatchNamespace string `json:"watchNamespace"`
}

// UnmarshallConfig returns a deserialized and validated *bundle.Config based on bytes and validated
// against rv1 and the desired install namespaces. It will error if:
// - rv is nil
// - bytes is not a valid YAML/JSON object
// - bytes is a valid YAML/JSON object but does not follow the registry+v1 schema
// if bytes is nil a nil bundle.Config is returned
func UnmarshallConfig(bytes []byte, rv1 *RegistryV1, installNamespace string) (*Config, error) {
if bytes == nil {
return nil, nil
}
if rv1 == nil {
return nil, errors.New("bundle is nil")
}

bundleConfig := &Config{}
if err := yaml.UnmarshalStrict(bytes, bundleConfig); err != nil {
return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshallError(err))
}

if err := validateConfig(bundleConfig, rv1, installNamespace); err != nil {
return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", err)
}

return bundleConfig, nil
}

func validateConfig(config *Config, rv1 *RegistryV1, installNamespace string) error {
// no config, no problem
if config == nil {
return nil
}

// collect bundle install modes
installModeSet := sets.New(rv1.CSV.Spec.InstallModes...)
Copy link
Contributor

Choose a reason for hiding this comment

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

rv1 is passed into this function only to create this set of install modes. Hence, we can pass to the function these install modes directly, e.g.

func validateConfig(config *Config, rv1 *RegistryV1, installNamespace string, installModes sets.Set[v1alpha1.InstallMode]) error

nit: also, perhaps would be more readable if we implement validateConfig as receiver function on Config:

function (c *Config) ValidateFor(installNamespaces string, installModes sets.Set[v1alpha1.InstallMode]) error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the case now. But, not necessarily in the future. We want to validate based on what's in the bundle. At the moment the only pertinent thing is the install mode. I think as an API, taking in the bundle makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think as an API, taking in the bundle makes more sense.

This is a private function, so API can change whenever we see a need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry - I misread I thought this comment was under the Unmarshal function. I'll fix that up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then I wouldn't make it package public though...for the reasons I sited in the first response. I think as an API taking the bundle as a parameter is the right thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// only accept a non-empty value for watchNamespace if the bundle configuration accepts the watchNamespace config
if config.WatchNamespace != "" && !hasWatchNamespaceAsConfig(installModeSet) {
return errors.New(`unknown field "watchNamespace"`)
}

// validate input format
if errs := validation.IsDNS1123Subdomain(config.WatchNamespace); len(errs) > 0 {
return fmt.Errorf("invalid 'watchNamespace' %q: namespace must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character", config.WatchNamespace)
}

// only accept install namespace if OwnNamespace install mode is supported
if config.WatchNamespace == installNamespace &&
!installModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}) {
return fmt.Errorf("invalid 'watchNamespace' %q: must not be install namespace (%s)", config.WatchNamespace, installNamespace)
}

if config.WatchNamespace != installNamespace &&
!installModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) {
return fmt.Errorf("invalid 'watchNamespace' %q: must be install namespace (%s)", config.WatchNamespace, installNamespace)
}

return nil
}

func hasWatchNamespaceAsConfig(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

the function is used once only - thus for the readability it would be ok to put it inline in validate() function. BTW, the function name mentions watchnamespace, but it does not appear in the arguments - perhaps the function name should be rephrased to reflect what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question the function answers is whether the bundle supports the watchNamespace configuration - this is driven off the install mode support. If I don't need it again for the "required" case I'll inline it and add the appropriate comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored to isWatchNamespaceConfigSupported. I prefer to have these more complex clauses aliased under a more understandable function. So, I've left it in.

return bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) ||
bundleInstallModeSet.HasAll(
v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true},
v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true})
}

func formatUnmarshallError(err error) error {
var unmarshalErr *json.UnmarshalTypeError
if errors.As(err, &unmarshalErr) {
if unmarshalErr.Field == "" {
return errors.New("input is not a valid JSON object")
} else {
return fmt.Errorf("invalid value type for field %q: expected %q but got %q", unmarshalErr.Field, unmarshalErr.Type.String(), unmarshalErr.Value)
}
}

// unwrap error until the core and process it
for {
unwrapped := errors.Unwrap(err)
if unwrapped == nil {
// usually the errors present in the form json: <message> or yaml: <message>
// we want to extract <message> if we can
errMessageComponents := strings.Split(err.Error(), ":")
coreErrMessage := strings.TrimSpace(errMessageComponents[len(errMessageComponents)-1])
return errors.New(coreErrMessage)
}
err = unwrapped
}
}
Loading
Loading