Skip to content
This repository has been archived by the owner on Aug 12, 2024. It is now read-only.

[Add] Support for other Install strategies #772

Closed
wants to merge 1 commit into from
Closed

[Add] Support for other Install strategies #772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions api/v1alpha1/bundle_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ type BundleSpec struct {
ProvisionerClassName string `json:"provisionerClassName"`
// Source defines the configuration for the underlying Bundle content.
Source BundleSource `json:"source"`
// +kubebuilder:validation:Optional
// watchNamespaces indicates which namespaces the operator should watch.
WatchNamespaces []string `json:"watchNamespaces,omitempty"`
Comment on lines +59 to +61
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I had imagined that we'd put this on the BundleDeployment, not the Bundle. But the wrinkle with needing to generate extra RBAC during the conversion means this is essential information in that step.

One possible solution here would be to make a registry BundleDeployment provisioner and move the conversion logic there. If we did that, we could probably make the Bundle provisioner super generic and reusable for registry/plain/helm/etc. Basically it would just be a "give me the filesystem with no format-specific validation" provisioner. And then the BD would be the place for registry/plain/helm to have separate provisioners and do whatever specific validation was necessary.

From an API design standpoint, I argue that configuration of the templating of the bundle belongs in the BundleDeployment.

All of this highlights the weird dichotomy between Bundle and BundleDeployment, and I think it's another good reason to move to a single API for "source/template/apply" like the Carvel App API. I think moving toward a super-generic Bundle provisioner gets us more aligned with that concept anyway, so perhaps that would be a good step in that direction?

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I missed this was on Bundle and not BundleDeployment. +100 to getting rid of Bundle. @varshaprasad96 if you're around this week and want help, please let me know.

Copy link
Member Author

@varshaprasad96 varshaprasad96 Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@joelanford @ncdc fair. The reason for having this on template, was to be able to pass this to bundle provisioner which handles the conversion. If we move that logic to Bundle, there doesn't seem to be much value in having a separate API.

I think it's another good reason to move to a single API for "source/template/apply" like the Carvel App API

Removing Bundle would change the BundleDeployment API, we would need an alpha2? Nevertheless, I'll get a draft PR ready, and we can discuss the specifics of the API there.

Copy link
Member

Choose a reason for hiding this comment

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

Yes to alpha2

}

type BundleSource struct {
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 14 additions & 14 deletions internal/convert/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type Plain struct {
Objects []client.Object
}

func RegistryV1ToPlain(rv1 fs.FS) (fs.FS, error) {
func RegistryV1ToPlain(rv1 fs.FS, watchNamespaces []string) (fs.FS, error) {
reg := RegistryV1{}
fileData, err := fs.ReadFile(rv1, filepath.Join("metadata", "annotations.yaml"))
if err != nil {
Expand Down Expand Up @@ -102,7 +102,7 @@ func RegistryV1ToPlain(rv1 fs.FS) (fs.FS, error) {
}
}

plain, err := Simple(reg)
plain, err := Simple(reg, watchNamespaces)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -165,8 +165,8 @@ func validateTargetNamespaces(supportedInstallModes sets.Set[string], installNam
return fmt.Errorf("supported install modes %v do not support target namespaces %v", sets.List[string](supportedInstallModes), targetNamespaces)
}

func Simple(in RegistryV1) (*Plain, error) {
return Convert(in, "", nil)
func Simple(in RegistryV1, watchNamespaces []string) (*Plain, error) {
return Convert(in, "", watchNamespaces)
}
Comment on lines +168 to 170
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd just drop the Simple function at this point.


func saNameOrDefault(saName string) string {
Expand All @@ -189,9 +189,6 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
supportedInstallModes.Insert(string(im.Type))
}
}
if !supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) {
return nil, fmt.Errorf("AllNamespace install mode must be enabled")
}
if targetNamespaces == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind changing this to check for len=0 instead please?

if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) {
targetNamespaces = []string{""}
Expand Down Expand Up @@ -274,15 +271,18 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
permissions = nil
}

for _, permission := range permissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
if err != nil {
return nil, err
for _, ns := range targetNamespaces {
ncdc marked this conversation as resolved.
Show resolved Hide resolved
for _, permission := range permissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
if err != nil {
return nil, err
}
roles = append(roles, newRole(ns, name, permission.Rules))
roleBindings = append(roleBindings, newRoleBinding(ns, name, name, installNamespace, saName))
}
roles = append(roles, newRole(installNamespace, name, permission.Rules))
roleBindings = append(roleBindings, newRoleBinding(installNamespace, name, name, installNamespace, saName))
}

for _, permission := range clusterPermissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
Expand Down
4 changes: 2 additions & 2 deletions internal/provisioner/registry/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ const (
ProvisionerID = "core-rukpak-io-registry"
)

func HandleBundle(_ context.Context, fsys fs.FS, _ *rukpakv1alpha1.Bundle) (fs.FS, error) {
plainFS, err := convert.RegistryV1ToPlain(fsys)
func HandleBundle(_ context.Context, fsys fs.FS, bundle *rukpakv1alpha1.Bundle) (fs.FS, error) {
plainFS, err := convert.RegistryV1ToPlain(fsys, bundle.Spec.WatchNamespaces)
if err != nil {
return nil, fmt.Errorf("convert registry+v1 bundle to plain+v0 bundle: %v", err)
}
Expand Down
6 changes: 6 additions & 0 deletions manifests/base/apis/crds/core.rukpak.io_bundledeployments.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,12 @@ spec:
required:
- type
type: object
watchNamespaces:
description: watchNamespaces indicates which namespaces the
operator should watch.
items:
type: string
type: array
required:
- provisionerClassName
- source
Expand Down
6 changes: 6 additions & 0 deletions manifests/base/apis/crds/core.rukpak.io_bundles.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ spec:
required:
- type
type: object
watchNamespaces:
description: watchNamespaces indicates which namespaces the operator
should watch.
items:
type: string
type: array
required:
- provisionerClassName
- source
Expand Down
Loading