Skip to content

✨ OPRUN-4113: Compute target namespace defaults #2178

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.

Sign up for GitHub

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 3 commits into from
Sep 4, 2025
Merged

✨ OPRUN-4113: Compute target namespace defaults #2178

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc73f58
Compute target namespace defaults
Sep 2, 2025
4fc183a
Update target namespace validation to reject own namespace installati…
Sep 3, 2025
01b28a8
Update WithTargetNamespace behavior and MultiNamespace validation
Sep 4, 2025
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
58 changes: 41 additions & 17 deletions internal/operator-controller/rukpak/render/render.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"errors"
"fmt"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets"
"sigs.k8s.io/controller-runtime/pkg/client"

Expand Down Expand Up @@ -74,9 +74,6 @@ func (o *Options) apply(opts ...Option) *Options {

func (o *Options) validate(rv1 *bundle.RegistryV1) (*Options, []error) {
var errs []error
if len(o.TargetNamespaces) == 0 {
errs = append(errs, errors.New("at least one target namespace must be specified"))
}
if o.UniqueNameGenerator == nil {
errs = append(errs, errors.New("unique name generator must be specified"))
}
Expand Down Expand Up @@ -121,7 +118,7 @@ func (r BundleRenderer) Render(rv1 bundle.RegistryV1, installNamespace string, o
genOpts, errs := (&Options{
// default options
InstallNamespace: installNamespace,
TargetNamespaces: []string{metav1.NamespaceAll},
TargetNamespaces: defaultTargetNamespacesForBundle(&rv1, installNamespace),
UniqueNameGenerator: DefaultUniqueNameGenerator,
CertificateProvider: nil,
}).apply(opts...).validate(&rv1)
Expand All @@ -147,31 +144,58 @@ func DefaultUniqueNameGenerator(base string, o interface{}) (string, error) {
}

func validateTargetNamespaces(rv1 *bundle.RegistryV1, installNamespace string, targetNamespaces []string) error {
supportedInstallModes := sets.New[string]()
for _, im := range rv1.CSV.Spec.InstallModes {
if im.Supported {
supportedInstallModes.Insert(string(im.Type))
}
}
supportedInstallModes := supportedBundleInstallModes(rv1)

set := sets.New[string](targetNamespaces...)
switch {
case set.Len() == 0 || (set.Len() == 1 && set.Has("")):
if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) {
case set.Len() == 0:
if supportedInstallModes.Len() == 1 && supportedInstallModes.Has(v1alpha1.InstallModeTypeSingleNamespace) {
Copy link
Member

Choose a reason for hiding this comment

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

Trying to make sense of this. Am I understanding correctly that set.Len() will only be 0 if we were unable to set a default target namespace, and that only happens when neither AllNamespaces nor OwnNamespace are supported.

And therefore, by the time we get to this code, the possibilities are:

  • only SingleNamespace
  • only MultiNamespace
  • both SingleNamespace and MultiNamespace

And of those three cases, the only one that requires exactly one target namespace is "only SingleNamespace. Hence the if conditional you have here.

If that's correct. Can you add a comment that explains this a little bit? I don't think it is obvious why we aren't checking other supported install mode combinations.

Copy link
Member

@joelanford joelanford Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, what if someone explicitly uses option WithTargetNamespaces(nil) or WithTargetNamespaces([]string{}])?

The result of that would be:

  1. The defaulting logic runs (setting a default in the case of All or Own namespace)
  2. The functional option runs (unsetting the default)

So we could have set.Len() == 0 for any combination of supported install modes unless we somehow require at least one namespace to be defined with that option. What if we did this?

func WithTargetNamespaces(namespace string, extraNamespaces ...string) Option {
	return func(o *Options) {
		o.TargetNamespaces = append([]string{namespace}, extraNamespaces...)
	}
}

That way, using WithTargetNamespaces only accepts 1..N namespaces, not 0..N.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I think this change in option does guarantee that at least one namespace is specified, it's annoying to use for the caller because it will force me to break up my slice of string into components. I won't be able to do targetNamespaces..., I'd need to do something like targetNamespace[0], targetNamespaces[1:]... but that will involve length checks, etc.

I think its ok for you to specify nil or empty...the validator will pick it up.

alternatively, we could just wrap an if around it and only update the value if len(namespaces) > 0? wdyt?

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 went with my proposed solution above. I've also added more checks for MultiNamespace (no including install namespace if no OwnNamespace support)

Copy link
Member

Choose a reason for hiding this comment

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

Good points! Your solution seems like a good compromise.

return errors.New("exactly one target namespace must be specified")
}
return errors.New("at least one target namespace must be specified")
case set.Len() == 1 && set.Has(""):
if supportedInstallModes.Has(v1alpha1.InstallModeTypeAllNamespaces) {
return nil
}
return fmt.Errorf("supported install modes %v do not support targeting all namespaces", sets.List(supportedInstallModes))
case set.Len() == 1 && !set.Has(""):
if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeSingleNamespace)) {
if targetNamespaces[0] == installNamespace {
if !supportedInstallModes.Has(v1alpha1.InstallModeTypeOwnNamespace) {
return fmt.Errorf("supported install modes %v do not support targeting own namespace", sets.List(supportedInstallModes))
}
return nil
}
if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeOwnNamespace)) && targetNamespaces[0] == installNamespace {
if supportedInstallModes.Has(v1alpha1.InstallModeTypeSingleNamespace) {
return nil
}
default:
if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeMultiNamespace)) && !set.Has("") {
if supportedInstallModes.Has(v1alpha1.InstallModeTypeMultiNamespace) && !set.Has("") {
return nil
}
}
return fmt.Errorf("supported install modes %v do not support target namespaces %v", sets.List[string](supportedInstallModes), targetNamespaces)
return fmt.Errorf("supported install modes %v do not support target namespaces %v", sets.List[v1alpha1.InstallModeType](supportedInstallModes), targetNamespaces)
}

