e2e: fix test issues #154
e2e: fix test issues #154
Conversation
WalkthroughThe changes introduce a helper for URL query escaping in Prometheus scaler code, update and streamline Kubernetes end-to-end test manifests and scripts, and refactor test steps for clarity and explicitness. Several deployment definitions are removed from test YAMLs, commands are restructured for better readability, and a new Pod manifest is added for curl-based testing. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant K8sCluster
participant CurlPod
TestRunner->>K8sCluster: Apply pod-curl-target-gw.yaml (create CurlPod)
TestRunner->>K8sCluster: Scale target-deployment to 0
TestRunner->>K8sCluster: Wait for target pods to be deleted
TestRunner->>K8sCluster: Scale target-deployment to 1 (if needed)
TestRunner->>K8sCluster: Wait for target pods to be ready
TestRunner->>CurlPod: kubectl exec curl to service endpoint
CurlPod-->>TestRunner: Return HTTP status code/result
sequenceDiagram
participant PrometheusScaler
participant PrometheusServer
PrometheusScaler->>PrometheusScaler: queryEscape(query)
PrometheusScaler->>PrometheusServer: GET /api/v1/query?query=escaped_query
PrometheusServer-->>PrometheusScaler: Query result
Estimated code review effort2 (~20 minutes) Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (2)
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (1)
4-28: Harden shell script for early failuresAdd strict mode to surface errors quickly and avoid silent mis-matches (e.g., empty variables).
- - script: | - #!/bin/sh + - script: | + #!/bin/sh + set -euo pipefailOptional but improves debuggability and reliability.
pkg/scaling/scalers/prometheus_scaler.go (1)
32-41: Potential data race on the globalpromQueryResponsevariable
promQueryResponseis package-level and shared across all concurrent calls toexecutePromQuery. If your controller runs multiple scaler instances in parallel, JSON decoding will race and corrupt data.Fix: make it a local variable inside
executePromQuery:-func (s *prometheusScaler) executePromQuery(...) (float64, error) { - ... - if err := json.NewDecoder(resp.Body).Decode(&promQueryResponse); err != nil { +func (s *prometheusScaler) executePromQuery(...) (float64, error) { + var promQueryResponse struct { + Status string `json:"status"` + ... + } + if err := json.NewDecoder(resp.Body).Decode(&promQueryResponse); err != nil {
♻️ Duplicate comments (1)
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (1)
4-6: Same namespace / resource-type inconsistency as aboveReplicate the correction here to avoid flaky waits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
pkg/scaling/scalers/prometheus_scaler.go(2 hunks)tests/e2e/kuttl-test.yaml(1 hunks)tests/e2e/manifest/pod-curl-target-gw.yaml(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/manifest/target-elastiservice.yaml(1 hunks)tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml(1 hunks)tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tests/e2e/manifest/reset.sh
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 11-11: Double quote to prevent globbing and word splitting.
(SC2086)
🪛 Checkov (3.2.334)
tests/e2e/manifest/pod-curl-target-gw.yaml
[MEDIUM] 2-14: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[LOW] 2-14: CPU limits should be set
(CKV_K8S_11)
[LOW] 2-14: CPU requests should be set
(CKV_K8S_10)
[LOW] 2-14: Apply security context to your containers
(CKV_K8S_30)
[LOW] 2-14: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[LOW] 2-14: Image should use digest
(CKV_K8S_43)
[LOW] 2-14: Image Pull Policy should be Always
(CKV_K8S_15)
[LOW] 2-14: Liveness Probe Should be Configured
(CKV_K8S_8)
[LOW] 2-14: Memory limits should be set
(CKV_K8S_13)
[LOW] 2-14: Memory requests should be set
(CKV_K8S_12)
[LOW] 2-14: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[LOW] 2-14: Apply security context to your pods and containers
(CKV_K8S_29)
[LOW] 2-14: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[LOW] 2-14: Readiness Probe Should be Configured
(CKV_K8S_9)
[MEDIUM] 2-14: Minimize the admission of root containers
(CKV_K8S_23)
[LOW] 2-14: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[LOW] 2-14: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[LOW] 2-14: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🪛 YAMLlint (1.37.1)
tests/e2e/manifest/pod-curl-target-gw.yaml
[error] 11-11: wrong indentation: expected 4 but found 2
(indentation)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
🔇 Additional comments (6)
tests/e2e/manifest/target-elastiservice.yaml (1)
8-8: Cooldown raised to 30 s – double-check it matches metric scrape intervalLooks good but make sure Prometheus
scrape_interval≥ 30 s, otherwise the scaler will act on stale data.tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (1)
4-6: NamespacetargetPortlooks suspiciousScaling happens in
target, but the wait usestargetPort. Is this intentional? If not, the wait will never complete.- - command: kubectl wait --for=delete pods -l app=target-deployment -n targetPort + - command: kubectl wait --for=delete pod -l app=target-deployment -n target --timeout=60stests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (1)
4-5: Namespace mismatch between scale and wait commands
kubectl scaletargets namespacetarget, whereas the subsequentkubectl waitwatches intargetPort. If the pods truly live intarget, the wait will never observe their deletion and the step will hit the 60-second timeout, yielding a false failure.- - command: kubectl wait --for=delete pods -l app=target-deployment -n targetPort + - command: kubectl wait --for=delete pods -l app=target-deployment -n targetPlease verify the intended namespace and align both commands accordingly.
tests/e2e/kuttl-test.yaml (1)
9-10: Looks good – curl pod applied early in suite bootstrapThe additional
kubectl applycorrectly installs the helper pod before any traffic-related steps. Usingnamespaced: falseis appropriate because the command already specifies-n target.pkg/scaling/scalers/prometheus_scaler.go (1)
79-81: 👍 Correct fix for the Gourl.QueryEscapespace bugSwitching to the new helper eliminates the
+vs%20problem that broke Prometheus queries.tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml (1)
3-6: LGTM – consistent scale/-wait pattern
The step uses the same command structure adopted across the suite and the correct namespace.
tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
pkg/scaling/scalers/prometheus_scaler.go (1)
32-41: Shared globalpromQueryResponsecauses data races under concurrencyThe JSON is decoded into a package-level variable that is overwritten on every call.
If multiple scalers callexecutePromQueryconcurrently (common in parallel reconcile loops) this leads to undefined behaviour and crashes under-race.Move the struct into
executePromQueryso each invocation has its own instance.- if err := json.NewDecoder(resp.Body).Decode(&promQueryResponse); err != nil { + var promQueryResponse struct { + Status string `json:"status"` + Data struct { + ResultType string `json:"resultType"` + Result []struct { + Metric map[string]string `json:"metric"` + Value []interface{} `json:"value"` + } `json:"result"` + } `json:"data"` + } + if err := json.NewDecoder(resp.Body).Decode(&promQueryResponse); err != nil {tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (1)
3-26:jqdependency + wrong jsonpath makes the count always fail
- The runner image often lacks
jq; the step will error out.-o jsonpath='{.endpoints}'returns a Go-formatted map string, not valid JSON, sojq 'length'will exit non-zero even ifjqwere present.Proposed self-contained fix eliminating the external tool and using native JSONPath:
- ENDPOINTS_COUNT=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 -o jsonpath='{.endpoints}' | jq 'length') + ENDPOINTS_COUNT=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 \ + -o jsonpath='{.endpoints[*].addresses}' | tr ' ' '\n' | grep -c .)This counts the addresses with shell built-ins only.
If keeping
jq, ensure the test image includes it and switch to-o json:kubectl get ... -o json | jq '.endpoints | length'
♻️ Duplicate comments (3)
tests/e2e/manifest/pod-curl-target-gw.yaml (1)
13-14: Container never starts –infitiytypo breaks the pod
sleepexits immediately because the argument is misspelled, causing the pod to CrashLoop and all subsequentkubectl execsteps to fail.- command: [ "sleep" ] - args: [ "infitiy" ] + command: ["sleep"] + args: ["infinity"]Please fix the typo and consider adding a minimal
securityContext/ resource stanza so the manifest passes basic cluster policies.pkg/scaling/scalers/prometheus_scaler.go (1)
69-75: Minor optimisation: reuse astrings.Replacerto avoid per-call allocations
strings.ReplaceAllallocates on every invocation. A package-levelvar spaceReplacer = strings.NewReplacer("+", "%20")keeps it zero-alloc and thread-safe in hot paths.-func queryEscape(query string) string { - queryEscaped := url.QueryEscape(query) - plusEscaped := strings.ReplaceAll(queryEscaped, "+", "%20") - return plusEscaped -} +var spaceReplacer = strings.NewReplacer("+", "%20") + +func queryEscape(query string) string { + return spaceReplacer.Replace(url.QueryEscape(query)) +}tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (1)
6-15: Re-enable exit-status check and use POSIX‐compliant string comparisonNetwork / DNS failures are currently ignored because the
curlexit status validation is commented out.
Additionally,[…]with==is a Bashism; the POSIX/bin/shused bykuttlpods only guarantees=.- # [ "$result" == "0" ] || exit 1 - [ "$code" == "200" ] || exit 2 + [ "$result" = "0" ] || exit 1 # network / DNS failure + [ "$code" = "200" ] || exit 2 # unexpected HTTP statusKeeping distinct exit codes makes post-mortem debugging easier and eliminates false positives when the request never reaches the service.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
pkg/scaling/scalers/prometheus_scaler.go(2 hunks)tests/e2e/kuttl-test.yaml(1 hunks)tests/e2e/manifest/pod-curl-target-gw.yaml(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/manifest/target-elastiservice.yaml(1 hunks)tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml(1 hunks)tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml
🧰 Additional context used
🪛 Checkov (3.2.334)
tests/e2e/manifest/pod-curl-target-gw.yaml
[MEDIUM] 2-14: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[LOW] 2-14: CPU limits should be set
(CKV_K8S_11)
[LOW] 2-14: CPU requests should be set
(CKV_K8S_10)
[LOW] 2-14: Apply security context to your containers
(CKV_K8S_30)
[LOW] 2-14: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[LOW] 2-14: Image should use digest
(CKV_K8S_43)
[LOW] 2-14: Image Pull Policy should be Always
(CKV_K8S_15)
[LOW] 2-14: Liveness Probe Should be Configured
(CKV_K8S_8)
[LOW] 2-14: Memory limits should be set
(CKV_K8S_13)
[LOW] 2-14: Memory requests should be set
(CKV_K8S_12)
[LOW] 2-14: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[LOW] 2-14: Apply security context to your pods and containers
(CKV_K8S_29)
[LOW] 2-14: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[LOW] 2-14: Readiness Probe Should be Configured
(CKV_K8S_9)
[MEDIUM] 2-14: Minimize the admission of root containers
(CKV_K8S_23)
[LOW] 2-14: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[LOW] 2-14: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[LOW] 2-14: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🪛 YAMLlint (1.37.1)
tests/e2e/manifest/pod-curl-target-gw.yaml
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
🔇 Additional comments (8)
tests/e2e/manifest/target-elastiservice.yaml (1)
8-8: Verify that the longercooldownPeriodwon’t mask legitimate scale-down eventsBumping the period from 5 → 30 s gives the scaler six-times more inertia.
Double-check that this still satisfies SLOs (especially latency/CPU budgets) and matches the Prometheus scrape interval used in tests.tests/e2e/manifest/reset.sh (1)
8-11: Quoting fix looks goodThe previously unquoted
$1arguments are now safely wrapped in quotes; this removes the ShellCheck warnings and prevents word-splitting issues.tests/e2e/kuttl-test.yaml (1)
9-10: Nokubectl execinvocations in this TestSuite – waiting for Pod readiness isn’t neededI reviewed tests/e2e/kuttl-test.yaml and did not find any
kubectl execcommands in this file. It only applies manifests and runs a reset script. Since there are no exec steps that could race against Pod readiness, adding akubectl waithere isn’t necessary.Likely an incorrect or invalid review comment.
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (1)
3-5: Commands split out cleanly – good fix 🟢Moving from a wrapped shell script to discrete
kubectlcommands greatly improves readability and avoids the old plural-resource pitfall.
No functional concerns here.tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml (1)
3-7: Scale-down / up sequence looks solidThe namespace typo and plural-resource issue from the previous revision are both resolved – nice catch.
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml (1)
3-5: Solid clean-up; plural alias issue goneBreaking the shell script into explicit commands and switching to the singular
podresource removes the flake seen earlier.tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml (1)
3-5: Namespace corrected – flake fixedThe old
targetPorttypo is gone, so the wait will no longer hang. 👍tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (1)
3-5: Clean, explicit step definitionThe plural alias issue is resolved and the step is now self-describing.
tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tests/e2e/manifest/reset.sh (1)
17-18: Re-order & singularisekubectl waitfor portability
kubectl waitexpects flags before the resource selector. Using the canonical order avoids “unknown flag” errors on stricter versions. Also switch to singularpod.-kubectl wait pods -l app=target-deployment -n target --for=condition=Ready --timeout=30s -kubectl wait pods -l app=elasti-resolver -n elasti --for=condition=Ready --timeout=30s +kubectl wait --for=condition=Ready pod -l app=target-deployment -n target --timeout=30s +kubectl wait --for=condition=Ready pod -l app=elasti-resolver -n elasti --timeout=30s
♻️ Duplicate comments (11)
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (2)
5-6: Enable fail-fast behaviour withset -euo pipefailThe script still runs in permissive mode: any sub-command that fails will be silently ignored, potentially yielding false-positive test passes.
-#!/bin/sh +#!/bin/sh +set -euo pipefail
12-13: Hard-coded EndpointSlice name is brittle; resolve it dynamicallyThe suffix
9696239e87changes across clusters/k8s versions. Hard-coding it will break the test on fresh installs or future upgrades.-# Check if resolver IPs are present in the endpointslice -ENDPOINTSLICE_IPS=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 -o jsonpath='{.endpoints[*].addresses[*]}') +# Resolve EndpointSlice name dynamically based on labels +ES_NAME=$(kubectl get endpointslice -n target -l app=target-deployment -o jsonpath='{.items[0].metadata.name}') +# Check if resolver IPs are present in the endpointslice +ENDPOINTSLICE_IPS=$(kubectl get endpointslice -n target "$ES_NAME" -o jsonpath='{.endpoints[*].addresses[*]}')tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (2)
16-16: External dependency onjq(repeat of earlier feedback)
The reliance onjqnoted in the previous review is still present here. Either bundlejqin the test image or drop the dependency as suggested above.
15-25: Remove thejqdependency & harden the numeric checkThe runner image still might not contain
jq, and the current comparison will break ifENDPOINTS_COUNTis non-numeric (e.g. empty/null). A pure-kubectlsolution keeps the test self-contained and eliminates that failure mode.- ENDPOINTS_COUNT=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 -o jsonpath='{.endpoints}' | jq 'length') + # Count addresses without relying on external tools + ENDPOINTS_COUNT=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 \ + -o jsonpath='{range .endpoints[*]}{.addresses[*]}{"\n"}{end}' | wc -l)Consider also adding
set -euo pipefailnear the top of the script to fail fast on any error:- #!/bin/sh + #!/usr/bin/env sh + set -euo pipefailpkg/scaling/scalers/prometheus_scaler.go (1)
69-75: Reuse a shared strings.Replacer to avoid per-call allocations
The helper now allocates a new replacement map for every query, which is hot-path (called once per decision). The same refactor was suggested previously.+var spaceReplacer = strings.NewReplacer("+", "%20") // alloc-free & thread-safe + func queryEscape(query string) string { - queryEscaped := url.QueryEscape(query) - plusEscaped := strings.ReplaceAll(queryEscaped, "+", "%20") - - return plusEscaped + return spaceReplacer.Replace(url.QueryEscape(query)) }tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml (1)
3-4: Style: keep flag form consistent (--timeout=60s)
Same nit as earlier review – using the=form matches other steps and avoids rare parsing quirks.- - command: kubectl wait --for=condition=Ready pod -l app=target-deployment -n target --timeout 60s + - command: kubectl wait --for=condition=Ready pod -l app=target-deployment -n target --timeout=60stests/e2e/manifest/pod-curl-target-gw.yaml (1)
10-14: Harden the curl pod & satisfy basic policy gatesRunning as root with no limits will be flagged by most clusters. Minimal hardening suffices for tests.
spec: containers: - - name: curl - image: ghcr.io/curl/curl-container/curl:master - command: [ "sleep" ] - args: [ "infinity" ] + - name: curl + image: ghcr.io/curl/curl-container/curl:master + imagePullPolicy: IfNotPresent + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 50m + memory: 128Mi + command: ["sleep"] + args: ["infinity"] restartPolicy: Nevertests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml (1)
4-5: Timeout still tight & plural resource – previous feedback unresolvedThe earlier review (see history) suggested (a) bumping the timeout to 120 s to reduce CI flakes and (b) using singular
pod. Neither change landed.- - command: kubectl wait --for=condition=Ready pods -l app=target-deployment -n target --timeout=60s + - command: kubectl wait --for=condition=Ready pod -l app=target-deployment -n target --timeout=120stests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml (1)
4-5: Keep resource kind singular for consistency (already raised earlier)We previously suggested using the singular resource (
pod) to avoid plural alias quirks and stay consistent with the rest of the suite, but the command is still usingpods.- - command: kubectl wait --for=delete pods -l app=elasti-resolver -n elasti --timeout=60s + - command: kubectl wait --for=delete pod -l app=elasti-resolver -n elasti --timeout=60stests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml (1)
7-7: Usekubectl rollout statusfor a more reliable readiness checkWaiting on individual Pods can pass when only a subset is ready or if Pods churn;
kubectl rollout statusgives a higher-level, deployment-aware signal.- - command: kubectl wait --for=condition=Ready pods -l app=target-deployment -n target --timeout=60s + - command: kubectl rollout status deployment/target-deployment -n target --timeout=60stests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (1)
12-13: Re-enable the curl exit-status check to catch hard network failuresThis was pointed out earlier but is still commented out; without it, DNS/connection errors are silently ignored, giving false positives.
- # if [ "$result" != "0" ]; then exit 1; fi + [ "$result" = "0" ] || exit 1 # network / DNS failure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
pkg/scaling/scalers/prometheus_scaler.go(2 hunks)tests/e2e/kuttl-test.yaml(1 hunks)tests/e2e/manifest/pod-curl-target-gw.yaml(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/manifest/target-elastiservice.yaml(1 hunks)tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml(1 hunks)tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rethil
PR: truefoundry/KubeElasti#153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
pkg/scaling/scalers/prometheus_scaler.go (1)
Learnt from: rethil
PR: #153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
🪛 Checkov (3.2.334)
tests/e2e/manifest/pod-curl-target-gw.yaml
[MEDIUM] 2-14: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[LOW] 2-14: CPU limits should be set
(CKV_K8S_11)
[LOW] 2-14: CPU requests should be set
(CKV_K8S_10)
[LOW] 2-14: Apply security context to your containers
(CKV_K8S_30)
[LOW] 2-14: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[LOW] 2-14: Image should use digest
(CKV_K8S_43)
[LOW] 2-14: Image Pull Policy should be Always
(CKV_K8S_15)
[LOW] 2-14: Liveness Probe Should be Configured
(CKV_K8S_8)
[LOW] 2-14: Memory limits should be set
(CKV_K8S_13)
[LOW] 2-14: Memory requests should be set
(CKV_K8S_12)
[LOW] 2-14: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[LOW] 2-14: Apply security context to your pods and containers
(CKV_K8S_29)
[LOW] 2-14: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[LOW] 2-14: Readiness Probe Should be Configured
(CKV_K8S_9)
[MEDIUM] 2-14: Minimize the admission of root containers
(CKV_K8S_23)
[LOW] 2-14: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[LOW] 2-14: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[LOW] 2-14: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🪛 YAMLlint (1.37.1)
tests/e2e/manifest/pod-curl-target-gw.yaml
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
🔇 Additional comments (6)
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (1)
3-4: Migration tocommandsarray is spot-onSwitching from the deprecated single
scriptfield to thecommandslist is fully aligned with current kuttl expectations and eliminates the parser warnings we were seeing.tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (1)
3-4:commandslist looks good – kuttl will now execute the stepSwitching from the (ignored)
command:key tocommands:fixes the schema mismatch that previously caused the step to be skipped.tests/e2e/manifest/target-elastiservice.yaml (1)
8-8: Cooldown bump might stall fast-flapping workloadsGoing from 5 → 30 seconds means Elasti will ignore legitimate scale signals for half a minute. Double-check that this aligns with your Prom scrape interval and business SLOs; otherwise scale-up/-down latency will increase noticeably.
tests/e2e/kuttl-test.yaml (1)
9-10: Race: pod may not be ready before laterkubectl execcalls
kubectl applyis fire-and-forget; subsequent steps that exec intocurl-target-gwcan fail if the container isn’t yet Running. Add an explicit wait:- - command: kubectl apply -f ./manifest/pod-curl-target-gw.yaml -n target + - command: kubectl apply -f ./manifest/pod-curl-target-gw.yaml -n target + - command: kubectl wait --for=condition=Ready pod/curl-target-gw -n target --timeout=60stests/e2e/manifest/reset.sh (1)
8-11: 👍 Quoting fixes look goodThanks for quoting
$1; this removes the ShellCheck SC2086 warnings and guards against paths containing spaces.tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (1)
3-5: LGTM – commands are clear and robustThe explicit scale-down and pod-deletion wait align with the new test-suite style. No further action needed.
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
tests/e2e/manifest/reset.sh (1)
1-7: Add strict-mode guards (set -euo pipefail) and argument validation earlyWithout
set -euo pipefailthe script happily continues on silent failures (e.g., a bad manifest path or a failedkubectlapply).
Guarding for the single required argument also prevents obscure “file not found” errors later.#!/bin/sh +set -euo pipefail + +# Expect exactly one argument – path to manifest dir +if [ "$#" -ne 1 ]; then + echo "Usage: $0 <path-to-manifest-dir>" >&2 + exit 64 # EX_USAGE +fi
♻️ Duplicate comments (3)
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (1)
3-27: Same robustness concerns as step 05
For consistency, consider addingset -euo pipefailand resolving the EndpointSlice name dynamically here as well.tests/e2e/manifest/pod-curl-target-gw.yaml (1)
1-14: Well-designed test utility pod.This curl gateway pod provides a clean solution for running in-cluster traffic tests, addressing the networking limitations mentioned in the PR description. The
sleep infinityapproach ensures the pod stays available forkubectl execcommands.For production-like testing, consider adding basic resource limits to prevent resource exhaustion during test runs:
containers: - name: curl image: ghcr.io/curl/curl-container/curl:master command: ["sleep"] args: ["infinity"] + resources: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 50m + memory: 128Mitests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (1)
4-13: Re-enable exit-status check to catch network/exec failuresIf
kubectl execorcurlfails (DNS, connection reset, pod crash) the loop still reports HTTP code000and passes, masking real outages.- # if [ "$result" != "0" ]; then exit 1; fi + if [ "$result" != "0" ]; then + echo "kubectl exec or curl failed with exit code $result" >&2 + exit 1 + fiOptionally add
set -eright after the shebang for immediate failure propagation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
pkg/scaling/scalers/prometheus_scaler.go(2 hunks)tests/e2e/README.md(1 hunks)tests/e2e/kuttl-test.yaml(1 hunks)tests/e2e/manifest/pod-curl-target-gw.yaml(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/manifest/target-elastiservice.yaml(1 hunks)tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml(1 hunks)tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like `jq` should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: `jq` should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like `tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml` for JSON parsing.
Learnt from: rethil
PR: truefoundry/KubeElasti#153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
tests/e2e/README.md (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/kuttl-test.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
pkg/scaling/scalers/prometheus_scaler.go (2)
Learnt from: rethil
PR: #153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
Learnt from: rethil
PR: #154
File: pkg/scaling/scalers/prometheus_scaler.go:69-75
Timestamp: 2025-07-21T19:03:53.348Z
Learning: In Go, strings.NewReplacer() creates a new Replacer instance each time it's called, so using it inside a function would allocate on every invocation, making it less efficient than strings.ReplaceAll for simple single-replacement scenarios. Package-level variables should be used for strings.Replacer to avoid repeated allocations.
tests/e2e/manifest/reset.sh (1)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
🪛 Checkov (3.2.334)
tests/e2e/manifest/pod-curl-target-gw.yaml
[MEDIUM] 2-14: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[LOW] 2-14: CPU limits should be set
(CKV_K8S_11)
[LOW] 2-14: CPU requests should be set
(CKV_K8S_10)
[LOW] 2-14: Apply security context to your containers
(CKV_K8S_30)
[LOW] 2-14: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[LOW] 2-14: Image should use digest
(CKV_K8S_43)
[LOW] 2-14: Image Pull Policy should be Always
(CKV_K8S_15)
[LOW] 2-14: Liveness Probe Should be Configured
(CKV_K8S_8)
[LOW] 2-14: Memory limits should be set
(CKV_K8S_13)
[LOW] 2-14: Memory requests should be set
(CKV_K8S_12)
[LOW] 2-14: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[LOW] 2-14: Apply security context to your pods and containers
(CKV_K8S_29)
[LOW] 2-14: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[LOW] 2-14: Readiness Probe Should be Configured
(CKV_K8S_9)
[MEDIUM] 2-14: Minimize the admission of root containers
(CKV_K8S_23)
[LOW] 2-14: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[LOW] 2-14: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[LOW] 2-14: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🪛 YAMLlint (1.37.1)
tests/e2e/manifest/pod-curl-target-gw.yaml
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
🔇 Additional comments (11)
tests/e2e/manifest/target-elastiservice.yaml (1)
8-8: Confirm the 6× cooldown increase won’t make scaling feel sluggish
Going from 5 s → 30 s heavily delays the first down-/up-scale that KEDA can trigger. Make sure this larger window still meets SLOs for responsiveness and does not hide real load spikes in your E2E scenarios.tests/e2e/kuttl-test.yaml (1)
9-11: LGTM – pre-create curl pod before tests run
Applying the curl pod up-front avoids repeated pod spawns inside steps and keeps logs centralised. Nice improvement.tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (1)
3-25: Harden the script for future-proofing
- Add
set -euo pipefailso invisible failures abort immediately.- Derive the EndpointSlice name via a label selector instead of relying on the hashed suffix – this changes across K8s versions and breaks when CR names differ.
[ suggest_optional_refactor ]
- - script: | + - script: | #!/bin/sh + set -euo pipefail + + ES_NAME=$(kubectl get endpointslice -n target -l app=target-deployment -o jsonpath='{.items[0].metadata.name}') - kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 -o jsonpath='{.metadata.name}' | grep "$ES_NAME" || exit 1 + kubectl get endpointslice -n target "$ES_NAME" -o jsonpath='{.metadata.name}' | grep "$ES_NAME" || exit 1 ... - ENDPOINTS_COUNT=$(kubectl get endpointslice -n target elasti-target-deployment-endpointslice-to-resolver-9696239e87 -o jsonpath='{.endpoints}' | jq 'length') + ENDPOINTS_COUNT=$(kubectl get endpointslice -n target "$ES_NAME" -o jsonpath='{.endpoints}' | jq 'length')tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml (1)
3-4: Good refactoring to structured commands format.The conversion from shell script to explicit commands improves test clarity and makes the wait operation more explicit.
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (1)
3-5: Consistent refactoring aligns with test suite standardization.The explicit scale and wait commands improve test clarity compared to static deployment manifests. This change aligns well with the broader test suite refactoring.
tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (1)
3-5: Excellent consistency across test suite.This follows the same explicit command pattern as other test files, creating a unified approach to deployment scaling and pod lifecycle management in the E2E tests.
pkg/scaling/scalers/prometheus_scaler.go (3)
11-11: Required import for the queryEscape fix.The strings import is correctly added to support the new queryEscape function.
69-75: Excellent fix for Go's URL encoding quirk.The
queryEscapefunction properly addresses the known Go issue whereurl.QueryEscapeencodes spaces as+instead of%20. The implementation is clean, well-documented with the GitHub issue reference, and efficiently handles the single replacement case.
79-79: Proper integration of the escape fix.Using the new
queryEscapehelper ensures Prometheus queries with spaces are correctly encoded in the URL.tests/e2e/manifest/reset.sh (1)
17-18: Consider a longer timeout for pod readiness
kubectl wait … --timeout=30sis tight on congested CI clusters; sporadic flakes are likely.
60-90 s is usually a safer lower bound for deployment/pod readiness during E2E.tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (1)
3-5: Looks good – concise & idiomatickubectlusageScaling to zero followed by
kubectl wait --for=delete pods …is clear and consistent with the rest of the suite.
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml
Outdated
Show resolved
Hide resolved
tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
tests/e2e/manifest/pod-curl-target-gw.yaml (1)
1-15: LGTM with suggestions for test robustness.The Pod manifest correctly addresses the network isolation issues mentioned in the PR objectives. The typo fix from "infitiy" to "infinity" is good.
For more robust E2E testing, consider adding minimal resource constraints and security context:
spec: containers: - name: curl image: ghcr.io/curl/curl-container/curl:master + imagePullPolicy: IfNotPresent command: ["sleep"] args: ["infinity"] + resources: + requests: + cpu: 10m + memory: 32Mi + limits: + cpu: 50m + memory: 128Mi + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false
♻️ Duplicate comments (4)
tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml (1)
3-4: Prefer singularpodforkubectl waitKubernetes docs and most examples use the singular resource (
pod) – a few older kubectl builds reject the plural form here. Switching keeps the suite consistent with other files already updated.tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml (1)
3-5: Same determinism concern as in 06-resolver testFor the reasons explained in the previous file (race-free confirmation that the Deployment is fully reconciled), consider inserting a
kubectl rollout statusafter the scale command and before any follow-up assertions.
Implementation is identical to the diff shown in the other file.tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (1)
6-13: Still ignores hard network/DNS failures – re-enable the curl exit-status checkThe
resultguard is commented out, so the loop happily succeeds even when
curlcannot resolve the host or connect (exit ≠ 0, HTTP code 000).
This reproduces the same false-positive risk flagged in a previous review.- # if [ "$result" != "0" ]; then exit 1; fi + [ "$result" = "0" ] || exit 1 # abort on network/DNS error(Use
=for POSIX[…]consistency and keep the distinct exit codes.)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml (1)
4-5: Consider waiting on the Deployment rollout instead of individual Pods
kubectl rollout status deployment/target-deployment -n target --timeout=60s
gives a single high-level success signal and handles ReplicaSet churn,
reducing flakiness if the Deployment ever scales beyond one replica.No functional blocker, just a sturdier alternative.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
pkg/scaling/scalers/prometheus_scaler.go(2 hunks)tests/e2e/README.md(1 hunks)tests/e2e/kuttl-test.yaml(1 hunks)tests/e2e/manifest/pod-curl-target-gw.yaml(1 hunks)tests/e2e/manifest/reset.sh(1 hunks)tests/e2e/manifest/target-elastiservice.yaml(1 hunks)tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml(1 hunks)tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml(1 hunks)tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml(1 hunks)tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml(0 hunks)tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml(1 hunks)tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/tests/05-elasti-crd-remove/03-reset.yaml
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like `jq` should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: truefoundry/KubeElasti#154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: `jq` should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like `tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml` for JSON parsing.
Learnt from: rethil
PR: truefoundry/KubeElasti#153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
tests/e2e/README.md (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
pkg/scaling/scalers/prometheus_scaler.go (2)
Learnt from: rethil
PR: #153
File: pkg/scaling/scalers/prometheus_scaler.go:46-46
Timestamp: 2025-07-21T17:07:03.210Z
Learning: The ElastiService.spec.triggers[].metadata.query field in KubeElasti has a broader attack vector for Prometheus query injection compared to the uptimeFilter parameter, as it allows arbitrary Prometheus queries to be executed.
Learnt from: rethil
PR: #154
File: pkg/scaling/scalers/prometheus_scaler.go:69-75
Timestamp: 2025-07-21T19:03:53.348Z
Learning: In Go, strings.NewReplacer() creates a new Replacer instance each time it's called, so using it inside a function would allocate on every invocation, making it less efficient than strings.ReplaceAll for simple single-replacement scenarios. Package-level variables should be used for strings.Replacer to avoid repeated allocations.
tests/e2e/kuttl-test.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/04-scale-to-0-resolver.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/02-enable-serve-via-traffic/02-wait-ready.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/06-resolver-endpoint-sync/01-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/manifest/reset.sh (1)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/02-enable-serve-via-traffic/01-send-traffic.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/02-enable-serve-via-traffic/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
tests/e2e/tests/03-enable-serve-via-manual/01-manual-scale.yaml (2)
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: In KubeElasti E2E tests, kuttl TestStep scripts do not run inside the kuttl container - they run in the host environment or separate execution context, so external dependencies like jq should be documented as E2E test prerequisites rather than worrying about container image contents.
Learnt from: rethil
PR: #154
File: tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml:16-17
Timestamp: 2025-07-21T19:06:12.046Z
Learning: jq should be added as a prerequisite for running KubeElasti E2E tests since it's used in test scripts like tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml for JSON parsing.
🪛 YAMLlint (1.37.1)
tests/e2e/manifest/pod-curl-target-gw.yaml
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 13-13: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
[error] 14-14: too many spaces inside brackets
(brackets)
🪛 Checkov (3.2.334)
tests/e2e/manifest/pod-curl-target-gw.yaml
[MEDIUM] 2-14: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[LOW] 2-14: CPU limits should be set
(CKV_K8S_11)
[LOW] 2-14: CPU requests should be set
(CKV_K8S_10)
[LOW] 2-14: Apply security context to your containers
(CKV_K8S_30)
[LOW] 2-14: Minimize the admission of containers with the NET_RAW capability
(CKV_K8S_28)
[LOW] 2-14: Image should use digest
(CKV_K8S_43)
[LOW] 2-14: Image Pull Policy should be Always
(CKV_K8S_15)
[LOW] 2-14: Liveness Probe Should be Configured
(CKV_K8S_8)
[LOW] 2-14: Memory limits should be set
(CKV_K8S_13)
[LOW] 2-14: Memory requests should be set
(CKV_K8S_12)
[LOW] 2-14: Minimize the admission of containers with capabilities assigned
(CKV_K8S_37)
[LOW] 2-14: Apply security context to your pods and containers
(CKV_K8S_29)
[LOW] 2-14: Use read-only filesystem for containers where possible
(CKV_K8S_22)
[LOW] 2-14: Readiness Probe Should be Configured
(CKV_K8S_9)
[MEDIUM] 2-14: Minimize the admission of root containers
(CKV_K8S_23)
[LOW] 2-14: Containers should run as a high UID to avoid host conflict
(CKV_K8S_40)
[LOW] 2-14: Ensure that the seccomp profile is set to docker/default or runtime/default
(CKV_K8S_31)
[LOW] 2-14: Ensure that Service Account Tokens are only mounted where necessary
(CKV_K8S_38)
🔇 Additional comments (11)
pkg/scaling/scalers/prometheus_scaler.go (2)
69-75: Well-implemented fix for the Go URL encoding issue.The
queryEscapefunction correctly addresses the Go bug whereurl.QueryEscapeencodes spaces as+instead of%20. The implementation usingstrings.ReplaceAllis appropriate and efficient for this single replacement scenario.
79-79: Good integration of the escape fix.The usage of
queryEscapeinstead of directurl.QueryEscapeensures proper URL encoding for Prometheus queries.tests/e2e/manifest/target-elastiservice.yaml (1)
8-8: Good alignment with metric collection period.Increasing the cooldownPeriod from 5 to 30 seconds aligns with the PR objectives to prevent scaler health check failures by matching the metric collection period. This should improve E2E test stability.
tests/e2e/README.md (1)
18-18: Correct documentation of jq requirement.Adding jq v1.7+ as a prerequisite is appropriate, as it's used in E2E test scripts for JSON parsing. The version specification correctly references the latest official release.
tests/e2e/kuttl-test.yaml (1)
9-10: Proper integration of the curl gateway pod.The addition of the kubectl apply command for the curl pod manifest correctly integrates the new testing approach where curl commands run within the cluster network.
tests/e2e/tests/06-resolver-endpoint-sync/03-check-endpointslice-has-resolver-ips.yaml (1)
3-27: No additional concerns — previous feedback already covers hard-coded names and fail-fast handling, and the author has deferred that refactor.tests/e2e/tests/03-enable-serve-via-manual/00-scale-to-0-target.yaml (1)
3-5: No new issues spotted.tests/e2e/tests/04-public-private-svc-sync/01-scale-to-0-target.yaml (1)
3-5: No new issues spotted.tests/e2e/tests/05-elasti-crd-remove/00-scale-target.yaml (1)
4-7: Looks good – clearer than the old inline shell snippetBreaking the scale-down / scale-up cycle into explicit commands with time-bounded waits makes the step much easier to read and debug. ✅
tests/e2e/manifest/reset.sh (1)
8-11: 👍 Quoting the manifest path eliminates the ShellCheck warningsQuoting
$1prevents accidental word-splitting and glob expansion – thanks for addressing the earlier feedback.tests/e2e/tests/01-enable-proxy-via-manual/00-scale-to-0-target.yaml (1)
3-5: Looks good – concise and consistent with suite conventions.
tests/e2e/tests/06-resolver-endpoint-sync/05-check-endpointslice-with-resolver-scaled-to-0.yaml
Show resolved
Hide resolved
|
Hey, @ramantehlan & @shubhamrai1993, |
Description
E2E tests are misusing
kuttl, which makes them not really working properly.Fixes:
kuttldoesn't like mixingTestStepwith anything elseTestStep.scriptdoesn't exists based on https://kuttl.dev/docs/testing/reference.html#teststepcurlfrom insidecommands[].scriptdoesn't usekindnetwork.curlpod which acts as gateway was addedcooldownPeriodin CRD has to correlate with metric collection period, otherwise scaler health check is failingdeployment/targetis being poisoned in04-public-private-svc-sync, so full replace rather than apply is called.Remarks:
1.
tests are unstable, as manual scaling is probably interfering withtypo in testElasti2.
02-enable-serve-via-trafficshows problems with proxy. When deployment is scaled up, proxy isn't forwarding whole response.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores