Repository navigation
feat: Obfuscate collected must-gather output with must-gather-clean [RHIDP-16944 ] - #411
Conversation
Run must-gather-clean after the existing secret sanitizer so collected artifacts are obfuscated before they are shared. IP and MAC addresses are replaced consistently in file contents and paths, and discovered OpenShift and API domains are rewritten while ConfigMaps and Secrets stay in the gather. The reversible report is left out of the published tree, a failed obfuscation keeps the collected output and fails the command, and --no-obfuscate skips this step. RHIDP-16944 Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
PR Summary by QodoObfuscate must-gather output after secret sanitization
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
InProcess was only reached from tests. Those tests now call Clean directly, which is the function the gather command already uses. Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Code Review by Qodo
1.
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
A ConfigMap or Secret named report is written as report.yaml. Obfuscation was treating every file with that name as the reversible must-gather-clean report and failing the gather. RHIDP-16944 Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
Registering a subcommand made Cobra reject stray positional tokens. Flags are unchanged, and the hidden obfuscate command still runs when it is named. RHIDP-16944 Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 4d762a6 |
|
PR images are available (for 1 week):
|
errcheck fails the unit-test job when defer os.RemoveAll drops its error. Cleanup of the temporary directory cannot change the gather result. RHIDP-16944 Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
|
PR images are available (for 1 week):
|
|
/cc |
| // The hidden obfuscate command makes this a parent command. Without an | ||
| // explicit Args func, Cobra then rejects positional tokens as unknown commands. | ||
| cmd.Args = cobra.ArbitraryArgs | ||
| cmd.AddCommand(newObfuscateCmd()) |
There was a problem hiding this comment.
Why do we need a separate cli command and run it in a subprocess? If the risk is that must-gather-clean might exit abruptly, I think it should be possible to take a similar approach as with the namespace-inspect (same process but hook using kcmdutil.BehaviorOnFatal for example).
Going in-process would eliminate those hundred of lines of collateral complexity the subprocess creates
There was a problem hiding this comment.
I looked at doing this in the same process, the way namespace inspect hooks BehaviorOnFatal. That hook only catches kubectl CheckErr. must-gather-clean calls klog.Exitf from its own error goroutine, and that calls os.Exit. A hook or a recover in this process does not catch that, so the whole gather would stop and the caller would not get a clear failure. The hidden obfuscate command is the same binary run again. If the child fails, the original files stay and the command says the output was not obfuscated.
| // | ||
| // When discovery finds nothing, IP and MAC obfuscation still run. Set | ||
| // EnvDomains to supply names that discovery cannot see. | ||
| func Discover(ctx context.Context, client *kube.Client) []string { |
There was a problem hiding this comment.
IIUC, this will only be useful on OCP clusters, right? What would be the behavior on non-OCP clusters? Maybe the discovery logic in this Discover() function could detect the platform and branch accordingly, like so:
- OCP: current path
- Non-OCP: read hosts from Ingress resources, plus the API server hostname.
Thoughts?
There was a problem hiding this comment.
Good point. On OpenShift we still read the DNS cluster base domain, the default ingress controller domain, and the API server hostname. On Kubernetes, and whenever those OpenShift domains cannot be read, we now read hosts from Ingress resources and from Routes in the namespaces being collected, plus the API hostname. When the OpenShift suffixes are found, we do not also add every individual Ingress host, because those names already sit under the cluster domains. This is in cb15337.
| flags.StringVar(&opts.heapDumpMethod, "heap-dump-method", "inspector", "Heap dump collection method: inspector or sigusr2") | ||
| flags.StringVar(&opts.heapDumpInstances, "heap-dump-instances", "", "Comma-separated list of instance names to collect heap dumps from") | ||
| flags.BoolVar(&opts.clusterInfo, "cluster-info", false, "Collect cluster-wide diagnostic information") | ||
| flags.BoolVar(&opts.noObfuscate, "no-obfuscate", false, "Skip IP, MAC, and domain obfuscation (secret sanitization still runs)") |
There was a problem hiding this comment.
I would suggest not exposing this flag for now. In my understanding, the consistent replacements (x-ipv4-..., domain0000000001) should preserve the structure we need for debugging, so I guess most analysis should still work on obfuscated output.
If the need or complaints come later, we could consider adding it, but for now, I think it should just be the opinionated behavior to obfuscate (similar to the automatic sanitization which is done with no option to skip). WDYT?
There was a problem hiding this comment.
I would like to keep the flag. Obfuscation is already the default, the same way sanitization always runs. The consistent tokens are enough for most debugging. Heap dumps collected with --with-heap-dumps go through this pass, and looking at a memory snapshot needs the real addresses. The flag is also the way out when the clean step fails and the command refuses to publish the result. The docs say to pass --no-obfuscate for heap snapshots and for local debugging.
OpenShift DNS and ingress-controller reads stay the source of cluster suffixes. When those objects cannot be read, discovery uses Ingress and Route hosts in the namespaces being collected, plus the API hostname. The two OpenShift field reads share one lookup. RHIDP-16944 Signed-off-by: Fortune-Ndlovu <fndlovu@redhat.com>
|
PR images are available (for 1 week):
|
This branch has not been deployed
2 participants
Description
After the existing secret sanitizer, the gather now runs must-gather-clean so IP addresses, MAC addresses, and discovered OpenShift and API domains are rewritten before the output is shared. ConfigMaps and Secrets stay in the gather, secret values are still redacted by the current sanitizer, and the reversible report.yaml is left out of the published tree. If obfuscation fails, the command exits with an error and the collected output is left unchanged. --no-obfuscate skips this step, and RHDH_OBFUSCATE_DOMAINS adds domains that cluster discovery misses.
Which issue(s) does this PR fix or relate to
PR acceptance criteria
How to test changes / Special notes to the reviewer
Prerequisites
gofromgo.mod).oclogged in to an OpenShift cluster where an RHDH operator instance is running.oc whoamimust succeed. The account needs to read that namespace, plusdnses/clusterand the default ingress controller. A cluster-admin login does.Obfuscating N domain name(s)when those objects are missing, but the output then has no domain string to rewrite. Confirm withoc get route,ingress -n <rhdh-namespace>.Run
To include Secrets as well:
BASE_COLLECTION_PATH=/tmp/rhdh-mg-test \ make run-local OPTS="--namespaces <rhdh-namespace> --with-secrets"The command should exit 0. The log should include
Obfuscating N domain name(s) plus IP and MAC addresseswith N greater than 0.Check the output
Expect a watermark, a sanitization report, no
.obfuscated-stagingdirectory, ConfigMaps still present,x-ipv4-tokens, anddomain0000000001inall-routes.txtorall-ingresses.txtwhen that namespace has a Route or Ingress.127.0.0.1stays as-is.Skip obfuscation with
OPTS="--namespaces <rhdh-namespace> --no-obfuscate". Secret sanitization still runs, and the log says obfuscation is disabled.