Enable server-side diffs

[MeNsaaH]

This adds an option to send your manifests to the server for diffing

[github-actions]

Temporary image available at ghcr.io/zapier/kubechecks:0.0.0-pr314.

[zapier-sre-bot]

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of .github/workflows/on_pull-request_docs.yaml

@@ -24,4 +24,4 @@ jobs:
         run: ./earthly.sh +rebuild-docs
 
       - name: verify that the checked in file has not changed
-        run: ./hacks/exit-on-changed-files.sh "Please run './earthly +rebuild-docs' and commit the results to this PR"
+        run: ./hacks/exit-on-changed-files.sh "Please run './earthly.sh +rebuild-docs' and commit the results to this PR"

Feedback & Suggestions:

• 🔍 Consistency Check: The change ensures consistency in the script usage by adding .sh to the earthly command in the message. This is a good practice to avoid confusion. However, ensure that the script earthly.sh is indeed the correct script to be run, as this change assumes that earthly.sh is the intended script and not earthly without the .sh.
• 📄 Documentation Update: If this change reflects a broader change in how scripts are named or used, consider updating any related documentation to reflect this change to prevent any confusion for future contributors.

---

😼 Mergecat review of cmd/root.go

@@ -119,6 +119,10 @@ func init() {
 		newStringOpts().
 			withDefault("kubechecks again"))
 
+	boolFlag(flags, "server-side-diff", "Enable server-side diff.",
+		newBoolOpts().
+			withDefault(false))
+
 	panicIfError(viper.BindPFlags(flags))
 	setupLogOutput()
 }

Feedback & Suggestions:

1. Security Consideration: Ensure that the new server-side-diff flag is validated and sanitized if it affects any server-side operations. This is crucial to prevent potential security vulnerabilities such as injection attacks.

2. Documentation: Consider updating any relevant documentation to include the new server-side-diff flag. This will help users understand its purpose and usage.

3. Testing: Add tests to verify the behavior of the server-side-diff flag. Ensure that it correctly enables or disables the intended functionality.

4. Default Value: The default value is set to false, which is generally safe. However, ensure that this default aligns with the expected behavior of your application.

5. Code Consistency: The addition is consistent with the existing pattern of flag definitions, which is good for maintainability. 👍

---

😼 Mergecat review of localdev/kubechecks/values.yaml

@@ -10,6 +10,7 @@ configMap:
     KUBECHECKS_NAMESPACE: 'kubechecks'
     KUBECHECKS_FALLBACK_K8S_VERSION: "1.25.0"
     KUBECHECKS_SHOW_DEBUG_INFO: "true"
+    KUBECHECKS_VCS_TYPE: "gitlab"
     # OTEL
     KUBECHECKS_OTEL_COLLECTOR_PORT: "4317"
     KUBECHECKS_OTEL_ENABLED: "false"
@@ -21,14 +22,16 @@ configMap:
     # KUBECHECKS_SCHEMAS_LOCATION: https://github.com/zapier/kubecheck-schemas.git
     KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE: "delete"
     KUBECHECKS_ENABLE_CONFTEST: "false"
+    KUBECHECKS_SERVER_SIDE_DIFF: "true"
+    GRPC_ENFORCE_ALPN_ENABLED: false
 
 
 deployment:
   annotations:
     reloader.stakater.com/auto: "true" 
   
   image:
-    pullPolicy: Never
+    pullPolicy: IfNotPresent
     name: "kubechecks"
     tag: ""
 

Feedback & Suggestions:

1. Security Concern: The GRPC_ENFORCE_ALPN_ENABLED is set to false. This could potentially lead to security vulnerabilities if ALPN (Application-Layer Protocol Negotiation) is required for secure communication. Consider setting it to true if ALPN is necessary for your use case. 🔒

2. Image Pull Policy: Changing the pullPolicy from Never to IfNotPresent is generally a good practice for environments where you want to ensure the image is pulled if not available locally. However, ensure that this change aligns with your deployment strategy and that it won't inadvertently cause issues in environments where the image should not be pulled from a registry. 🚀

3. New Environment Variables: The addition of KUBECHECKS_VCS_TYPE and KUBECHECKS_SERVER_SIDE_DIFF seems appropriate, but ensure that these new configurations are documented and that their implications are well understood by the team. 📚

---

😼 Mergecat review of pkg/config/config.go

