Skip to content
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.

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

fix: retain count and order of revisions for multi source apps (#14108) #14113

Merged
merged 4 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 10 additions & 1 deletion controller/appcontroller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ type namespacedResource struct {
type fakeData struct {
apps []runtime.Object
manifestResponse *apiclient.ManifestResponse
manifestResponses []*apiclient.ManifestResponse
managedLiveObjs map[kube.ResourceKey]*unstructured.Unstructured
namespacedResources map[kube.ResourceKey]namespacedResource
configMapData map[string]string
Expand All @@ -65,7 +66,15 @@ func newFakeController(data *fakeData) *ApplicationController {

// Mock out call to GenerateManifest
mockRepoClient := mockrepoclient.RepoServerServiceClient{}
mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil)

if len(data.manifestResponses) > 0 {
for _, response := range data.manifestResponses {
mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(response, nil).Once()
}
} else {
mockRepoClient.On("GenerateManifest", mock.Anything, mock.Anything).Return(data.manifestResponse, nil)
}

mockRepoClientset := mockrepoclient.Clientset{RepoServerServiceClient: &mockRepoClient}

secret := corev1.Secret{
Expand Down
27 changes: 13 additions & 14 deletions controller/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ type appStateManager struct {
persistResourceHealth bool
}

func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse, error) {
func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alpha1.ApplicationSource, appLabelKey string, revisions []string, noCache, noRevisionCache, verifySignature bool, proj *v1alpha1.AppProject) ([]*unstructured.Unstructured, []*apiclient.ManifestResponse, error) {

ts := stats.NewTimingStats()
helmRepos, err := m.db.ListHelmRepositories(context.Background())
Expand Down Expand Up @@ -163,7 +163,7 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
}
defer io.Close(conn)

manifestInfoMap := make(map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse)
manifestInfos := make([]*apiclient.ManifestResponse, 0)
targetObjs := make([]*unstructured.Unstructured, 0)

// Store the map of all sources having ref field into a map for applications with sources field
Expand Down Expand Up @@ -236,7 +236,8 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
return nil, nil, err
}
targetObjs = append(targetObjs, targetObj...)
manifestInfoMap[&source] = manifestInfo

manifestInfos = append(manifestInfos, manifestInfo)
}

ts.AddCheckpoint("unmarshal_ms")
Expand All @@ -246,7 +247,7 @@ func (m *appStateManager) getRepoObjs(app *v1alpha1.Application, sources []v1alp
}
logCtx = logCtx.WithField("time_ms", time.Since(ts.StartTime).Milliseconds())
logCtx.Info("getRepoObjs stats")
return targetObjs, manifestInfoMap, nil
return targetObjs, manifestInfos, nil
}

func unmarshalManifests(manifests []string) ([]*unstructured.Unstructured, error) {
Expand Down Expand Up @@ -407,7 +408,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
var targetObjs []*unstructured.Unstructured
now := metav1.Now()

var manifestInfoMap map[*v1alpha1.ApplicationSource]*apiclient.ManifestResponse
var manifestInfos []*apiclient.ManifestResponse

if len(localManifests) == 0 {
// If the length of revisions is not same as the length of sources,
Expand All @@ -419,7 +420,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
}
}

targetObjs, manifestInfoMap, err = m.getRepoObjs(app, sources, appLabelKey, revisions, noCache, noRevisionCache, verifySignature, project)
targetObjs, manifestInfos, err = m.getRepoObjs(app, sources, appLabelKey, revisions, noCache, noRevisionCache, verifySignature, project)
if err != nil {
targetObjs = make([]*unstructured.Unstructured, 0)
conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now})
Expand All @@ -442,9 +443,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
}
}
// empty out manifestInfoMap
for as := range manifestInfoMap {
delete(manifestInfoMap, as)
}
manifestInfos = make([]*apiclient.ManifestResponse, 0)
}
ts.AddCheckpoint("git_ms")

Expand Down Expand Up @@ -524,12 +523,12 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
}
manifestRevisions := make([]string, 0)

