diff --git a/cmd/cozystack-operator/main.go b/cmd/cozystack-operator/main.go index 2ca09c709..0a252dc7b 100644 --- a/cmd/cozystack-operator/main.go +++ b/cmd/cozystack-operator/main.go @@ -54,6 +54,7 @@ import ( "github.com/cozystack/cozystack/internal/fluxinstall" "github.com/cozystack/cozystack/internal/operator" "github.com/cozystack/cozystack/internal/telemetry" + "github.com/cozystack/cozystack/pkg/config" // +kubebuilder:scaffold:imports ) @@ -150,7 +151,7 @@ func main() { ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) parseFlag := func(flagName, raw string) time.Duration { - d, err := parsePositiveDuration(flagName, raw) + d, err := config.ParsePositiveDuration(flagName, raw) if err != nil { setupLog.Error(err, "invalid duration flag") os.Exit(1) @@ -439,21 +440,6 @@ func installPlatformSourceResource(ctx context.Context, k8sClient client.Client, return nil } -// parsePositiveDuration parses raw as a time.Duration and rejects malformed -// or non-positive values. Flux HelmRelease fields (Interval, Timeout, -// RetryInterval) require strictly positive durations, so a misconfigured -// flag must fail fast at startup rather than propagating into every HR. -func parsePositiveDuration(flagName, raw string) (time.Duration, error) { - d, err := time.ParseDuration(raw) - if err != nil { - return 0, fmt.Errorf("invalid duration for %s=%q: %w", flagName, raw, err) - } - if d <= 0 { - return 0, fmt.Errorf("%s must be > 0 (got %q)", flagName, raw) - } - return d, nil -} - // parsePlatformSourceURL parses the source URL and returns the source type and repository URL. // Supports formats: // - oci://registry.example.com/repo diff --git a/cmd/cozystack-operator/main_test.go b/cmd/cozystack-operator/main_test.go index 1f872e7ba..f13a22d0d 100644 --- a/cmd/cozystack-operator/main_test.go +++ b/cmd/cozystack-operator/main_test.go @@ -19,7 +19,6 @@ package main import ( "context" "testing" - "time" cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -140,11 +139,11 @@ func TestInstallPlatformPackageSource_Updates(t *testing.T) { func TestParsePlatformSourceURL(t *testing.T) { tests := []struct { - name string - url string - wantType string - wantURL string - wantErr bool + name string + url string + wantType string + wantURL string + wantErr bool }{ { name: "OCI URL", @@ -573,38 +572,3 @@ func TestGenerateGitRepository_InvalidRef(t *testing.T) { t.Fatal("expected error for invalid Git ref key, got nil") } } - -func TestParsePositiveDuration(t *testing.T) { - tests := []struct { - name string - raw string - want time.Duration - wantErr bool - }{ - {name: "valid seconds", raw: "30s", want: 30 * time.Second}, - {name: "valid minutes", raw: "5m", want: 5 * time.Minute}, - {name: "valid compound", raw: "1h30m", want: 90 * time.Minute}, - {name: "zero rejected", raw: "0s", wantErr: true}, - {name: "negative rejected", raw: "-5m", wantErr: true}, - {name: "malformed rejected", raw: "5x", wantErr: true}, - {name: "empty rejected", raw: "", wantErr: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := parsePositiveDuration("--test-flag", tt.raw) - if tt.wantErr { - if err == nil { - t.Fatalf("expected error for %q, got nil", tt.raw) - } - return - } - if err != nil { - t.Fatalf("unexpected error for %q: %v", tt.raw, err) - } - if got != tt.want { - t.Errorf("got %v, want %v", got, tt.want) - } - }) - } -} diff --git a/internal/operator/package_reconciler_test.go b/internal/operator/package_reconciler_test.go index e6b1e4bcb..f687998a5 100644 --- a/internal/operator/package_reconciler_test.go +++ b/internal/operator/package_reconciler_test.go @@ -122,10 +122,11 @@ func TestBuildHelmReleaseSpec(t *testing.T) { if spec.Install.Strategy.RetryInterval == nil || spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", spec.Install.Strategy.RetryInterval) } - // Remediation must remain nil: helm-controller's XValidation rule rejects - // Strategy.Name=RetryOnFailure with RetryInterval set alongside a - // Remediation entry, so a future "for safety" re-introduction of - // Remediation{Retries: -1} would silently break every HR. + // Remediation must remain nil: retries are driven solely by + // Strategy.Name=RetryOnFailure with an explicit RetryInterval. + // Re-introducing Remediation{Retries: -1} "for safety" would add a + // second, conflicting retry mechanism alongside the strategy retry + // path. if spec.Install.Remediation != nil { t.Errorf("Install.Remediation = %+v, want nil", spec.Install.Remediation) } diff --git a/pkg/cmd/server/start.go b/pkg/cmd/server/start.go index 7ebbbc899..ce9785317 100644 --- a/pkg/cmd/server/start.go +++ b/pkg/cmd/server/start.go @@ -57,6 +57,17 @@ type CozyServerOptions struct { // Add a field to store the configuration ResourceConfig *config.ResourceConfig + + // Raw HelmRelease generation flag values; parsed and validated in + // Complete() with config.ParsePositiveDuration so a misconfigured flag + // fails fast at startup. Kept in sync with the cozystack-operator + // flags of the same name so both HelmRelease-generating paths share + // the same retry strategy and cadence. + HelmReleaseInterval string + HelmReleaseRetryInterval string + HelmReleaseInstallTimeout string + HelmReleaseUpgradeTimeout string + HelmReleaseMaxHistory int } // NewCozyServerOptions returns a new instance of CozyServerOptions @@ -71,6 +82,13 @@ func NewCozyServerOptions(out, errOut io.Writer) *CozyServerOptions { ), StdOut: out, StdErr: errOut, + // Defaults match cozystack-operator: production semantics are + // identical between the two HelmRelease-generating paths. + HelmReleaseInterval: "5m", + HelmReleaseRetryInterval: "30s", + HelmReleaseInstallTimeout: "10m", + HelmReleaseUpgradeTimeout: "10m", + HelmReleaseMaxHistory: 5, } o.RecommendedOptions.Etcd = nil return o @@ -100,14 +118,83 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions) flags := cmd.Flags() o.RecommendedOptions.AddFlags(flags) + // HelmRelease generation knobs. Names, defaults, and validation match + // the cozystack-operator flags of the same name so both + // HelmRelease-generating paths can be tuned together. + flags.StringVar(&o.HelmReleaseInterval, "helmrelease-interval", o.HelmReleaseInterval, + "Reconcile interval applied to HelmReleases generated from Application resources "+ + "(Spec.Interval). Mirrors the cozystack-operator flag of the same name.") + flags.StringVar(&o.HelmReleaseRetryInterval, "helmrelease-retry-interval", o.HelmReleaseRetryInterval, + "Retry interval applied to Install.Strategy and Upgrade.Strategy of HelmReleases generated "+ + "from Application resources. With Strategy.Name=RetryOnFailure, this controls how long "+ + "the controller waits between failed install/upgrade attempts. Decoupled from "+ + "--helmrelease-interval so failures recover fast without polling healthy releases at the "+ + "same cadence.") + flags.StringVar(&o.HelmReleaseInstallTimeout, "helmrelease-install-timeout", o.HelmReleaseInstallTimeout, + "Default timeout for the Helm install action of HelmReleases generated from Application "+ + "resources (Spec.Install.Timeout). Overridden per-Application by the "+ + "release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition; the "+ + "same annotation also overrides --helmrelease-upgrade-timeout.") + flags.StringVar(&o.HelmReleaseUpgradeTimeout, "helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout, + "Default timeout for the Helm upgrade action of HelmReleases generated from Application "+ + "resources (Spec.Upgrade.Timeout). Overridden per-Application by the "+ + "release.cozystack.io/helm-install-timeout annotation (which sets both install and "+ + "upgrade), or by release.cozystack.io/helm-upgrade-timeout to override only the upgrade "+ + "side for a kind that needs an asymmetric budget.") + flags.IntVar(&o.HelmReleaseMaxHistory, "helmrelease-max-history", o.HelmReleaseMaxHistory, + "Number of release revisions Helm keeps for HelmReleases generated from Application "+ + "resources (Spec.MaxHistory). 0 means unlimited; 5 matches Helm's default.") + // Note: KEP-4330 component versioning functionality (k8s.io/apiserver/pkg/util/version) // is not available in Kubernetes v0.34.1. The component versioning code has been removed. return cmd } +// helmReleaseFlagValues holds the parsed, validated HelmRelease generation +// flags. Populated by parseAndValidateHelmReleaseFlags before any kubernetes +// I/O so a misconfigured server restarts loudly instead of waiting until the +// first Application is created. +type helmReleaseFlagValues struct { + interval time.Duration + retryInterval time.Duration + installTimeout time.Duration + upgradeTimeout time.Duration + maxHistory int +} + +// parseAndValidateHelmReleaseFlags parses the five HelmRelease generation +// flags and rejects malformed or non-positive durations and negative +// MaxHistory. Same shape as cozystack-operator's main. +func (o *CozyServerOptions) parseAndValidateHelmReleaseFlags() (helmReleaseFlagValues, error) { + var v helmReleaseFlagValues + var err error + if v.interval, err = config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval); err != nil { + return v, err + } + if v.retryInterval, err = config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval); err != nil { + return v, err + } + if v.installTimeout, err = config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout); err != nil { + return v, err + } + if v.upgradeTimeout, err = config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout); err != nil { + return v, err + } + if o.HelmReleaseMaxHistory < 0 { + return v, fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory) + } + v.maxHistory = o.HelmReleaseMaxHistory + return v, nil +} + // Complete fills in the fields that are not set func (o *CozyServerOptions) Complete() error { + hrFlags, err := o.parseAndValidateHelmReleaseFlags() + if err != nil { + return err + } + scheme := runtime.NewScheme() if err := v1alpha1.AddToScheme(scheme); err != nil { return fmt.Errorf("failed to register types: %w", err) @@ -168,6 +255,16 @@ func (o *CozyServerOptions) Complete() error { Name: crd.Spec.Release.ChartRef.Name, Namespace: crd.Spec.Release.ChartRef.Namespace, }, + // Per-Application HelmRelease generation defaults from server + // flags. The same five values are applied to every Resource, + // matching cozystack-operator's PackageReconciler. The + // per-Application HelmInstallTimeout annotation populated below + // still wins over HelmReleaseInstallTimeout/UpgradeTimeout. + HelmReleaseInterval: hrFlags.interval, + HelmReleaseRetryInterval: hrFlags.retryInterval, + HelmReleaseInstallTimeout: hrFlags.installTimeout, + HelmReleaseUpgradeTimeout: hrFlags.upgradeTimeout, + HelmReleaseMaxHistory: hrFlags.maxHistory, } // Per-Application HelmRelease Install/Upgrade timeout. Applications // whose parent chart contains asynchronously-provisioned resources @@ -177,8 +274,10 @@ func (o *CozyServerOptions) Complete() error { // layer when building the HelmRelease Spec. The parser rejects // units Flux would reject at webhook time, so a bad annotation // surfaces as a loud startup failure instead of a silent drop to - // defaults. - d, err := config.ParseHelmInstallTimeoutAnnotation( + // defaults. helm-install-timeout covers both install and upgrade; + // helm-upgrade-timeout overrides only the upgrade side for kinds + // that need an asymmetric budget. + installTimeout, err := config.ParseHelmTimeoutAnnotation( crd.Annotations[config.HelmInstallTimeoutAnnotation], ) if err != nil { @@ -187,7 +286,17 @@ func (o *CozyServerOptions) Complete() error { crd.Name, config.HelmInstallTimeoutAnnotation, err, ) } - release.HelmInstallTimeout = d + release.HelmInstallTimeout = installTimeout + upgradeTimeout, err := config.ParseHelmTimeoutAnnotation( + crd.Annotations[config.HelmUpgradeTimeoutAnnotation], + ) + if err != nil { + return fmt.Errorf( + "ApplicationDefinition %q has invalid %s annotation: %w", + crd.Name, config.HelmUpgradeTimeoutAnnotation, err, + ) + } + release.HelmUpgradeTimeout = upgradeTimeout resource := config.Resource{ Application: config.ApplicationConfig{ Kind: crd.Spec.Application.Kind, diff --git a/pkg/cmd/server/start_test.go b/pkg/cmd/server/start_test.go index 9dc2000c6..599b085fa 100644 --- a/pkg/cmd/server/start_test.go +++ b/pkg/cmd/server/start_test.go @@ -15,3 +15,172 @@ limitations under the License. */ package server + +import ( + "strings" + "testing" + "time" +) + +// validHelmReleaseFlags returns a CozyServerOptions populated with the same +// defaults NewCozyServerOptions sets, so each test case mutates a single +// flag and asserts the resulting validation behaviour in isolation. +func validHelmReleaseFlags() *CozyServerOptions { + return &CozyServerOptions{ + HelmReleaseInterval: "5m", + HelmReleaseRetryInterval: "30s", + HelmReleaseInstallTimeout: "10m", + HelmReleaseUpgradeTimeout: "10m", + HelmReleaseMaxHistory: 5, + } +} + +// Pins that each malformed/non-positive duration and a negative MaxHistory +// is rejected at startup. Without this, a regression that swaps `<` for +// `<=` on the MaxHistory branch — breaking the documented "0 means +// unlimited" contract — or that drops a `return err` line on any of the +// four duration branches would ship green. +func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Rejects(t *testing.T) { + cases := []struct { + name string + mutate func(o *CozyServerOptions) + wantSubs string + }{ + { + name: "interval zero", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "0s" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "interval negative", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "-5m" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "interval malformed", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "5x" }, + wantSubs: "--helmrelease-interval", + }, + { + name: "retry-interval empty", + mutate: func(o *CozyServerOptions) { o.HelmReleaseRetryInterval = "" }, + wantSubs: "--helmrelease-retry-interval", + }, + { + name: "install-timeout zero", + mutate: func(o *CozyServerOptions) { o.HelmReleaseInstallTimeout = "0s" }, + wantSubs: "--helmrelease-install-timeout", + }, + { + name: "upgrade-timeout malformed", + mutate: func(o *CozyServerOptions) { o.HelmReleaseUpgradeTimeout = "tenminutes" }, + wantSubs: "--helmrelease-upgrade-timeout", + }, + { + name: "max-history negative", + mutate: func(o *CozyServerOptions) { o.HelmReleaseMaxHistory = -1 }, + wantSubs: "--helmrelease-max-history must be >= 0", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + o := validHelmReleaseFlags() + tc.mutate(o) + _, err := o.parseAndValidateHelmReleaseFlags() + if err == nil { + t.Fatalf("expected error, got nil") + } + if !strings.Contains(err.Error(), tc.wantSubs) { + t.Errorf("error %q does not contain %q", err.Error(), tc.wantSubs) + } + }) + } +} + +// Pins the happy path: valid flag strings parse to the expected +// time.Duration values and MaxHistory=0 is accepted (the documented +// "unlimited per Helm semantics" branch). +func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Accepts(t *testing.T) { + cases := []struct { + name string + o *CozyServerOptions + wantInterval time.Duration + wantRetry time.Duration + wantInstall time.Duration + wantUpgrade time.Duration + wantMaxHistory int + }{ + { + name: "production defaults", + o: validHelmReleaseFlags(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 10 * time.Minute, + wantUpgrade: 10 * time.Minute, + wantMaxHistory: 5, + }, + { + name: "max-history zero accepted as unlimited", + o: func() *CozyServerOptions { + o := validHelmReleaseFlags() + o.HelmReleaseMaxHistory = 0 + return o + }(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 10 * time.Minute, + wantUpgrade: 10 * time.Minute, + wantMaxHistory: 0, + }, + { + name: "asymmetric install/upgrade timeouts both honored", + o: func() *CozyServerOptions { + o := validHelmReleaseFlags() + o.HelmReleaseInstallTimeout = "8m" + o.HelmReleaseUpgradeTimeout = "12m" + return o + }(), + wantInterval: 5 * time.Minute, + wantRetry: 30 * time.Second, + wantInstall: 8 * time.Minute, + wantUpgrade: 12 * time.Minute, + wantMaxHistory: 5, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + v, err := tc.o.parseAndValidateHelmReleaseFlags() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if v.interval != tc.wantInterval { + t.Errorf("interval = %v, want %v", v.interval, tc.wantInterval) + } + if v.retryInterval != tc.wantRetry { + t.Errorf("retryInterval = %v, want %v", v.retryInterval, tc.wantRetry) + } + if v.installTimeout != tc.wantInstall { + t.Errorf("installTimeout = %v, want %v", v.installTimeout, tc.wantInstall) + } + if v.upgradeTimeout != tc.wantUpgrade { + t.Errorf("upgradeTimeout = %v, want %v", v.upgradeTimeout, tc.wantUpgrade) + } + if v.maxHistory != tc.wantMaxHistory { + t.Errorf("maxHistory = %d, want %d", v.maxHistory, tc.wantMaxHistory) + } + }) + } +} + +// Pins that NewCozyServerOptions seeds defaults that pass validation, so +// `cozystack-api ...` with no HelmRelease flags overridden does not fail +// at startup. Guards against a future change to defaults that accidentally +// violates the validator. +func TestNewCozyServerOptions_DefaultsValidate(t *testing.T) { + o := NewCozyServerOptions(nil, nil) + if _, err := o.parseAndValidateHelmReleaseFlags(); err != nil { + t.Fatalf("default options must validate, got: %v", err) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 21de27b20..5a4c9d45d 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -27,6 +27,14 @@ import ( // Upgrade.Timeout for a given Application kind. const HelmInstallTimeoutAnnotation = "release.cozystack.io/helm-install-timeout" +// HelmUpgradeTimeoutAnnotation is the ApplicationDefinition metadata +// annotation key that overrides only the Flux HelmRelease Upgrade.Timeout +// for a given Application kind. When set it takes precedence over the +// Upgrade.Timeout value HelmInstallTimeoutAnnotation would otherwise apply, +// letting a kind keep a short install budget but a longer upgrade budget +// (or vice versa). +const HelmUpgradeTimeoutAnnotation = "release.cozystack.io/helm-upgrade-timeout" + // helmTimeoutPattern mirrors the CRD validation pattern used by Flux // helm-controller on HelmReleaseSpec.Install.Timeout (ms/s/m/h units only). // time.ParseDuration accepts ns/us/µs, but Flux rejects them - parsing here @@ -36,14 +44,15 @@ const HelmInstallTimeoutAnnotation = "release.cozystack.io/helm-install-timeout" // in the go module cache. var helmTimeoutPattern = regexp.MustCompile(`^([0-9]+(\.[0-9]+)?(ms|s|m|h))+$`) -// ParseHelmInstallTimeoutAnnotation parses the value of the -// release.cozystack.io/helm-install-timeout annotation. The empty string is -// treated as "unset" and returns (0, nil) so callers can leave -// HelmInstallTimeout zeroed and let flux defaults apply. Values accepted by +// ParseHelmTimeoutAnnotation parses the value of a Flux HelmRelease timeout +// annotation (release.cozystack.io/helm-install-timeout or +// release.cozystack.io/helm-upgrade-timeout). The empty string is treated as +// "unset" and returns (0, nil) so callers can leave the corresponding +// ReleaseConfig field zeroed and let flux defaults apply. Values accepted by // time.ParseDuration but rejected by Flux (ns/us/µs) return a helpful // error instead of silently parsing and failing later at HelmRelease // admission. -func ParseHelmInstallTimeoutAnnotation(raw string) (time.Duration, error) { +func ParseHelmTimeoutAnnotation(raw string) (time.Duration, error) { if raw == "" { return 0, nil } @@ -58,6 +67,23 @@ func ParseHelmInstallTimeoutAnnotation(raw string) (time.Duration, error) { return d, nil } +// ParsePositiveDuration parses raw as a time.Duration and rejects malformed +// or non-positive values. Flux HelmRelease fields (Interval, Timeout, +// RetryInterval) require strictly positive durations, so a misconfigured +// flag must fail fast at startup rather than propagating into every HR. +// Shared between cozystack-operator and cozystack-api so both paths reject +// the same set of inputs at startup. +func ParsePositiveDuration(flagName, raw string) (time.Duration, error) { + d, err := time.ParseDuration(raw) + if err != nil { + return 0, fmt.Errorf("invalid duration for %s=%q: %w", flagName, raw, err) + } + if d <= 0 { + return 0, fmt.Errorf("%s must be > 0 (got %q)", flagName, raw) + } + return d, nil +} + // ResourceConfig represents the structure of the configuration file. type ResourceConfig struct { Resources []Resource `yaml:"resources"` @@ -83,12 +109,44 @@ type ReleaseConfig struct { Prefix string `yaml:"prefix"` Labels map[string]string `yaml:"labels"` ChartRef ChartRefConfig `yaml:"chartRef"` - // HelmInstallTimeout overrides the Flux HelmRelease Install.Timeout and - // Upgrade.Timeout for this Application kind. When zero, flux defaults - // apply. Populated from the - // release.cozystack.io/helm-install-timeout annotation on the - // ApplicationDefinition at start-up. + // HelmInstallTimeout is a per-Application override of Install.Timeout + // and Upgrade.Timeout. When non-zero, it wins over + // HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout below. + // Populated from the release.cozystack.io/helm-install-timeout + // annotation on the ApplicationDefinition at start-up. HelmInstallTimeout time.Duration `yaml:"helmInstallTimeout,omitempty"` + // HelmUpgradeTimeout is a per-Application override of Upgrade.Timeout + // only. When non-zero, it wins over both HelmReleaseUpgradeTimeout and + // the Upgrade.Timeout value HelmInstallTimeout would otherwise apply, + // so a kind can carry an asymmetric install/upgrade budget. Populated + // from the release.cozystack.io/helm-upgrade-timeout annotation on the + // ApplicationDefinition at start-up. + HelmUpgradeTimeout time.Duration `yaml:"helmUpgradeTimeout,omitempty"` + // HelmReleaseInterval is the global default for Spec.Interval on + // HelmReleases generated by cozystack-api. Set from the api server + // --helmrelease-interval flag; mirrors the cozystack-operator flag of + // the same name so both HelmRelease-generating paths use the same + // reconcile cadence. + HelmReleaseInterval time.Duration `yaml:"helmReleaseInterval,omitempty"` + // HelmReleaseRetryInterval is the global default for + // Install/Upgrade.Strategy.RetryInterval. Decoupled from + // HelmReleaseInterval so failed install/upgrade retries recover fast + // without polling healthy releases at the same cadence. + HelmReleaseRetryInterval time.Duration `yaml:"helmReleaseRetryInterval,omitempty"` + // HelmReleaseInstallTimeout is the global default for + // Spec.Install.Timeout. Overridden per-Application by HelmInstallTimeout + // when the annotation is set. + HelmReleaseInstallTimeout time.Duration `yaml:"helmReleaseInstallTimeout,omitempty"` + // HelmReleaseUpgradeTimeout is the global default for + // Spec.Upgrade.Timeout. Overridden per-Application by HelmInstallTimeout + // when the annotation is set. + HelmReleaseUpgradeTimeout time.Duration `yaml:"helmReleaseUpgradeTimeout,omitempty"` + // HelmReleaseMaxHistory is the global default for Spec.MaxHistory. + // 0 means unlimited per Helm semantics; matches the cozystack-operator + // flag of the same name. No omitempty: 0 ("unlimited") must survive a + // round-trip distinct from an unset field if ReleaseConfig is ever + // marshalled. + HelmReleaseMaxHistory int `yaml:"helmReleaseMaxHistory"` } // ChartRefConfig references a Flux source artifact for the Helm chart. diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index eb6990afd..fe7321163 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -15,7 +15,7 @@ import ( // - compound forms (the CRD pattern accepts repeats), // - units time.ParseDuration accepts but Flux rejects (ns, us, µs), // - outright garbage. -func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { +func TestParseHelmTimeoutAnnotation(t *testing.T) { cases := []struct { name string input string @@ -98,7 +98,7 @@ func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := ParseHelmInstallTimeoutAnnotation(tc.input) + got, err := ParseHelmTimeoutAnnotation(tc.input) if tc.wantErr { if err == nil { t.Fatalf("expected error, got duration=%v", got) @@ -117,3 +117,50 @@ func TestParseHelmInstallTimeoutAnnotation(t *testing.T) { }) } } + +// ParsePositiveDuration is the startup validator shared by cozystack-operator +// and cozystack-api. Both binaries reject zero/negative/malformed values so a +// misconfigured flag fails fast instead of propagating into every generated +// HelmRelease and being rejected by helm-controller's CRD validation later. +func TestParsePositiveDuration(t *testing.T) { + tests := []struct { + name string + raw string + want time.Duration + // errMatch pins which of the two error branches fired: a + // time.ParseDuration failure ("invalid duration") versus a + // non-positive value ("must be > 0"). Keeps the two diagnostics + // from drifting and operators seeing the wrong message. + errMatch string + wantErr bool + }{ + {name: "valid seconds", raw: "30s", want: 30 * time.Second}, + {name: "valid minutes", raw: "5m", want: 5 * time.Minute}, + {name: "valid compound", raw: "1h30m", want: 90 * time.Minute}, + {name: "zero rejected", raw: "0s", wantErr: true, errMatch: "must be > 0"}, + {name: "negative rejected", raw: "-5m", wantErr: true, errMatch: "must be > 0"}, + {name: "malformed rejected", raw: "5x", wantErr: true, errMatch: "invalid duration"}, + {name: "empty rejected", raw: "", wantErr: true, errMatch: "invalid duration"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParsePositiveDuration("--test-flag", tt.raw) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error for %q, got nil", tt.raw) + } + if tt.errMatch != "" && !strings.Contains(err.Error(), tt.errMatch) { + t.Errorf("error %q does not contain %q", err.Error(), tt.errMatch) + } + return + } + if err != nil { + t.Fatalf("unexpected error for %q: %v", tt.raw, err) + } + if got != tt.want { + t.Errorf("got %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 080e57ada..d3014eac7 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -1509,8 +1509,47 @@ func latestMonitorTime(m *cozyv1alpha1.WorkloadMonitor) metav1.Time { return m.CreationTimestamp } -// convertApplicationToHelmRelease implements the actual conversion logic +// convertApplicationToHelmRelease implements the actual conversion logic. +// +// Spec.Interval, Install/Upgrade.Strategy{Name=RetryOnFailure,RetryInterval}, +// Install/Upgrade.Timeout, and Spec.MaxHistory are populated from +// ReleaseConfig fields fed by the cozystack-api server flags +// (--helmrelease-{interval,retry-interval,install-timeout,upgrade-timeout, +// max-history}). This mirrors cozystack-operator's +// PackageReconciler.buildHelmReleaseSpec so both HelmRelease-generating paths +// share the same retry strategy, history retention, and reconcile cadence. +// +// Strategy.Name=RetryOnFailure with an explicit RetryInterval (rather than +// Remediation{Retries:-1}) decouples failed install/upgrade retry timing +// from Spec.Interval — the previous coupling caused failed installs to +// retry only every 5m, exceeding E2E budgets. func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*helmv2.HelmRelease, error) { + // Per-Application annotation overrides win over the global defaults + // (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout): + // - HelmInstallTimeout (release.cozystack.io/helm-install-timeout) + // sets both Install.Timeout and Upgrade.Timeout; + // - HelmUpgradeTimeout (release.cozystack.io/helm-upgrade-timeout) + // then overrides only Upgrade.Timeout, so a kind can carry an + // asymmetric budget (short install, long upgrade or vice versa). + // kubernetes-rd and tenant-rd carry helm-install-timeout today: the + // Kubernetes Application's parent chart contains CAPI/Kamaji + // resources whose admin-kubeconfig Secret is provisioned + // asynchronously and Kamaji cold-start routinely exceeds flux's + // default wait budget, and the Tenant parent chart bootstraps the + // seaweedfs-db CNPG cluster whose first reconcile exceeds it too. + // Any future kind with the same shape can opt in by setting the + // same annotation. + installTimeout := r.releaseConfig.HelmReleaseInstallTimeout + upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout + if r.releaseConfig.HelmInstallTimeout > 0 { + installTimeout = r.releaseConfig.HelmInstallTimeout + upgradeTimeout = r.releaseConfig.HelmInstallTimeout + } + if r.releaseConfig.HelmUpgradeTimeout > 0 { + upgradeTimeout = r.releaseConfig.HelmUpgradeTimeout + } + + maxHistory := r.releaseConfig.HelmReleaseMaxHistory helmRelease := &helmv2.HelmRelease{ TypeMeta: metav1.TypeMeta{ APIVersion: "helm.toolkit.fluxcd.io/v2", @@ -1530,15 +1569,20 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* Name: r.releaseConfig.ChartRef.Name, Namespace: r.releaseConfig.ChartRef.Namespace, }, - Interval: metav1.Duration{Duration: 5 * time.Minute}, + Interval: metav1.Duration{Duration: r.releaseConfig.HelmReleaseInterval}, + MaxHistory: &maxHistory, Install: &helmv2.Install{ - Remediation: &helmv2.InstallRemediation{ - Retries: -1, + Timeout: &metav1.Duration{Duration: installTimeout}, + Strategy: &helmv2.InstallStrategy{ + Name: string(helmv2.ActionStrategyRetryOnFailure), + RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, }, }, Upgrade: &helmv2.Upgrade{ - Remediation: &helmv2.UpgradeRemediation{ - Retries: -1, + Timeout: &metav1.Duration{Duration: upgradeTimeout}, + Strategy: &helmv2.UpgradeStrategy{ + Name: string(helmv2.ActionStrategyRetryOnFailure), + RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval}, }, }, ValuesFrom: []helmv2.ValuesReference{ @@ -1551,26 +1595,6 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (* }, } - // Per-Application HelmRelease wait budget. The mechanism is generic: - // an ApplicationDefinition that sets - // release.cozystack.io/helm-install-timeout gets Install.Timeout and - // Upgrade.Timeout populated from ReleaseConfig.HelmInstallTimeout - // (parsed at startup). Applications that leave it unset keep flux - // defaults so their failed installs remediate on the normal cadence. - // kubernetes-rd and tenant-rd carry the annotation today: the - // Kubernetes Application's parent chart contains CAPI/Kamaji - // resources whose admin-kubeconfig Secret is provisioned - // asynchronously and Kamaji cold-start routinely exceeds flux's - // default wait budget, and the Tenant parent chart bootstraps the - // seaweedfs-db CNPG cluster whose first reconcile exceeds it too. - // Any future kind with the same shape can opt in by setting the - // same annotation. - if r.releaseConfig.HelmInstallTimeout > 0 { - timeout := metav1.Duration{Duration: r.releaseConfig.HelmInstallTimeout} - helmRelease.Spec.Install.Timeout = &timeout - helmRelease.Spec.Upgrade.Timeout = &timeout - } - return helmRelease, nil } diff --git a/pkg/registry/apps/application/rest_helmrelease_spec_test.go b/pkg/registry/apps/application/rest_helmrelease_spec_test.go new file mode 100644 index 000000000..7c210d606 --- /dev/null +++ b/pkg/registry/apps/application/rest_helmrelease_spec_test.go @@ -0,0 +1,235 @@ +/* +Copyright 2026 The Cozystack Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package application + +import ( + "testing" + "time" + + helmv2 "github.com/fluxcd/helm-controller/api/v2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1" + "github.com/cozystack/cozystack/pkg/config" +) + +// Mirrors the operator-side TestBuildHelmReleaseSpec +// (internal/operator/package_reconciler_test.go) so the api-side +// HelmRelease shape stays in lockstep with cozystack-operator. The two +// HelmRelease-generating paths must populate Interval, MaxHistory, +// Strategy, RetryInterval, and Install/Upgrade.Timeout identically; drift +// between them is the failure mode this test guards against. +func TestConvertApplicationToHelmRelease_BuildsSpecFromConfig(t *testing.T) { + r := &REST{ + kindName: "Postgres", + releaseConfig: config.ReleaseConfig{ + Prefix: "postgres-", + ChartRef: config.ChartRefConfig{ + Kind: "HelmChart", + Name: "postgres", + Namespace: "cozy-system", + }, + HelmReleaseInterval: 42 * time.Second, + HelmReleaseRetryInterval: 17 * time.Second, + HelmReleaseInstallTimeout: 11 * time.Minute, + HelmReleaseUpgradeTimeout: 13 * time.Minute, + HelmReleaseMaxHistory: 7, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + + if hr.Spec.Interval.Duration != 42*time.Second { + t.Errorf("Interval = %v, want 42s", hr.Spec.Interval.Duration) + } + if hr.Spec.MaxHistory == nil { + t.Fatal("MaxHistory is nil, want pointer to 7") + } + if *hr.Spec.MaxHistory != 7 { + t.Errorf("MaxHistory = %d, want 7", *hr.Spec.MaxHistory) + } + + if hr.Spec.ChartRef == nil { + t.Fatal("ChartRef is nil") + } + if hr.Spec.ChartRef.Kind != "HelmChart" { + t.Errorf("ChartRef.Kind = %q, want HelmChart", hr.Spec.ChartRef.Kind) + } + if hr.Spec.ChartRef.Name != "postgres" { + t.Errorf("ChartRef.Name = %q, want postgres", hr.Spec.ChartRef.Name) + } + if hr.Spec.ChartRef.Namespace != "cozy-system" { + t.Errorf("ChartRef.Namespace = %q, want cozy-system", hr.Spec.ChartRef.Namespace) + } + + if hr.Spec.Install == nil { + t.Fatal("Install is nil") + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != 11*time.Minute { + t.Errorf("Install.Timeout = %v, want 11m", hr.Spec.Install.Timeout) + } + if hr.Spec.Install.Strategy == nil { + t.Fatal("Install.Strategy is nil") + } + if hr.Spec.Install.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Install.Strategy.Name = %q, want %q", hr.Spec.Install.Strategy.Name, helmv2.ActionStrategyRetryOnFailure) + } + if hr.Spec.Install.Strategy.RetryInterval == nil || hr.Spec.Install.Strategy.RetryInterval.Duration != 17*time.Second { + t.Errorf("Install.Strategy.RetryInterval = %v, want 17s", hr.Spec.Install.Strategy.RetryInterval) + } + // Remediation must remain nil: retries are driven solely by + // Strategy.Name=RetryOnFailure with an explicit RetryInterval, the + // same single retry path cozystack-operator's PackageReconciler + // uses. Re-introducing Remediation{Retries: -1} "for safety" would + // add a second, conflicting retry mechanism and break parity with + // the operator path. + if hr.Spec.Install.Remediation != nil { + t.Errorf("Install.Remediation = %+v, want nil", hr.Spec.Install.Remediation) + } + + if hr.Spec.Upgrade == nil { + t.Fatal("Upgrade is nil") + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != 13*time.Minute { + t.Errorf("Upgrade.Timeout = %v, want 13m", hr.Spec.Upgrade.Timeout) + } + if hr.Spec.Upgrade.Strategy == nil { + t.Fatal("Upgrade.Strategy is nil") + } + if hr.Spec.Upgrade.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Upgrade.Strategy.Name = %q, want %q", hr.Spec.Upgrade.Strategy.Name, helmv2.ActionStrategyRetryOnFailure) + } + if hr.Spec.Upgrade.Strategy.RetryInterval == nil || hr.Spec.Upgrade.Strategy.RetryInterval.Duration != 17*time.Second { + t.Errorf("Upgrade.Strategy.RetryInterval = %v, want 17s", hr.Spec.Upgrade.Strategy.RetryInterval) + } + if hr.Spec.Upgrade.Remediation != nil { + t.Errorf("Upgrade.Remediation = %+v, want nil", hr.Spec.Upgrade.Remediation) + } +} + +// Pins that MaxHistory=0 (unlimited history per Helm semantics) survives the +// conversion — i.e. is set as a non-nil pointer to 0 rather than dropped or +// replaced with a default. Mirrors the operator-side +// TestBuildHelmReleaseSpecZeroMaxHistory. +func TestConvertApplicationToHelmRelease_ZeroMaxHistory(t *testing.T) { + r := &REST{ + kindName: "Postgres", + releaseConfig: config.ReleaseConfig{ + Prefix: "postgres-", + HelmReleaseInterval: 30 * time.Second, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseMaxHistory: 0, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "x", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.MaxHistory == nil { + t.Fatal("MaxHistory is nil for HelmReleaseMaxHistory=0; want pointer to 0") + } + if *hr.Spec.MaxHistory != 0 { + t.Errorf("MaxHistory = %d, want 0", *hr.Spec.MaxHistory) + } +} + +// HelmInstallTimeout (the per-Application annotation override) wins over the +// global HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout defaults. This +// is the contract that lets ApplicationDefinitions like Kubernetes (with its +// long Kamaji bootstrap) opt out of the global default without operators +// having to widen the global flag for every kind. +// +// The asymmetric-globals case pins that the override wipes both install and +// upgrade timeouts to the same value, even when the globals differ — a +// regression that only assigned one would slip through if the test only +// checked symmetric globals. +func TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal(t *testing.T) { + cases := []struct { + name string + globalInstall time.Duration + globalUpgrade time.Duration + override time.Duration + wantInstall time.Duration + wantUpgrade time.Duration + }{ + { + name: "symmetric globals, override wins", + globalInstall: 10 * time.Minute, + globalUpgrade: 10 * time.Minute, + override: 30 * time.Minute, + wantInstall: 30 * time.Minute, + wantUpgrade: 30 * time.Minute, + }, + { + name: "asymmetric globals, override wipes both to same value", + globalInstall: 8 * time.Minute, + globalUpgrade: 12 * time.Minute, + override: 30 * time.Minute, + wantInstall: 30 * time.Minute, + wantUpgrade: 30 * time.Minute, + }, + { + name: "asymmetric globals, no override keeps each side independent", + globalInstall: 8 * time.Minute, + globalUpgrade: 12 * time.Minute, + override: 0, + wantInstall: 8 * time.Minute, + wantUpgrade: 12 * time.Minute, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := &REST{ + kindName: "Kubernetes", + releaseConfig: config.ReleaseConfig{ + Prefix: "kubernetes-", + HelmReleaseInterval: 5 * time.Minute, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseInstallTimeout: tc.globalInstall, + HelmReleaseUpgradeTimeout: tc.globalUpgrade, + HelmReleaseMaxHistory: 5, + HelmInstallTimeout: tc.override, + }, + } + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) + } + }) + } +} diff --git a/pkg/registry/apps/application/rest_timeout_test.go b/pkg/registry/apps/application/rest_timeout_test.go index 85ac0b974..7c9f8e50b 100644 --- a/pkg/registry/apps/application/rest_timeout_test.go +++ b/pkg/registry/apps/application/rest_timeout_test.go @@ -4,12 +4,26 @@ import ( "testing" "time" + helmv2 "github.com/fluxcd/helm-controller/api/v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1" "github.com/cozystack/cozystack/pkg/config" ) +// Default HelmRelease global durations the api server applies after flag +// parsing. Tests use these so the fixture exercises the same code path the +// production binary takes — i.e. global timeouts are always non-zero. +const ( + testGlobalInstallTimeout = 10 * time.Minute + testGlobalUpgradeTimeout = 10 * time.Minute +) + +// newRESTForTimeout builds a REST struct focused on the per-Application +// HelmInstallTimeout annotation override path. Global HelmRelease* defaults +// are populated with production-shaped values so the unset-annotation case +// exercises "global default applies". The full spec contract is covered by +// rest_helmrelease_spec_test.go. func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *REST { return &REST{ kindName: kind, @@ -20,7 +34,12 @@ func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *R Name: "x", Namespace: "cozy-system", }, - HelmInstallTimeout: helmInstallTimeout, + HelmReleaseInterval: 5 * time.Minute, + HelmReleaseRetryInterval: 30 * time.Second, + HelmReleaseInstallTimeout: testGlobalInstallTimeout, + HelmReleaseUpgradeTimeout: testGlobalUpgradeTimeout, + HelmReleaseMaxHistory: 5, + HelmInstallTimeout: helmInstallTimeout, }, } } @@ -29,41 +48,45 @@ func newRESTForTimeout(kind, prefix string, helmInstallTimeout time.Duration) *R // budget. The Kubernetes kind's parent chart contains CAPI/Kamaji resources // whose admin-kubeconfig Secret is provisioned asynchronously, so its // ApplicationDefinition sets release.cozystack.io/helm-install-timeout=15m -// (or longer). Other kinds leave the annotation unset and keep flux defaults -// so their failed installs remediate on the normal cadence. The test must -// cover both paths: a kind with the timeout set and one without. +// (or longer). Other kinds leave the annotation unset and inherit the +// global default. The test must cover both paths: a kind with the override +// set and one without. func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing.T) { cases := []struct { - name string - kind string - prefix string - configured time.Duration - wantSet bool + name string + kind string + prefix string + configured time.Duration + wantInstall time.Duration + wantUpgrade time.Duration }{ { - name: "Kubernetes kind with 15m configured gets Install and Upgrade Timeout", - kind: "Kubernetes", - prefix: "kubernetes-", - configured: 15 * time.Minute, - wantSet: true, + name: "Kubernetes kind with 15m override beats global defaults", + kind: "Kubernetes", + prefix: "kubernetes-", + configured: 15 * time.Minute, + wantInstall: 15 * time.Minute, + wantUpgrade: 15 * time.Minute, }, { // Fictional kind on purpose: the test is about the unset path // regardless of which real Application kind ends up needing a // timeout override. Using a real kind name would create false // coupling to that Application's ApplicationDefinition. - name: "unrelated kind without configured timeout keeps flux defaults", - kind: "PlaceholderKindForDefaults", - prefix: "placeholder-", - configured: 0, - wantSet: false, + name: "unrelated kind without override inherits global defaults", + kind: "PlaceholderKindForDefaults", + prefix: "placeholder-", + configured: 0, + wantInstall: testGlobalInstallTimeout, + wantUpgrade: testGlobalUpgradeTimeout, }, { - name: "arbitrary future kind with 20m configured gets 20m", - kind: "TalosCluster", - prefix: "talos-", - configured: 20 * time.Minute, - wantSet: true, + name: "arbitrary future kind with 20m override gets 20m", + kind: "TalosCluster", + prefix: "talos-", + configured: 20 * time.Minute, + wantInstall: 20 * time.Minute, + wantUpgrade: 20 * time.Minute, }, } @@ -83,33 +106,92 @@ func TestConvertApplicationToHelmRelease_AppliesReleaseConfigTimeout(t *testing. t.Fatalf("Spec.Install/Upgrade must be non-nil") } - if tc.wantSet { - if hr.Spec.Install.Timeout == nil { - t.Fatalf("Spec.Install.Timeout must be set when HelmInstallTimeout=%v", tc.configured) - } - if hr.Spec.Install.Timeout.Duration != tc.configured { - t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout.Duration, tc.configured) - } - if hr.Spec.Upgrade.Timeout == nil { - t.Fatalf("Spec.Upgrade.Timeout must be set when HelmInstallTimeout=%v", tc.configured) - } - if hr.Spec.Upgrade.Timeout.Duration != tc.configured { - t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout.Duration, tc.configured) - } - } else { - if hr.Spec.Install.Timeout != nil { - t.Errorf("Spec.Install.Timeout must be nil when HelmInstallTimeout=0, got %v", hr.Spec.Install.Timeout.Duration) - } - if hr.Spec.Upgrade.Timeout != nil { - t.Errorf("Spec.Upgrade.Timeout must be nil when HelmInstallTimeout=0, got %v", hr.Spec.Upgrade.Timeout.Duration) - } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) } - if hr.Spec.Install.Remediation == nil || hr.Spec.Install.Remediation.Retries != -1 { - t.Errorf("Spec.Install.Remediation.Retries must remain -1, got %+v", hr.Spec.Install.Remediation) + // Strategy must be RetryOnFailure with Remediation kept nil: + // retries are driven solely by Strategy.RetryInterval, the + // same single retry path cozystack-operator's + // PackageReconciler uses. Re-introducing + // Remediation{Retries: -1} "for safety" would add a second, + // conflicting retry mechanism and break operator parity. + if hr.Spec.Install.Strategy == nil || + hr.Spec.Install.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Spec.Install.Strategy must be RetryOnFailure, got %+v", hr.Spec.Install.Strategy) } - if hr.Spec.Upgrade.Remediation == nil || hr.Spec.Upgrade.Remediation.Retries != -1 { - t.Errorf("Spec.Upgrade.Remediation.Retries must remain -1, got %+v", hr.Spec.Upgrade.Remediation) + if hr.Spec.Install.Remediation != nil { + t.Errorf("Spec.Install.Remediation must be nil, got %+v", hr.Spec.Install.Remediation) + } + if hr.Spec.Upgrade.Strategy == nil || + hr.Spec.Upgrade.Strategy.Name != string(helmv2.ActionStrategyRetryOnFailure) { + t.Errorf("Spec.Upgrade.Strategy must be RetryOnFailure, got %+v", hr.Spec.Upgrade.Strategy) + } + if hr.Spec.Upgrade.Remediation != nil { + t.Errorf("Spec.Upgrade.Remediation must be nil, got %+v", hr.Spec.Upgrade.Remediation) + } + }) + } +} + +// HelmUpgradeTimeout (the per-Application release.cozystack.io/helm-upgrade-timeout +// annotation override) overrides only Upgrade.Timeout, and wins over the +// Upgrade.Timeout value HelmInstallTimeout would otherwise apply. This lets a +// kind carry an asymmetric budget — e.g. a short install but a long upgrade. +func TestConvertApplicationToHelmRelease_UpgradeTimeoutAnnotation(t *testing.T) { + cases := []struct { + name string + installTimeout time.Duration // helm-install-timeout annotation + upgradeTimeout time.Duration // helm-upgrade-timeout annotation + wantInstall time.Duration + wantUpgrade time.Duration + }{ + { + name: "upgrade override alone leaves install on the global default", + installTimeout: 0, + upgradeTimeout: 20 * time.Minute, + wantInstall: testGlobalInstallTimeout, + wantUpgrade: 20 * time.Minute, + }, + { + name: "upgrade override wins over the install-annotation copy", + installTimeout: 15 * time.Minute, + upgradeTimeout: 25 * time.Minute, + wantInstall: 15 * time.Minute, + wantUpgrade: 25 * time.Minute, + }, + { + name: "install override alone still sets both (regression guard)", + installTimeout: 15 * time.Minute, + upgradeTimeout: 0, + wantInstall: 15 * time.Minute, + wantUpgrade: 15 * time.Minute, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := newRESTForTimeout("Example", "example-", tc.installTimeout) + r.releaseConfig.HelmUpgradeTimeout = tc.upgradeTimeout + app := &appsv1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Namespace: "tenant-root"}, + } + + hr, err := r.convertApplicationToHelmRelease(app) + if err != nil { + t.Fatalf("convertApplicationToHelmRelease returned error: %v", err) + } + if hr.Spec.Install == nil || hr.Spec.Upgrade == nil { + t.Fatalf("Spec.Install/Upgrade must be non-nil") + } + if hr.Spec.Install.Timeout == nil || hr.Spec.Install.Timeout.Duration != tc.wantInstall { + t.Errorf("Spec.Install.Timeout = %v, want %v", hr.Spec.Install.Timeout, tc.wantInstall) + } + if hr.Spec.Upgrade.Timeout == nil || hr.Spec.Upgrade.Timeout.Duration != tc.wantUpgrade { + t.Errorf("Spec.Upgrade.Timeout = %v, want %v", hr.Spec.Upgrade.Timeout, tc.wantUpgrade) } }) }