Skip to content

Commit 238dcf6

Browse files
perdasilvaPer G. da Silva
perdasilva
and
Per G. da Silva
authored
🌱 Add registry+v1 bundle config unmarshal and validation layer (#2278)
* Add registry+v1 bundle config unmarshal function Signed-off-by: Per G. da Silva <[email protected]> * Update manifest provider to use config unmarshal and remove getWatchNamespace Signed-off-by: Per G. da Silva <[email protected]> * Address reviewer comments and add required case Signed-off-by: Per G. da Silva <[email protected]> --------- Signed-off-by: Per G. da Silva <[email protected]> Co-authored-by: Per G. da Silva <[email protected]>
1 parent 57345c8 commit 238dcf6

File tree

5 files changed

+501
-136
lines changed

5 files changed

+501
-136
lines changed

‎internal/operator-controller/applier/provider.go‎

Lines changed: 9 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -3,20 +3,17 @@ package applier
33
import (
44
"crypto/sha256"
55
"encoding/json"
6-
"errors"
76
"fmt"
87
"io/fs"
9-
"strings"
108

119
"helm.sh/helm/v3/pkg/chart"
1210
"k8s.io/apimachinery/pkg/util/sets"
13-
"k8s.io/apimachinery/pkg/util/validation"
1411
"sigs.k8s.io/controller-runtime/pkg/client"
15-
"sigs.k8s.io/yaml"
1612

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

1915
ocv1 "github.com/operator-framework/operator-controller/api/v1"
16+
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle"
2017
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/bundle/source"
2118
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
2219
)
@@ -35,9 +32,6 @@ type RegistryV1ManifestProvider struct {
3532
IsWebhookSupportEnabled bool
3633
IsSingleOwnNamespaceEnabled bool
3734
}
38-
type registryV1Config struct {
39-
WatchNamespace string `json:"watchNamespace"`
40-
}
4135

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

77-
watchNamespace, err := r.getWatchNamespace(ext)
78-
if err != nil {
79-
return nil, fmt.Errorf("invalid bundle configuration: %w", err)
80-
}
81-
82-
if watchNamespace != "" {
83-
opts = append(opts, render.WithTargetNamespaces(watchNamespace))
84-
}
85-
86-
return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
87-
}
88-
89-
// getWatchNamespace determines the watch namespace the ClusterExtension should use based on the
90-
// configuration in .spec.config.Inline. Only active if SingleOwnNamespace support is enabled.
91-
func (r *RegistryV1ManifestProvider) getWatchNamespace(ext *ocv1.ClusterExtension) (string, error) {
92-
if !r.IsSingleOwnNamespaceEnabled {
93-
return "", nil
94-
}
95-
96-
var watchNamespace string
97-
if ext.Spec.Config != nil && ext.Spec.Config.Inline != nil {
98-
cfg := &registryV1Config{}
99-
// Using k8s.io/yaml package as that is able to handle both json and yaml
100-
// In most cases, at this point we should have a valid JSON/YAML object in the byte slice and failures will
101-
// be related to object structure (e.g. additional fields).
102-
if err := yaml.UnmarshalStrict(ext.Spec.Config.Inline.Raw, cfg); err != nil {
103-
return "", fmt.Errorf("error unmarshalling registry+v1 configuration: %w", formatUnmarshallError(err))
71+
if r.IsSingleOwnNamespaceEnabled && ext.Spec.Config != nil && ext.Spec.Config.ConfigType == ocv1.ClusterExtensionConfigTypeInline {
72+
bundleConfig, err := bundle.UnmarshallConfig(ext.Spec.Config.Inline.Raw, rv1, ext.Spec.Namespace)
73+
if err != nil {
74+
return nil, fmt.Errorf("invalid bundle configuration: %w", err)
10475
}
105-
watchNamespace = cfg.WatchNamespace
106-
} else {
107-
return "", nil
108-
}
10976

110-
if errs := validation.IsDNS1123Subdomain(watchNamespace); len(errs) > 0 {
111-
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)
77+
if bundleConfig != nil && bundleConfig.WatchNamespace != nil {
78+
opts = append(opts, render.WithTargetNamespaces(*bundleConfig.WatchNamespace))
79+
}
11280
}
11381

114-
return watchNamespace, nil
82+
return r.BundleRenderer.Render(rv1, ext.Spec.Namespace, opts...)
11583
}
11684

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

161129
return chrt, nil
162130
}
163-
164-
func formatUnmarshallError(err error) error {
165-
var unmarshalErr *json.UnmarshalTypeError
166-
if errors.As(err, &unmarshalErr) {
167-
if unmarshalErr.Field == "" {
168-
return errors.New("input is not a valid JSON object")
169-
} else {
170-
return fmt.Errorf("invalid value type for field %q: expected %q but got %q", unmarshalErr.Field, unmarshalErr.Type.String(), unmarshalErr.Value)
171-
}
172-
}
173-
174-
// unwrap error until the core and process it
175-
for {
176-
unwrapped := errors.Unwrap(err)
177-
if unwrapped == nil {
178-
// usually the errors present in the form json: <message> or yaml: <message>
179-
// we want to extract <message> if we can
180-
errMessageComponents := strings.Split(err.Error(), ":")
181-
coreErrMessage := strings.TrimSpace(errMessageComponents[len(errMessageComponents)-1])
182-
return errors.New(coreErrMessage)
183-
}
184-
err = unwrapped
185-
}
186-
}

