Skip to content

✨ Refactor rukpak convert (now with testing!) #1893

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 1 commit into from
Apr 28, 2025
Merged

✨ Refactor rukpak convert (now with testing!) #1893

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
290 changes: 21 additions & 269 deletions internal/operator-controller/rukpak/convert/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,25 @@ import (
"fmt"
"io/fs"
"path/filepath"
"strings"

"helm.sh/helm/v3/pkg/chart"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/cli-runtime/pkg/resource"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/yaml"

"github.com/operator-framework/api/pkg/operators/v1alpha1"
"github.com/operator-framework/operator-registry/alpha/property"
registrybundle "github.com/operator-framework/operator-registry/pkg/lib/bundle"

registry "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/operator-registry"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/generators"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render/validators"
)

type RegistryV1 struct {
PackageName string
CSV v1alpha1.ClusterServiceVersion
CRDs []apiextensionsv1.CustomResourceDefinition
Others []unstructured.Unstructured
}

type Plain struct {
Objects []client.Object
}
Expand Down Expand Up @@ -70,16 +58,16 @@ func RegistryV1ToHelmChart(rv1 fs.FS, installNamespace string, watchNamespace st
return chrt, nil
}

// ParseFS converts the rv1 filesystem into a RegistryV1.
// ParseFS converts the rv1 filesystem into a render.RegistryV1.
// ParseFS expects the filesystem to conform to the registry+v1 format:
// metadata/annotations.yaml
// manifests/
// - csv.yaml
// - ...
//
// manifests directory does not contain subdirectories
func ParseFS(rv1 fs.FS) (RegistryV1, error) {
reg := RegistryV1{}
func ParseFS(rv1 fs.FS) (render.RegistryV1, error) {
reg := render.RegistryV1{}
annotationsFileData, err := fs.ReadFile(rv1, filepath.Join("metadata", "annotations.yaml"))
if err != nil {
return reg, err
Expand Down Expand Up @@ -224,22 +212,23 @@ 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 saNameOrDefault(saName string) string {
if saName == "" {
return "default"
}
return saName
var PlainConverter = Converter{
BundleRenderer: render.BundleRenderer{
BundleValidator: validators.RegistryV1BundleValidator,
ResourceGenerators: []render.ResourceGenerator{
generators.BundleCSVRBACResourceGenerator.ResourceGenerator(),
generators.BundleCRDGenerator,
generators.BundleAdditionalResourcesGenerator,
generators.BundleCSVDeploymentGenerator,
},
},
}

type Converter struct {
BundleValidator BundleValidator
render.BundleRenderer
}

Comment on lines +215 to 230
Copy link
Contributor

@anik120 anik120 Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This feels a bit weird to me. I get the feeling that these changes are incorporating something new, to something existing. But do we have to keep the existing layout?

What are your thoughts on the following:

var PlainConverter = render.BundleRenderer 

func (r BundleRenderer) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
.
.
}

PlainConverter.Convert(...)

And then take that one step further:

var RegistryV1ToPlainBundleRenderer = render.BundleRenderer 

RegistryV1ToPlainBundleRenderer.Convert(...)

func (r BundleRenderer) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
.
.
}

ie get rid of Converter totally, with the Convert function defined in the render package as a BundleRenderer component.

Isn't the PR essentially replacing Converter with Renderer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually I'd like to do away with the convert package all together in favor of the renderer. I just didn't want to also carry over all the logic checking the install and target namespaces, and validating install modes, and conversion support, etc. to the renderer (yet). In hindsight, I wish I had hidden everything inside the Convert() function for now to also reduce touch points with other parts of the code (leaving that for later).

I don't quite get the code suggesting. Seems its to rename PlainConverter to RegistryV1ToPlainBundleRenderer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually I'd like to do away with the convert package all together in favor of the renderer.

I was talking about this - getting rid of the Converter in favor of using render.BundleRenderer directly, instead of wrapping renderer.BundleRender inside a Converter and then using Converter. That'd mean

package render 

type BundleRenderer struct{
.
.
}

func (r BundleRenderer) Convert (rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error)}
.
.
}

would have to be defined.

But that would also mean

carry over all the logic checking the install and target namespaces, and validating install modes, and conversion support, etc. to the renderer (yet)

