Skip to content

Commit 0f40ed9

Browse files
myasnikovdaniilmyasnikovdaniil
committed
refactor(api): align HelmRelease conversion with operator, add validator tests
Production change: - convertApplicationToHelmRelease now sets Install/Upgrade.Timeout unconditionally (matching cozystack-operator's buildHelmReleaseSpec). The previous "stay nil if zero" branch was a leaky test shim — it kept fixtures with empty ReleaseConfig{} green at the cost of api/operator drift. Test changes: - newRESTForTimeout seeds the new HelmRelease* globals with production-shaped defaults so the unset-annotation case exercises "global default applies" rather than "field stays nil". - TestConvertApplicationToHelmRelease_PerAppTimeoutOverridesGlobal is table-driven and now covers asymmetric globals (install != upgrade) with and without per-app override — guards against a regression that only assigns one side under override. - New start_test.go: table-driven validator coverage for each malformed/non-positive duration flag, the negative MaxHistory branch, the MaxHistory=0 unlimited branch, asymmetric timeout acceptance, and a smoke test that NewCozyServerOptions defaults validate. Refactor: - Extracted parseAndValidateHelmReleaseFlags from Complete() so the flag-validation portion is unit-testable without kubernetes I/O. Documentation: - Reworded --helmrelease-install/upgrade-timeout help so admins reading --help see the dual-purpose annotation explicitly (one annotation overrides both timeouts together). - Dropped PR-#2509 references and "preserving the pre-#2509 behaviour for tests" comments — kept the technical rationale (Strategy.Name vs Remediation.Retries:-1 + 5m Interval coupling) on its own merits. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent b10a8cc commit 0f40ed9

5 files changed

Lines changed: 365 additions & 140 deletions

File tree

‎pkg/cmd/server/start.go‎

Lines changed: 52 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,8 @@ type CozyServerOptions struct {
6161
// Raw HelmRelease generation flag values; parsed and validated in
6262
// Complete() with config.ParsePositiveDuration so a misconfigured flag
6363
// fails fast at startup. Kept in sync with the cozystack-operator
64-
// flags of the same name (PR #2509) so both HelmRelease-generating
65-
// paths share the same retry strategy and cadence.
64+
// flags of the same name so both HelmRelease-generating paths share
65+
// the same retry strategy and cadence.
6666
HelmReleaseInterval string
6767
HelmReleaseRetryInterval string
6868
HelmReleaseInstallTimeout string
@@ -119,12 +119,11 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions)
119119
o.RecommendedOptions.AddFlags(flags)
120120

121121
// HelmRelease generation knobs. Names, defaults, and validation match
122-
// the cozystack-operator flags from PR #2509 so both HelmRelease-
123-
// generating paths can be tuned together.
122+
// the cozystack-operator flags of the same name so both
123+
// HelmRelease-generating paths can be tuned together.
124124
flags.StringVar(&o.HelmReleaseInterval, "helmrelease-interval", o.HelmReleaseInterval,
125125
"Reconcile interval applied to HelmReleases generated from Application resources "+
126-
"(Spec.Interval). Mirrors the cozystack-operator flag of the same name; same default 5m "+
127-
"matches existing behaviour.")
126+
"(Spec.Interval). Mirrors the cozystack-operator flag of the same name.")
128127
flags.StringVar(&o.HelmReleaseRetryInterval, "helmrelease-retry-interval", o.HelmReleaseRetryInterval,
129128
"Retry interval applied to Install.Strategy and Upgrade.Strategy of HelmReleases generated "+
130129
"from Application resources. With Strategy.Name=RetryOnFailure, this controls how long "+
@@ -134,11 +133,13 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions)
134133
flags.StringVar(&o.HelmReleaseInstallTimeout, "helmrelease-install-timeout", o.HelmReleaseInstallTimeout,
135134
"Default timeout for the Helm install action of HelmReleases generated from Application "+
136135
"resources (Spec.Install.Timeout). Overridden per-Application by the "+
137-
"release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.")
136+
"release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition; the "+
137+
"same annotation also overrides --helmrelease-upgrade-timeout.")
138138
flags.StringVar(&o.HelmReleaseUpgradeTimeout, "helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout,
139139
"Default timeout for the Helm upgrade action of HelmReleases generated from Application "+
140-
"resources (Spec.Upgrade.Timeout). Overridden per-Application by the "+
141-
"release.cozystack.io/helm-install-timeout annotation on the ApplicationDefinition.")
140+
"resources (Spec.Upgrade.Timeout). Overridden per-Application by the same "+
141+
"release.cozystack.io/helm-install-timeout annotation as --helmrelease-install-timeout: "+
142+
"one annotation overrides both install and upgrade timeouts together.")
142143
flags.IntVar(&o.HelmReleaseMaxHistory, "helmrelease-max-history", o.HelmReleaseMaxHistory,
143144
"Number of release revisions Helm keeps for HelmReleases generated from Application "+
144145
"resources (Spec.MaxHistory). 0 means unlimited; 5 matches Helm's default.")
@@ -149,29 +150,48 @@ func NewCommandStartCozyServer(ctx context.Context, defaults *CozyServerOptions)
149150
return cmd
150151
}
151152

