Skip to content

Commit a5a903c

Browse files
author
Per G. da Silva
committed
Address reviewer comments and add required case
Signed-off-by: Per G. da Silva <[email protected]>
1 parent 3a28e97 commit a5a903c

File tree

4 files changed

+130
-40
lines changed

4 files changed

+130
-40
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,13 @@ func (r *RegistryV1ManifestProvider) Get(bundleFS fs.FS, ext *ocv1.ClusterExtens
6969
}
7070

7171
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)
72+
bundleConfig, err := bundle.UnmarshallConfig(ext.Spec.Config.Inline.Raw, rv1, ext.Spec.Namespace)
7373
if err != nil {
7474
return nil, fmt.Errorf("invalid bundle configuration: %w", err)
7575
}
7676

77-
if bundleConfig != nil && bundleConfig.WatchNamespace != "" {
78-
opts = append(opts, render.WithTargetNamespaces(bundleConfig.WatchNamespace))
77+
if bundleConfig != nil && bundleConfig.WatchNamespace != nil {
78+
opts = append(opts, render.WithTargetNamespaces(*bundleConfig.WatchNamespace))
7979
}
8080
}
8181

‎internal/operator-controller/rukpak/bundle/config.go‎

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
)
1515

1616
type Config struct {
17-
WatchNamespace string `json:"watchNamespace"`
17+
WatchNamespace *string `json:"watchNamespace"`
1818
}
1919

2020
// UnmarshallConfig returns a deserialized and validated *bundle.Config based on bytes and validated
@@ -23,66 +23,83 @@ type Config struct {
2323
// - bytes is not a valid YAML/JSON object
2424
// - bytes is a valid YAML/JSON object but does not follow the registry+v1 schema
2525
// if bytes is nil a nil bundle.Config is returned
26-
func UnmarshallConfig(bytes []byte, rv1 *RegistryV1, installNamespace string) (*Config, error) {
26+
func UnmarshallConfig(bytes []byte, rv1 RegistryV1, installNamespace string) (*Config, error) {
2727
if bytes == nil {
2828
return nil, nil
2929
}
30-
if rv1 == nil {
31-
return nil, errors.New("bundle is nil")
32-
}
3330

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

39-
if err := validateConfig(bundleConfig, rv1, installNamespace); err != nil {
36+
// collect bundle install modes
37+
bundleInstallModeSet := sets.New(rv1.CSV.Spec.InstallModes...)
38+
39+
if err := validateConfig(bundleConfig, installNamespace, bundleInstallModeSet); err != nil {
4040
return nil, fmt.Errorf("error unmarshalling registry+v1 configuration: %w", err)
4141
}
4242

4343
return bundleConfig, nil
4444
}
4545

46-
func validateConfig(config *Config, rv1 *RegistryV1, installNamespace string) error {
46+
// validateConfig validates a *bundle.Config against the bundle's supported install modes and the user-give installNamespace.
47+
func validateConfig(config *Config, installNamespace string, bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) error {
4748
// no config, no problem
4849
if config == nil {
4950
return nil
5051
}
5152

52-
// collect bundle install modes
53-
installModeSet := sets.New(rv1.CSV.Spec.InstallModes...)
54-
55-
// only accept a non-empty value for watchNamespace if the bundle configuration accepts the watchNamespace config
56-
if config.WatchNamespace != "" && !hasWatchNamespaceAsConfig(installModeSet) {
53+
// if the bundle does not support the watchNamespace configuration and it is set, treat it like any unknown field
54+
if config.WatchNamespace != nil && !isWatchNamespaceConfigSupported(bundleInstallModeSet) {
5755
return errors.New(`unknown field "watchNamespace"`)
5856
}
5957

60-
// validate input format
61-
if errs := validation.IsDNS1123Subdomain(config.WatchNamespace); len(errs) > 0 {
62-
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)
58+
// if watchNamespace is required then ensure that it is set
59+
if config.WatchNamespace == nil && isWatchNamespaceConfigRequired(bundleInstallModeSet) {
60+
return errors.New(`required field "watchNamespace" is missing`)
61+
}
62+
63+
// if watchNamespace is set then ensure it is a valid namespace
64+
if config.WatchNamespace != nil {
65+
if errs := validation.IsDNS1123Subdomain(*config.WatchNamespace); len(errs) > 0 {
66+
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)
67+
}
6368
}
6469

6570
// only accept install namespace if OwnNamespace install mode is supported
66-
if config.WatchNamespace == installNamespace &&
67-
!installModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}) {
68-
return fmt.Errorf("invalid 'watchNamespace' %q: must not be install namespace (%s)", config.WatchNamespace, installNamespace)
71+
if config.WatchNamespace != nil && *config.WatchNamespace == installNamespace &&
72+
!bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true}) {
73+
return fmt.Errorf("invalid 'watchNamespace' %q: must not be install namespace (%s)", *config.WatchNamespace, installNamespace)
6974
}
7075

71-
if config.WatchNamespace != installNamespace &&
72-
!installModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) {
73-
return fmt.Errorf("invalid 'watchNamespace' %q: must be install namespace (%s)", config.WatchNamespace, installNamespace)
76+
// only accept non-install namespace is SingleNamespace is supported
77+
if config.WatchNamespace != nil && *config.WatchNamespace != installNamespace &&
78+
!bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) {
79+
return fmt.Errorf("invalid 'watchNamespace' %q: must be install namespace (%s)", *config.WatchNamespace, installNamespace)
7480
}
7581

7682
return nil
7783
}
7884