‎internal/operator-controller/applier/provider_test.go‎

Lines changed: 84 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,42 @@ func Test_RegistryV1ManifestProvider_Integration(t *testing.T) {
6464
require.Contains(t, err.Error(), "some error")
6565
})
6666

67+
t.Run("surfaces bundle config unmarshall errors", func(t *testing.T) {
68+
provider := applier.RegistryV1ManifestProvider{
69+
BundleRenderer: render.BundleRenderer{
70+
ResourceGenerators: []render.ResourceGenerator{
71+
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
72+
return nil, nil
73+
},
74+
},
75+
},
76+
// must be true for now as we only unmarshal configuration when this feature is on
77+
// once we go GA and remove IsSingleOwnNamespaceEnabled it's ok to just delete this
78+
IsSingleOwnNamespaceEnabled: true,
79+
}
80+
81+
// The contents of the bundle are not important for this tesy, only that it be a valid bundle
82+
// to avoid errors in the deserialization process
83+
bundleFS := bundlefs.Builder().WithPackageName("test").
84+
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()
85+
86+
ext := &ocv1.ClusterExtension{
87+
Spec: ocv1.ClusterExtensionSpec{
88+
Namespace: "install-namespace",
89+
Config: &ocv1.ClusterExtensionConfig{
90+
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
91+
Inline: &apiextensionsv1.JSON{
92+
Raw: []byte(`{"watchNamespace": "install-namespace"}`),
93+
},
94+
},
95+
},
96+
}
97+
98+
_, err := provider.Get(bundleFS, ext)
99+
require.Error(t, err)
100+
require.Contains(t, err.Error(), "invalid bundle configuration")
101+
})
102+
67103
t.Run("returns rendered manifests", func(t *testing.T) {
68104
provider := applier.RegistryV1ManifestProvider{
69105
BundleRenderer: registryv1.Renderer,
@@ -188,77 +224,6 @@ func Test_RegistryV1ManifestProvider_WebhookSupport(t *testing.T) {
188224
})
189225
}
190226

191-
func Test_RegistryV1ManifestProvider_ConfigUnmarshalling(t *testing.T) {
192-
for _, tc := range []struct {
193-
name string
194-
configBytes []byte
195-
expectedErrMessage string
196-
}{
197-
{
198-
name: "accepts json config",
199-
configBytes: []byte(`{"watchNamespace": "some-namespace"}`),
200-
},
201-
{
202-
name: "accepts yaml config",
203-
configBytes: []byte(`watchNamespace: some-namespace`),
204-
},
205-
{
206-
name: "rejects invalid json",
207-
configBytes: []byte(`{"hello`),
208-
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: found unexpected end of stream`,
209-
},
210-
{
211-
name: "rejects valid json that isn't of object type",
212-
configBytes: []byte(`true`),
213-
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: input is not a valid JSON object`,
214-
},
215-
{
216-
name: "rejects additional fields",
217-
configBytes: []byte(`somekey: somevalue`),
218-
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: unknown field "somekey"`,
219-
},
220-
{
221-
name: "rejects valid json but invalid registry+v1",
222-
configBytes: []byte(`{"watchNamespace": {"hello": "there"}}`),
223-
expectedErrMessage: `invalid bundle configuration: error unmarshalling registry+v1 configuration: invalid value type for field "watchNamespace": expected "string" but got "object"`,
224-
},
225-
} {
226-
t.Run(tc.name, func(t *testing.T) {
227-
provider := applier.RegistryV1ManifestProvider{
228-
BundleRenderer: render.BundleRenderer{
229-
ResourceGenerators: []render.ResourceGenerator{
230-
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
231-
return nil, nil
232-
},
233-
},
234-
},
235-
IsSingleOwnNamespaceEnabled: true,
236-
}
237-
238-
bundleFS := bundlefs.Builder().WithPackageName("test").
239-
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()
240-
241-
_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
242-
Spec: ocv1.ClusterExtensionSpec{
243-
Namespace: "install-namespace",
244-
Config: &ocv1.ClusterExtensionConfig{
245-
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
246-
Inline: &apiextensionsv1.JSON{
247-
Raw: tc.configBytes,
248-
},
249-
},
250-
},
251-
})
252-
if tc.expectedErrMessage != "" {
253-
require.Error(t, err)
254-
require.Contains(t, err.Error(), tc.expectedErrMessage)
255-
} else {
256-
require.NoError(t, err)
257-
}
258-
})
259-
}
260-
}
261-
262227
func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
263228
t.Run("rejects bundles without AllNamespaces install mode when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
264229
provider := applier.RegistryV1ManifestProvider{
@@ -276,6 +241,54 @@ func Test_RegistryV1ManifestProvider_SingleOwnNamespaceSupport(t *testing.T) {
276241
require.Equal(t, "unsupported bundle: bundle does not support AllNamespaces install mode", err.Error())
277242
})
278243

244+
t.Run("rejects bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
245+
expectedWatchNamespace := "some-namespace"
246+
provider := applier.RegistryV1ManifestProvider{
247+
BundleRenderer: render.BundleRenderer{
248+
ResourceGenerators: []render.ResourceGenerator{
249+
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
250+
t.Log("ensure watch namespace is appropriately configured")
251+
require.Equal(t, []string{expectedWatchNamespace}, opts.TargetNamespaces)
252+
return nil, nil
253+
},
254+
},
255+
},
256+
IsSingleOwnNamespaceEnabled: false,
257+
}
258+
259+
bundleFS := bundlefs.Builder().WithPackageName("test").
260+
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace).Build()).Build()
261+
262+
_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
263+
Spec: ocv1.ClusterExtensionSpec{
264+
Namespace: "install-namespace",
265+
Config: &ocv1.ClusterExtensionConfig{
266+
ConfigType: ocv1.ClusterExtensionConfigTypeInline,
267+
Inline: &apiextensionsv1.JSON{
268+
Raw: []byte(`{"watchNamespace": "` + expectedWatchNamespace + `"}`),
269+
},
270+
},
271+
},
272+
})
273+
require.Error(t, err)
274+
require.Contains(t, err.Error(), "unsupported bundle")
275+
})
276+
277+
t.Run("rejects bundles without AllNamespaces install mode and with OwnNamespace support when Single/OwnNamespace install mode support is disabled", func(t *testing.T) {
278+
provider := applier.RegistryV1ManifestProvider{
279+
IsSingleOwnNamespaceEnabled: false,
280+
}
281+
bundleFS := bundlefs.Builder().WithPackageName("test").
282+
WithCSV(clusterserviceversion.Builder().WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace).Build()).Build()
283+
_, err := provider.Get(bundleFS, &ocv1.ClusterExtension{
284+
Spec: ocv1.ClusterExtensionSpec{
285+
Namespace: "install-namespace",
286+
},
287+
})
288+
require.Error(t, err)
289+
require.Contains(t, err.Error(), "unsupported bundle")
290+
})
291+
279292
t.Run("accepts bundles without AllNamespaces install mode and with SingleNamespace support when Single/OwnNamespace install mode support is enabled", func(t *testing.T) {
280293
expectedWatchNamespace := "some-namespace"
281294
provider := applier.RegistryV1ManifestProvider{

0 commit comments

Comments
 (0)