feat(sensor): Add secrets to fake workloads#19479
Open
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 28930366 | Triggered | Generic High Entropy Secret | 1c298ba | sensor/kubernetes/fake/secret.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
getDockerConfigSecret, thejson.Marshalerror is ignored (data, _ := json.Marshal(cfg)); consider handling or logging the error so mis-serialization of the docker config doesn’t silently produce an invalid secret. updateRandomSecretcurrently lists secrets withLimit: 1and always updates the first item, which isn’t actually random and may repeatedly hit the same secret; consider either caching IDs fromgetSecretsor listing a broader set and selecting a random element to better simulate random churn and reduce repeated updates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getDockerConfigSecret`, the `json.Marshal` error is ignored (`data, _ := json.Marshal(cfg)`); consider handling or logging the error so mis-serialization of the docker config doesn’t silently produce an invalid secret.
- `updateRandomSecret` currently lists secrets with `Limit: 1` and always updates the first item, which isn’t actually random and may repeatedly hit the same secret; consider either caching IDs from `getSecrets` or listing a broader set and selecting a random element to better simulate random churn and reduce repeated updates.
## Individual Comments
### Comment 1
<location path="sensor/kubernetes/fake/secret.go" line_range="35" />
<code_context>
+ },
+ },
+ }
+ data, _ := json.Marshal(cfg)
+ return &corev1.Secret{
+ TypeMeta: metav1.TypeMeta{
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid discarding the json.Marshal error to surface malformed docker config generation issues
Ignoring the json.Marshal error can hide regressions if dockerConfigJSON or its tags change and secret generation starts failing silently. Even for test-only/fake workloads, handle the error explicitly: either return nil and log, or fail fast (panic/log.Fatalf) so such issues are immediately visible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at a1f9dce. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19479 +/- ##
==========================================
- Coverage 49.25% 49.22% -0.04%
==========================================
Files 2725 2726 +1
Lines 205582 205718 +136
==========================================
- Hits 101268 101265 -3
- Misses 96780 96917 +137
- Partials 7534 7536 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a9a4125 to
a0b51ea
Compare
a0b51ea to
fa21c31
Compare
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
updateAllDockerConfigSecrets, theListerrors are silently ignored; consider at least debug-level logging (possibly rate-limited) so it's visible when a namespace’s secrets cannot be listed during an update wave. - The new
SecretWorkloadvalidation currently clamps negative counts to 0 and returns an error; if the intent is to auto-correct user input (as with the openPortReuseProbability rounding), consider downgrading this to a warning-style error or logging so callers don’t have to treat a fully-corrected workload as a hard failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `updateAllDockerConfigSecrets`, the `List` errors are silently ignored; consider at least debug-level logging (possibly rate-limited) so it's visible when a namespace’s secrets cannot be listed during an update wave.
- The new `SecretWorkload` validation currently clamps negative counts to 0 and returns an error; if the intent is to auto-correct user input (as with the openPortReuseProbability rounding), consider downgrading this to a warning-style error or logging so callers don’t have to treat a fully-corrected workload as a hard failure.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add secret workloads to the fake workload generator so that
local-sensorcan reproduce the
ResolveAllDeployments()amplification that causes sensorOOM at scale - see https://redhat.atlassian.net/browse/ROX-33638.
A cluster with high number of docker-config secrets and deployments
triggered O(secrets × deployments) resolver work on every secret informer
re-list wave, growing memory until OOM-kill.
This change lets us reproduce and measure that behavior locally:
SecretWorkloadconfig in the fake workload YAML controls the numberof docker-config and opaque secrets, plus a periodic update interval.
manageSecretsgoroutine sends waves of annotation-bumps to everydocker-config secret on the configured interval, simulating informer
re-lists.
secret-oom-repro.yaml— full-scale (3 k secrets × 1 k deployments)secret-oom-repro-monitor.sh) polls/metricsfor heap, RSS, resolver queue, and secret/deployment event counts.User-facing documentation
Testing and quality
Automated testing
How I validated my change
Ran
local-sensorwithsecret-oom-repro-light.yamland confirmed thatresolver queue saturation and memory growth are visible via the monitoring
script.
go run ./tools/local-sensor \ -with-fakeworkload scale/workloads/secret-oom-repro.yaml \ -with-metrics \ -with-pprof-server \ -duration 30m \ -skip-central-output