79-
func hasWatchNamespaceAsConfig(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool {
85+
// isWatchNamespaceConfigSupported returns true when the bundle exposes a watchNamespace configuration. This happens when:
86+
// - SingleNamespace install more is supported, or
87+
// - OwnNamespace and AllNamespaces install modes are supported
88+
func isWatchNamespaceConfigSupported(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool {
8089
return bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true}) ||
8190
bundleInstallModeSet.HasAll(
8291
v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeOwnNamespace, Supported: true},
8392
v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true})
8493
}
8594

95+
// isWatchNamespaceConfigRequired returns true if the watchNamespace configuration is required. This happens when
96+
// AllNamespaces install mode is not supported and SingleNamespace is supported
97+
func isWatchNamespaceConfigRequired(bundleInstallModeSet sets.Set[v1alpha1.InstallMode]) bool {
98+
return !bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeAllNamespaces, Supported: true}) &&
99+
bundleInstallModeSet.Has(v1alpha1.InstallMode{Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true})
100+
}
101+
102+
// formatUnmarshallError format JSON unmarshal errors to be more readable
86103
func formatUnmarshallError(err error) error {
87104
var unmarshalErr *json.UnmarshalTypeError
88105
if errors.As(err, &unmarshalErr) {

‎internal/operator-controller/rukpak/bundle/config_test.go‎

Lines changed: 64 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"testing"
55

66
"github.com/stretchr/testify/require"
7+
"k8s.io/utils/ptr"
78

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

@@ -25,25 +26,20 @@ func Test_UnmarshallConfig(t *testing.T) {
2526
rawConfig: nil,
2627
expectedConfig: nil,
2728
},
28-
{
29-
name: "rejects nil rv1",
30-
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
31-
expectedErrMessage: `bundle is nil`,
32-
},
3329
{
3430
name: "accepts json config",
3531
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace},
3632
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
3733
expectedConfig: &bundle.Config{
38-
WatchNamespace: "some-namespace",
34+
WatchNamespace: ptr.To("some-namespace"),
3935
},
4036
},
4137
{
4238
name: "accepts yaml config",
4339
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace},
4440
rawConfig: []byte(`watchNamespace: some-namespace`),
4541
expectedConfig: &bundle.Config{
46-
WatchNamespace: "some-namespace",
42+
WatchNamespace: ptr.To("some-namespace"),
4743
},
4844
},
4945
{
@@ -111,23 +107,23 @@ func Test_UnmarshallConfig(t *testing.T) {
111107
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace},
112108
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
113109
expectedConfig: &bundle.Config{
114-
WatchNamespace: "some-namespace",
110+
WatchNamespace: ptr.To("some-namespace"),
115111
},
116112
},
117113
{
118114
name: "accepts when install modes {AllNamespaces, SingleNamespace} and watchNamespace != install namespace",
119115
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace},
120116
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
121117
expectedConfig: &bundle.Config{
122-
WatchNamespace: "some-namespace",
118+
WatchNamespace: ptr.To("some-namespace"),
123119
},
124120
},
125121
{
126122
name: "accepts when install modes {MultiNamespace, SingleNamespace} and watchNamespace != install namespace",
127123
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeMultiNamespace, v1alpha1.InstallModeTypeSingleNamespace},
128124
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
129125
expectedConfig: &bundle.Config{
130-
WatchNamespace: "some-namespace",
126+
WatchNamespace: ptr.To("some-namespace"),
131127
},
132128
},
133129
{
@@ -136,7 +132,7 @@ func Test_UnmarshallConfig(t *testing.T) {
136132
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
137133
installNamespace: "not-namespace",
138134
expectedConfig: &bundle.Config{
139-
WatchNamespace: "some-namespace",
135+
WatchNamespace: ptr.To("some-namespace"),
140136
},
141137
},
142138
{
@@ -166,7 +162,7 @@ func Test_UnmarshallConfig(t *testing.T) {
166162
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
167163
installNamespace: "some-namespace",
168164
expectedConfig: &bundle.Config{
169-
WatchNamespace: "some-namespace",
165+
WatchNamespace: ptr.To("some-namespace"),
170166
},
171167
},
172168
{
@@ -175,7 +171,7 @@ func Test_UnmarshallConfig(t *testing.T) {
175171
rawConfig: []byte(`{"watchNamespace": "some-namespace"}`),
176172
installNamespace: "some-namespace",
177173
expectedConfig: &bundle.Config{
178-
WatchNamespace: "some-namespace",
174+
WatchNamespace: ptr.To("some-namespace"),
179175
},
180176
},
181177
{
@@ -185,11 +181,64 @@ func Test_UnmarshallConfig(t *testing.T) {
185181
installNamespace: "not-some-namespace",
186182
expectedErrMessage: "invalid 'watchNamespace' \"some-namespace\": must be install namespace (not-some-namespace)",
187183
},
184+
{
185+
name: "rejects with required field error when install modes {SingleNamespace} and watchNamespace is nil",
186+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace},
187+
rawConfig: []byte(`{"watchNamespace": null}`),
188+
installNamespace: "not-some-namespace",
189+
expectedErrMessage: "required field \"watchNamespace\" is missing",
190+
},
191+
{
192+
name: "rejects with required field error when install modes {SingleNamespace, OwnNamespace} and watchNamespace is nil",
193+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace},
194+
rawConfig: []byte(`{"watchNamespace": null}`),
195+
installNamespace: "not-some-namespace",
196+
expectedErrMessage: "required field \"watchNamespace\" is missing",
197+
},
198+
{
199+
name: "rejects with required field error when install modes {SingleNamespace, MultiNamespace} and watchNamespace is nil",
200+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace},
201+
rawConfig: []byte(`{"watchNamespace": null}`),
202+
installNamespace: "not-some-namespace",
203+
expectedErrMessage: "required field \"watchNamespace\" is missing",
204+
},
205+
{
206+
name: "rejects with required field error when install modes {SingleNamespace, OwnNamespace, MultiNamespace} and watchNamespace is nil",
207+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace},
208+
rawConfig: []byte(`{"watchNamespace": null}`),
209+
installNamespace: "not-some-namespace",
210+
expectedErrMessage: "required field \"watchNamespace\" is missing",
211+
},
212+
{
213+
name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace} and watchNamespace is nil",
214+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace},
215+
rawConfig: []byte(`{"watchNamespace": null}`),
216+
installNamespace: "not-some-namespace",
217+
expectedConfig: &bundle.Config{
218+
WatchNamespace: nil,
219+
},
220+
},
221+
{
222+
name: "accepts null watchNamespace when install modes {AllNamespaces, OwnNamespace, MultiNamespace} and watchNamespace is nil",
223+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace},
224+
rawConfig: []byte(`{"watchNamespace": null}`),
225+
installNamespace: "not-some-namespace",
226+
expectedConfig: &bundle.Config{
227+
WatchNamespace: nil,
228+
},
229+
},
230+
{
231+
name: "rejects with format error when install modes are {SingleNamespace, OwnNamespace} and watchNamespace is ''",
232+
supportedInstallModes: []v1alpha1.InstallModeType{v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace},
233+
rawConfig: []byte(`{"watchNamespace": ""}`),
234+
installNamespace: "not-some-namespace",
235+
expectedErrMessage: "invalid 'watchNamespace' \"\": namespace must consist of lower case alphanumeric characters",
236+
},
188237
} {
189238
t.Run(tc.name, func(t *testing.T) {
190-
var rv1 *bundle.RegistryV1
239+
var rv1 bundle.RegistryV1
191240
if tc.supportedInstallModes != nil {
192-
rv1 = &bundle.RegistryV1{
241+
rv1 = bundle.RegistryV1{
193242
CSV: clusterserviceversion.Builder().
194243
WithName("test-operator").
195244
WithInstallModeSupportFor(tc.supportedInstallModes...).

‎internal/operator-controller/rukpak/render/fake.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package render
2+
3+
import (
4+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
5+
"sigs.k8s.io/controller-runtime/pkg/client"
6+
)
7+
8+
type FakeCertProvider struct {
9+
InjectCABundleFn func(obj client.Object, cfg CertificateProvisionerConfig) error
10+
AdditionalObjectsFn func(cfg CertificateProvisionerConfig) ([]unstructured.Unstructured, error)
11+
GetCertSecretInfoFn func(cfg CertificateProvisionerConfig) CertSecretInfo
12+
}
13+
14+
func (f FakeCertProvider) InjectCABundle(obj client.Object, cfg CertificateProvisionerConfig) error {
15+
return f.InjectCABundleFn(obj, cfg)
16+
}
17+
18+
func (f FakeCertProvider) AdditionalObjects(cfg CertificateProvisionerConfig) ([]unstructured.Unstructured, error) {
19+
return f.AdditionalObjectsFn(cfg)
20+
}
21+
22+
func (f FakeCertProvider) GetCertSecretInfo(cfg CertificateProvisionerConfig) CertSecretInfo {
23+
return f.GetCertSecretInfoFn(cfg)
24+
}

0 commit comments

Comments
 (0)