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

feat(server): make deep copies of objects returned by informers (#22173) #22179

Merged
merged 22 commits into from
Mar 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
db54abd
feat(server): make deep copies of objects returned by informers
rumstead Mar 4, 2025
761f560
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 4, 2025
859e4b1
feat(server): make deep copies of objects returned by informers
rumstead Mar 4, 2025
101e6c2
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
24497e3
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
299922e
Merge remote-tracking branch 'upstream/master' into fix/appinformers-…
rumstead Mar 5, 2025
cd53d21
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
683b267
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
228a043
Merge remote-tracking branch 'upstream/master' into fix/appinformers-…
rumstead Mar 5, 2025
30c78ca
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
6813f5d
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
26a2023
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 5, 2025
18f6bc4
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
960af00
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
f47f931
feat(server): make deep copies of objects returned by informers
rumstead Mar 5, 2025
556af50
feat(server): add some real world tests
rumstead Mar 6, 2025
a4730b4
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 6, 2025
49bdd53
fix my own test
rumstead Mar 6, 2025
add4c5f
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 6, 2025
60058f8
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 7, 2025
f382b58
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 7, 2025
56e3ec4
Merge branch 'master' into fix/appinformers-should-return-copy
rumstead Mar 10, 2025
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
5 changes: 4 additions & 1 deletion .mockery.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,7 @@ packages:
SessionServiceClient:
github.com/argoproj/argo-cd/v3/pkg/apiclient/cluster:
interfaces:
ClusterServiceServer:
ClusterServiceServer:
github.com/argoproj/argo-cd/v3/pkg/client/clientset/versioned/typed/application/v1alpha1:
interfaces:
AppProjectInterface:

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 6 additions & 13 deletions server/application/application.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ import (
"github.com/argoproj/argo-cd/v3/reposerver/apiclient"
servercache "github.com/argoproj/argo-cd/v3/server/cache"
"github.com/argoproj/argo-cd/v3/server/deeplinks"
"github.com/argoproj/argo-cd/v3/util"
"github.com/argoproj/argo-cd/v3/util/argo"
"github.com/argoproj/argo-cd/v3/util/collections"
"github.com/argoproj/argo-cd/v3/util/db"
Expand Down Expand Up @@ -127,8 +126,8 @@ func NewServer(
}
s := &Server{
ns: namespace,
appclientset: appclientset,
appLister: appLister,
appclientset: &deepCopyAppClientset{appclientset},
appLister: &deepCopyApplicationLister{appLister},
appInformer: appInformer,
appBroadcaster: appBroadcaster,
kubeclientset: kubeclientset,
Expand Down Expand Up @@ -261,9 +260,7 @@ func (s *Server) getApplicationEnforceRBACClient(ctx context.Context, action, pr
if err != nil {
return nil, err
}
// Objects returned by the lister must be treated as read-only.
// To allow us to modify the app later, make a copy
return app.DeepCopy(), nil
return app, nil
})
}

Expand Down Expand Up @@ -380,9 +377,6 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq
if err != nil {
return nil, status.Errorf(codes.Internal, "unable to check existing application details (%s): %v", appNs, err)
}
// Objects returned from listers have to be treated as read-only
// Take a deep copy so we can edit it below
existing = existing.DeepCopy()

if _, err := argo.GetDestinationCluster(ctx, existing.Spec.Destination, s.db); err != nil {
return nil, status.Errorf(codes.InvalidArgument, "application destination spec for %s is invalid: %s", existing.Name, err.Error())
Expand Down Expand Up @@ -491,7 +485,7 @@ func (s *Server) GetManifests(ctx context.Context, q *application.ApplicationMan
}

sources := make([]v1alpha1.ApplicationSource, 0)
appSpec := a.Spec.DeepCopy()
appSpec := a.Spec
if a.Spec.HasMultipleSources() {
numOfSources := int64(len(a.Spec.GetSources()))
for i, pos := range q.SourcePositions {
Expand Down Expand Up @@ -888,7 +882,7 @@ func (s *Server) ListResourceEvents(ctx context.Context, q *application.Applicat
if err != nil {
return nil, fmt.Errorf("error listing resource events: %w", err)
}
return list, nil
return list.DeepCopy(), nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to wrap the kubernetes.Interface i think is too large of a task :-p. there are so too many methods.

}

// validateAndUpdateApp validates and updates the application. currentProject is the name of the project the app
Expand Down Expand Up @@ -1226,7 +1220,6 @@ func (s *Server) Watch(q *application.ApplicationQuery, ws application.Applicati
if err != nil {
return fmt.Errorf("error listing apps with selector: %w", err)
}
apps = util.SliceCopy(apps)
sort.Slice(apps, func(i, j int) bool {
return apps[i].QualifiedName() < apps[j].QualifiedName()
})
Expand Down Expand Up @@ -2330,7 +2323,7 @@ func (s *Server) TerminateOperation(ctx context.Context, termOpReq *application.
}
log.Warnf("failed to set operation for app %q due to update conflict. retrying again...", *termOpReq.Name)
time.Sleep(100 * time.Millisecond)
a, err = s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{})
_, err = s.appclientset.ArgoprojV1alpha1().Applications(appNs).Get(ctx, appName, metav1.GetOptions{})
if err != nil {
return nil, fmt.Errorf("error getting application by name: %w", err)
}
Expand Down
Loading
Loading