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

NE-1907: Manage OSSM operator subscription manually to ensure a compatible version is installed #1112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions manifests/00-cluster-role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ rules:
- operators.coreos.com
resources:
- subscriptions
- installplans
verbs:
- '*'

Expand Down
20 changes: 20 additions & 0 deletions pkg/operator/controller/gatewayclass/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"

logf "github.com/openshift/cluster-ingress-operator/pkg/log"
operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"

"k8s.io/client-go/tools/record"

Expand Down Expand Up @@ -61,6 +62,22 @@ func NewUnmanaged(mgr manager.Manager, config Config) (controller.Controller, er
if err := c.Watch(source.Kind[client.Object](operatorCache, &gatewayapiv1.GatewayClass{}, &handler.EnqueueRequestForObject{}, isOurGatewayClass, notIstioGatewayClass)); err != nil {
return nil, err
}
isOurInstallPlan := predicate.NewPredicateFuncs(func(o client.Object) bool {
installPlan := o.(*operatorsv1alpha1.InstallPlan)
for _, csv := range installPlan.Spec.ClusterServiceVersionNames {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you check to make sure len(ClusterServiceVersionNames) > 0 first?

if csv == serviceMeshOperatorDesiredVersion {
return true
}
}
return false
})
isInstallPlanApproved := predicate.NewPredicateFuncs(func(o client.Object) bool {
installPlan := o.(*operatorsv1alpha1.InstallPlan)
return installPlan.Spec.Approved
})
if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.InstallPlan{}, &handler.EnqueueRequestForObject{}, isOurInstallPlan, predicate.Not(isInstallPlanApproved))); err != nil {
return nil, err
}
return c, nil
}

Expand Down Expand Up @@ -97,6 +114,9 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
if _, _, err := r.ensureServiceMeshOperatorSubscription(ctx); err != nil {
errs = append(errs, err)
}
if _, _, err := r.ensureServiceMeshOperatorInstallPlan(ctx); err != nil {
errs = append(errs, err)
}
if _, _, err := r.ensureServiceMeshControlPlane(ctx, &gatewayclass); err != nil {
errs = append(errs, err)
}
Expand Down
46 changes: 45 additions & 1 deletion pkg/operator/controller/gatewayclass/subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"sigs.k8s.io/controller-runtime/pkg/client"

operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"

Expand All @@ -16,6 +17,11 @@ import (
"k8s.io/apimachinery/pkg/types"
)

var (
serviceMeshOperatorDesiredVersion = "servicemeshoperator.v2.6.2"
Copy link
Contributor

@candita candita Dec 18, 2024

Choose a reason for hiding this comment

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

I'm not sure this is going to work. If we change the version in a future release and need to backport a different change in the same file, won't this get overwritten? I think normally version information is recorded in a manifest yaml. Can you find a way to do this without hardcoding the version here?

I'll check with @Miciah because I don't know the answer.

serviceMeshOperatorNamespace = "openshift-operators"
)

// ensureServiceMeshOperatorSubscription attempts to ensure that a subscription
// for servicemeshoperator is present and returns a Boolean indicating whether
// it exists, the subscription if it exists, and an error value.
Expand Down Expand Up @@ -56,10 +62,11 @@ func desiredSubscription(name types.NamespacedName) (*operatorsv1alpha1.Subscrip
},
Spec: &operatorsv1alpha1.SubscriptionSpec{
Channel: "stable",
InstallPlanApproval: operatorsv1alpha1.ApprovalAutomatic,
InstallPlanApproval: operatorsv1alpha1.ApprovalManual,
Package: "servicemeshoperator",
CatalogSource: "redhat-operators",
CatalogSourceNamespace: "openshift-marketplace",
StartingCSV: serviceMeshOperatorDesiredVersion,
},
}
return &subscription, nil
Expand Down Expand Up @@ -115,3 +122,40 @@ func subscriptionChanged(current, expected *operatorsv1alpha1.Subscription) (boo

return true, updated
}

// ensureServiceMeshOperatorInstallPlan attempts to ensure that the install plan for the appropriate OSSM operator
// version is approved.
func (r *reconciler) ensureServiceMeshOperatorInstallPlan(ctx context.Context) (bool, *operatorsv1alpha1.InstallPlan, error) {
currentInstallPlan, err := r.getCurrentInstallPlan(ctx)
if err != nil {
return false, nil, err
} else if currentInstallPlan == nil {
return false, nil, nil
}
if !currentInstallPlan.Spec.Approved {
Copy link
Contributor

Choose a reason for hiding this comment

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

The idiomatic way to write an "ensure" function is to get the current object, get the desired object, compare them, and use the "have" and "want" values to determine whether to create or update. Can you update this to use the idiomatic form?

currentInstallPlan.Spec.Approved = true
if err := r.client.Update(ctx, currentInstallPlan); err != nil {
return false, nil, fmt.Errorf("Failed to update %s/%s: %w", currentInstallPlan.Namespace, currentInstallPlan.Name, err)
}
return true, currentInstallPlan, nil
}
return false, currentInstallPlan, nil
}

func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) {
Copy link
Contributor

@candita candita Dec 18, 2024

Choose a reason for hiding this comment

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

nit: use the function name we normally use.

Suggested change
func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) {
func (r *reconciler) currentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) {

InstallPlans := &operatorsv1alpha1.InstallPlanList{}
if err := r.client.List(ctx, InstallPlans, client.InNamespace(serviceMeshOperatorNamespace)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

So the installPlan doesn't have a unique name?

return nil, err
}
if InstallPlans == nil || len(InstallPlans.Items) == 0 {
return nil, nil
}
for _, InstallPlan := range InstallPlans.Items {
for _, CSVName := range InstallPlan.Spec.ClusterServiceVersionNames {
if CSVName == serviceMeshOperatorDesiredVersion {
return &InstallPlan, nil
}
}
}
return nil, fmt.Errorf("No InstallPlan with cluster service version %s found", serviceMeshOperatorDesiredVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("No InstallPlan with cluster service version %s found", serviceMeshOperatorDesiredVersion)
return nil, fmt.Errorf("no InstallPlan with cluster service version %s found in namespace %s", serviceMeshOperatorDesiredVersion, serviceMeshOperatorNamespace)

}