for _, manifestInfo := range manifestInfoMap {
for _, manifestInfo := range manifestInfos {
manifestRevisions = append(manifestRevisions, manifestInfo.Revision)
}

// restore comparison using cached diff result if previous comparison was performed for the same revision
revisionChanged := len(manifestInfoMap) != len(sources) || !reflect.DeepEqual(app.Status.Sync.Revisions, manifestRevisions)
revisionChanged := len(manifestInfos) != len(sources) || !reflect.DeepEqual(app.Status.Sync.Revisions, manifestRevisions)
specChanged := !reflect.DeepEqual(app.Status.Sync.ComparedTo, v1alpha1.ComparedTo{Source: app.Spec.GetSource(), Destination: app.Spec.Destination, Sources: sources})

_, refreshRequested := app.IsRefreshRequested()
Expand Down Expand Up @@ -698,7 +697,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
// Git has already performed the signature verification via its GPG interface, and the result is available
// in the manifest info received from the repository server. We now need to form our opinion about the result
// and stop processing if we do not agree about the outcome.
for _, manifestInfo := range manifestInfoMap {
for _, manifestInfo := range manifestInfos {
if gpg.IsGPGEnabled() && verifySignature && manifestInfo != nil {
conditions = append(conditions, verifyGnuPGSignature(manifestInfo.Revision, project, manifestInfo)...)
}
Expand All @@ -715,11 +714,11 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1
}

if hasMultipleSources {
for _, manifestInfo := range manifestInfoMap {
for _, manifestInfo := range manifestInfos {
compRes.appSourceTypes = append(compRes.appSourceTypes, v1alpha1.ApplicationSourceType(manifestInfo.SourceType))
}
} else {
for _, manifestInfo := range manifestInfoMap {
for _, manifestInfo := range manifestInfos {
compRes.appSourceType = v1alpha1.ApplicationSourceType(manifestInfo.SourceType)
break
}
Expand Down
68 changes: 45 additions & 23 deletions controller/state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,8 @@ func TestCompareAppStateExtraHook(t *testing.T) {
assert.Equal(t, 0, len(app.Status.Conditions))
}

// TestAppRevisions tests that revisions are properly propagated
func TestAppRevisions(t *testing.T) {
// TestAppRevisions tests that revisions are properly propagated for a single source app
func TestAppRevisionsSingleSource(t *testing.T) {
obj1 := NewPod()
obj1.SetNamespace(test.FakeDestNamespace)
data := fakeData{
Expand All @@ -333,28 +333,50 @@ func TestAppRevisions(t *testing.T) {
}
ctrl := newFakeController(&data)

// single source
{
app := newFakeApp()
revisions := make([]string, 0)
revisions = append(revisions, "")
compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
assert.NotNil(t, compRes)
assert.NotNil(t, compRes.syncStatus)
assert.NotEmpty(t, compRes.syncStatus.Revision)
assert.Len(t, compRes.syncStatus.Revisions, 0)
}
// multisource
{
app := newFakeMultiSourceApp()
revisions := make([]string, 0)
revisions = append(revisions, "")
compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
assert.NotNil(t, compRes)
assert.NotNil(t, compRes.syncStatus)
assert.Empty(t, compRes.syncStatus.Revision)
assert.Len(t, compRes.syncStatus.Revisions, 2)
app := newFakeApp()
revisions := make([]string, 0)
revisions = append(revisions, "")
compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
assert.NotNil(t, compRes)
assert.NotNil(t, compRes.syncStatus)
assert.NotEmpty(t, compRes.syncStatus.Revision)
assert.Len(t, compRes.syncStatus.Revisions, 0)

}

// TestAppRevisions tests that revisions are properly propagated for a multi source app
func TestAppRevisionsMultiSource(t *testing.T) {
obj1 := NewPod()
obj1.SetNamespace(test.FakeDestNamespace)
data := fakeData{
manifestResponses: []*apiclient.ManifestResponse{
{
Manifests: []string{toJSON(t, obj1)},
Namespace: test.FakeDestNamespace,
Server: test.FakeClusterURL,
Revision: "abc123",
},
{
Manifests: []string{toJSON(t, obj1)},
Namespace: test.FakeDestNamespace,
Server: test.FakeClusterURL,
Revision: "def456",
},
},
managedLiveObjs: make(map[kube.ResourceKey]*unstructured.Unstructured),
}
ctrl := newFakeController(&data)

app := newFakeMultiSourceApp()
revisions := make([]string, 0)
revisions = append(revisions, "")
compRes := ctrl.appStateManager.CompareAppState(app, &defaultProj, revisions, app.Spec.GetSources(), false, false, nil, app.Spec.HasMultipleSources())
assert.NotNil(t, compRes)
assert.NotNil(t, compRes.syncStatus)
assert.Empty(t, compRes.syncStatus.Revision)
assert.Len(t, compRes.syncStatus.Revisions, 2)
assert.Equal(t, "abc123", compRes.syncStatus.Revisions[0])
assert.Equal(t, "def456", compRes.syncStatus.Revisions[1])
}

func toJSON(t *testing.T, obj *unstructured.Unstructured) string {
Expand Down