You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign rm3l for approval. For more information see the Code Review Process.
The full list of commands accepted by this bot can be found here.
Details
Needs approval from an approver in each of these files:
The metrics server options are initialized with TLSOpts: webhookTLSOpts, which couples metrics TLS settings to webhook certificate configuration. This is likely unintended (metrics should use its own TLS opts and its own cert watcher when configured), and it can produce surprising behavior when only webhook cert flags are set.
varmetricsCertWatcher, webhookCertWatcher*certwatcher.CertWatcherwebhookTLSOpts:=tlsOptsiflen(webhookCertPath) >0 {
setupLog.Info("Initializing webhook certificate watcher using provided certificates",
"webhook-cert-path", webhookCertPath, "webhook-cert-name", webhookCertName, "webhook-cert-key", webhookCertKey)
varerrerrorwebhookCertWatcher, err=certwatcher.New(
filepath.Join(webhookCertPath, webhookCertName),
filepath.Join(webhookCertPath, webhookCertKey),
)
iferr!=nil {
setupLog.Error(err, "Failed to initialize webhook certificate watcher")
os.Exit(1)
}
webhookTLSOpts=append(webhookTLSOpts, func(config*tls.Config) {
config.GetCertificate=webhookCertWatcher.GetCertificate
})
}
// Metrics endpoint is enabled in 'config/default/kustomization.yaml'. The Metrics options configure the server.// More info:// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/metrics/server// - https://book.kubebuilder.io/reference/metrics.htmlmetricsServerOptions:= metricsserver.Options{
BindAddress: metricsAddr,
SecureServing: secureMetrics,
// TODO(user): TLSOpts is used to allow configuring the TLS config used for the server. If certificates are// not provided, self-signed certificates will be generated by default. This option is not recommended for// production environments as self-signed certificates do not offer the same level of trust and security// as certificates issued by a trusted Certificate Authority (CA). The primary risk is potentially allowing// unauthorized access to sensitive metrics data. Consider replacing with CertDir, CertName, and KeyName// to provide certificates, ensuring the server communicates using trusted and secure certificates.TLSOpts: webhookTLSOpts,
}
ifsecureMetrics {
// FilterProvider is used to protect the metrics endpoint with authn/authz.// These configurations ensure that only authorized users and service accounts// can access the metrics endpoint. The RBAC are configured in 'config/rbac/kustomization.yaml'. More info:// https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/metrics/filters#WithAuthenticationAndAuthorizationmetricsServerOptions.FilterProvider=filters.WithAuthenticationAndAuthorization
}
webhookServer:=webhook.NewServer(webhook.Options{
TLSOpts: tlsOpts,
})
iflen(metricsCertPath) >0 {
setupLog.Info("Initializing metrics certificate watcher using provided certificates",
"metrics-cert-path", metricsCertPath, "metrics-cert-name", metricsCertName, "metrics-cert-key", metricsCertKey)
varerrerrormetricsCertWatcher, err=certwatcher.New(
filepath.Join(metricsCertPath, metricsCertName),
filepath.Join(metricsCertPath, metricsCertKey),
)
iferr!=nil {
setupLog.Error(err, "Failed to initialize metrics certificate watcher")
os.Exit(1)
}
metricsServerOptions.TLSOpts=append(metricsServerOptions.TLSOpts, func(config*tls.Config) {
config.GetCertificate=metricsCertWatcher.GetCertificate
})
}
mgr, err:=ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
Metrics: metricsServerOptions,
WebhookServer: webhookServer,
checkMountWSubpath now declares its first parameter as _ Gomega but the function body still calls g.Expect(...), which will not compile because g is undefined. Rename the parameter back to g Gomega (or update the body to not reference g) and ensure the helper is invoked accordingly. (Ref 6, Ref 8)
Reference reasoning: Existing integration tests consistently pass a named g Gomega into Eventually(func(g Gomega) { ... }) and use g.Expect(...) within the scope. Keeping the parameter named aligns with the established Ginkgo/Gomega pattern and avoids undefined identifier errors.
Changed import style from "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" to v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" for consistency
Changed import style from "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" to v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" for consistency
Changed import style from "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" to v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" for consistency
Why: The suggestion correctly identifies a bug where the webhook server is not using the configured TLS options with the certificate watcher, which would prevent it from using user-provided certificates.
High
Fix incorrect path in install macro
Fix the go-install-tool macro in the Makefile by correcting the source path in the mv command to use the actual location of the installed binary.
Why: The suggestion correctly identifies a critical bug in the go-install-tool macro where the mv command uses an incorrect source path, which would cause the tool installation to fail.
High
Remove contradictory dependency replacement
Remove the replace directive for sigs.k8s.io/controller-runtime in go.mod as it contradicts the version specified in the require block and prevents the dependency upgrade.
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a contradictory replace directive that prevents the intended upgrade of sigs.k8s.io/controller-runtime, which could lead to build or runtime issues.
Medium
Fail fast on test setup error
Modify getFirstFoundEnvTestBinaryDir to panic if it fails to read the directory, ensuring the test setup fails fast with a clear error message.
func getFirstFoundEnvTestBinaryDir() string {
basePath := filepath.Join("..", "bin", "k8s")
entries, err := os.ReadDir(basePath)
if err != nil {
- logf.Log.Error(err, "Failed to read directory", "path", basePath)- return ""+ // If we can't read the directory, it's a setup problem. Let's fail fast.+ panic(fmt.Sprintf("failed to read envtest binary directory at %s: %v", basePath, err))
}
for _, entry := range entries {
if entry.IsDir() {
return filepath.Join(basePath, entry.Name())
}
}
return ""
}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly proposes to fail fast on a test setup error by using panic, which improves test suite robustness and makes debugging setup issues easier.
Low
General
Decouple metrics and webhook TLS configurations
Decouple the metrics server's TLS configuration from the webhook's by initializing metricsServerOptions.TLSOpts with the base tlsOpts instead of webhookTLSOpts.
metricsServerOptions := metricsserver.Options{
BindAddress: metricsAddr,
SecureServing: secureMetrics,
// TODO(user): TLSOpts is used to allow configuring the TLS config used for the server. If certificates are
// not provided, self-signed certificates will be generated by default. This option is not recommended for
// production environments as self-signed certificates do not offer the same level of trust and security
// as certificates issued by a trusted Certificate Authority (CA). The primary risk is potentially allowing
// unauthorized access to sensitive metrics data. Consider replacing with CertDir, CertName, and KeyName
// to provide certificates, ensuring the server communicates using trusted and secure certificates.
- TLSOpts: webhookTLSOpts,+ TLSOpts: tlsOpts,
}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly points out that coupling the metrics server's TLS options with the webhook's is confusing and bad practice, proposing a change that improves code clarity and robustness.
Low
Fix PKGS exclude regex
Update the regex in the PKGS variable in the Makefile to correctly match multi-digit alpha versions, such as v1alpha10.
-PKGS := $(shell go list ./... | grep -vE 'github.com/redhat-developer/rhdh-operator/(tests/|api/v1alpha([1-9]+))')+PKGS := $(shell go list ./... | grep -vE 'github.com/redhat-developer/rhdh-operator/(tests/|api/v1alpha[1-9][0-9]*)')
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies a flaw in the regex that would fail to exclude API versions with two or more digits (e.g., v1alpha10), improving the robustness of the package filter.
Low
Return nil for unneeded cancel function
In appUrlProvider, return nil for the cancelFunc instead of an empty function to explicitly indicate that no cleanup action is needed.
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 5
__
Why: The suggestion correctly points out that returning nil instead of an empty function for a cancelFunc is more idiomatic and clearer, improving code readability and maintainability.
Low
Organization best practice
Make manifest defaults explicit
Explicitly set --metrics-secure=true in the manager args to document/lock in the intended secure behavior now that the code default changed.
Why:
Relevant best practice - Keep manifests synchronized with implementation defaults and behavior to avoid user confusion (explicitly set key flags when defaults change).
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
rhdh-qodo-merge
bot
added
Review effort 4/5
enhancement
Description
Bumps the version of the operator-sdk, which requires updating the linter configuration (and fixing the issues reported).
Which issue(s) does this PR fix or relate to
PR acceptance criteria
How to test changes / Special notes to the reviewer