Skip to content

Commit 6eb91df

Browse files
joaopapereirajoaopapereira
authored
Merge pull request #1259 from imusmanmalik/feat/defaultPackageInstallSyncPeriod
feat: Configurable default package install sync period
2 parents fdf9e62 + 8df5644 commit 6eb91df

File tree

12 files changed

+388
-31
lines changed

12 files changed

+388
-31
lines changed

‎cmd/controller/run.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ func Run(opts Options, runLog logr.Logger) error {
226226
pkgToPkgInstallHandler := pkginstall.NewPackageInstallVersionHandler(
227227
kcClient, opts.PackagingGlobalNS, runLog.WithName("handler"))
228228

229-
reconciler := pkginstall.NewReconciler(kcClient, pkgClient, coreClient, pkgToPkgInstallHandler, runLog.WithName("pkgi"), compInfo)
229+
reconciler := pkginstall.NewReconciler(kcClient, pkgClient, coreClient, pkgToPkgInstallHandler, runLog.WithName("pkgi"), compInfo, kcConfig)
230230

231231
ctrl, err := controller.New("pkgi", mgr, controller.Options{
232232
Reconciler: reconciler,

‎pkg/config/config.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ type configData struct {
3838
kappDeployRawOptions []string
3939
skipTLSVerify string
4040

41-
appDefaultSyncPeriod time.Duration
42-
appMinimumSyncPeriod time.Duration
41+
appDefaultSyncPeriod time.Duration
42+
appMinimumSyncPeriod time.Duration
43+
packageInstallDefaultSyncPeriod time.Duration
4344
}
4445

4546
const (
@@ -191,6 +192,20 @@ func (gc *Config) AppMinimumSyncPeriod() time.Duration {
191192
return min
192193
}
193194

195+
// PackageInstallDefaultSyncPeriod returns duration that is used by Apps
196+
// that do not explicitly specify sync period.
197+
func (gc *Config) PackageInstallDefaultSyncPeriod() time.Duration {
198+
const defaultSyncPeriod = 10 * time.Minute
199+
const minDefaultSyncPeriod = 30 * time.Second
200+
if gc.data.packageInstallDefaultSyncPeriod != 0 {
201+
if gc.data.packageInstallDefaultSyncPeriod > minDefaultSyncPeriod {
202+
return gc.data.packageInstallDefaultSyncPeriod
203+
}
204+
return minDefaultSyncPeriod
205+
}
206+
return defaultSyncPeriod
207+
}
208+
194209
func (gc *Config) addSecretDataToConfig(secret *v1.Secret) error {
195210
extractedValues := map[string]string{}
196211
for key, value := range secret.Data {
@@ -218,6 +233,14 @@ func (gc *Config) addDataToConfig(rawData map[string]string) error {
218233
data.appDefaultSyncPeriod = dur
219234
}
220235

236+
if val := rawData["packageInstallDefaultSyncPeriod"]; len(val) > 0 {
237+
dur, err := time.ParseDuration(val)
238+
if err != nil {
239+
return fmt.Errorf("Unmarshaling packageInstallDefaultSyncPeriod as duration: %s", err)
240+
}
241+
data.packageInstallDefaultSyncPeriod = dur
242+
}
243+
221244
if val := rawData["appMinimumSyncPeriod"]; len(val) > 0 {
222245
dur, err := time.ParseDuration(val)
223246
if err != nil {

‎pkg/config/config_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,36 @@ func Test_NewConfig_ReturnsSecret_WhenBothConfigMapAndSecretExist(t *testing.T)
4545
}, config.ProxyOpts())
4646
}
4747

48+
func Test_NewConfig_PackageInstallDefaultSyncPeriod(t *testing.T) {
49+
t.Run("with empty config value, returns 10m", func(t *testing.T) {
50+
secret := &v1.Secret{
51+
ObjectMeta: metav1.ObjectMeta{
52+
Name: "kapp-controller-config",
53+
Namespace: "default",
54+
},
55+
Data: map[string][]byte{},
56+
}
57+
config, err := kcconfig.NewConfig(k8sfake.NewSimpleClientset(secret))
58+
assert.NoError(t, err)
59+
assert.Equal(t, 10*time.Minute, config.PackageInstallDefaultSyncPeriod())
60+
})
61+
62+
t.Run("with value, returns 80s", func(t *testing.T) {
63+
secret := &v1.Secret{
64+
ObjectMeta: metav1.ObjectMeta{
65+
Name: "kapp-controller-config",
66+
Namespace: "default",
67+
},
68+
Data: map[string][]byte{
69+
"packageInstallDefaultSyncPeriod": []byte("80s"),
70+
},
71+
}
72+
config, err := kcconfig.NewConfig(k8sfake.NewSimpleClientset(secret))
73+
assert.NoError(t, err)
74+
assert.Equal(t, 80*time.Second, config.PackageInstallDefaultSyncPeriod())
75+
})
76+
}
77+
4878
func Test_NewConfig_AppDefaultSyncPeriod(t *testing.T) {
4979
t.Run("with empty config value, returns 30s", func(t *testing.T) {
5080
secret := &v1.Secret{

‎pkg/packageinstall/app.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"fmt"
88
"sort"
99
"strings"
10-
"time"
1110

1211
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
1312
kcv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
@@ -33,7 +32,9 @@ const (
3332
ExtFetchSecretNameAnnKeyFmt = "ext.packaging.carvel.dev/fetch-%d-secret-name"
3433
)
3534

36-
func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall, pkgVersion datapkgingv1alpha1.Package) (*v1alpha1.App, error) {
35+
// NewApp creates a new instance of v1alpha1.App based on the provided parameters.
36+
// It takes an existingApp, pkgInstall, pkgVersion, and opts, and returns the newly created App.
37+
func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall, pkgVersion datapkgingv1alpha1.Package, opts Opts) (*v1alpha1.App, error) {
3738
desiredApp := existingApp.DeepCopy()
3839

3940
if _, found := existingApp.Annotations[ManuallyControlledAnnKey]; found {
@@ -53,7 +54,7 @@ func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall
5354
desiredApp.Spec = *pkgVersion.Spec.Template.Spec
5455
desiredApp.Spec.ServiceAccountName = pkgInstall.Spec.ServiceAccountName
5556
if pkgInstall.Spec.SyncPeriod == nil {
56-
desiredApp.Spec.SyncPeriod = &metav1.Duration{Duration: time.Minute * 10}
57+
desiredApp.Spec.SyncPeriod = &metav1.Duration{Duration: opts.DefaultSyncPeriod}
5758
} else {
5859
desiredApp.Spec.SyncPeriod = pkgInstall.Spec.SyncPeriod
5960
}

‎pkg/packageinstall/app_test.go‎

Lines changed: 81 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
)
1919

2020
// several tests below have no SyncPeriod set so they'll all use the same default.
21-
var defaultSyncPeriod metav1.Duration = metav1.Duration{10 * time.Minute}
21+
var defaultSyncPeriod metav1.Duration = metav1.Duration{Duration: 10 * time.Minute}
2222

2323
func TestAppExtPathsFromSecretNameAnn(t *testing.T) {
2424
ipkg := &pkgingv1alpha1.PackageInstall{
@@ -55,7 +55,7 @@ func TestAppExtPathsFromSecretNameAnn(t *testing.T) {
5555
},
5656
}
5757

58-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
58+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
5959
if err != nil {
6060
t.Fatalf("Expected no err, but was: %s", err)
6161
}
@@ -167,7 +167,7 @@ func TestAppHelmOverlaysFromAnn(t *testing.T) {
167167
},
168168
}
169169

170-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
170+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
171171
if err != nil {
172172
t.Fatalf("Expected no err, but was: %s", err)
173173
}
@@ -239,7 +239,7 @@ func TestAppExtYttDataValuesOverlaysAnn(t *testing.T) {
239239
},
240240
}
241241

242-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
242+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
243243
if err != nil {
244244
t.Fatalf("Expected no err, but was: %s", err)
245245
}
@@ -309,7 +309,7 @@ func TestAppYttValues(t *testing.T) {
309309
},
310310
}
311311

312-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
312+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
313313
if err != nil {
314314
t.Fatalf("Expected no err, but was: %s", err)
315315
}
@@ -378,7 +378,7 @@ func TestAppHelmTemplateValues(t *testing.T) {
378378
},
379379
}
380380

381-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
381+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
382382
if err != nil {
383383
t.Fatalf("Expected no err, but was: %s", err)
384384
}
@@ -458,7 +458,7 @@ func TestAppManuallyControlled(t *testing.T) {
458458
},
459459
}
460460

461-
app, err := packageinstall.NewApp(existingApp, ipkg, pkgVersion)
461+
app, err := packageinstall.NewApp(existingApp, ipkg, pkgVersion, packageinstall.Opts{})
462462
if err != nil {
463463
t.Fatalf("Expected no err, but was: %s", err)
464464
}
@@ -523,7 +523,7 @@ func TestAppCustomFetchSecretNames(t *testing.T) {
523523
},
524524
}
525525

526-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
526+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{})
527527
require.NoError(t, err)
528528

529529
expectedApp := &kcv1alpha1.App{
@@ -563,6 +563,78 @@ func TestAppCustomFetchSecretNames(t *testing.T) {
563563
require.Equal(t, expectedApp, app, "App does not match expected app")
564564
}
565565

566+
// TestAppPackageIntallDefaultSyncPeriod tests the creation of an App and expects syncPeriod is set to the default value.
567+
func TestAppPackageIntallDefaultSyncPeriod(t *testing.T) {
568+
ipkg := &pkgingv1alpha1.PackageInstall{
569+
ObjectMeta: metav1.ObjectMeta{
570+
Name: "app",
571+
Namespace: "default",
572+
},
573+
}
574+
575+
pkgVersion := datapkgingv1alpha1.Package{
576+
Spec: datapkgingv1alpha1.PackageSpec{
577+
RefName: "expec-pkg",
578+
Version: "1.5.0",
579+
Template: datapkgingv1alpha1.AppTemplateSpec{
580+
Spec: &kcv1alpha1.AppSpec{},
581+
},
582+
},
583+
}
584+
585+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
586+
require.NoError(t, err)
587+
588+
// Define the expected app object, with the sync period attribute set to the default value
589+
expectedApp := &kcv1alpha1.App{
590+
Spec: kcv1alpha1.AppSpec{
591+
SyncPeriod: &defaultSyncPeriod,
592+
},
593+
}
594+
595+
// Not interested in metadata in this test
596+
app.ObjectMeta = metav1.ObjectMeta{}
597+
598+
require.Equal(t, expectedApp, app, "App does not match expected app")
599+
}
600+
601+
// TestAppCustomPackageIntallSyncPeriod tests the creation of an App when using PackageInstall with a defined syncPeriod.
602+
func TestAppCustomPackageIntallSyncPeriod(t *testing.T) {
603+
ipkg := &pkgingv1alpha1.PackageInstall{
604+
ObjectMeta: metav1.ObjectMeta{
605+
Name: "app",
606+
Namespace: "default",
607+
},
608+
Spec: pkgingv1alpha1.PackageInstallSpec{
609+
SyncPeriod: &metav1.Duration{Duration: 100 * time.Second},
610+
},
611+
}
612+
613+
pkgVersion := datapkgingv1alpha1.Package{
614+
Spec: datapkgingv1alpha1.PackageSpec{
615+
RefName: "expec-pkg",
616+
Version: "1.5.0",
617+
Template: datapkgingv1alpha1.AppTemplateSpec{
618+
Spec: &kcv1alpha1.AppSpec{},
619+
},
620+
},
621+
}
622+
623+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{})
624+
require.NoError(t, err)
625+
626+
expectedApp := &kcv1alpha1.App{
627+
Spec: kcv1alpha1.AppSpec{
628+
SyncPeriod: &metav1.Duration{Duration: 100 * time.Second},
629+
},
630+
}
631+
632+
// Not interested in metadata in this test
633+
app.ObjectMeta = metav1.ObjectMeta{}
634+
635+
require.Equal(t, expectedApp, app, "App does not match expected app")
636+
}
637+
566638
func TestAppPackageDetailsAnnotations(t *testing.T) {
567639
ipkg := &pkgingv1alpha1.PackageInstall{
568640
ObjectMeta: metav1.ObjectMeta{
@@ -582,7 +654,7 @@ func TestAppPackageDetailsAnnotations(t *testing.T) {
582654
},
583655
}
584656

585-
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion)
657+
app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{})
586658
require.NoError(t, err)
587659

588660
trueVal := true

‎pkg/packageinstall/packageinstall.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package packageinstall
66
import (
77
"context"
88
"fmt"
9+
"time"
910

1011
"github.com/go-logr/logr"
1112
semver "github.com/k14s/semver/v4"
@@ -52,13 +53,19 @@ type PackageInstallCR struct {
5253
pkgclient pkgclient.Interface
5354
coreClient kubernetes.Interface
5455
compInfo ComponentInfo
56+
opts Opts
57+
}
58+
59+
// nolint: revive
60+
type Opts struct {
61+
DefaultSyncPeriod time.Duration
5562
}
5663

5764
func NewPackageInstallCR(model *pkgingv1alpha1.PackageInstall, log logr.Logger,
58-
kcclient kcclient.Interface, pkgclient pkgclient.Interface, coreClient kubernetes.Interface, compInfo ComponentInfo) *PackageInstallCR {
65+
kcclient kcclient.Interface, pkgclient pkgclient.Interface, coreClient kubernetes.Interface, compInfo ComponentInfo, opts Opts) *PackageInstallCR {
5966

6067
return &PackageInstallCR{model: model, unmodifiedModel: model.DeepCopy(), log: log,
61-
kcclient: kcclient, pkgclient: pkgclient, coreClient: coreClient, compInfo: compInfo}
68+
kcclient: kcclient, pkgclient: pkgclient, coreClient: coreClient, compInfo: compInfo, opts: opts}
6269
}
6370

6471
func (pi *PackageInstallCR) Reconcile() (reconcile.Result, error) {
@@ -174,7 +181,7 @@ func (pi *PackageInstallCR) reconcile(modelStatus *reconciler.Status) (reconcile
174181
}
175182

176183
func (pi *PackageInstallCR) createAppFromPackage(pkg datapkgingv1alpha1.Package) (reconcile.Result, error) {
177-
desiredApp, err := NewApp(&v1alpha1.App{}, pi.model, pkg)
184+
desiredApp, err := NewApp(&v1alpha1.App{}, pi.model, pkg, pi.opts)
178185
if err != nil {
179186
return reconcile.Result{Requeue: true}, err
180187
}
@@ -193,7 +200,7 @@ func (pi *PackageInstallCR) reconcileAppWithPackage(existingApp *kcv1alpha1.App,
193200
return reconcile.Result{}, err
194201
}
195202

196-
desiredApp, err := NewApp(existingApp, pi.model, pkgWithPlaceholderSecrets)
203+
desiredApp, err := NewApp(existingApp, pi.model, pkgWithPlaceholderSecrets, pi.opts)
197204
if err != nil {
198205
return reconcile.Result{Requeue: true}, err
199206
}

‎pkg/packageinstall/packageinstall_deletion_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ func Test_PackageInstallDeletion(t *testing.T) {
6565
appClient := fakekappctrl.NewSimpleClientset(pkgInstall, existingApp)
6666
coreClient := fake.NewSimpleClientset()
6767

68-
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, coreClient, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")})
68+
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, coreClient, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{})
6969
_, err := ip.Reconcile()
7070
assert.Nil(t, err)
7171

‎pkg/packageinstall/packageinstall_downgrade_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ func Test_PackageInstallVersionDowngrades(t *testing.T) {
9898
GitVersion: "v0.20.0",
9999
}
100100

101-
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")})
101+
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{})
102102
_, err := ip.Reconcile()
103103
assert.Nil(t, err)
104104

@@ -147,7 +147,7 @@ func Test_PackageInstallVersionDowngrades(t *testing.T) {
147147
GitVersion: "v0.20.0",
148148
}
149149

150-
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")})
150+
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{})
151151
_, err := ip.Reconcile()
152152
assert.Nil(t, err)
153153

@@ -196,7 +196,7 @@ func Test_PackageInstallVersionDowngrades(t *testing.T) {
196196
GitVersion: "v0.20.0",
197197
}
198198

199-
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")})
199+
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{})
200200
_, err := ip.Reconcile()
201201
assert.Nil(t, err)
202202

@@ -251,7 +251,7 @@ func Test_PackageInstallVersionDowngrades(t *testing.T) {
251251
GitVersion: "v0.20.0",
252252
}
253253

254-
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")})
254+
ip := NewPackageInstallCR(pkgInstall, log, appClient, pkgClient, fakek8s, FakeComponentInfo{KCVersion: semver.MustParse("0.42.31337")}, Opts{})
255255
_, err := ip.Reconcile()
256256
assert.Nil(t, err)
257257

0 commit comments

Comments
 (0)