Skip to content

Conversation

wernken-ng
Copy link
Collaborator

Description

Added generated-store flag to configure kubepug store URL and upgraded kubepug to v1.7.1.

Changes

  • Added generated-store flag to allow configuring kubepug store URL
  • Updated preupgrade check to use configured store URL
  • Upgraded kubepug dependency to v1.7.1

How to Test

  1. Set KUBECHECKS_GENERATED_STORE environment variable or use --generated-store flag
  2. Verify kubepug uses the configured store URL for deprecation checks

Additional Notes

Default store URL remains https://kubepug.xyz/data/data.json if not configured

Signed-off-by: Ken Ng <ken.ng@zapier.com>
Copy link

github-actions bot commented Mar 5, 2025

Temporary image deleted.

Signed-off-by: Ken Ng <ken.ng@zapier.com>
@wernken-ng wernken-ng marked this pull request as ready for review March 5, 2025 05:08
@wernken-ng wernken-ng requested a review from Greyeye March 5, 2025 05:09
Signed-off-by: Ken Ng <ken.ng@zapier.com>
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/config/config.go

@@ -70,6 +70,7 @@ type ServerConfig struct {
 	// -- preupgrade
 	EnablePreupgrade     bool            `mapstructure:"enable-preupgrade"`
 	WorstPreupgradeState pkg.CommitState `mapstructure:"worst-preupgrade-state"`
+	GeneratedStore       string          `mapstructure:"generated-store"`
 
 	// misc
 	AdditionalAppsNamespaces []string      `mapstructure:"additional-apps-namespaces"`

Feedback & Suggestions:

  1. Security Consideration: Ensure that the GeneratedStore field, which is a string, does not inadvertently expose sensitive information. If this field is intended to store paths or URLs, validate and sanitize the input to prevent potential security vulnerabilities such as path traversal or injection attacks. 🛡️

  2. Documentation: Consider adding a comment above the GeneratedStore field to describe its purpose and usage. This will help other developers understand its role within the ServerConfig struct. 📚

  3. Validation: If there are specific constraints or expected formats for the GeneratedStore value, consider implementing validation logic to enforce these constraints. This can prevent misconfigurations and runtime errors. ✅


😼 Mergecat review of cmd/root.go

@@ -125,6 +125,9 @@ func init() {
 	stringFlag(flags, "identifier", "Identifier for the kubechecks instance. Used to differentiate between multiple kubechecks instances.",
 		newStringOpts().
 			withDefault(""))
+	stringFlag(flags, "generated-store", "URL for the kubepug generated store.",
+		newStringOpts().
+			withDefault("https://kubepug.xyz/data/data.json"))
 
 	panicIfError(viper.BindPFlags(flags))
 	setupLogOutput()

Feedback & Suggestions:

  1. Security Concern: The default URL for the generated-store flag is hardcoded. If this URL is sensitive or can change, consider making it configurable through an environment variable or a configuration file. This will enhance security and flexibility. 🔒

  2. Validation: Ensure that the URL provided for the generated-store is validated to prevent potential misuse or errors. You might want to add a check to ensure it's a valid URL format. 🛡️

  3. Documentation: If this new flag is part of a public API or CLI, ensure that the documentation is updated to reflect this change. This will help users understand the new functionality. 📚


😼 Mergecat review of docs/usage.md

@@ -51,6 +51,7 @@ The full list of supported environment variables is described below:
 |`KUBECHECKS_ENABLE_PREUPGRADE`|Enable preupgrade checks.|`true`|
 |`KUBECHECKS_ENSURE_WEBHOOKS`|Ensure that webhooks are created in repositories referenced by argo.|`false`|
 |`KUBECHECKS_FALLBACK_K8S_VERSION`|Fallback target Kubernetes version for schema / upgrade checks.|`1.23.0`|
+|`KUBECHECKS_GENERATED_STORE`|URL for the kubepug generated store.|`https://kubepug.xyz/data/data.json`|
 |`KUBECHECKS_GITHUB_APP_ID`|Github App ID.|`0`|
 |`KUBECHECKS_GITHUB_INSTALLATION_ID`|Github Installation ID.|`0`|
 |`KUBECHECKS_GITHUB_PRIVATE_KEY`|Github App Private Key.||

Feedback & Suggestions:

  1. Security Consideration: Ensure that the URL https://kubepug.xyz/data/data.json is from a trusted source. If this URL is used to fetch data that influences application behavior, it could be a potential security risk if the source is compromised. Consider documenting the purpose of this URL and any security measures in place to verify the data integrity. 🔒

  2. Documentation Clarity: It might be helpful to provide a brief description of what the "kubepug generated store" is and how it is used within the application. This will aid users in understanding the significance of this environment variable. 📚


😼 Mergecat review of pkg/checks/preupgrade/check.go

@@ -8,5 +8,5 @@ import (
 )
 
 func Check(ctx context.Context, request checks.Request) (msg.Result, error) {
-	return checkApp(ctx, request.AppName, request.KubernetesVersion, request.YamlManifests)
+	return checkApp(ctx, request.Container, request.AppName, request.KubernetesVersion, request.YamlManifests)
 }

Feedback & Suggestions:

  1. Parameter Order Consistency: 🧐 The checkApp function now takes an additional parameter request.Container. Ensure that the order of parameters in the checkApp function definition matches the order in which they are passed here. If checkApp expects Container as the first argument, this change is correct. Otherwise, adjust the order accordingly.

  2. Type Safety: 🔍 Verify that request.Container is of the expected type for the checkApp function. If checkApp was not originally designed to handle a Container parameter, ensure that it is properly updated to handle this new input.

  3. Documentation Update: 📚 If this change alters the behavior of the Check function, update any relevant documentation or comments to reflect the new functionality, especially if Container is a critical parameter.

  4. Testing: 🧪 Ensure that there are corresponding tests for this change. Adding a new parameter could affect the function's behavior, so tests should cover scenarios with and without the Container parameter to ensure robustness.


😼 Mergecat review of pkg/checks/preupgrade/kubepug.go

@@ -7,22 +7,23 @@ import (
 	"os"
 	"strings"
 
+	"github.com/kubepug/kubepug/lib"
+	"github.com/kubepug/kubepug/pkg/results"
 	"github.com/masterminds/semver"
 	"github.com/olekukonko/tablewriter"
-	"github.com/rikatz/kubepug/lib"
-	"github.com/rikatz/kubepug/pkg/results"
 	"github.com/rs/zerolog/log"
 	"go.opentelemetry.io/otel"
 
 	"github.com/zapier/kubechecks/pkg"
+	"github.com/zapier/kubechecks/pkg/container"
 	"github.com/zapier/kubechecks/pkg/msg"
 )
 
 const docLinkFmt = "[%s Deprecation Notes](https://kubernetes.io/docs/reference/using-api/deprecation-guide/#%s-v%d%d)"
 
 var tracer = otel.Tracer("pkg/checks/preupgrade")
 
-func checkApp(ctx context.Context, appName, targetKubernetesVersion string, manifests []string) (msg.Result, error) {
+func checkApp(ctx context.Context, ctr container.Container, appName, targetKubernetesVersion string, manifests []string) (msg.Result, error) {
 	_, span := tracer.Start(ctx, "KubePug")
 	defer span.End()
 
@@ -56,14 +57,17 @@ func checkApp(ctx context.Context, appName, targetKubernetesVersion string, mani
 	if err != nil {
 		return msg.Result{}, err
 	}
+
 	config := lib.Config{
-		K8sVersion:      fmt.Sprintf("v%s", nextVersion.String()),
-		ForceDownload:   false,
-		APIWalk:         true,
-		ShowDescription: true,
-		Input:           tempDir,
+		K8sVersion:     fmt.Sprintf("v%s", nextVersion.String()),
+		Input:          tempDir,
+		GeneratedStore: ctr.Config.GeneratedStore,
+	}
+
+	kubepug, err := lib.NewKubepug(&config)
+	if err != nil {
+		return msg.Result{}, err
 	}
-	kubepug := lib.NewKubepug(config)
 
 	result, err := kubepug.GetDeprecated()
 	if err != nil {

Feedback & Suggestions:

  1. Dependency Update: The change in import paths from rikatz/kubepug to kubepug/kubepug suggests a dependency update. Ensure that the new library version is compatible with the rest of your codebase. 🛠️

  2. Error Handling: The NewKubepug function now returns an error, which is a good practice for handling potential initialization issues. Ensure that any error returned is logged for better traceability. Consider adding more context to the error message to make debugging easier. 🐛

  3. Function Signature Change: The addition of the container.Container parameter to checkApp suggests that additional configuration is being passed. Ensure that this change is reflected in all calls to checkApp throughout the codebase to prevent runtime errors. 📦

  4. Configuration Changes: The removal of ForceDownload, APIWalk, and ShowDescription from the lib.Config struct may affect the behavior of the kubepug tool. Verify that these changes align with the intended functionality and that no required features are inadvertently disabled. 🔧

  5. Code Consistency: Ensure that the formatting changes, such as the alignment of struct fields, are consistent with the rest of the codebase to maintain readability. 📏


😼 Mergecat review of go.mod

@@ -24,6 +24,7 @@ require (
 	github.com/heptiolabs/healthcheck v0.0.0-20211123025425-613501dd5deb
 	github.com/imdario/mergo v0.3.16
 	github.com/jeremywohl/flatten v1.0.1
+	github.com/kubepug/kubepug v1.7.1
 	github.com/labstack/echo-contrib v0.17.1
 	github.com/labstack/echo/v4 v4.13.3
 	github.com/masterminds/semver v1.5.0
@@ -33,7 +34,6 @@ require (
 	github.com/pkg/errors v0.9.1
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2
 	github.com/prometheus/client_golang v1.20.3
-	github.com/rikatz/kubepug v1.4.0
 	github.com/rs/zerolog v1.33.0
 	github.com/sashabaranov/go-openai v1.36.0
 	github.com/shurcooL/githubv4 v0.0.0-20231126234147-1cffa1f02456
@@ -63,6 +63,8 @@ require (
 	k8s.io/apimachinery v0.31.3
 	k8s.io/client-go v0.31.3
 	sigs.k8s.io/controller-runtime v0.19.3
+	sigs.k8s.io/kustomize/api v0.17.2
+	sigs.k8s.io/kustomize/kyaml v0.17.1
 	sigs.k8s.io/yaml v1.4.0
 )
 
@@ -145,6 +147,7 @@ require (
 	github.com/go-openapi/swag v0.23.0 // indirect
 	github.com/go-redis/cache/v9 v9.0.0 // indirect
 	github.com/gobwas/glob v0.2.3 // indirect
+	github.com/goccy/go-json v0.10.2 // indirect
 	github.com/gogo/protobuf v1.3.2 // indirect
 	github.com/golang-jwt/jwt/v4 v4.5.1 // indirect
 	github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
@@ -272,11 +275,13 @@ require (
 	go.uber.org/automaxprocs v1.5.3 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	golang.org/x/crypto v0.31.0 // indirect
+	golang.org/x/mod v0.18.0 // indirect
 	golang.org/x/sync v0.10.0 // indirect
 	golang.org/x/sys v0.28.0 // indirect
 	golang.org/x/term v0.27.0 // indirect
 	golang.org/x/text v0.21.0 // indirect
 	golang.org/x/time v0.8.0 // indirect
+	golang.org/x/tools v0.22.0 // indirect
 	google.golang.org/api v0.171.0 // indirect
 	google.golang.org/genproto v0.0.0-20240227224415-6ceb2ff114de // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20241104194629-dd2ea8efbc28 // indirect
@@ -294,6 +299,7 @@ require (
 	k8s.io/cli-runtime v0.31.3 // indirect
 	k8s.io/component-base v0.31.3 // indirect
 	k8s.io/component-helpers v0.31.3 // indirect
+	k8s.io/gengo v0.0.0-20210813121822-485abfe95c7c // indirect
 	k8s.io/klog/v2 v2.130.1 // indirect
 	k8s.io/kube-aggregator v0.31.2 // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
@@ -305,8 +311,6 @@ require (
 	olympos.io/encoding/edn v0.0.0-20201019073823-d3554ca0b0a3 // indirect
 	oras.land/oras-go/v2 v2.5.0 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
-	sigs.k8s.io/kustomize/api v0.17.2 // indirect
-	sigs.k8s.io/kustomize/kyaml v0.17.1 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.3 // indirect
 )
 

Feedback & Suggestions:

  1. Duplicate Dependency: The addition of sigs.k8s.io/kustomize/api and sigs.k8s.io/kustomize/kyaml as direct dependencies while they are also listed as indirect dependencies might lead to confusion. Ensure that these are necessary as direct dependencies, or remove them if they are not needed directly. 🧐

  2. Version Consistency: The removal of github.com/rikatz/kubepug and addition of github.com/kubepug/kubepug suggests a change in the module path. Verify that this change is intentional and that the new module path is correct. Also, ensure that the version v1.7.1 is compatible with your project requirements. 🔍

  3. Indirect Dependencies: The addition of github.com/goccy/go-json, golang.org/x/mod, golang.org/x/tools, and k8s.io/gengo as indirect dependencies should be reviewed to ensure they are necessary. Unnecessary indirect dependencies can bloat the project and complicate dependency management. 🧹

  4. Dependency Management: Regularly review and update dependencies to ensure compatibility and security. Consider using tools like go mod tidy to clean up unused dependencies. 🛠️



Dependency Review

Click to read mergecats review!

No suggestions found

@wernken-ng wernken-ng self-assigned this Mar 5, 2025
Copy link
Collaborator

@Greyeye Greyeye left a comment

Choose a reason for hiding this comment

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

LGTM

@wernken-ng wernken-ng merged commit 484c48d into main Mar 5, 2025
5 checks passed
@wernken-ng wernken-ng deleted the upgrade-kubepug branch March 5, 2025 23:55
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.

3 participants