has to be done. It didn't look like this'd be too much of a heavy lift to me, but maybe I'm missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not about the effort, it's just about the scope of the change in the PR - and I'd also like to consider what to do with the Helm converter as part of this, and that will definitely influence everything up from that. I'd also like to think over whether that logic should live in the renderer itself or outside of it, or just in the Plain renderer, etc. It might have made things tidier, but I don't think it's necessary for this PR (though I do think its necessary).

func (c Converter) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
if err := c.BundleValidator.Validate(&rv1); err != nil {
return nil, err
}

func (c Converter) Convert(rv1 render.RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
if installNamespace == "" {
installNamespace = rv1.CSV.Annotations["operatorframework.io/suggested-namespace"]
}
Expand Down Expand Up @@ -272,246 +261,9 @@ func (c Converter) Convert(rv1 RegistryV1, installNamespace string, targetNamesp
return nil, fmt.Errorf("webhookDefinitions are not supported")
}

deployments := []appsv1.Deployment{}
serviceAccounts := map[string]corev1.ServiceAccount{}
for _, depSpec := range rv1.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
annotations := util.MergeMaps(rv1.CSV.Annotations, depSpec.Spec.Template.Annotations)
annotations["olm.targetNamespaces"] = strings.Join(targetNamespaces, ",")
depSpec.Spec.Template.Annotations = annotations

// Hardcode the deployment with RevisionHistoryLimit=1 to replicate OLMv0 behavior
// https://github.com/operator-framework/operator-lifecycle-manager/blob/dfd0b2bea85038d3c0d65348bc812d297f16b8d2/pkg/controller/install/deployment.go#L181
depSpec.Spec.RevisionHistoryLimit = ptr.To(int32(1))

deployments = append(deployments, appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
Kind: "Deployment",
APIVersion: appsv1.SchemeGroupVersion.String(),
},

ObjectMeta: metav1.ObjectMeta{
Namespace: installNamespace,
Name: depSpec.Name,
Labels: depSpec.Label,
},
Spec: depSpec.Spec,
})
saName := saNameOrDefault(depSpec.Spec.Template.Spec.ServiceAccountName)
serviceAccounts[saName] = newServiceAccount(installNamespace, saName)
}

// NOTES:
// 1. There's an extra Role for OperatorConditions: get/update/patch; resourceName=csv.name
// - This is managed by the OperatorConditions controller here: https://github.com/operator-framework/operator-lifecycle-manager/blob/9ced412f3e263b8827680dc0ad3477327cd9a508/pkg/controller/operators/operatorcondition_controller.go#L106-L109
// 2. There's an extra RoleBinding for the above mentioned role.
// - Every SA mentioned in the OperatorCondition.spec.serviceAccounts is a subject for this role binding: https://github.com/operator-framework/operator-lifecycle-manager/blob/9ced412f3e263b8827680dc0ad3477327cd9a508/pkg/controller/operators/operatorcondition_controller.go#L171-L177
// 3. strategySpec.permissions are _also_ given a clusterrole/clusterrole binding.
// - (for AllNamespaces mode only?)
// - (where does the extra namespaces get/list/watch rule come from?)

roles := []rbacv1.Role{}
roleBindings := []rbacv1.RoleBinding{}
clusterRoles := []rbacv1.ClusterRole{}
clusterRoleBindings := []rbacv1.ClusterRoleBinding{}

permissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.Permissions
clusterPermissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.ClusterPermissions
allPermissions := append(permissions, clusterPermissions...)

// Create all the service accounts
for _, permission := range allPermissions {
saName := saNameOrDefault(permission.ServiceAccountName)
if _, ok := serviceAccounts[saName]; !ok {
serviceAccounts[saName] = newServiceAccount(installNamespace, saName)
}
}

// If we're in AllNamespaces mode, promote the permissions to clusterPermissions
if len(targetNamespaces) == 1 && targetNamespaces[0] == "" {
for _, p := range permissions {
p.Rules = append(p.Rules, rbacv1.PolicyRule{
Verbs: []string{"get", "list", "watch"},
APIGroups: []string{corev1.GroupName},
Resources: []string{"namespaces"},
})
clusterPermissions = append(clusterPermissions, p)
}
permissions = nil
}