func defaultTargetNamespacesForBundle(rv1 *bundle.RegistryV1, installNamespace string) []string {
supportedInstallModes := supportedBundleInstallModes(rv1)

if supportedInstallModes.Has(v1alpha1.InstallModeTypeAllNamespaces) {
return []string{corev1.NamespaceAll}
}

if supportedInstallModes.Has(v1alpha1.InstallModeTypeOwnNamespace) {
return []string{installNamespace}
}

return nil
}

func supportedBundleInstallModes(rv1 *bundle.RegistryV1) sets.Set[v1alpha1.InstallModeType] {
supportedInstallModes := sets.New[v1alpha1.InstallModeType]()
for _, im := range rv1.CSV.Spec.InstallModes {
if im.Supported {
supportedInstallModes.Insert(im.Type)
}
}
return supportedInstallModes
}
149 changes: 135 additions & 14 deletions internal/operator-controller/rukpak/render/render_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,136 @@ func Test_BundleRenderer_ValidatesBundle(t *testing.T) {
require.Contains(t, err.Error(), "this bundle is invalid")
}

func Test_BundleRenderer_CreatesCorrectDefaultOptions(t *testing.T) {
expectedInstallNamespace := "install-namespace"
expectedTargetNamespaces := []string{""}
expectedUniqueNameGenerator := render.DefaultUniqueNameGenerator
func Test_BundleRenderer_CreatesCorrectRenderOptions_WithDefaults(t *testing.T) {
const (
expectedInstallNamespace = "install-namespace"
)

renderer := render.BundleRenderer{
ResourceGenerators: []render.ResourceGenerator{
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
for _, tc := range []struct {
name string
csv v1alpha1.ClusterServiceVersion
validate func(t *testing.T, opts render.Options)
expectedErrMsgFragment string
}{
{
name: "sets install-namespace correctly",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, expectedInstallNamespace, opts.InstallNamespace)
require.Equal(t, expectedTargetNamespaces, opts.TargetNamespaces)
require.Equal(t, reflect.ValueOf(expectedUniqueNameGenerator).Pointer(), reflect.ValueOf(render.DefaultUniqueNameGenerator).Pointer(), "options has unexpected default unique name generator")
return nil, nil
},
}, {
name: "uses DefaultUniqueNameGenerator by default",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, reflect.ValueOf(render.DefaultUniqueNameGenerator).Pointer(), reflect.ValueOf(opts.UniqueNameGenerator).Pointer(), "options has unexpected default unique name generator")
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, SingleNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, OwnNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, OwnNamespace, SingleNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, OwnNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, SingleNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [corev1.NamespaceAll] by default if bundle supports install modes [AllNamespaces, SingleNamespace, OwnNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{corev1.NamespaceAll}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [install-namespace] by default if bundle supports install modes [OwnNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{expectedInstallNamespace}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [install-namespace] by default if bundle supports install modes [OwnNamespace, SingleNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{expectedInstallNamespace}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [install-namespace] by default if bundle supports install modes [OwnNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{expectedInstallNamespace}, opts.TargetNamespaces)
},
}, {
name: "sets target namespaces to [install-namespace] by default if bundle supports install modes [OwnNamespace, SingleNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeOwnNamespace, v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
validate: func(t *testing.T, opts render.Options) {
require.Equal(t, []string{expectedInstallNamespace}, opts.TargetNamespaces)
},
}, {
name: "returns error if target namespaces is not set bundle supports install modes [SingleNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace)),
expectedErrMsgFragment: "exactly one target namespace must be specified",
}, {
name: "returns error if target namespaces is not set bundle supports install modes [MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeMultiNamespace)),
expectedErrMsgFragment: "at least one target namespace must be specified",
}, {
name: "returns error if target namespaces is not set bundle supports install modes [SingleNamespace, MultiNamespace]",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeMultiNamespace)),
expectedErrMsgFragment: "at least one target namespace must be specified",
},
}
} {
t.Run(tc.name, func(t *testing.T) {
renderer := render.BundleRenderer{
ResourceGenerators: []render.ResourceGenerator{
func(rv1 *bundle.RegistryV1, opts render.Options) ([]client.Object, error) {
tc.validate(t, opts)
return nil, nil
},
},
}

_, _ = renderer.Render(bundle.RegistryV1{}, expectedInstallNamespace)
_, err := renderer.Render(bundle.RegistryV1{
CSV: tc.csv,
}, expectedInstallNamespace)
if tc.expectedErrMsgFragment == "" {
require.NoError(t, err)
} else {
require.Error(t, err)
require.Contains(t, err.Error(), tc.expectedErrMsgFragment)
}
})
}
}