@@ -79,6 +79,7 @@ type ServerConfig struct {
 	MaxQueueSize             int64         `mapstructure:"max-queue-size"`
 	MaxConcurrenctChecks     int           `mapstructure:"max-concurrenct-checks"`
 	ReplanCommentMessage     string        `mapstructure:"replan-comment-msg"`
+	ServerSideDiff           bool          `mapstructure:"server-side-diff"`
 }
 
 func New() (ServerConfig, error) {
@@ -115,6 +116,7 @@ func NewWithViper(v *viper.Viper) (ServerConfig, error) {
 	log.Info().Msgf("Webhook URL Prefix: %s", cfg.UrlPrefix)
 	log.Info().Msgf("VCS Type: %s", cfg.VcsType)
 	log.Info().Msgf("ArgoCD Namespace: %s", cfg.ArgoCDNamespace)
+	log.Info().Msgf("Server-Side Diff: %v", cfg.ServerSideDiff)
 
 	return cfg, nil
 }

Feedback & Suggestions:

1. Typographical Consistency: There is a typo in the field name MaxConcurrenctChecks. It should be MaxConcurrentChecks. This typo is present in the original code, but it would be beneficial to correct it to maintain consistency and avoid confusion.

2. Logging Information: The addition of the log statement for ServerSideDiff is a good practice for debugging and monitoring. Ensure that logging sensitive information is avoided, although in this case, a boolean value is generally safe.

3. Documentation: Consider adding a comment or documentation for the new ServerSideDiff field to explain its purpose and usage. This will help other developers understand the intent behind this configuration option.

4. Testing: Ensure that there are corresponding tests to verify the behavior of the new ServerSideDiff configuration. This will help in maintaining the reliability of the codebase.

---

😼 Mergecat review of docs/usage.md

@@ -67,6 +67,7 @@ The full list of supported environment variables is described below:
 |`KUBECHECKS_REPLAN_COMMENT_MSG`|comment message which re-triggers kubechecks on PR.|`kubechecks again`|
 |`KUBECHECKS_REPO_REFRESH_INTERVAL`|Interval between static repo refreshes (for schemas and policies).|`5m`|
 |`KUBECHECKS_SCHEMAS_LOCATION`|Sets schema locations to be used for every check request. Can be common paths inside the repos being checked or git urls in either git or http(s) format.|`[]`|
+|`KUBECHECKS_SERVER_SIDE_DIFF`|Enable server-side diff.|`false`|
 |`KUBECHECKS_SHOW_DEBUG_INFO`|Set to true to print debug info to the footer of MR comments.|`false`|
 |`KUBECHECKS_TIDY_OUTDATED_COMMENTS_MODE`|Sets the mode to use when tidying outdated comments. One of hide, delete.|`hide`|
 |`KUBECHECKS_VCS_BASE_URL`|VCS base url, useful if self hosting gitlab, enterprise github, etc.||

Feedback & Suggestions:

1. Description Clarity: The description for KUBECHECKS_SERVER_SIDE_DIFF is quite brief. Consider expanding it to explain what "server-side diff" entails and how it impacts the behavior of kubechecks. This will help users understand when and why they might want to enable this feature. 📚

2. Default Value Explanation: If false is the default value, it might be helpful to briefly mention the implications of keeping it disabled. This can guide users in making an informed decision about whether to change the default setting. 🤔

3. Consistency Check: Ensure that the new entry is consistent with the formatting and style of the other entries in the table. This includes capitalization, punctuation, and phrasing. Consistency improves readability and professionalism. 📏

---

😼 Mergecat review of pkg/argo_client/manifests.go

@@ -11,7 +11,9 @@ import (
 	repoapiclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient"
 	"github.com/argoproj/argo-cd/v2/reposerver/repository"
 	"github.com/argoproj/argo-cd/v2/util/git"
+	"github.com/argoproj/argo-cd/v2/util/manifeststream"
 	"github.com/ghodss/yaml"
+	grpc_retry "github.com/grpc-ecosystem/go-grpc-middleware/retry"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -102,6 +104,48 @@ func (argo *ArgoClient) generateManifests(
 	)
 }
 
+// adapted fromm https://github.com/argoproj/argo-cd/blob/d3ff9757c460ae1a6a11e1231251b5d27aadcdd1/cmd/argocd/commands/app.go#L894
+func (argo *ArgoClient) GetManifestsServerSide(ctx context.Context, name, tempRepoDir, changedAppFilePath string, app argoappv1.Application) ([]string, error) {
+	var err error
+
+	ctx, span := tracer.Start(ctx, "GetManifestsServerSide")
+	defer span.End()
+
+	log.Debug().Str("name", name).Msg("GetManifestsServerSide")
+
+	start := time.Now()
+	defer func() {
+		duration := time.Since(start)
+		getManifestsDuration.WithLabelValues(name).Observe(duration.Seconds())
+	}()
+
+	appCloser, appClient := argo.GetApplicationClient()
+	defer appCloser.Close()
+
+	client, err := appClient.GetManifestsWithFiles(ctx, grpc_retry.Disable())
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get manifest client")
+	}
+	localIncludes := []string{"*"}
+	log.Debug().Str("name", name).Str("repo_path", tempRepoDir).Msg("sending application manifest query with files")
+
+	err = manifeststream.SendApplicationManifestQueryWithFiles(ctx, client, name, app.Namespace, tempRepoDir, localIncludes)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to send manifest query")
+	}
+
+	res, err := client.CloseAndRecv()
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to receive manifest response")
+	}
+
+	if res.Manifests == nil {
+		return nil, nil
+	}
+	getManifestsSuccess.WithLabelValues(name).Inc()
+	return res.Manifests, nil
+}
+
 func ConvertJsonToYamlManifests(jsonManifests []string) []string {
 	var manifests []string
 	for _, manifest := range jsonManifests {

Feedback & Suggestions:

1. Error Handling Consistency: The new function GetManifestsServerSide uses errors.Wrap for error handling, which is consistent with the existing code. However, consider adding more context to the error messages to make debugging easier. For example, include the name or tempRepoDir in the error message where applicable.

2. Logging: The logging in the new function is consistent with the existing code, which is good. However, ensure that sensitive information is not logged, especially if tempRepoDir or name could contain sensitive data.

3. Performance: The use of grpc_retry.Disable() is noted. If retries are not needed, this is fine, but consider if there are scenarios where transient errors might benefit from a retry mechanism.

4. Documentation: The comment above the new function mentions "adapted fromm" with a typo. Correct it to "adapted from" for clarity. Additionally, consider providing more detailed comments on what the function does, especially since it involves network operations.

5. Security: Ensure that the localIncludes variable, which is set to []string{"*"}, does not inadvertently expose sensitive files or directories. Review the security implications of this wildcard usage.

6. Code Duplication: The new function GetManifestsServerSide shares some logic with GetManifestsLocal, such as logging and metrics. Consider refactoring common logic into helper functions to reduce code duplication and improve maintainability.

---

😼 Mergecat review of pkg/events/worker.go

@@ -105,9 +105,15 @@ func (w *worker) processApp(ctx context.Context, app v1alpha1.Application) {
 		return
 	}
 	repoPath := repo.Directory
+	var jsonManifests []string
 
-	logger.Debug().Str("repo_path", repoPath).Msg("Getting manifests")
-	jsonManifests, err := w.ctr.ArgoClient.GetManifestsLocal(ctx, appName, repoPath, appPath, app)
+	if !w.ctr.Config.ServerSideDiff {
+		logger.Debug().Str("repo_path", repoPath).Msg("Getting manifests")
+		jsonManifests, err = w.ctr.ArgoClient.GetManifestsLocal(ctx, appName, repoPath, appPath, app)
+	} else {
+		logger.Debug().Str("repo_path", repoPath).Msg("Getting server-side manifests")
+		jsonManifests, err = w.ctr.ArgoClient.GetManifestsServerSide(ctx, appName, repoPath, appPath, app)
+	}
 	if err != nil {
 		logger.Error().Err(err).Msg("Unable to get manifests")
 		w.vcsNote.AddToAppMessage(ctx, appName, msg.Result{

Feedback & Suggestions:

1. Error Handling Consistency: Ensure that the error handling logic is consistent between the two branches of the if statement. Currently, both branches handle errors in the same way, which is good, but make sure this remains consistent if the logic evolves. 🛠️

2. Variable Initialization: The jsonManifests variable is declared before the if statement. This is necessary for scope reasons, but consider adding a comment to clarify why it's declared outside the if block for future maintainers. 📝

3. Logging Clarity: The log messages are clear, but consider adding more context if possible, such as the specific server or environment being used for server-side manifests. This can help in debugging and understanding the flow of execution. 🔍

4. Performance Consideration: If the GetManifestsServerSide method is significantly slower than GetManifestsLocal, consider adding performance metrics or warnings if the operation takes too long. This can help in identifying bottlenecks. ⏱️

Overall, the changes are well-structured and improve the flexibility of the manifest retrieval process. Keep up the good work! 👍

---

---

Dependency Review

Click to read mergecats review!

No suggestions found

[MeNsaaH]

I was getting the error credentials: cannot check peer: missing selected ALPN property, hence why I disabled this, for local. I don't know why it was breaking

argoproj/argo-cd#20121

[MeNsaaH]

Closed in favour of #298