get services from API, removing sidecar requirement#772
Conversation
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
📝 WalkthroughWalkthroughThe system transitions service URL loading from file-based discovery to API-based discovery. The scan failure reporter initialization now derives the service endpoint from an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/config.go (1)
293-295: Wrap client-construction errors with API URL context.At Line 295, returning the raw error makes debugging harder upstream. Include
apiURLin the error to make misconfiguration/root-cause visible.Proposed patch
func LoadServiceURLs(apiURL string) (schema.IBackendServices, error) { client, err := servicediscoveryv3.NewServiceDiscoveryClientV3(apiURL) if err != nil { - return nil, err + return nil, fmt.Errorf("create service discovery client for apiURL %q: %w", apiURL, err) } return servicediscovery.GetServices(client) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config.go` around lines 293 - 295, The call to servicediscoveryv3.NewServiceDiscoveryClientV3(apiURL) returns a raw error that is returned as-is (return nil, err); change this to wrap the error with the apiURL context so callers can see which API endpoint failed (e.g., replace the plain return with a wrapped error using fmt.Errorf or errors.Wrapf mentioning apiURL), and add the necessary import if missing; keep the existing client/err variables and only modify the error return site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Around line 402-405: The current LoadServiceURLs call swallows svcErr when
service discovery fails; update the block around LoadServiceURLs so that if
svcErr != nil you emit a warning via the logger (include svcErr.Error()) and
only proceed to create failureReporter when svcErr == nil and
services.GetReportReceiverHttpUrl() != ""; also log a warning if svcErr == nil
but services.GetReportReceiverHttpUrl() is empty to indicate a missing
configured URL; reference the LoadServiceURLs call, the svcErr variable,
services.GetReportReceiverHttpUrl(), and failureReporter when making these
changes.
- Around line 398-401: The current apiURL variable may be a bare hostname which
breaks NewServiceDiscoveryClientV3() because it expects an absolute URL; change
the default to include a scheme (e.g., "https://api.armosec.io") and normalize
any user-provided API_URL by parsing it (using url.Parse) and prepending
"https://" when the scheme is empty before passing to
NewServiceDiscoveryClientV3(); update the apiURL assignment/normalization logic
around the apiURL variable to perform this fix.
---
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 293-295: The call to
servicediscoveryv3.NewServiceDiscoveryClientV3(apiURL) returns a raw error that
is returned as-is (return nil, err); change this to wrap the error with the
apiURL context so callers can see which API endpoint failed (e.g., replace the
plain return with a wrapped error using fmt.Errorf or errors.Wrapf mentioning
apiURL), and add the necessary import if missing; keep the existing client/err
variables and only modify the error return site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28210547-e38c-497e-9cd0-97df6382a2db
📒 Files selected for processing (2)
cmd/main.gopkg/config/config.go
| apiURL := os.Getenv("API_URL") | ||
| if apiURL == "" { | ||
| apiURL = "api.armosec.io" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In github.com/kubescape/backend/pkg/servicediscovery/v3, does NewServiceDiscoveryClientV3 require an absolute URL including scheme (e.g., https://...), or does it accept bare hostnames like api.armosec.io?
💡 Result:
NewServiceDiscoveryClientV3 in github.com/kubescape/backend/pkg/servicediscovery/v3 likely requires an absolute URL including scheme. Evidence from Kubescape operator usage shows service discovery fetching from https://api.armosec.io/api/v1/servicediscovery, indicating HTTPS absolute URLs are used. Bare hostnames like api.armosec.io are used in contexts like firewall rules but curl commands prepend https://, suggesting the client expects full URLs. No source code found explicitly stating bare hostnames are accepted, and standard Go HTTP clients typically require schemes for absolute URLs.
Citations:
- 1: Kubescape helm install fails | kollector-0 pod crashes kubescape#1461
- 2: https://github.com/kubescape/helm-charts/blob/main/charts/kubescape-operator/values.yaml
- 3: https://github.com/armosec/kubescape/blob/master/go.mod
🏁 Script executed:
# Find and read cmd/main.go around lines 398-401
cat -n cmd/main.go | sed -n '395,405p'Repository: kubescape/node-agent
Length of output: 776
🏁 Script executed:
# Search for NewServiceDiscoveryClientV3 to understand its signature and usage
rg "NewServiceDiscoveryClientV3" -A 3 -B 1Repository: kubescape/node-agent
Length of output: 348
🏁 Script executed:
# Look for how apiURL is used after being set
rg "apiURL" cmd/main.go -A 2 -B 2Repository: kubescape/node-agent
Length of output: 695
🏁 Script executed:
# Check LoadServiceURLs implementation
rg "func LoadServiceURLs" -A 10 pkg/config/config.goRepository: kubescape/node-agent
Length of output: 446
🏁 Script executed:
# Check if servicediscoveryv3 is from an external module and its go.mod
cat go.mod | grep -i servicediscoveryRepository: kubescape/node-agent
Length of output: 46
🏁 Script executed:
# Look for any URL normalization or scheme-handling in the codebase
rg "https://" cmd/main.go | head -20Repository: kubescape/node-agent
Length of output: 46
🏁 Script executed:
# Check if there are other places where api.armosec.io is set and how
rg "armosec.io" -B 2 -A 2Repository: kubescape/node-agent
Length of output: 6609
🏁 Script executed:
# Search for other calls to LoadServiceURLs
rg "LoadServiceURLs" -B 2 -A 2Repository: kubescape/node-agent
Length of output: 823
🏁 Script executed:
# Check imports in cmd/main.go to see if strings is available
head -50 cmd/main.go | grep -E "^import|strings"Repository: kubescape/node-agent
Length of output: 83
🏁 Script executed:
# Check if there are tests for LoadServiceURLs or config
fd "config.*test" --type fRepository: kubescape/node-agent
Length of output: 89
🏁 Script executed:
# Check config_test.go to see if there are tests for LoadServiceURLs
cat -n pkg/config/config_test.go | head -50Repository: kubescape/node-agent
Length of output: 2052
🏁 Script executed:
# Search for how the scheme is handled elsewhere in similar code
rg "://" pkg/config/ -B 1 -A 1Repository: kubescape/node-agent
Length of output: 1890
Add URL scheme to default API_URL and normalize user-provided values.
Line 400 defaults to api.armosec.io without a scheme. When passed to NewServiceDiscoveryClientV3(), this bare hostname will fail service discovery (which requires an absolute URL), preventing scan-failure reporting from being enabled.
Additionally, users who provide a custom API_URL environment variable without a scheme will encounter the same failure.
Proposed patch
apiURL := os.Getenv("API_URL")
if apiURL == "" {
- apiURL = "api.armosec.io"
+ apiURL = "https://api.armosec.io"
+ } else if !strings.Contains(apiURL, "://") {
+ apiURL = "https://" + apiURL
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/main.go` around lines 398 - 401, The current apiURL variable may be a
bare hostname which breaks NewServiceDiscoveryClientV3() because it expects an
absolute URL; change the default to include a scheme (e.g.,
"https://api.armosec.io") and normalize any user-provided API_URL by parsing it
(using url.Parse) and prepending "https://" when the scheme is empty before
passing to NewServiceDiscoveryClientV3(); update the apiURL
assignment/normalization logic around the apiURL variable to perform this fix.
| if services, svcErr := config.LoadServiceURLs(apiURL); svcErr == nil && services.GetReportReceiverHttpUrl() != "" { | ||
| failureReporter = sbommanagerv1.NewHTTPSbomFailureReporter(services.GetReportReceiverHttpUrl(), accessKey, clusterData.AccountID, clusterData.ClusterName) | ||
| logger.L().Info("scan failure reporting enabled", helpers.String("eventReceiverURL", services.GetReportReceiverHttpUrl())) | ||
| } |
There was a problem hiding this comment.
Don’t swallow service-discovery errors silently.
At Line 402, svcErr is ignored on failure. Add a warning log so misconfiguration/network issues are diagnosable.
Proposed patch
if services, svcErr := config.LoadServiceURLs(apiURL); svcErr == nil && services.GetReportReceiverHttpUrl() != "" {
failureReporter = sbommanagerv1.NewHTTPSbomFailureReporter(services.GetReportReceiverHttpUrl(), accessKey, clusterData.AccountID, clusterData.ClusterName)
logger.L().Info("scan failure reporting enabled", helpers.String("eventReceiverURL", services.GetReportReceiverHttpUrl()))
+ } else if svcErr != nil {
+ logger.L().Warning("scan failure reporting disabled: unable to load backend services", helpers.Error(svcErr), helpers.String("apiURL", apiURL))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if services, svcErr := config.LoadServiceURLs(apiURL); svcErr == nil && services.GetReportReceiverHttpUrl() != "" { | |
| failureReporter = sbommanagerv1.NewHTTPSbomFailureReporter(services.GetReportReceiverHttpUrl(), accessKey, clusterData.AccountID, clusterData.ClusterName) | |
| logger.L().Info("scan failure reporting enabled", helpers.String("eventReceiverURL", services.GetReportReceiverHttpUrl())) | |
| } | |
| if services, svcErr := config.LoadServiceURLs(apiURL); svcErr == nil && services.GetReportReceiverHttpUrl() != "" { | |
| failureReporter = sbommanagerv1.NewHTTPSbomFailureReporter(services.GetReportReceiverHttpUrl(), accessKey, clusterData.AccountID, clusterData.ClusterName) | |
| logger.L().Info("scan failure reporting enabled", helpers.String("eventReceiverURL", services.GetReportReceiverHttpUrl())) | |
| } else if svcErr != nil { | |
| logger.L().Warning("scan failure reporting disabled: unable to load backend services", helpers.Error(svcErr), helpers.String("apiURL", apiURL)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/main.go` around lines 402 - 405, The current LoadServiceURLs call
swallows svcErr when service discovery fails; update the block around
LoadServiceURLs so that if svcErr != nil you emit a warning via the logger
(include svcErr.Error()) and only proceed to create failureReporter when svcErr
== nil and services.GetReportReceiverHttpUrl() != ""; also log a warning if
svcErr == nil but services.GetReportReceiverHttpUrl() is empty to indicate a
missing configured URL; reference the LoadServiceURLs call, the svcErr variable,
services.GetReportReceiverHttpUrl(), and failureReporter when making these
changes.
1 participant
Summary by CodeRabbit
API_URLenvironment variable for dynamic endpoint configuration, defaulting toapi.armosec.iowhen unset.