for _, ns := range targetNamespaces {
for _, permission := range permissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", rv1.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))
}
}

for _, permission := range clusterPermissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
if err != nil {
return nil, err
}
clusterRoles = append(clusterRoles, newClusterRole(name, permission.Rules))
clusterRoleBindings = append(clusterRoleBindings, newClusterRoleBinding(name, name, installNamespace, saName))
}

objs := []client.Object{}
for _, obj := range serviceAccounts {
obj := obj
if obj.GetName() != "default" {
objs = append(objs, &obj)
}
}
for _, obj := range roles {
obj := obj
objs = append(objs, &obj)
}
for _, obj := range roleBindings {
obj := obj
objs = append(objs, &obj)
}
for _, obj := range clusterRoles {
obj := obj
objs = append(objs, &obj)
}
for _, obj := range clusterRoleBindings {
obj := obj
objs = append(objs, &obj)
}
for _, obj := range rv1.CRDs {
objs = append(objs, &obj)
}
for _, obj := range rv1.Others {
obj := obj
supported, namespaced := registrybundle.IsSupported(obj.GetKind())
if !supported {
return nil, fmt.Errorf("bundle contains unsupported resource: Name: %v, Kind: %v", obj.GetName(), obj.GetKind())
}
if namespaced {
obj.SetNamespace(installNamespace)
}
objs = append(objs, &obj)
}
for _, obj := range deployments {
obj := obj
objs = append(objs, &obj)
}
return &Plain{Objects: objs}, nil
}

var PlainConverter = Converter{
BundleValidator: RegistryV1BundleValidator,
}

const maxNameLength = 63

func generateName(base string, o interface{}) (string, error) {
hashStr, err := util.DeepHashObject(o)
objs, err := c.BundleRenderer.Render(rv1, installNamespace, targetNamespaces)
if err != nil {
return "", err
}
if len(base)+len(hashStr) > maxNameLength {
base = base[:maxNameLength-len(hashStr)-1]
}

return fmt.Sprintf("%s-%s", base, hashStr), nil
}

func newServiceAccount(namespace, name string) corev1.ServiceAccount {
return corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
APIVersion: corev1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
},
}
}

func newRole(namespace, name string, rules []rbacv1.PolicyRule) rbacv1.Role {
return rbacv1.Role{
TypeMeta: metav1.TypeMeta{
Kind: "Role",
APIVersion: rbacv1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
},
Rules: rules,
}
}

func newClusterRole(name string, rules []rbacv1.PolicyRule) rbacv1.ClusterRole {
return rbacv1.ClusterRole{
TypeMeta: metav1.TypeMeta{
Kind: "ClusterRole",
APIVersion: rbacv1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Rules: rules,
}
}

func newRoleBinding(namespace, name, roleName, saNamespace string, saNames ...string) rbacv1.RoleBinding {
subjects := make([]rbacv1.Subject, 0, len(saNames))
for _, saName := range saNames {
subjects = append(subjects, rbacv1.Subject{
Kind: "ServiceAccount",
Namespace: saNamespace,
Name: saName,
})
}
return rbacv1.RoleBinding{
TypeMeta: metav1.TypeMeta{
Kind: "RoleBinding",
APIVersion: rbacv1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
},
Subjects: subjects,
RoleRef: rbacv1.RoleRef{
APIGroup: rbacv1.GroupName,
Kind: "Role",
Name: roleName,
},
}
}

func newClusterRoleBinding(name, roleName, saNamespace string, saNames ...string) rbacv1.ClusterRoleBinding {
subjects := make([]rbacv1.Subject, 0, len(saNames))
for _, saName := range saNames {
subjects = append(subjects, rbacv1.Subject{
Kind: "ServiceAccount",
Namespace: saNamespace,
Name: saName,
})
}
return rbacv1.ClusterRoleBinding{
TypeMeta: metav1.TypeMeta{
Kind: "ClusterRoleBinding",
APIVersion: rbacv1.SchemeGroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Subjects: subjects,
RoleRef: rbacv1.RoleRef{
APIGroup: rbacv1.GroupName,
Kind: "ClusterRole",
Name: roleName,
},
return nil, err
}
return &Plain{Objects: objs}, nil
}
Loading
Loading