Skip to content

refactor: extract shared reconciliation logic into pkg/reconcile #1572

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
istio-testing merged 3 commits into from
Feb 6, 2026
Merged

refactor: extract shared reconciliation logic into pkg/reconcile #1572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5eb5ab1
refactor: extract shared reconciliation logic into pkg/reconcile
aslakknutsen Feb 2, 2026
e96e291
refactor(validation): consolidate validation and move CRD-specific ch…
aslakknutsen Feb 5, 2026
c51614a
refactor(reconcile): use GetChartPath helper and tidy up exports
aslakknutsen Feb 6, 2026
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
110 changes: 15 additions & 95 deletions controllers/istiocni/istiocni_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"context"
"errors"
"fmt"
"path"
"reflect"

"github.com/go-logr/logr"
Expand All @@ -28,12 +27,10 @@ import (
"github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger"
"github.com/istio-ecosystem/sail-operator/pkg/errlist"
"github.com/istio-ecosystem/sail-operator/pkg/helm"
"github.com/istio-ecosystem/sail-operator/pkg/istiovalues"
"github.com/istio-ecosystem/sail-operator/pkg/istioversion"
"github.com/istio-ecosystem/sail-operator/pkg/kube"
"github.com/istio-ecosystem/sail-operator/pkg/predicate"
sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile"
"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
"github.com/istio-ecosystem/sail-operator/pkg/validation"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
Expand All @@ -53,11 +50,6 @@ import (
"istio.io/istio/pkg/ptr"
)

const (
cniReleaseName = "istio-cni"
cniChartName = "cni"
)

// Reconciler reconciles an IstioCNI object
type Reconciler struct {
client.Client
Expand Down Expand Up @@ -107,33 +99,19 @@ func (r *Reconciler) Reconcile(ctx context.Context, cni *v1.IstioCNI) (ctrl.Resu
}

func (r *Reconciler) Finalize(ctx context.Context, cni *v1.IstioCNI) error {
return r.uninstallHelmChart(ctx, cni)
cniReconciler := r.newCNIReconciler()
return cniReconciler.Uninstall(ctx, cni.Spec.Namespace)
}

func (r *Reconciler) doReconcile(ctx context.Context, cni *v1.IstioCNI) error {
log := logf.FromContext(ctx)
if err := r.validate(ctx, cni); err != nil {
return err
}

log.Info("Installing Helm chart")
return r.installHelmChart(ctx, cni)
}
cniReconciler := r.newCNIReconciler()

func (r *Reconciler) validate(ctx context.Context, cni *v1.IstioCNI) error {
if cni.Spec.Version == "" {
return reconciler.NewValidationError("spec.version not set")
}
if cni.Spec.Namespace == "" {
return reconciler.NewValidationError("spec.namespace not set")
}
if err := validation.ValidateTargetNamespace(ctx, r.Client, cni.Spec.Namespace); err != nil {
if err := cniReconciler.Validate(ctx, cni.Spec.Version, cni.Spec.Namespace); err != nil {
return err
}
return nil
}

func (r *Reconciler) installHelmChart(ctx context.Context, cni *v1.IstioCNI) error {
log.Info("Installing Helm chart")
ownerReference := metav1.OwnerReference{
APIVersion: v1.GroupVersion.String(),
Kind: v1.IstioCNIKind,
Expand All @@ -142,75 +120,17 @@ func (r *Reconciler) installHelmChart(ctx context.Context, cni *v1.IstioCNI) err
Controller: ptr.Of(true),
BlockOwnerDeletion: ptr.Of(true),
}

version, err := istioversion.Resolve(cni.Spec.Version)
if err != nil {
return fmt.Errorf("failed to resolve IstioCNI version for %q: %w", cni.Name, err)
}

// get userValues from Istio.spec.values
userValues := cni.Spec.Values

// apply image digests from configuration, if not already set by user
userValues = applyImageDigests(version, userValues, config.Config)

// apply vendor-specific default values
userValues, err = istiovalues.ApplyIstioCNIVendorDefaults(version, userValues)
if err != nil {
return fmt.Errorf("failed to apply vendor defaults: %w", err)
}

// apply userValues on top of defaultValues from profiles
mergedHelmValues, err := istiovalues.ApplyProfilesAndPlatform(
r.Config.ResourceFS, version, r.Config.Platform, r.Config.DefaultProfile, cni.Spec.Profile, helm.FromValues(userValues))
if err != nil {
return fmt.Errorf("failed to apply profile: %w", err)
}

_, err = r.ChartManager.UpgradeOrInstallChart(
ctx, r.Config.ResourceFS, r.getChartPath(version), mergedHelmValues, cni.Spec.Namespace, cniReleaseName, &ownerReference)
if err != nil {
return fmt.Errorf("failed to install/update Helm chart %q: %w", cniChartName, err)
}
return nil
return cniReconciler.Install(ctx, cni.Spec.Version, cni.Spec.Namespace, cni.Spec.Values, cni.Spec.Profile, &ownerReference)
}

func (r *Reconciler) getChartPath(version string) string {
return path.Join(version, "charts", cniChartName)
}

func applyImageDigests(version string, values *v1.CNIValues, config config.OperatorConfig) *v1.CNIValues {
imageDigests, digestsDefined := config.ImageDigests[version]
// if we don't have default image digests defined for this version, it's a no-op
if !digestsDefined {
return values
}

// if a global hub or tag value is configured by the user, don't set image digests
if values != nil && values.Global != nil && (values.Global.Hub != nil || values.Global.Tag != nil) {
return values
}

if values == nil {
values = &v1.CNIValues{}
}

// set image digest unless any part of the image has been configured by the user
if values.Cni == nil {
values.Cni = &v1.CNIConfig{}
}
if values.Cni.Image == nil && values.Cni.Hub == nil && values.Cni.Tag == nil {
values.Cni.Image = &imageDigests.CNIImage
}
return values
}

func (r *Reconciler) uninstallHelmChart(ctx context.Context, cni *v1.IstioCNI) error {
_, err := r.ChartManager.UninstallChart(ctx, cniReleaseName, cni.Spec.Namespace)
if err != nil {
return fmt.Errorf("failed to uninstall Helm chart %q: %w", cniChartName, err)
}
return nil
func (r *Reconciler) newCNIReconciler() *sharedreconcile.CNIReconciler {
return sharedreconcile.NewCNIReconciler(sharedreconcile.Config{
ResourceFS: r.Config.ResourceFS,
Platform: r.Config.Platform,
DefaultProfile: r.Config.DefaultProfile,
OperatorNamespace: r.Config.OperatorNamespace,
ChartManager: r.ChartManager,
}, r.Client)
}

// SetupWithManager sets up the controller with the Manager.
Expand Down
18 changes: 12 additions & 6 deletions controllers/istiocni/istiocni_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/istio-ecosystem/sail-operator/pkg/config"
"github.com/istio-ecosystem/sail-operator/pkg/istiovalues"
"github.com/istio-ecosystem/sail-operator/pkg/istioversion"
sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile"
"github.com/istio-ecosystem/sail-operator/pkg/scheme"
. "github.com/onsi/gomega"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -77,7 +78,7 @@ func TestValidate(t *testing.T) {
},
},
objects: []client.Object{ns},
expectErr: "spec.version not set",
expectErr: "version not set",
},
{
name: "no namespace",
Expand All @@ -90,7 +91,7 @@ func TestValidate(t *testing.T) {
},
},
objects: []client.Object{ns},
expectErr: "spec.namespace not set",
expectErr: "namespace not set",
},
{
name: "namespace not found",
Expand All @@ -111,9 +112,14 @@ func TestValidate(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.objects...).Build()
r := NewReconciler(cfg, cl, scheme.Scheme, nil)

err := r.validate(context.TODO(), tc.cni)
cniReconciler := sharedreconcile.NewCNIReconciler(sharedreconcile.Config{
ResourceFS: cfg.ResourceFS,
Platform: cfg.Platform,
DefaultProfile: cfg.DefaultProfile,
OperatorNamespace: cfg.OperatorNamespace,
}, cl)

err := cniReconciler.Validate(context.TODO(), tc.cni.Spec.Version, tc.cni.Spec.Namespace)
if tc.expectErr == "" {
g.Expect(err).ToNot(HaveOccurred())
} else {
Expand Down Expand Up @@ -496,7 +502,7 @@ func TestApplyImageDigests(t *testing.T) {
if err != nil {
t.Errorf("failed to resolve IstioCNI version for %q: %v", tc.input.Name, err)
}
result := applyImageDigests(version, tc.input.Spec.Values, tc.config)
result := sharedreconcile.ApplyCNIImageDigests(version, tc.input.Spec.Values, tc.config)
if diff := cmp.Diff(tc.expectValues, result); diff != "" {
t.Errorf("unexpected merge result; diff (-expected, +actual):\n%v", diff)
}
Expand Down
121 changes: 51 additions & 70 deletions controllers/istiorevision/istiorevision_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"context"
"errors"
"fmt"
"path"
"reflect"
"regexp"

Expand All @@ -31,6 +30,7 @@ import (
"github.com/istio-ecosystem/sail-operator/pkg/helm"
"github.com/istio-ecosystem/sail-operator/pkg/kube"
predicate2 "github.com/istio-ecosystem/sail-operator/pkg/predicate"
sharedreconcile "github.com/istio-ecosystem/sail-operator/pkg/reconcile"
"github.com/istio-ecosystem/sail-operator/pkg/reconciler"
"github.com/istio-ecosystem/sail-operator/pkg/revision"
"github.com/istio-ecosystem/sail-operator/pkg/validation"
Expand Down Expand Up @@ -115,57 +115,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, rev *v1.IstioRevision) (ctrl

func (r *Reconciler) doReconcile(ctx context.Context, rev *v1.IstioRevision) error {
log := logf.FromContext(ctx)
if err := r.validate(ctx, rev); err != nil {
return err
}

log.Info("Installing Helm chart")
return r.installHelmCharts(ctx, rev)
}

func (r *Reconciler) Finalize(ctx context.Context, rev *v1.IstioRevision) error {
return r.uninstallHelmCharts(ctx, rev)
}
istiodReconciler := r.newIstiodReconciler()

func (r *Reconciler) validate(ctx context.Context, rev *v1.IstioRevision) error {
if rev.Spec.Version == "" {
return reconciler.NewValidationError("spec.version not set")
}
if rev.Spec.Namespace == "" {
return reconciler.NewValidationError("spec.namespace not set")
}
if err := validation.ValidateTargetNamespace(ctx, r.Client, rev.Spec.Namespace); err != nil {
// CRD-specific validations
if err := r.validateRevisionConsistency(rev); err != nil {
return err
}

if rev.Spec.Values == nil {
return reconciler.NewValidationError("spec.values not set")
}

revName := rev.Spec.Values.Revision
if rev.Name == v1.DefaultRevision && (revName != nil && *revName != "") {
return reconciler.NewValidationError(fmt.Sprintf("spec.values.revision must be \"\" when IstioRevision name is %s", v1.DefaultRevision))
} else if rev.Name != v1.DefaultRevision && (revName == nil || *revName != rev.Name) {
return reconciler.NewValidationError("spec.values.revision does not match IstioRevision name")
}

if rev.Spec.Values.Global == nil || rev.Spec.Values.Global.IstioNamespace == nil || *rev.Spec.Values.Global.IstioNamespace != rev.Spec.Namespace {
return reconciler.NewValidationError("spec.values.global.istioNamespace does not match spec.namespace")
if err := r.validateNoTagConflict(ctx, rev); err != nil {
return err
}

tag := v1.IstioRevisionTag{}
if err := r.Client.Get(ctx, types.NamespacedName{Name: rev.Name}, &tag); err == nil {
if validation.ResourceTakesPrecedence(&tag.ObjectMeta, &rev.ObjectMeta) {
return reconciler.NewNameAlreadyExistsError("an IstioRevisionTag exists with this name", nil)
}
} else if !apierrors.IsNotFound(err) {
// General validations
if err := istiodReconciler.Validate(ctx, rev.Spec.Version, rev.Spec.Namespace, rev.Spec.Values); err != nil {
return err
}

return nil
}

func (r *Reconciler) installHelmCharts(ctx context.Context, rev *v1.IstioRevision) error {
log.Info("Installing Helm chart")
ownerReference := metav1.OwnerReference{
APIVersion: v1.GroupVersion.String(),
Kind: v1.IstioRevisionKind,
Expand All @@ -174,39 +139,55 @@ func (r *Reconciler) installHelmCharts(ctx context.Context, rev *v1.IstioRevisio
Controller: ptr.Of(true),
BlockOwnerDeletion: ptr.Of(true),
}

values := helm.FromValues(rev.Spec.Values)
_, err := r.ChartManager.UpgradeOrInstallChart(ctx, r.Config.ResourceFS, r.getChartPath(rev, constants.IstiodChartName),
values, rev.Spec.Namespace, getReleaseName(rev, constants.IstiodChartName), &ownerReference)
if err != nil {
return fmt.Errorf("failed to install/update Helm chart %q: %w", constants.IstiodChartName, err)
}
if rev.Name == v1.DefaultRevision {
_, err := r.ChartManager.UpgradeOrInstallChart(ctx, r.Config.ResourceFS, r.getChartPath(rev, constants.BaseChartName),
values, r.Config.OperatorNamespace, getReleaseName(rev, constants.BaseChartName), &ownerReference)
if err != nil {
return fmt.Errorf("failed to install/update Helm chart %q: %w", constants.BaseChartName, err)
}
}
return nil
return istiodReconciler.Install(ctx, rev.Spec.Version, rev.Spec.Namespace, rev.Spec.Values, rev.Name, &ownerReference)
}

func getReleaseName(rev *v1.IstioRevision, chartName string) string {
return fmt.Sprintf("%s-%s", rev.Name, chartName)
func (r *Reconciler) Finalize(ctx context.Context, rev *v1.IstioRevision) error {
istiodReconciler := r.newIstiodReconciler()
return istiodReconciler.Uninstall(ctx, rev.Spec.Namespace, rev.Name)
}

func (r *Reconciler) getChartPath(rev *v1.IstioRevision, chartName string) string {
return path.Join(rev.Spec.Version, "charts", chartName)
func (r *Reconciler) newIstiodReconciler() *sharedreconcile.IstiodReconciler {
return sharedreconcile.NewIstiodReconciler(sharedreconcile.Config{
ResourceFS: r.Config.ResourceFS,
Platform: r.Config.Platform,
DefaultProfile: r.Config.DefaultProfile,
OperatorNamespace: r.Config.OperatorNamespace,
ChartManager: r.ChartManager,
}, r.Client)
}

func (r *Reconciler) uninstallHelmCharts(ctx context.Context, rev *v1.IstioRevision) error {
if _, err := r.ChartManager.UninstallChart(ctx, getReleaseName(rev, constants.IstiodChartName), rev.Spec.Namespace); err != nil {
return fmt.Errorf("failed to uninstall Helm chart %q: %w", constants.IstiodChartName, err)
// validateRevisionConsistency validates that the IstioRevision CR fields are consistent
// with the Helm values. This is CRD-specific validation.
func (r *Reconciler) validateRevisionConsistency(rev *v1.IstioRevision) error {
values := rev.Spec.Values
if values == nil {
return nil // values nil check is done in general validation
}

// Validate revision name consistency
revName := values.Revision
if rev.Name == v1.DefaultRevision && (revName != nil && *revName != "") {
return reconciler.NewValidationError(fmt.Sprintf("values.revision must be \"\" when revision name is %s", v1.DefaultRevision))
} else if rev.Name != v1.DefaultRevision && (revName == nil || *revName != rev.Name) {
return reconciler.NewValidationError("values.revision does not match revision name")
}

// Validate namespace consistency
if values.Global == nil || values.Global.IstioNamespace == nil || *values.Global.IstioNamespace != rev.Spec.Namespace {
return reconciler.NewValidationError("values.global.istioNamespace does not match namespace")
}
if rev.Name == v1.DefaultRevision {
_, err := r.ChartManager.UninstallChart(ctx, getReleaseName(rev, constants.BaseChartName), r.Config.OperatorNamespace)
if err != nil {
return fmt.Errorf("failed to uninstall Helm chart %q: %w", constants.BaseChartName, err)

return nil
}

// validateNoTagConflict checks that no IstioRevisionTag exists with the same name
// as this IstioRevision. This is CRD-specific validation.
func (r *Reconciler) validateNoTagConflict(ctx context.Context, rev *v1.IstioRevision) error {
tag := v1.IstioRevisionTag{}
if err := r.Client.Get(ctx, types.NamespacedName{Name: rev.Name}, &tag); err == nil {
if validation.ResourceTakesPrecedence(&tag.ObjectMeta, &rev.ObjectMeta) {
return reconciler.NewNameAlreadyExistsError("an IstioRevisionTag exists with this name", nil)
}
}
return nil
Expand Down
Loading
Loading