Skip to content
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.

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

🐛 Fix reconciliation blocked on improper permissions for establishing watches on managed content #1119

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions api/v1alpha1/clusterextension_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,7 @@ const (
// TODO(user): add more Types, here and into init()
TypeInstalled = "Installed"
TypeResolved = "Resolved"
TypeHealthy = "Healthy"

// TypeDeprecated is a rollup condition that is present when
// any of the deprecated conditions are present.
Expand All @@ -438,6 +439,8 @@ const (

ReasonErrorGettingReleaseState = "ErrorGettingReleaseState"

ReasonUnverifiable = "Unverifiable"

CRDUpgradeSafetyPolicyEnabled CRDUpgradeSafetyPolicy = "Enabled"
CRDUpgradeSafetyPolicyDisabled CRDUpgradeSafetyPolicy = "Disabled"
)
Expand All @@ -452,6 +455,7 @@ func init() {
TypeChannelDeprecated,
TypeBundleDeprecated,
TypeUnpacked,
TypeHealthy,
)
// TODO(user): add Reasons from above
conditionsets.ConditionReasons = append(conditionsets.ConditionReasons,
Expand All @@ -465,6 +469,7 @@ func init() {
ReasonUnpackSuccess,
ReasonUnpackFailed,
ReasonErrorGettingReleaseState,
ReasonUnverifiable,
)
}

Expand Down
13 changes: 12 additions & 1 deletion cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,14 +249,25 @@ func main() {
Preflights: preflights,
}

cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
err := cm.Delete(ext)
return crfinalizer.Result{}, err
}))
if err != nil {
setupLog.Error(err, "unable to register content manager cleanup finalizer")
os.Exit(1)
}

if err = (&controllers.ClusterExtensionReconciler{
Client: cl,
Resolver: resolver,
Unpacker: unpacker,
Applier: applier,
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
Finalizers: clusterExtensionFinalizers,
Watcher: contentmanager.New(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()),
Manager: cm,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand Down
4 changes: 2 additions & 2 deletions config/samples/olm_v1alpha1_clusterextension.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ rules:
# Manage ArgoCD CRDs
- apiGroups: [apiextensions.k8s.io]
resources: [customresourcedefinitions]
verbs: [create]
verbs: [create, list, watch]
Copy link
Member

Choose a reason for hiding this comment

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

Fixes #1195

- apiGroups: [apiextensions.k8s.io]
resources: [customresourcedefinitions]
verbs: [get, list, watch, update, patch, delete]
verbs: [get, update, patch, delete]
resourceNames:
- appprojects.argoproj.io
- argocds.argoproj.io
Expand Down
59 changes: 48 additions & 11 deletions internal/applier/helm.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,23 @@
package applier

import (
"bytes"
"context"
"errors"
"fmt"
"io"
"io/fs"
"strings"

"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/postrender"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
"sigs.k8s.io/controller-runtime/pkg/client"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
Expand Down Expand Up @@ -51,7 +56,7 @@ type Helm struct {
Preflights []Preflight
}

func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.ClusterExtension, labels map[string]string) ([]client.Object, string, error) {
func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Install.Namespace, []string{corev1.NamespaceAll})
if err != nil {
return nil, "", err
Expand All @@ -63,7 +68,11 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.Clust
return nil, "", err
}

rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, labels)
post := &postrenderer{
labels: objectLabels,
}

rel, desiredRel, state, err := h.getReleaseState(ac, ext, chrt, values, post)
if err != nil {
return nil, "", err
}
Expand Down Expand Up @@ -94,18 +103,18 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.Clust
case StateNeedsInstall:
rel, err = ac.Install(ext.GetName(), ext.Spec.Install.Namespace, chrt, values, func(install *action.Install) error {
install.CreateNamespace = false
install.Labels = labels
install.Labels = storageLabels
return nil
})
}, helmclient.AppendInstallPostRenderer(post))
if err != nil {
return nil, state, err
}
case StateNeedsUpgrade:
rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Install.Namespace, chrt, values, func(upgrade *action.Upgrade) error {
upgrade.MaxHistory = maxHelmReleaseHistory
upgrade.Labels = labels
upgrade.Labels = storageLabels
return nil
})
}, helmclient.AppendUpgradePostRenderer(post))
if err != nil {
return nil, state, err
}
Expand All @@ -125,7 +134,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.Clust
return relObjects, state, nil
}

func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, labels map[string]string) (*release.Release, *release.Release, string, error) {
func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) {
currentRelease, err := cl.Get(ext.GetName())
if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
return nil, nil, StateError, err
Expand All @@ -138,9 +147,8 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.Cl
desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Install.Namespace, chrt, values, func(i *action.Install) error {
i.DryRun = true
i.DryRunOption = "server"
i.Labels = labels
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question for reviewers: Should the helm release secrets also have these labels? I assumed they weren't necessary with the release name being deterministic based on the ClusterExtension name

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not necessary; would they otherwise be getting merged with labels by the postrenderer? If so I can see how they might get into a funny looking state.

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 don't think so. I think these labels are explicitly on the release secrets created by the Helm client.

My understanding is that the postrenderer would add the labels to the manifests only within the Helm Release

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 would take ^ with a grain of salt though. I'm assuming the release secret is not included in the set of release manifests, but I don't actually know for sure.

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 @gallettilance is planning to add release secret labels to populate CSV annotations there.

Seems like we should be able to trace what labels we currently set on the release secret and make sure we understand their purpose to decide whether we can stop populating them . I think we put these labels there, and they are used in a variety of ways (setup performant informers, get the installed bundles)

return nil
})
}, helmclient.AppendInstallPostRenderer(post))
if err != nil {
return nil, nil, StateError, err
}
Expand All @@ -150,9 +158,8 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.Cl
upgrade.MaxHistory = maxHelmReleaseHistory
upgrade.DryRun = true
upgrade.DryRunOption = "server"
upgrade.Labels = labels
return nil
})
}, helmclient.AppendUpgradePostRenderer(post))
if err != nil {
return currentRelease, nil, StateError, err
}
Expand All @@ -164,3 +171,33 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1alpha1.Cl
}
return currentRelease, desiredRelease, relState, nil
}

type postrenderer struct {
labels map[string]string
cascade postrender.PostRenderer
}

func (p *postrenderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, error) {
var buf bytes.Buffer
dec := apimachyaml.NewYAMLOrJSONDecoder(renderedManifests, 1024)
for {
obj := unstructured.Unstructured{}
err := dec.Decode(&obj)
if errors.Is(err, io.EOF) {
break
}
if err != nil {
return nil, err
}
obj.SetLabels(util.MergeMaps(obj.GetLabels(), p.labels))
b, err := obj.MarshalJSON()
if err != nil {
return nil, err
}
buf.Write(b)
}
if p.cascade != nil {
return p.cascade.Run(&buf)
}
return &buf, nil
}
everettraven marked this conversation as resolved.
Show resolved Hide resolved
Loading