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

Update argocd to 2.10.10 #222

Merged
merged 3 commits into from
May 24, 2024
Merged

Update argocd to 2.10.10 #222

merged 3 commits into from
May 24, 2024

Conversation

ahinh43
Copy link
Collaborator

@ahinh43 ahinh43 commented May 24, 2024

Fixes #221

Upgrades the ArgoCD dependency to 2.10.10 and applies a fix for a new argument requirement introduced in the new version

Copy link

github-actions bot commented May 24, 2024

Temporary image deleted.

@ahinh43 ahinh43 requested a review from djeebus May 24, 2024 16:53
@ahinh43 ahinh43 marked this pull request as ready for review May 24, 2024 16:53
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of go.mod

@@ -5,8 +5,8 @@ go 1.21
 toolchain go1.21.6
 
 require (
-	github.com/argoproj/argo-cd/v2 v2.10.6
-	github.com/argoproj/gitops-engine v0.7.1-0.20240411122334-1ade3a199867
+	github.com/argoproj/argo-cd/v2 v2.10.10
+	github.com/argoproj/gitops-engine v0.7.1-0.20240416142647-fbecbb86e412
 	github.com/cenkalti/backoff/v4 v4.3.0
 	github.com/chainguard-dev/git-urls v1.0.2
 	github.com/creasty/defaults v1.7.0
@@ -210,7 +210,7 @@ require (
 	github.com/sergi/go-diff v1.3.1 // indirect
 	github.com/shteou/go-ignore v0.3.1 // indirect
 	github.com/shurcooL/graphql v0.0.0-20230722043721-ed46e5a46466 // indirect
-	github.com/skeema/knownhosts v1.2.1 // indirect
+	github.com/skeema/knownhosts v1.2.2 // indirect
 	github.com/sourcegraph/conc v0.3.0 // indirect
 	github.com/spdx/tools-golang v0.5.3 // indirect
 	github.com/spf13/afero v1.11.0 // indirect

Feedback & Suggestions:

  1. Dependency Updates: The updates to github.com/argoproj/argo-cd/v2, github.com/argoproj/gitops-engine, and github.com/skeema/knownhosts are generally good practices to keep dependencies up-to-date. However, ensure that these updates are compatible with your codebase and do not introduce breaking changes. Running your test suite after these updates is crucial.

  2. Version Pinning: The new versions are pinned to specific commits or versions, which is good for reproducibility. However, consider using semantic versioning tags if available, as they provide more context about the nature of the changes (e.g., patch, minor, major).

  3. Security Considerations: Always review the changelogs or release notes of the updated dependencies for any security patches or vulnerabilities that might have been addressed. This ensures that your application remains secure.

  4. Indirect Dependencies: The change to github.com/skeema/knownhosts is marked as indirect. Ensure that this update does not inadvertently affect other parts of your dependency tree.

  5. Documentation: Update your documentation or dependency management notes to reflect these changes. This helps other developers understand why these specific versions were chosen.


By following these suggestions, you can ensure that your dependency management remains robust and your application continues to function correctly and securely. 🛡️🚀

😼 Mergecat review of pkg/checks/diff/diff.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"strings"
+	"time"
 
 	cmdutil "github.com/argoproj/argo-cd/v2/cmd/util"
 	"github.com/argoproj/argo-cd/v2/controller"
@@ -14,6 +15,7 @@ import (
 	argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 	"github.com/argoproj/argo-cd/v2/util/argo"
 	argodiff "github.com/argoproj/argo-cd/v2/util/argo/diff"
+	"github.com/argoproj/argo-cd/v2/util/argo/normalizers"
 	"github.com/argoproj/gitops-engine/pkg/diff"
 	"github.com/argoproj/gitops-engine/pkg/sync/hook"
 	"github.com/argoproj/gitops-engine/pkg/sync/ignore"
@@ -196,9 +198,12 @@ func generateDiff(ctx context.Context, request checks.Request, argoSettings *set
 	}
 
 	ignoreAggregatedRoles := false
+	ignoreNormalizerOpts := normalizers.IgnoreNormalizerOpts{
+		JQExecutionTimeout: 1 * time.Second,
+	}
 	diffConfig, err := argodiff.NewDiffConfigBuilder().
 		WithLogger(zerologr.New(&log.Logger)).
-		WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles).
+		WithDiffSettings(request.App.Spec.IgnoreDifferences, overrides, ignoreAggregatedRoles, ignoreNormalizerOpts).
 		WithTracking(argoSettings.AppLabelKey, argoSettings.TrackingMethod).
 		WithNoCache().
 		Build()

Feedback & Suggestions:

  1. Timeout Configuration: The addition of JQExecutionTimeout in ignoreNormalizerOpts is a good step towards preventing long-running operations. However, it would be beneficial to make this timeout configurable via the request or environment variables to provide flexibility for different use cases.

  2. Error Handling: Ensure that the new timeout setting is properly handled in the argodiff.NewDiffConfigBuilder method. If the timeout is exceeded, it should provide a clear error message indicating the cause.

  3. Documentation: Update the function documentation to reflect the new parameter ignoreNormalizerOpts and its purpose. This will help future developers understand the changes and the reason behind them.

  4. Unit Tests: Add unit tests to verify the behavior of the JQExecutionTimeout setting. Ensure that the timeout is respected and appropriate errors are raised when the timeout is exceeded.

  5. Performance Impact: Monitor the performance impact of the new timeout setting. While it prevents long-running operations, it might also affect the performance of legitimate operations that require more time. Adjust the timeout value based on performance testing results.



Dependency Review

Click to read mergecats review!

No suggestions found

@djeebus djeebus merged commit b518720 into main May 24, 2024
5 checks passed
@djeebus djeebus deleted the argo-cd-2.10.10 branch May 24, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CVE-2024-31989
3 participants