Skip to content

Fix the duplicate reference to the secretRef field of the PackageInstall object within the App object #1860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
klzsysy wants to merge 1 commit into from
Closed

Fix the duplicate reference to the secretRef field of the PackageInstall object within the App object #1860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 24 additions & 11 deletions pkg/packageinstall/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,10 @@ func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall
desiredApp.Annotations["packaging.carvel.dev/package-ref-name"] = pkgVersion.Spec.RefName
desiredApp.Annotations["packaging.carvel.dev/package-version"] = pkgVersion.Spec.Version

desiredApp.Spec = *pkgVersion.Spec.Template.Spec
// Deep copy the package's spec: the caller's PackageInstall reconciler shares
// this Package across calls, and the values appended below would otherwise
// mutate it in place (the App spec holds pointers into it).
desiredApp.Spec = *pkgVersion.Spec.Template.Spec.DeepCopy()
desiredApp.Spec.ServiceAccountName = pkgInstall.Spec.ServiceAccountName
if pkgInstall.Spec.SyncPeriod == nil {
desiredApp.Spec.SyncPeriod = &metav1.Duration{Duration: opts.DefaultSyncPeriod}
Expand Down Expand Up @@ -125,11 +128,7 @@ func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall
valuesApplied = true

for _, value := range pkgInstall.Spec.Values {
templateStep.HelmTemplate.ValuesFrom = append(templateStep.HelmTemplate.ValuesFrom, kcv1alpha1.AppTemplateValuesSource{
SecretRef: &kcv1alpha1.AppTemplateValuesSourceRef{
Name: value.SecretRef.Name,
},
})
templateStep.HelmTemplate.ValuesFrom = appendValuesFromSecret(templateStep.HelmTemplate.ValuesFrom, value.SecretRef.Name)
}
}
}
Expand Down Expand Up @@ -166,11 +165,7 @@ func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall
}
} else {
for _, value := range pkgInstall.Spec.Values {
templateStep.Ytt.ValuesFrom = append(templateStep.Ytt.ValuesFrom, kcv1alpha1.AppTemplateValuesSource{
SecretRef: &kcv1alpha1.AppTemplateValuesSourceRef{
Name: value.SecretRef.Name,
},
})
templateStep.Ytt.ValuesFrom = appendValuesFromSecret(templateStep.Ytt.ValuesFrom, value.SecretRef.Name)
}
}
}
Expand All @@ -182,6 +177,24 @@ func NewApp(existingApp *v1alpha1.App, pkgInstall *pkgingv1alpha1.PackageInstall
return desiredApp, nil
}

// appendValuesFromSecret returns sources with a secretRef for secretName added,
// unless that secret is already referenced by one of them. Referencing the same
// secret twice does not change how the values are applied, so it would only be
// noise in the App spec.
func appendValuesFromSecret(sources []kcv1alpha1.AppTemplateValuesSource, secretName string) []kcv1alpha1.AppTemplateValuesSource {
for _, source := range sources {
if source.SecretRef != nil && source.SecretRef.Name == secretName {
return sources
}
}

return append(sources, kcv1alpha1.AppTemplateValuesSource{
SecretRef: &kcv1alpha1.AppTemplateValuesSourceRef{
Name: secretName,
},
})
}

func secretNamesFromAnn(installedPkg *pkgingv1alpha1.PackageInstall, annKey string) []string {
var suffixes []string
suffixToSecretName := map[string]string{}
Expand Down
183 changes: 183 additions & 0 deletions pkg/packageinstall/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -716,3 +716,186 @@ func TestAppPackageIntallDefaultNamespace(t *testing.T) {

require.Equal(t, expectedApp, app, "App does not match expected app")
}

func valuesFromNames(sources []kcv1alpha1.AppTemplateValuesSource) []string {
names := make([]string, 0, len(sources))
for _, source := range sources {
if source.SecretRef != nil {
names = append(names, source.SecretRef.Name)
} else {
names = append(names, source.Path)
}
}
return names
}

// NewApp must not write back into the Package it is given. The PackageInstall
// reconciler shares a single Package across calls, so mutating it makes every
// call append the PackageInstall's values again.
func TestAppDoesNotMutatePackage(t *testing.T) {
ipkg := &pkgingv1alpha1.PackageInstall{
ObjectMeta: metav1.ObjectMeta{
Name: "app",
Namespace: "default",
},
Spec: pkgingv1alpha1.PackageInstallSpec{
Values: []pkgingv1alpha1.PackageInstallValues{
{SecretRef: &pkgingv1alpha1.PackageInstallValuesSecretRef{Name: "values1"}},
},
},
}

pkgVersion := datapkgingv1alpha1.Package{
Spec: datapkgingv1alpha1.PackageSpec{
RefName: "expec-pkg",
Version: "1.5.0",
Template: datapkgingv1alpha1.AppTemplateSpec{
Spec: &kcv1alpha1.AppSpec{
Template: []kcv1alpha1.AppTemplate{
{Ytt: &kcv1alpha1.AppTemplateYtt{}},
},
},
},
},
}

app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
require.NoError(t, err)

// The App picks up the PackageInstall's values...
require.Equal(t, []string{"values1"}, valuesFromNames(app.Spec.Template[0].Ytt.ValuesFrom))

// ...without the Package it was built from seeing them.
require.Empty(t, pkgVersion.Spec.Template.Spec.Template[0].Ytt.ValuesFrom,
"NewApp mutated the Package it was given")
}

// The PackageInstall reconciler calls NewApp twice with the same Package: once to
// detect whether the App needs updating, and once inside the update closure. Both
// calls have to produce the same App, otherwise the App is rewritten on every
// reconcile instead of converging.
func TestAppValuesFromNotAppendedTwice(t *testing.T) {
ipkg := &pkgingv1alpha1.PackageInstall{
ObjectMeta: metav1.ObjectMeta{
Name: "app",
Namespace: "default",
},
Spec: pkgingv1alpha1.PackageInstallSpec{
Values: []pkgingv1alpha1.PackageInstallValues{
{SecretRef: &pkgingv1alpha1.PackageInstallValuesSecretRef{Name: "values1"}},
},
},
}

pkgVersion := datapkgingv1alpha1.Package{
Spec: datapkgingv1alpha1.PackageSpec{
RefName: "expec-pkg",
Version: "1.5.0",
Template: datapkgingv1alpha1.AppTemplateSpec{
Spec: &kcv1alpha1.AppSpec{
Template: []kcv1alpha1.AppTemplate{
{Ytt: &kcv1alpha1.AppTemplateYtt{
ValuesFrom: []kcv1alpha1.AppTemplateValuesSource{
kcv1alpha1.AppTemplateValuesSource{Path: "values.yaml"},
},
}},
},
},
},
},
}

opts := packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute}

first, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, opts)
require.NoError(t, err)

second, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, opts)
require.NoError(t, err)

require.Equal(t, []string{"values.yaml", "values1"}, valuesFromNames(first.Spec.Template[0].Ytt.ValuesFrom))
require.Equal(t, valuesFromNames(first.Spec.Template[0].Ytt.ValuesFrom),
valuesFromNames(second.Spec.Template[0].Ytt.ValuesFrom),
"the second NewApp call appended the PackageInstall values again")
}

// A Package that already declares the secret a PackageInstall passes must not end
// up referencing that secret twice.
func TestAppYttValuesSkipsAlreadyDeclaredSecret(t *testing.T) {
ipkg := &pkgingv1alpha1.PackageInstall{
ObjectMeta: metav1.ObjectMeta{
Name: "app",
Namespace: "default",
},
Spec: pkgingv1alpha1.PackageInstallSpec{
Values: []pkgingv1alpha1.PackageInstallValues{
{SecretRef: &pkgingv1alpha1.PackageInstallValuesSecretRef{Name: "values1"}},
},
},
}

pkgVersion := datapkgingv1alpha1.Package{
Spec: datapkgingv1alpha1.PackageSpec{
RefName: "expec-pkg",
Version: "1.5.0",
Template: datapkgingv1alpha1.AppTemplateSpec{
Spec: &kcv1alpha1.AppSpec{
Template: []kcv1alpha1.AppTemplate{
{Ytt: &kcv1alpha1.AppTemplateYtt{
ValuesFrom: []kcv1alpha1.AppTemplateValuesSource{
kcv1alpha1.AppTemplateValuesSource{
SecretRef: &kcv1alpha1.AppTemplateValuesSourceRef{Name: "values1"},
},
},
}},
},
},
},
},
}

app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
require.NoError(t, err)

require.Equal(t, []string{"values1"}, valuesFromNames(app.Spec.Template[0].Ytt.ValuesFrom))
}

// Same as above for the helmTemplate step.
func TestAppHelmTemplateValuesSkipsAlreadyDeclaredSecret(t *testing.T) {
ipkg := &pkgingv1alpha1.PackageInstall{
ObjectMeta: metav1.ObjectMeta{
Name: "app",
Namespace: "default",
},
Spec: pkgingv1alpha1.PackageInstallSpec{
Values: []pkgingv1alpha1.PackageInstallValues{
{SecretRef: &pkgingv1alpha1.PackageInstallValuesSecretRef{Name: "values1"}},
},
},
}

pkgVersion := datapkgingv1alpha1.Package{
Spec: datapkgingv1alpha1.PackageSpec{
RefName: "expec-pkg",
Version: "1.5.0",
Template: datapkgingv1alpha1.AppTemplateSpec{
Spec: &kcv1alpha1.AppSpec{
Template: []kcv1alpha1.AppTemplate{
{HelmTemplate: &kcv1alpha1.AppTemplateHelmTemplate{
ValuesFrom: []kcv1alpha1.AppTemplateValuesSource{
kcv1alpha1.AppTemplateValuesSource{
SecretRef: &kcv1alpha1.AppTemplateValuesSourceRef{Name: "values1"},
},
},
}},
},
},
},
},
}

app, err := packageinstall.NewApp(&kcv1alpha1.App{}, ipkg, pkgVersion, packageinstall.Opts{DefaultSyncPeriod: 10 * time.Minute})
require.NoError(t, err)

require.Equal(t, []string{"values1"}, valuesFromNames(app.Spec.Template[0].HelmTemplate.ValuesFrom))
}
Loading