Skip to content

Commit d277843

Browse files
migmartrimigmartri
authored
fix(config): use seconds for workflow run expiration defaults (#3477)
1 parent 5f7f714 commit d277843

8 files changed

Lines changed: 134 additions & 34 deletions

File tree

‎app/controlplane/cmd/main_test.go‎

Lines changed: 111 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package main
1616

1717
import (
18+
"fmt"
1819
"os"
1920
"path/filepath"
2021
"testing"
@@ -26,6 +27,7 @@ import (
2627
"github.com/stretchr/testify/assert"
2728
"github.com/stretchr/testify/require"
2829
"google.golang.org/protobuf/types/known/durationpb"
30+
"sigs.k8s.io/yaml"
2931
)
3032

3133
func TestWorkflowRunExpirerOptsMapping(t *testing.T) {
@@ -92,34 +94,125 @@ func TestWorkflowRunExpirationDurationValidation(t *testing.T) {
9294
}
9395
}
9496

95-
func TestMalformedWorkflowRunExpirationWindow(t *testing.T) {
96-
configPath := filepath.Join(t.TempDir(), "config.yaml")
97-
require.NoError(t, os.WriteFile(configPath, []byte(
98-
"attestations:\n workflow_run_expiration_window: definitely-not-a-duration\n",
99-
), 0o600))
97+
// Duration fields are decoded with protojson, which only accepts seconds with
98+
// an "s" suffix. Go-style units such as "1m" or "1h" are rejected.
99+
func TestWorkflowRunExpirationDurationFormat(t *testing.T) {
100+
testCases := []struct {
101+
name string
102+
config string
103+
wantErr bool
104+
wantWindow time.Duration
105+
wantInterval time.Duration
106+
}{
107+
{
108+
name: "seconds",
109+
config: "attestations:\n workflow_run_expiration_window: 3600s\n workflow_run_expiration_check_interval: 60s\n",
110+
wantWindow: time.Hour,
111+
wantInterval: time.Minute,
112+
},
113+
{
114+
name: "quoted seconds",
115+
config: "attestations:\n workflow_run_expiration_window: \"3600s\"\n workflow_run_expiration_check_interval: \"60s\"\n",
116+
wantWindow: time.Hour,
117+
wantInterval: time.Minute,
118+
},
119+
{
120+
name: "fractional seconds",
121+
config: "attestations:\n workflow_run_expiration_check_interval: 1.5s\n",
122+
wantInterval: 1500 * time.Millisecond,
123+
},
124+
{
125+
name: "window in hours",
126+
config: "attestations:\n workflow_run_expiration_window: 1h\n",
127+
wantErr: true,
128+
},
129+
{
130+
name: "check interval in minutes",
131+
config: "attestations:\n workflow_run_expiration_check_interval: 1m\n",
132+
wantErr: true,
133+
},
134+
{
135+
name: "malformed window",
136+
config: "attestations:\n workflow_run_expiration_window: definitely-not-a-duration\n",
137+
wantErr: true,
138+
},
139+
{
140+
name: "malformed check interval",
141+
config: "attestations:\n workflow_run_expiration_check_interval: definitely-not-a-duration\n",
142+
wantErr: true,
143+
},
144+
}
100145

101-
c := config.New(config.WithSource(file.NewSource(configPath)))
102-
t.Cleanup(func() {
103-
require.NoError(t, c.Close())
104-
})
105-
require.NoError(t, c.Load())
146+
for _, tc := range testCases {
147+
t.Run(tc.name, func(t *testing.T) {
148+
configPath := filepath.Join(t.TempDir(), "config.yaml")
149+
require.NoError(t, os.WriteFile(configPath, []byte(tc.config), 0o600))
106150

107-
var bootstrap conf.Bootstrap
108-
require.Error(t, c.Scan(&bootstrap))
151+
bootstrap, err := loadBootstrap(t, configPath)
152+
if tc.wantErr {
153+
require.Error(t, err)
154+
return
155+
}
156+
157+
require.NoError(t, err)
158+
assert.Equal(t, tc.wantWindow, bootstrap.GetAttestations().GetWorkflowRunExpirationWindow().AsDuration())
159+
assert.Equal(t, tc.wantInterval, bootstrap.GetAttestations().GetWorkflowRunExpirationCheckInterval().AsDuration())
160+
})
161+
}
109162
}
110163

111-
func TestMalformedWorkflowRunExpirationCheckInterval(t *testing.T) {
164+
// The development configuration is loaded by `make run`, so it must parse.
165+
func TestDevelConfigLoads(t *testing.T) {
166+
bootstrap, err := loadBootstrap(t, filepath.Join("..", "configs", "config.devel.yaml"))
167+
require.NoError(t, err)
168+
assert.Equal(t, time.Hour, bootstrap.GetAttestations().GetWorkflowRunExpirationWindow().AsDuration())
169+
assert.Equal(t, time.Minute, bootstrap.GetAttestations().GetWorkflowRunExpirationCheckInterval().AsDuration())
170+
}
171+
172+
// The Helm chart copies these values into the control plane configuration, so
173+
// the chart defaults must use a format that the control plane can parse.
174+
func TestHelmChartWorkflowRunExpirationDefaults(t *testing.T) {
175+
var values struct {
176+
Controlplane struct {
177+
Attestations struct {
178+
WorkflowRunExpirationWindow string `json:"workflowRunExpirationWindow"`
179+
WorkflowRunExpirationCheckInterval string `json:"workflowRunExpirationCheckInterval"`
180+
} `json:"attestations"`
181+
} `json:"controlplane"`
182+
}
183+
184+
raw, err := os.ReadFile(filepath.Join("..", "..", "..", "deployment", "chainloop", "values.yaml"))
185+
require.NoError(t, err)
186+
require.NoError(t, yaml.Unmarshal(raw, &values))
187+
188+
attestations := values.Controlplane.Attestations
189+
// Same shape as templates/controlplane/configmap.yaml
190+
rendered := fmt.Sprintf("attestations:\n workflow_run_expiration_window: %q\n workflow_run_expiration_check_interval: %q\n",
191+
attestations.WorkflowRunExpirationWindow, attestations.WorkflowRunExpirationCheckInterval)
192+
112193
configPath := filepath.Join(t.TempDir(), "config.yaml")
113-
require.NoError(t, os.WriteFile(configPath, []byte(
114-
"attestations:\n workflow_run_expiration_check_interval: definitely-not-a-duration\n",
115-
), 0o600))
194+
require.NoError(t, os.WriteFile(configPath, []byte(rendered), 0o600))
195+
196+
bootstrap, err := loadBootstrap(t, configPath)
197+
require.NoError(t, err)
198+
assert.Equal(t, time.Hour, bootstrap.GetAttestations().GetWorkflowRunExpirationWindow().AsDuration())
199+
assert.Equal(t, time.Minute, bootstrap.GetAttestations().GetWorkflowRunExpirationCheckInterval().AsDuration())
200+
}
201+
202+
// loadBootstrap loads a configuration file the same way as main.
203+
func loadBootstrap(t *testing.T, path string) (*conf.Bootstrap, error) {
204+
t.Helper()
116205

117-
c := config.New(config.WithSource(file.NewSource(configPath)))
206+
c := config.New(config.WithSource(file.NewSource(path)))
118207
t.Cleanup(func() {
119208
require.NoError(t, c.Close())
120209
})
121210
require.NoError(t, c.Load())
122211

123212
var bootstrap conf.Bootstrap
124-
require.Error(t, c.Scan(&bootstrap))
213+
if err := c.Scan(&bootstrap); err != nil {
214+
return nil, err
215+
}
216+
217+
return &bootstrap, nil
125218
}

‎app/controlplane/configs/config.devel.yaml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,8 @@ ui_dashboard_url: http://localhost:3000
125125

126126
attestations:
127127
skip_db_storage: true
128+
# Durations use seconds with an "s" suffix. Units such as 1m or 1h are not supported.
128129
# Unfinished workflow runs older than this are marked as expired.
129-
workflow_run_expiration_window: 1h
130+
workflow_run_expiration_window: 3600s
130131
# How often unfinished workflow runs are checked for expiration.
131-
workflow_run_expiration_check_interval: 1m
132+
workflow_run_expiration_check_interval: 60s

‎app/controlplane/configs/samples/config.yaml‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,5 +103,7 @@ enable_profiler: true
103103
# # Defaults to 1 hour when omitted.
104104
# # Interval between expiration checks for unfinished workflow runs.
105105
# # Defaults to 1 minute when omitted.
106-
# workflow_run_expiration_check_interval: 1m
107-
# workflow_run_expiration_window: 1h
106+
# # Durations use seconds with an "s" suffix (for example 60s). Units
107+
# # such as 1m or 1h are not supported.
108+
# workflow_run_expiration_check_interval: 60s
109+
# workflow_run_expiration_window: 3600s

‎app/controlplane/internal/conf/controlplane/config/v1/conf.pb.go‎

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎app/controlplane/internal/conf/controlplane/config/v1/conf.proto‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,10 +163,12 @@ message Attestations {
163163

164164
// Maximum amount of time an unfinished workflow run can remain active before
165165
// it is marked as expired. When unset, the control plane defaults to 1 hour.
166+
// In YAML configuration, use seconds with an "s" suffix, for example "3600s".
166167
google.protobuf.Duration workflow_run_expiration_window = 3 [(buf.validate.field).duration.gt = {}];
167168

168169
// Interval between checks for unfinished workflow runs. When unset, the
169170
// control plane defaults to 1 minute.
171+
// In YAML configuration, use seconds with an "s" suffix, for example "60s".
170172
google.protobuf.Duration workflow_run_expiration_check_interval = 4 [(buf.validate.field).duration.gt = {}];
171173
}
172174

@@ -241,7 +243,7 @@ message Data {
241243
int32 min_open_conns = 3;
242244
// default max(4, runtime.NumCPU())
243245
int32 max_open_conns = 4;
244-
// default 30 minutes
246+
// default: no limit, idle connections are not closed because of idle time
245247
google.protobuf.Duration max_conn_idle_time = 5;
246248
}
247249
Database database = 1;

‎deployment/chainloop/Chart.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ description: Chainloop is an open source software supply chain control plane, a
77

88
type: application
99
# Bump the patch (not minor, not major) version on each change in the Chart Source code
10-
version: 1.446.0
10+
version: 1.446.1
1111
# Do not update appVersion, this is handled automatically by the release process
1212
appVersion: v1.111.0
1313

‎deployment/chainloop/README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -565,8 +565,8 @@ Once done, you can access with [two predefined users](https://github.com/chainlo
565565
| `controlplane.image.registry` | Image registry | `REGISTRY_NAME` |
566566
| `controlplane.image.repository` | Image repository | `REPOSITORY_NAME` |
567567
| `controlplane.attestations` | Attestation processing configuration | |
568-
| `controlplane.attestations.workflowRunExpirationWindow` | Maximum age of an unfinished workflow run before it is marked as expired | `1h` |
569-
| `controlplane.attestations.workflowRunExpirationCheckInterval` | Interval between checks for unfinished workflow runs | `1m` |
568+
| `controlplane.attestations.workflowRunExpirationWindow` | Maximum age of an unfinished workflow run before it is marked as expired. Use seconds with an "s" suffix, for example "3600s". Units such as "1h" are not supported | `3600s` |
569+
| `controlplane.attestations.workflowRunExpirationCheckInterval` | Interval between checks for unfinished workflow runs. Use seconds with an "s" suffix, for example "60s". Units such as "1m" are not supported | `60s` |
570570
| `controlplane.containerPorts.http` | controlplane HTTP container port | `8000` |
571571
| `controlplane.containerPorts.grpc` | controlplane gRPC container port | `9000` |
572572
| `controlplane.containerPorts.metrics` | controlplane prometheus metrics container port | `5000` |
@@ -603,7 +603,7 @@ Once done, you can access with [two predefined users](https://github.com/chainlo
603603
| `controlplane.externalDatabase.password` | Password for the non-root username | `""` |
604604
| `controlplane.externalDatabase.maxOpenConns` | Maximum number of open connections to the database. Default: max(4, num_cpus) | |
605605
| `controlplane.externalDatabase.minOpenConns` | Min number of connections. Default: 0 | |
606-
| `controlplane.externalDatabase.maxIdleTime` | Max time a connection may be idle. Default: 30m | |
606+
| `controlplane.externalDatabase.maxIdleTime` | Max time a connection may be idle, in seconds with an "s" suffix (for example "300s"). Leave unset for no idle limit | |
607607

608608
### Control Plane Authentication
609609

‎deployment/chainloop/values.yaml‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,11 @@ controlplane:
145145
grpcMaxRecvMsgSize: ""
146146

147147
## @extra controlplane.attestations Attestation processing configuration
148-
## @param controlplane.attestations.workflowRunExpirationWindow Maximum age of an unfinished workflow run before it is marked as expired
149-
## @param controlplane.attestations.workflowRunExpirationCheckInterval Interval between checks for unfinished workflow runs
148+
## @param controlplane.attestations.workflowRunExpirationWindow Maximum age of an unfinished workflow run before it is marked as expired. Use seconds with an "s" suffix, for example "3600s". Units such as "1h" are not supported
149+
## @param controlplane.attestations.workflowRunExpirationCheckInterval Interval between checks for unfinished workflow runs. Use seconds with an "s" suffix, for example "60s". Units such as "1m" are not supported
150150
attestations:
151-
workflowRunExpirationWindow: "1h"
152-
workflowRunExpirationCheckInterval: "1m"
151+
workflowRunExpirationWindow: "3600s"
152+
workflowRunExpirationCheckInterval: "60s"
153153

154154
## @param controlplane.containerPorts.http controlplane HTTP container port
155155
## @param controlplane.containerPorts.grpc controlplane gRPC container port
@@ -262,7 +262,7 @@ controlplane:
262262
## @param controlplane.externalDatabase.password Password for the non-root username
263263
## @extra controlplane.externalDatabase.maxOpenConns Maximum number of open connections to the database. Default: max(4, num_cpus)
264264
## @extra controlplane.externalDatabase.minOpenConns Min number of connections. Default: 0
265-
## @extra controlplane.externalDatabase.maxIdleTime Max time a connection may be idle. Default: 30m
265+
## @extra controlplane.externalDatabase.maxIdleTime Max time a connection may be idle, in seconds with an "s" suffix (for example "300s"). Leave unset for no idle limit
266266
##
267267
externalDatabase:
268268
host: ""
@@ -272,7 +272,7 @@ controlplane:
272272
password: ""
273273
# maxOpenConns: 50
274274
# minOpenConns: 5
275-
# maxIdleTime: 5m
275+
# maxIdleTime: 300s
276276

277277
## @section Control Plane Authentication
278278
auth:

0 commit comments

Comments
 (0)