152-
// Complete fills in the fields that are not set
153-
func (o *CozyServerOptions) Complete() error {
154-
// Validate the HelmRelease generation flags before any kubernetes I/O
155-
// so a misconfigured operator restarts loudly instead of waiting until
156-
// the first Application is created. Same shape as cozystack-operator.
157-
hrInterval, err := config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval)
158-
if err != nil {
159-
return err
153+
// helmReleaseFlagValues holds the parsed, validated HelmRelease generation
154+
// flags. Populated by parseAndValidateHelmReleaseFlags before any kubernetes
155+
// I/O so a misconfigured server restarts loudly instead of waiting until the
156+
// first Application is created.
157+
type helmReleaseFlagValues struct {
158+
interval time.Duration
159+
retryInterval time.Duration
160+
installTimeout time.Duration
161+
upgradeTimeout time.Duration
162+
maxHistory int
163+
}
164+
165+
// parseAndValidateHelmReleaseFlags parses the five HelmRelease generation
166+
// flags and rejects malformed or non-positive durations and negative
167+
// MaxHistory. Same shape as cozystack-operator's main.
168+
func (o *CozyServerOptions) parseAndValidateHelmReleaseFlags() (helmReleaseFlagValues, error) {
169+
var v helmReleaseFlagValues
170+
var err error
171+
if v.interval, err = config.ParsePositiveDuration("--helmrelease-interval", o.HelmReleaseInterval); err != nil {
172+
return v, err
160173
}
161-
hrRetryInterval, err := config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval)
162-
if err != nil {
163-
return err
174+
if v.retryInterval, err = config.ParsePositiveDuration("--helmrelease-retry-interval", o.HelmReleaseRetryInterval); err != nil {
175+
return v, err
164176
}
165-
hrInstallTimeout, err := config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout)
166-
if err != nil {
167-
return err
177+
if v.installTimeout, err = config.ParsePositiveDuration("--helmrelease-install-timeout", o.HelmReleaseInstallTimeout); err != nil {
178+
return v, err
168179
}
169-
hrUpgradeTimeout, err := config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout)
170-
if err != nil {
171-
return err
180+
if v.upgradeTimeout, err = config.ParsePositiveDuration("--helmrelease-upgrade-timeout", o.HelmReleaseUpgradeTimeout); err != nil {
181+
return v, err
172182
}
173183
if o.HelmReleaseMaxHistory < 0 {
174-
return fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory)
184+
return v, fmt.Errorf("--helmrelease-max-history must be >= 0 (got %d)", o.HelmReleaseMaxHistory)
185+
}
186+
v.maxHistory = o.HelmReleaseMaxHistory
187+
return v, nil
188+
}
189+
190+
// Complete fills in the fields that are not set
191+
func (o *CozyServerOptions) Complete() error {
192+
hrFlags, err := o.parseAndValidateHelmReleaseFlags()
193+
if err != nil {
194+
return err
175195
}
176196

177197
scheme := runtime.NewScheme()
@@ -239,11 +259,11 @@ func (o *CozyServerOptions) Complete() error {
239259
// matching cozystack-operator's PackageReconciler. The
240260
// per-Application HelmInstallTimeout annotation populated below
241261
// still wins over HelmReleaseInstallTimeout/UpgradeTimeout.
242-
HelmReleaseInterval: hrInterval,
243-
HelmReleaseRetryInterval: hrRetryInterval,
244-
HelmReleaseInstallTimeout: hrInstallTimeout,
245-
HelmReleaseUpgradeTimeout: hrUpgradeTimeout,
246-
HelmReleaseMaxHistory: o.HelmReleaseMaxHistory,
262+
HelmReleaseInterval: hrFlags.interval,
263+
HelmReleaseRetryInterval: hrFlags.retryInterval,
264+
HelmReleaseInstallTimeout: hrFlags.installTimeout,
265+
HelmReleaseUpgradeTimeout: hrFlags.upgradeTimeout,
266+
HelmReleaseMaxHistory: hrFlags.maxHistory,
247267
}
248268
// Per-Application HelmRelease Install/Upgrade timeout. Applications
249269
// whose parent chart contains asynchronously-provisioned resources

‎pkg/cmd/server/start_test.go‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,172 @@ limitations under the License.
1515
*/
1616

1717
package server
18+
19+
import (
20+
"strings"
21+
"testing"
22+
"time"
23+
)
24+
25+
// validHelmReleaseFlags returns a CozyServerOptions populated with the same
26+
// defaults NewCozyServerOptions sets, so each test case mutates a single
27+
// flag and asserts the resulting validation behaviour in isolation.
28+
func validHelmReleaseFlags() *CozyServerOptions {
29+
return &CozyServerOptions{
30+
HelmReleaseInterval: "5m",
31+
HelmReleaseRetryInterval: "30s",
32+
HelmReleaseInstallTimeout: "10m",
33+
HelmReleaseUpgradeTimeout: "10m",
34+
HelmReleaseMaxHistory: 5,
35+
}
36+
}
37+
38+
// Pins that each malformed/non-positive duration and a negative MaxHistory
39+
// is rejected at startup. Without this, a regression that swaps `<` for
40+
// `<=` on the MaxHistory branch — breaking the documented "0 means
41+
// unlimited" contract — or that drops a `return err` line on any of the
42+
// four duration branches would ship green.
43+
func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Rejects(t *testing.T) {
44+
cases := []struct {
45+
name string
46+
mutate func(o *CozyServerOptions)
47+
wantSubs string
48+
}{
49+
{
50+
name: "interval zero",
51+
mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "0s" },
52+
wantSubs: "--helmrelease-interval",
53+
},
54+
{
55+
name: "interval negative",
56+
mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "-5m" },
57+
wantSubs: "--helmrelease-interval",
58+
},
59+
{
60+
name: "interval malformed",
61+
mutate: func(o *CozyServerOptions) { o.HelmReleaseInterval = "5x" },
62+
wantSubs: "--helmrelease-interval",
63+
},
64+
{
65+
name: "retry-interval empty",
66+
mutate: func(o *CozyServerOptions) { o.HelmReleaseRetryInterval = "" },
67+
wantSubs: "--helmrelease-retry-interval",
68+
},
69+
{
70+
name: "install-timeout zero",
71+
mutate: func(o *CozyServerOptions) { o.HelmReleaseInstallTimeout = "0s" },
72+
wantSubs: "--helmrelease-install-timeout",
73+
},
74+
{
75+
name: "upgrade-timeout malformed",
76+
mutate: func(o *CozyServerOptions) { o.HelmReleaseUpgradeTimeout = "tenminutes" },
77+
wantSubs: "--helmrelease-upgrade-timeout",
78+
},
79+
{
80+
name: "max-history negative",
81+
mutate: func(o *CozyServerOptions) { o.HelmReleaseMaxHistory = -1 },
82+
wantSubs: "--helmrelease-max-history must be >= 0",
83+
},
84+
}
85+
86+
for _, tc := range cases {
87+
t.Run(tc.name, func(t *testing.T) {
88+
o := validHelmReleaseFlags()
89+
tc.mutate(o)
90+
_, err := o.parseAndValidateHelmReleaseFlags()
91+
if err == nil {
92+
t.Fatalf("expected error, got nil")
93+
}
94+
if !strings.Contains(err.Error(), tc.wantSubs) {
95+
t.Errorf("error %q does not contain %q", err.Error(), tc.wantSubs)
96+
}
97+
})
98+
}
99+
}
100+
101+
// Pins the happy path: valid flag strings parse to the expected
102+
// time.Duration values and MaxHistory=0 is accepted (the documented
103+
// "unlimited per Helm semantics" branch).
104+
func TestCozyServerOptions_ParseAndValidateHelmReleaseFlags_Accepts(t *testing.T) {
105+
cases := []struct {
106+
name string
107+
o *CozyServerOptions
108+
wantInterval time.Duration
109+
wantRetry time.Duration
110+
wantInstall time.Duration
111+
wantUpgrade time.Duration
112+
wantMaxHistory int
113+
}{
114+
{
115+
name: "production defaults",
116+
o: validHelmReleaseFlags(),
117+
wantInterval: 5 * time.Minute,
118+
wantRetry: 30 * time.Second,
119+
wantInstall: 10 * time.Minute,
120+
wantUpgrade: 10 * time.Minute,
121+
wantMaxHistory: 5,
122+
},
123+
{
124+
name: "max-history zero accepted as unlimited",
125+
o: func() *CozyServerOptions {
126+
o := validHelmReleaseFlags()
127+
o.HelmReleaseMaxHistory = 0
128+
return o
129+
}(),
130+
wantInterval: 5 * time.Minute,
131+
wantRetry: 30 * time.Second,
132+
wantInstall: 10 * time.Minute,
133+
wantUpgrade: 10 * time.Minute,
134+
wantMaxHistory: 0,
135+
},
136+
{
137+
name: "asymmetric install/upgrade timeouts both honored",
138+
o: func() *CozyServerOptions {
139+
o := validHelmReleaseFlags()
140+
o.HelmReleaseInstallTimeout = "8m"
141+
o.HelmReleaseUpgradeTimeout = "12m"
142+
return o
143+
}(),
144+
wantInterval: 5 * time.Minute,
145+
wantRetry: 30 * time.Second,
146+
wantInstall: 8 * time.Minute,
147+
wantUpgrade: 12 * time.Minute,
148+
wantMaxHistory: 5,
149+
},
150+
}
151+
152+
for _, tc := range cases {
153+
t.Run(tc.name, func(t *testing.T) {
154+
v, err := tc.o.parseAndValidateHelmReleaseFlags()
155+
if err != nil {
156+
t.Fatalf("unexpected error: %v", err)
157+
}
158+
if v.interval != tc.wantInterval {
159+
t.Errorf("interval = %v, want %v", v.interval, tc.wantInterval)
160+
}
161+
if v.retryInterval != tc.wantRetry {
162+
t.Errorf("retryInterval = %v, want %v", v.retryInterval, tc.wantRetry)
163+
}
164+
if v.installTimeout != tc.wantInstall {
165+
t.Errorf("installTimeout = %v, want %v", v.installTimeout, tc.wantInstall)
166+
}
167+
if v.upgradeTimeout != tc.wantUpgrade {
168+
t.Errorf("upgradeTimeout = %v, want %v", v.upgradeTimeout, tc.wantUpgrade)
169+
}
170+
if v.maxHistory != tc.wantMaxHistory {
171+
t.Errorf("maxHistory = %d, want %d", v.maxHistory, tc.wantMaxHistory)
172+
}
173+
})
174+
}
175+
}
176+
177+
// Pins that NewCozyServerOptions seeds defaults that pass validation, so
178+
// `cozystack-api ...` with no HelmRelease flags overridden does not fail
179+
// at startup. Guards against a future change to defaults that accidentally
180+
// violates the validator.
181+
func TestNewCozyServerOptions_DefaultsValidate(t *testing.T) {
182+
o := NewCozyServerOptions(nil, nil)
183+
if _, err := o.parseAndValidateHelmReleaseFlags(); err != nil {
184+
t.Fatalf("default options must validate, got: %v", err)
185+
}
186+
}