func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
Expand All @@ -76,7 +189,7 @@ func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
opts: []render.Option{
render.WithTargetNamespaces(),
},
err: errors.New("invalid option(s): at least one target namespace must be specified"),
err: errors.New("invalid option(s): invalid target namespaces []: at least one target namespace must be specified"),
}, {
name: "rejects nil unique name generator",
installNamespace: "install-namespace",
Expand All @@ -100,7 +213,7 @@ func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
opts: []render.Option{
render.WithTargetNamespaces("install-namespace"),
},
err: errors.New("invalid option(s): invalid target namespaces [install-namespace]: supported install modes [AllNamespaces] do not support target namespaces [install-namespace]"),
err: errors.New("invalid option(s): invalid target namespaces [install-namespace]: supported install modes [AllNamespaces] do not support targeting own namespace"),
}, {
name: "rejects install out of own namespace if only OwnNamespace install mode is supported",
installNamespace: "install-namespace",
Expand Down Expand Up @@ -153,6 +266,14 @@ func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
opts: []render.Option{
render.WithTargetNamespaces("some-namespace"),
},
}, {
name: "rejects single namespace render if OwnNamespace install mode is unsupported and targets own namespace",
installNamespace: "install-namespace",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace)),
opts: []render.Option{
render.WithTargetNamespaces("install-namespace"),
},
err: errors.New("invalid option(s): invalid target namespaces [install-namespace]: supported install modes [SingleNamespace] do not support targeting own namespace"),
}, {
name: "accepts multi namespace render if MultiNamespace install mode is supported",
installNamespace: "install-namespace",
Expand Down
Loading