MG-293: proposal to integrate must-gather-clean in must-gather-operator#2058
MG-293: proposal to integrate must-gather-clean in must-gather-operator#2058shivprakashmuley wants to merge 3 commits into
Conversation
|
@shivprakashmuley: This pull request references MG-293 which is a valid jira issue. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| | `must-gather-clean` calls `klog.Exitf` on file errors, terminating the upload container | Upload fails hard instead of returning a recoverable error | The obfuscation runs in the upload container (not the operator controller), so the operator process itself is unaffected; document that file-level errors are fatal | | ||
| | Obfuscation increases Job duration for large bundles | Longer time to upload for support cases | Obfuscation is I/O-bound and parallelized | | ||
| | Container CPU limits may be lower than expected | Obfuscation runs slower with fewer workers | `automaxprocs` detects cgroup CPU limits and sets `GOMAXPROCS` accordingly; worker count is additionally capped at 4 to balance throughput and I/O contention | | ||
| | The `chown` patch to `must-gather-clean` has not yet been merged upstream | Development uses a `replace` directive in `go.mod` pointing to a local fork, which cannot ship to production | Propose the `chown` EPERM fix upstream as a prerequisite; once merged, pin to the upstream release and remove the `replace` directive | |
There was a problem hiding this comment.
there's no upstream and I haven't seen a patch, do you have anything for me to review?
There was a problem hiding this comment.
Actually yes, by upstream I meant, in this chown function here, https://github.com/openshift/must-gather-clean/blob/main/pkg/fsutil/fsutil_unix.go#L14, error that is returned is problematic.
Reason - Flow of must-gather job is:
- we have 2 containers - gather & upload
- gather runs the gather scripts and container image is the must-gather image.
- upload containers tars and uploads the collected bundle to SFTP. Container image here is must-gather-operator image. And it runs as
USER 65534:65534.
Gather container (uid 0) creates files in /must-gather -- all owned by 0:0 (root:root)
Upload container (uid 65534) runs must-gather-clean which reads each input file and notes its ownership:
uid := stat.Sys().(*syscall.Stat_t).Uid // 0 gid := stat.Sys().(*syscall.Stat_t).Gid // 0
For every output file it writes to /must-gather-upload/.must-gather-cleaned/, it calls:
os.Chown(outputPath, 0, 0)
This call fails with EPERM.
So the patch for this one is,
if errors.Is(err, syscall.EPERM) { klog.V(3).Infof("chown skipped: permission denied (running as non-root)") return nil // not an error, just skip it }
This is just a suggestion. Please let us know if you feel it can be handled in a better way @tjungblu .
There was a problem hiding this comment.
send a PR and we can discuss it there
There was a problem hiding this comment.
removed the need for this patch. Marking it resolved.
|
|
||
| 3. Should the obfuscation report (`report.yaml` with replacement mappings) be surfaced in the MustGather CR status, or is inclusion in the bundle sufficient? | ||
|
|
||
| 4. What is the SLA for the upstream `chown` patch to `openshift/must-gather-clean`? |
There was a problem hiding this comment.
Removed the proposal for patch. Marking it resolved.
|
|
||
| This mode allows administrators to redact an existing must-gather bundle stored on a PVC without re-collecting or uploading. | ||
|
|
||
| 1. The cluster administrator has a previously collected must-gather bundle persisted on a PVC (e.g., from a prior MustGather CR with `spec.storage`) |
There was a problem hiding this comment.
so in this case we are allowing update of an existing CR?
There was a problem hiding this comment.
User has to create another CR @Prashanth684 for this as update of existing CR is not handled/supported by the operator. We can call it out explicitly if you feel it is confusing.
shivprakashmuley
force-pushed
the
integrate-mg-clean
branch
from
cfbe590 to
0de9813
Compare
|
@shivprakashmuley: all tests passed! 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. |
4 participants
Summary:
Integrate the must-gather-clean library into the must-gather-operator to automatically obfuscate sensitive data (IP addresses, MAC addresses, Kubernetes Secrets, ConfigMaps) in must-gather bundles before upload to Red Hat support cases.
A new obfuscate field is added to the MustGather CR spec with three sub-fields:
enabled (*bool): activates obfuscation
obfuscationConfigRef (optional): references a ConfigMap with custom obfuscation rules
source (optional): references an existing bundle on a PVC for obfuscation without re-running a gather
Three operational modes are supported:
Gather + Obfuscate + Upload — full pipeline via obfuscate.enabled: true + uploadTarget
Obfuscate Only — redact an existing bundle on a PVC in-place, no gather, no upload
Obfuscate + Upload — redact an existing bundle on a PVC and upload, no gather
Obfuscation runs as a post-gather, pre-upload step inside the existing upload container using the vendored must-gather-clean library. No new images, containers, or sidecars are introduced. A default config ships with the operator (consistent IP/MAC replacement, Secret/ConfigMap omission), and users can override it with a custom ConfigMap.
Prerequisite: Upstream patch to openshift/must-gather-clean (pkg/fsutil/fsutil_unix.go) to handle chown EPERM gracefully for non-root execution.