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 all 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
76 changes: 58 additions & 18 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 All @@ -88,9 +85,14 @@ func (o *Options) validate(rv1 *bundle.RegistryV1) (*Options, []error) {

type Option func(*Options)

// WithTargetNamespaces sets the target namespaces to be used when rendering the bundle
// The value will only be used if len(namespaces) > 0. Otherwise, the default value for the bundle
// derived from its install mode support will be used (if such a value can be defined).
func WithTargetNamespaces(namespaces ...string) Option {
return func(o *Options) {
o.TargetNamespaces = namespaces
if len(namespaces) > 0 {
o.TargetNamespaces = namespaces
}
}
}

Expand Down Expand Up @@ -121,7 +123,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 +149,69 @@ 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:
// Note: this function generally expects targetNamespace to contain at least one value set by default
// in case the user does not specify the value. The option to set the targetNamespace is a no-op if it is empty.
// The only case for which a default targetNamespace is undefined is in the case of a bundle that only
// supports SingleNamespace install mode. The if statement here is added to provide a more friendly error
// message than just the generic (at least one target namespace must be specified) which would occur
// in case only the MultiNamespace install mode is supported by the bundle.
// If AllNamespaces mode is supported, the default will be [""] -> watch all namespaces
// If only OwnNamespace is supported, the default will be [install-namespace] -> only watch the install/own namespace
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.InstallModeTypeOwnNamespace) && set.Has(installNamespace) {
return fmt.Errorf("supported install modes %v do not support targeting own namespace", sets.List(supportedInstallModes))
}
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
}
13 changes: 10 additions & 3 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 @@ -70,13 +70,12 @@ func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
err error
}{
{
name: "rejects empty targetNamespaces",
name: "accepts empty targetNamespaces (because it is ignored)",
installNamespace: "install-namespace",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces)),
opts: []render.Option{
render.WithTargetNamespaces(),
},
err: errors.New("invalid option(s): at least one target namespace must be specified"),
}, {
name: "rejects nil unique name generator",
installNamespace: "install-namespace",
Expand All @@ -100,7 +99,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 @@ -160,6 +159,14 @@ func Test_BundleRenderer_ValidatesRenderOptions(t *testing.T) {
opts: []render.Option{
render.WithTargetNamespaces("n1", "n2", "n3"),
},
}, {
name: "reject multi namespace render if OwnNamespace install mode is not supported and target namespaces include install namespace",
installNamespace: "install-namespace",
csv: MakeCSV(WithInstallModeSupportFor(v1alpha1.InstallModeTypeMultiNamespace)),
opts: []render.Option{
render.WithTargetNamespaces("n1", "n2", "n3", "install-namespace"),
},
err: errors.New("invalid option(s): invalid target namespaces [n1 n2 n3 install-namespace]: supported install modes [MultiNamespace] do not support targeting own namespace"),
},
} {
t.Run(tc.name, func(t *testing.T) {
Expand Down
Loading