feat(test): Add functional test to verify splunk 403 behavior for LOG…#3329
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new functional test for Splunk output that deploys an invalid HEC token, checks collector logs for 403 failures, and verifies the collector metrics endpoint reports 403 responses. ChangesSplunk 403 Forbidden Test
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/functional/outputs/splunk/403_forbidden_test.go (2)
96-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed sleeps instead of polling risk test flakiness.
Line 97's
time.Sleep(15 * time.Second)and the single, non-retried metrics fetch (lines 120-125) hard-code timing assumptions about how quickly Vector retries/logs and exposes updated metrics. Under CI load these fixed windows are a classic source of flaky tests. PreferEventuallypolling (already used elsewhere in this package, e.g.splunk.WaitOnSplunk) for both the log assertion and the metrics fetch/assertion.♻️ Example refactor for the log assertion
- // Wait for logs to be attempted to be sent and retried - time.Sleep(15 * time.Second) - - // Read collector logs and verify forbidden error is logged - collectorLog, err := framework.ReadCollectorLogs() - Expect(err).To(BeNil(), "Expected no errors reading collector logs") - Expect(collectorLog).ToNot(BeEmpty(), "Expected collector logs to not be empty") - - // Verify that the collector logs contain a forbidden error message - // Vector logs HTTP errors with the status code - Expect(collectorLog).To( - Or( + // Read collector logs and verify forbidden error is logged + var collectorLog string + Eventually(func() (string, error) { + var err error + collectorLog, err = framework.ReadCollectorLogs() + return collectorLog, err + }, 30*time.Second, 3*time.Second).Should( + Or( ContainSubstring("forbidden"), ContainSubstring("Forbidden"), ContainSubstring("status: 403"), ContainSubstring("status=403"), And( ContainSubstring("error"), ContainSubstring("403"), ), ), "Expected collector logs to contain forbidden error or 403 status", )A similar
Eventuallywrap around the metrics curl +status="403"search would make that assertion resilient too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/outputs/splunk/403_forbidden_test.go` around lines 96 - 125, Replace the fixed wait and one-shot metrics check in the 403 forbidden test with polling-based assertions to reduce flakiness. In the test that uses `framework.ReadCollectorLogs()` and the `metricsURL`/`curlCmd` flow, use `Eventually` (similar to `splunk.WaitOnSplunk`) to repeatedly read collector logs until the forbidden/403 substring appears, and wrap the metrics curl plus `status="403"` verification in a retrying `Eventually` as well. Keep the existing `collectorLog` and `metrics` checks, but make both outcomes wait on the expected condition instead of relying on `time.Sleep(15 * time.Second)` and a single fetch.
54-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile auth-section removal via hardcoded property strings.
This relies on exact-match strings for every property inside
[sinks.prometheus_output.auth](strategy = "sar",path = "/metrics",verb = "get"). If the generated auth block ever gains an additional property that doesn't match one of the three hardcoded prefixes,skipNextis reset tofalseand that orphan line leaks into the output outside any section header, silently producing a malformed/incorrect config and breaking the premise of the test (unauthenticated/metrics).A simpler and more robust approach is to skip everything between the auth header and the next
[section]header, without needing to know the specific property names:♻️ Proposed simplification
framework.VisitConfig = func(conf string) string { - // Remove the [sinks.prometheus_output.auth] section - lines := strings.Split(conf, "\n") - var result []string - skipNext := false - for _, line := range lines { - // Skip the [sinks.prometheus_output.auth] section and its properties - if strings.Contains(line, "[sinks.prometheus_output.auth]") { - skipNext = true - continue - } - if skipNext { - // Skip lines that are part of the auth section (strategy, path, verb) - trimmed := strings.TrimSpace(line) - if trimmed == `strategy = "sar"` || trimmed == `path = "/metrics"` || trimmed == `verb = "get"` { - continue - } - // Stop skipping when we hit a non-auth line or another section - if !strings.HasPrefix(trimmed, "strategy") && !strings.HasPrefix(trimmed, "path") && !strings.HasPrefix(trimmed, "verb") { - skipNext = false - } - } - if !skipNext { - result = append(result, line) - } - } - return strings.Join(result, "\n") + // Remove the [sinks.prometheus_output.auth] section entirely by + // skipping every line until the next section header is found. + lines := strings.Split(conf, "\n") + var result []string + inAuthSection := false + for _, line := range lines { + trimmed := strings.TrimSpace(line) + if trimmed == `[sinks.prometheus_output.auth]` { + inAuthSection = true + continue + } + if inAuthSection { + if strings.HasPrefix(trimmed, "[") { + inAuthSection = false + } else { + continue + } + } + result = append(result, line) + } + return strings.Join(result, "\n") }For an even more robust option, consider parsing with a TOML library (e.g.
github.com/BurntSushi/tomlorgithub.com/pelletier/go-toml) instead of manual string manipulation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/outputs/splunk/403_forbidden_test.go` around lines 54 - 81, The auth-section stripping in VisitConfig is too brittle because it depends on hardcoded property strings inside [sinks.prometheus_output.auth]. Update the logic to skip every line from that section header until the next section header, so any future auth properties are removed automatically. Keep the fix localized to framework.VisitConfig in the 403_forbidden_test.go test helper and preserve the rest of the config unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/functional/outputs/splunk/403_forbidden_test.go`:
- Line 143: The GinkgoWriter logging in the 403 metric loop ignores the error
returned by fmt.Fprintf, which triggers the errcheck violation. Update the code
in the test helper/loop that writes “Found 403 metric” to either handle the
returned error explicitly or switch to a write path that cannot fail in a way
that satisfies linting, keeping the change localized to the GinkgoWriter usage.
---
Nitpick comments:
In `@test/functional/outputs/splunk/403_forbidden_test.go`:
- Around line 96-125: Replace the fixed wait and one-shot metrics check in the
403 forbidden test with polling-based assertions to reduce flakiness. In the
test that uses `framework.ReadCollectorLogs()` and the `metricsURL`/`curlCmd`
flow, use `Eventually` (similar to `splunk.WaitOnSplunk`) to repeatedly read
collector logs until the forbidden/403 substring appears, and wrap the metrics
curl plus `status="403"` verification in a retrying `Eventually` as well. Keep
the existing `collectorLog` and `metrics` checks, but make both outcomes wait on
the expected condition instead of relying on `time.Sleep(15 * time.Second)` and
a single fetch.
- Around line 54-81: The auth-section stripping in VisitConfig is too brittle
because it depends on hardcoded property strings inside
[sinks.prometheus_output.auth]. Update the logic to skip every line from that
section header until the next section header, so any future auth properties are
removed automatically. Keep the fix localized to framework.VisitConfig in the
403_forbidden_test.go test helper and preserve the rest of the config unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab95c3f9-103d-4950-8138-9a3805de3da7
📒 Files selected for processing (1)
test/functional/outputs/splunk/403_forbidden_test.go
|
/lgtm |
|
@jcantrill: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit 4bc3b90
into
openshift:master
2 participants
…-9610
Description
This PR:
Links
https://redhat.atlassian.net/browse/LOG-9610
cc @vparfonov @Clee2691
Summary by CodeRabbit
status="403".