Skip to content

Commit

Permalink
refactor(feature): decouples Feature from client (#1234)
Browse files Browse the repository at this point in the history 
Including the client in the function signature makes its usage
explicit and more idiomatic. This approach clarifies that the
client is intended for use within the function. Decoupling the
client from Feature struct  enhances code readability.

It's a follow-up to PR
[#1228](#1228 (comment)).
  • Loading branch information
@bartoszmajsak
bartoszmajsak authored Sep 16, 2024
1 parent df1add0 commit fde5d69
Show file tree
Hide file tree
Showing 22 changed files with 164 additions and 232 deletions.
8 changes: 4 additions & 4 deletions components/kserve/kserve_config_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,19 +148,19 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log
return dependOpsErrors
}

serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))

if err := serverlessFeatures.Apply(ctx); err != nil {
if err := serverlessFeatures.Apply(ctx, cli); err != nil {
return err
}
}
return nil
}

func (k *Kserve) removeServerlessFeatures(ctx context.Context, cli client.Client, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error {
serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))

return serverlessFeatures.Delete(ctx)
return serverlessFeatures.Delete(ctx, cli)
}

func checkDependentOperators(ctx context.Context, cli client.Client) *multierror.Error {
Expand Down
8 changes: 4 additions & 4 deletions components/kserve/servicemesh_setup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import (
func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error {
if dscispec.ServiceMesh != nil {
if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed {
serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
return serviceMeshInitializer.Apply(ctx)
serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
return serviceMeshInitializer.Apply(ctx, cli)
}
if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed {
return nil
Expand All @@ -32,8 +32,8 @@ func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, ow
}

func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error {
serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
return serviceMeshInitializer.Delete(ctx)
serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
return serviceMeshInitializer.Delete(ctx, cli)
}

func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) feature.FeaturesProvider {
Expand Down
15 changes: 8 additions & 7 deletions controllers/dscinitialization/servicemesh_setup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
operatorv1 "github.com/openshift/api/operator/v1"
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
Expand Down Expand Up @@ -39,7 +40,7 @@ func (r *DSCInitializationReconciler) configureServiceMesh(ctx context.Context,
capabilities = append(capabilities, authzCapability)

for _, capability := range capabilities {
capabilityErr := capability.Apply(ctx)
capabilityErr := capability.Apply(ctx, r.Client)
if capabilityErr != nil {
r.Log.Error(capabilityErr, "failed applying service mesh resources")
r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed applying service mesh resources")
Expand Down Expand Up @@ -77,7 +78,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins
capabilities = append(capabilities, authzCapability)

for _, capability := range capabilities {
capabilityErr := capability.Delete(ctx)
capabilityErr := capability.Delete(ctx, r.Client)
if capabilityErr != nil {
r.Log.Error(capabilityErr, "failed deleting service mesh resources")
r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed deleting service mesh resources")
Expand All @@ -91,7 +92,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins

func (r *DSCInitializationReconciler) serviceMeshCapability(instance *dsciv1.DSCInitialization, initialCondition *conditionsv1.Condition) *feature.HandlerWithReporter[*dsciv1.DSCInitialization] { //nolint:lll // Reason: generics are long
return feature.NewHandlerWithReporter(
feature.ClusterFeaturesHandler(r.Client, instance, r.serviceMeshCapabilityFeatures(instance)),
feature.ClusterFeaturesHandler(instance, r.serviceMeshCapabilityFeatures(instance)),
createCapabilityReporter(r.Client, instance, initialCondition),
)
}
Expand Down Expand Up @@ -119,7 +120,7 @@ func (r *DSCInitializationReconciler) authorizationCapability(ctx context.Contex
}

return feature.NewHandlerWithReporter(
feature.ClusterFeaturesHandler(r.Client, instance, r.authorizationFeatures(instance)),
feature.ClusterFeaturesHandler(instance, r.authorizationFeatures(instance)),
createCapabilityReporter(r.Client, instance, condition),
), nil
}
Expand All @@ -128,7 +129,7 @@ func (r *DSCInitializationReconciler) serviceMeshCapabilityFeatures(instance *ds
return func(registry feature.FeaturesRegistry) error {
controlPlaneSpec := instance.Spec.ServiceMesh.ControlPlane

meshMetricsCollection := func(_ context.Context, _ *feature.Feature) (bool, error) {
meshMetricsCollection := func(_ context.Context, _ client.Client, _ *feature.Feature) (bool, error) {
return controlPlaneSpec.MetricsCollection == "Istio", nil
}

Expand Down Expand Up @@ -221,13 +222,13 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC
Include(path.Join(Templates.AuthorinoDir, "deployment.injection.patch.tmpl.yaml")),
).
PreConditions(
func(ctx context.Context, f *feature.Feature) error {
func(ctx context.Context, cli client.Client, f *feature.Feature) error {
namespace, err := servicemesh.FeatureData.Authorization.Namespace.Extract(f)
if err != nil {
return fmt.Errorf("failed trying to resolve authorization provider namespace for feature '%s': %w", f.Name, err)
}

return feature.WaitForPodsToBeReady(namespace)(ctx, f)
return feature.WaitForPodsToBeReady(namespace)(ctx, cli, f)
},
).
WithData(servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction()).
Expand Down
61 changes: 1 addition & 60 deletions pkg/feature/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,8 @@ import (
"context"
"fmt"

"github.com/hashicorp/go-multierror"
"github.com/pkg/errors"
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/config"
"sigs.k8s.io/controller-runtime/pkg/log"

featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1"
Expand All @@ -27,8 +21,6 @@ type featureBuilder struct {
owner metav1.Object
targetNs string

client client.Client

builders []partialBuilder
}

Expand Down Expand Up @@ -193,16 +185,10 @@ func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder {
return fb
}

// UsingClient allows to provide a custom client to the feature. If not called, a default client will be created.
func (fb *featureBuilder) UsingClient(cli client.Client) *featureBuilder {
fb.client = cli
return fb
}

// Create creates a new Feature instance and add it to corresponding FeaturesHandler.
// The actual feature creation in the cluster is not performed here.
func (fb *featureBuilder) Create() (*Feature, error) {
alwaysEnabled := func(_ context.Context, _ *Feature) (bool, error) {
alwaysEnabled := func(_ context.Context, _ client.Client, _ *Feature) (bool, error) {
return true, nil
}

Expand All @@ -215,15 +201,6 @@ func (fb *featureBuilder) Create() (*Feature, error) {
owner: fb.owner,
}

// UsingClient has not been called, so we need to create a new client
if fb.client == nil {
if err := createDefaultClient()(f); err != nil {
return nil, err
}
} else {
f.Client = fb.client
}

for i := range fb.builders {
if err := fb.builders[i](f); err != nil {
return nil, err
Expand All @@ -232,39 +209,3 @@ func (fb *featureBuilder) Create() (*Feature, error) {

return f, nil
}

func createDefaultClient() partialBuilder {
return func(f *Feature) error {
var err error

restCfg, err := config.GetConfig()
if errors.Is(err, rest.ErrNotInCluster) {
// rollback to local kubeconfig - this can be helpful when running the process locally i.e. while debugging
kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
&clientcmd.ClientConfigLoadingRules{ExplicitPath: clientcmd.RecommendedHomeFile},
&clientcmd.ConfigOverrides{},
)

restCfg, err = kubeconfig.ClientConfig()
if err != nil {
return err
}
} else if err != nil {
return err
}

f.Client, err = client.New(restCfg, client.Options{})
if err != nil {
return errors.WithStack(err)
}

var multiErr *multierror.Error
s := f.Client.Scheme()
multiErr = multierror.Append(multiErr,
featurev1.AddToScheme(s),
apiextv1.AddToScheme(s),
)

return multiErr.ErrorOrNil()
}
}
12 changes: 6 additions & 6 deletions pkg/feature/conditions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ func (e *MissingOperatorError) Error() string {
}

func EnsureOperatorIsInstalled(operatorName string) Action {
return func(ctx context.Context, f *Feature) error {
if found, err := cluster.SubscriptionExists(ctx, f.Client, operatorName); !found || err != nil {
return func(ctx context.Context, cli client.Client, f *Feature) error {
if found, err := cluster.SubscriptionExists(ctx, cli, operatorName); !found || err != nil {
return fmt.Errorf(
"failed to find the pre-requisite operator subscription %q, please ensure operator is installed. %w",
operatorName,
Expand All @@ -53,13 +53,13 @@ func EnsureOperatorIsInstalled(operatorName string) Action {
}

func WaitForPodsToBeReady(namespace string) Action {
return func(ctx context.Context, f *Feature) error {
return func(ctx context.Context, cli client.Client, f *Feature) error {
f.Log.Info("waiting for pods to become ready", "namespace", namespace, "duration (s)", duration.Seconds())

return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) {
var podList corev1.PodList

err := f.Client.List(ctx, &podList, client.InNamespace(namespace))
err := cli.List(ctx, &podList, client.InNamespace(namespace))
if err != nil {
return false, err
}
Expand Down Expand Up @@ -103,14 +103,14 @@ func WaitForPodsToBeReady(namespace string) Action {
}

func WaitForResourceToBeCreated(namespace string, gvk schema.GroupVersionKind) Action {
return func(ctx context.Context, f *Feature) error {
return func(ctx context.Context, cli client.Client, f *Feature) error {
f.Log.Info("waiting for resource to be created", "namespace", namespace, "resource", gvk)

return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) {
list := &unstructured.UnstructuredList{}
list.SetGroupVersionKind(gvk)

err := f.Client.List(ctx, list, client.InNamespace(namespace), client.Limit(1))
err := cli.List(ctx, list, client.InNamespace(namespace), client.Limit(1))
if err != nil {
f.Log.Error(err, "failed waiting for resource", "namespace", namespace, "resource", gvk)

Expand Down
37 changes: 18 additions & 19 deletions pkg/feature/feature.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ type Feature struct {
Enabled EnabledFunc
Managed bool

Client client.Client
Log logr.Logger
Log logr.Logger

tracker *featurev1.FeatureTracker
source *featurev1.Source
Expand All @@ -61,7 +60,7 @@ type Feature struct {

// Action is a func type which can be used for different purposes during Feature's lifecycle
// while having access to Feature struct.
type Action func(ctx context.Context, f *Feature) error
type Action func(ctx context.Context, cli client.Client, f *Feature) error

// CleanupFunc defines how to clean up resources associated with a feature.
// By default, all resources created by the feature are deleted when the feature is,
Expand All @@ -70,70 +69,70 @@ type Action func(ctx context.Context, f *Feature) error
type CleanupFunc func(ctx context.Context, cli client.Client) error

// EnabledFunc is a func type used to determine if a feature should be enabled.
type EnabledFunc func(ctx context.Context, feature *Feature) (bool, error)
type EnabledFunc func(ctx context.Context, cli client.Client, feature *Feature) (bool, error)

// Apply applies the feature to the cluster.
// It creates a FeatureTracker resource to establish ownership and reports the result of the operation as a condition.
func (f *Feature) Apply(ctx context.Context) error {
func (f *Feature) Apply(ctx context.Context, cli client.Client) error {
// If the feature is disabled, but the FeatureTracker exists in the cluster, ensure clean-up is triggered.
// This means that the feature was previously enabled, but now it is not anymore.
if enabled, err := f.Enabled(ctx, f); !enabled || err != nil {
if enabled, err := f.Enabled(ctx, cli, f); !enabled || err != nil {
if err != nil {
return err
}

return f.Cleanup(ctx)
return f.Cleanup(ctx, cli)
}

if trackerErr := createFeatureTracker(ctx, f); trackerErr != nil {
if trackerErr := createFeatureTracker(ctx, cli, f); trackerErr != nil {
return trackerErr
}

if _, updateErr := status.UpdateWithRetry(ctx, f.Client, f.tracker, func(saved *featurev1.FeatureTracker) {
if _, updateErr := status.UpdateWithRetry(ctx, cli, f.tracker, func(saved *featurev1.FeatureTracker) {
status.SetProgressingCondition(&saved.Status.Conditions, string(featurev1.ConditionReason.FeatureCreated), fmt.Sprintf("Applying feature [%s]", f.Name))
saved.Status.Phase = status.PhaseProgressing
}); updateErr != nil {
return updateErr
}

applyErr := f.applyFeature(ctx)
_, reportErr := createFeatureTrackerStatusReporter(f).ReportCondition(ctx, applyErr)
applyErr := f.applyFeature(ctx, cli)
_, reportErr := createFeatureTrackerStatusReporter(cli, f).ReportCondition(ctx, applyErr)

return multierror.Append(applyErr, reportErr).ErrorOrNil()
}

func (f *Feature) applyFeature(ctx context.Context) error {
func (f *Feature) applyFeature(ctx context.Context, cli client.Client) error {
var multiErr *multierror.Error

for _, dataProvider := range f.dataProviders {
multiErr = multierror.Append(multiErr, dataProvider(ctx, f))
multiErr = multierror.Append(multiErr, dataProvider(ctx, cli, f))
}
if errDataLoad := multiErr.ErrorOrNil(); errDataLoad != nil {
return &withConditionReasonError{reason: featurev1.ConditionReason.LoadTemplateData, err: errDataLoad}
}

for _, precondition := range f.preconditions {
multiErr = multierror.Append(multiErr, precondition(ctx, f))
multiErr = multierror.Append(multiErr, precondition(ctx, cli, f))
}
if preconditionsErr := multiErr.ErrorOrNil(); preconditionsErr != nil {
return &withConditionReasonError{reason: featurev1.ConditionReason.PreConditions, err: preconditionsErr}
}

for _, clusterOperation := range f.clusterOperations {
if errClusterOperation := clusterOperation(ctx, f); errClusterOperation != nil {
if errClusterOperation := clusterOperation(ctx, cli, f); errClusterOperation != nil {
return &withConditionReasonError{reason: featurev1.ConditionReason.ResourceCreation, err: errClusterOperation}
}
}

for i := range f.appliers {
r := f.appliers[i]
if processErr := r.Apply(ctx, f.Client, f.data, DefaultMetaOptions(f)...); processErr != nil {
if processErr := r.Apply(ctx, cli, f.data, DefaultMetaOptions(f)...); processErr != nil {
return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: processErr}
}
}

for _, postcondition := range f.postconditions {
multiErr = multierror.Append(multiErr, postcondition(ctx, f))
multiErr = multierror.Append(multiErr, postcondition(ctx, cli, f))
}
if postConditionErr := multiErr.ErrorOrNil(); postConditionErr != nil {
return &withConditionReasonError{reason: featurev1.ConditionReason.PostConditions, err: postConditionErr}
Expand All @@ -142,14 +141,14 @@ func (f *Feature) applyFeature(ctx context.Context) error {
return nil
}

func (f *Feature) Cleanup(ctx context.Context) error {
func (f *Feature) Cleanup(ctx context.Context, cli client.Client) error {
// Ensure associated FeatureTracker instance has been removed as last one
// in the chain of cleanups.
f.addCleanup(removeFeatureTracker(f))

var cleanupErrors *multierror.Error
for _, cleanupFunc := range f.cleanups {
cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f.Client))
cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, cli))
}

return cleanupErrors.ErrorOrNil()
Expand Down
6 changes: 4 additions & 2 deletions pkg/feature/feature_data.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
"fmt"

"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider"
)

Expand All @@ -13,8 +15,8 @@ import (
//
// If the value is static, consider using provider.ValueOf(variable).Get as passed provider function.
func Entry[T any](key string, providerFunc provider.DataProviderFunc[T]) Action {
return func(ctx context.Context, f *Feature) error {
data, err := providerFunc(ctx, f.Client)
return func(ctx context.Context, cli client.Client, f *Feature) error {
data, err := providerFunc(ctx, cli)
if err != nil {
return err
}
Expand Down
Loading

0 comments on commit fde5d69

Please sign in to comment.