‎pkg/registry/apps/application/rest.go‎

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1489,15 +1489,34 @@ func latestMonitorTime(m *cozyv1alpha1.WorkloadMonitor) metav1.Time {
14891489
// convertApplicationToHelmRelease implements the actual conversion logic.
14901490
//
14911491
// Spec.Interval, Install/Upgrade.Strategy{Name=RetryOnFailure,RetryInterval},
1492-
// and Spec.MaxHistory are populated from ReleaseConfig fields fed by the
1493-
// cozystack-api server flags (--helmrelease-interval,
1494-
// --helmrelease-retry-interval, --helmrelease-max-history). This mirrors the
1495-
// operator's PackageReconciler.buildHelmReleaseSpec so both HelmRelease-
1496-
// generating paths share the same retry strategy, history retention, and
1497-
// reconcile cadence — see cozystack-operator's PR #2509 for the rationale
1498-
// (recurring 2-min E2E install timeouts caused by Remediation{Retries:-1}
1499-
// being coupled to a 5m Interval).
1492+
// Install/Upgrade.Timeout, and Spec.MaxHistory are populated from
1493+
// ReleaseConfig fields fed by the cozystack-api server flags
1494+
// (--helmrelease-{interval,retry-interval,install-timeout,upgrade-timeout,
1495+
// max-history}). This mirrors cozystack-operator's
1496+
// PackageReconciler.buildHelmReleaseSpec so both HelmRelease-generating paths
1497+
// share the same retry strategy, history retention, and reconcile cadence.
1498+
//
1499+
// Strategy.Name=RetryOnFailure with an explicit RetryInterval (rather than
1500+
// Remediation{Retries:-1}) decouples failed install/upgrade retry timing
1501+
// from Spec.Interval — the previous coupling caused failed installs to
1502+
// retry only every 5m, exceeding E2E budgets.
15001503
func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*helmv2.HelmRelease, error) {
1504+
// Per-Application annotation override (HelmInstallTimeout, populated
1505+
// from release.cozystack.io/helm-install-timeout on the
1506+
// ApplicationDefinition at startup) wins over the global defaults
1507+
// (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). Today only
1508+
// kubernetes-rd carries the annotation because the Kubernetes
1509+
// Application's parent chart contains CAPI/Kamaji resources whose
1510+
// admin-kubeconfig Secret is provisioned asynchronously and Kamaji
1511+
// cold-start routinely exceeds flux's default wait budget; any future
1512+
// kind with the same shape can opt in by setting the same annotation.
1513+
installTimeout := r.releaseConfig.HelmReleaseInstallTimeout
1514+
upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout
1515+
if r.releaseConfig.HelmInstallTimeout > 0 {
1516+
installTimeout = r.releaseConfig.HelmInstallTimeout
1517+
upgradeTimeout = r.releaseConfig.HelmInstallTimeout
1518+
}
1519+
15011520
maxHistory := r.releaseConfig.HelmReleaseMaxHistory
15021521
helmRelease := &helmv2.HelmRelease{
15031522
TypeMeta: metav1.TypeMeta{
@@ -1521,12 +1540,14 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*
15211540
Interval: metav1.Duration{Duration: r.releaseConfig.HelmReleaseInterval},
15221541
MaxHistory: &maxHistory,
15231542
Install: &helmv2.Install{
1543+
Timeout: &metav1.Duration{Duration: installTimeout},
15241544
Strategy: &helmv2.InstallStrategy{
15251545
Name: string(helmv2.ActionStrategyRetryOnFailure),
15261546
RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval},
15271547
},
15281548
},
15291549
Upgrade: &helmv2.Upgrade{
1550+
Timeout: &metav1.Duration{Duration: upgradeTimeout},
15301551
Strategy: &helmv2.UpgradeStrategy{
15311552
Name: string(helmv2.ActionStrategyRetryOnFailure),
15321553
RetryInterval: &metav1.Duration{Duration: r.releaseConfig.HelmReleaseRetryInterval},
@@ -1542,35 +1563,6 @@ func (r *REST) convertApplicationToHelmRelease(app *appsv1alpha1.Application) (*
15421563
},
15431564
}
15441565

1545-
// Install/Upgrade.Timeout: per-Application annotation override
1546-
// (HelmInstallTimeout) wins over the global default
1547-
// (HelmReleaseInstallTimeout / HelmReleaseUpgradeTimeout). When both
1548-
// are zero the field stays nil and flux defaults apply — preserving
1549-
// the pre-#2509 behaviour for tests that hand-construct ReleaseConfig
1550-
// without populating the new global fields.
1551-
//
1552-
// The annotation mechanism is generic: an ApplicationDefinition that
1553-
// sets release.cozystack.io/helm-install-timeout gets Install.Timeout
1554-
// and Upgrade.Timeout populated from ReleaseConfig.HelmInstallTimeout
1555-
// (parsed at startup). Today only kubernetes-rd carries the annotation
1556-
// because the Kubernetes Application's parent chart contains
1557-
// CAPI/Kamaji resources whose admin-kubeconfig Secret is provisioned
1558-
// asynchronously and Kamaji cold-start routinely exceeds flux's
1559-
// default wait budget; any future kind with the same shape can opt in
1560-
// by setting the same annotation.
1561-
installTimeout := r.releaseConfig.HelmReleaseInstallTimeout
1562-
upgradeTimeout := r.releaseConfig.HelmReleaseUpgradeTimeout
1563-
if r.releaseConfig.HelmInstallTimeout > 0 {
1564-
installTimeout = r.releaseConfig.HelmInstallTimeout
1565-
upgradeTimeout = r.releaseConfig.HelmInstallTimeout
1566-
}
1567-
if installTimeout > 0 {
1568-
helmRelease.Spec.Install.Timeout = &metav1.Duration{Duration: installTimeout}
1569-
}
1570-
if upgradeTimeout > 0 {
1571-
helmRelease.Spec.Upgrade.Timeout = &metav1.Duration{Duration: upgradeTimeout}
1572-
}
1573-
15741566
return helmRelease, nil
15751567
}
15761568

0 commit comments

Comments
 (0)