OAPE-824: add the agentic docs in must-gather-operator repo#370
OAPE-824: add the agentic docs in must-gather-operator repo#370praveencodes wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@praveencodes: This pull request references OAPE-824 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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
📝 WalkthroughWalkthroughThis PR adds a CodeRabbit configuration file and a comprehensive set of documentation files for the must-gather-operator repository, including an AGENTS.md onboarding guide, a rewritten CLAUDE.md, a new CONTRIBUTING.md, updated README.md, and six new docs/ guideline files (architecture, API contracts, error handling, integration, performance, security, testing). ChangesDocumentation and configuration additions
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: praveencodes 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 |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #370 +/- ##
=======================================
Coverage 82.32% 82.32%
=======================================
Files 8 8
Lines 905 905
=======================================
Hits 745 745
Misses 148 148
Partials 12 12 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
docs/api-contracts-guidelines.md (2)
110-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpell out the cleanup order and ownership check.
The contract here should be explicit: verify the target object's
OwnerReferencesagainst the current MustGather UID, delete Pods first, then delete the Job. The current wording is loose enough to be misread into the wrong cleanup sequence.🤖 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 `@docs/api-contracts-guidelines.md` around lines 110 - 126, Clarify the Finalizer Pattern cleanup flow by explicitly stating that the controller must first verify the target object's OwnerReferences against the current MustGather instance UID, then delete the Job's Pods before deleting the Job itself. Update the wording around the cleanup steps in the MustGather contract so the ownership check and delete order are unambiguous and tied to the MustGather UID and job cleanup logic.Source: Learnings
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't describe
Storageas a union.
Storageis just an enum plus a required nested config here; it doesn't use the union/discriminator markers. Keeping it in this section will steer future changes toward the wrong validation pattern.🤖 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 `@docs/api-contracts-guidelines.md` at line 33, The documentation currently describes Storage like a union, but it should be documented as a simple enum plus required nested config instead. Update the Storage guidance in the api-contracts section to remove any union/discriminator framing and make it clear that StorageType is the only enum involved today, with PersistentVolume as the current value. Keep the reference to Storage and StorageType so the correct section is easy to find.
🤖 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 `@AGENTS.md`:
- Around line 30-47: Add language tags to the two fenced Markdown blocks in
AGENTS.md so markdownlint MD040 is satisfied; update the unlabeled fences in the
repository layout section and the owner-reference block to use an explicit
language such as text, keeping the existing content unchanged. Use the
surrounding markdown list and the nearby owner-reference section to locate both
fences.
In `@CONTRIBUTING.md`:
- Around line 100-102: The fenced example in the CONTRIBUTING markdown needs a
language tag so markdownlint MD040 does not flag it. Update the fenced block
containing “MG-NNN: Short description of the change” to use a text language
label, and keep the example otherwise unchanged; locate the snippet by its
fenced markdown example rather than line numbers.
In `@docs/ARCHITECTURE.md`:
- Around line 17-25: The architecture note hardcodes the operator namespace as a
fixed value, but it should describe a configurable operator namespace instead.
Update the wording in the decision/rationale around the operator namespace to
refer to OPERATOR_NAMESPACE as the required startup setting, and mention that
config.OperatorName is only a local-mode fallback. Keep the rest of the RBAC and
watch/job placement explanation intact while removing the impression that
must-gather-operator is an invariant.
- Around line 31-39: The architecture note in the MustGather Job description
overstates permission isolation for the upload container; it shares the same
ServiceAccount as gather and does not have a separate Kubernetes identity.
Update the wording around the gather/upload container split to emphasize that
upload is trusted operator-controlled code that relies on process namespace
sharing for completion detection and does not need Kubernetes API access, rather
than claiming distinct pod-level permissions.
- Around line 41-43: Update the explanation in the architecture doc to reflect
the real init-container limitation: in the section near the sequential
containers discussion, clarify that init containers do share volumes and can use
shareProcessNamespace, but they run before app containers so they cannot wait
for gather to finish or observe the long-running gather process this design
relies on. Keep the reference to the sidecar vs. sequential container rationale
in ARCHITECTURE.md aligned with that distinction.
In `@docs/error-handling-guidelines.md`:
- Around line 29-35: The decision-tree block in the error-handling guidelines
uses a bare fenced code block, which triggers markdownlint MD040. Update the
fence to use a language tag such as text (or convert the block to a list) so the
documentation passes lint; the relevant section is the decision-tree snippet in
the guidelines doc.
- Around line 39-42: The transient error check in IsTransientError uses an
invalid errors.As target for net.Error, so update the validation.go logic to
populate a concrete net.Error variable before calling Timeout(). Keep the
existing context.DeadlineExceeded and context.Canceled checks, and replace the
interface target with a local variable inside IsTransientError so the SFTP retry
classification works correctly.
In `@docs/integration-guidelines.md`:
- Around line 50-54: The Secret-not-found handling described in the integration
guidelines does not match the actual reconcile flow in MustGatherController.
Update the MustGather controller logic around ManageError and the Secret lookup
path so a missing Secret is treated consistently with the documented behavior,
or change the doc bullet to describe it as a retried runtime error instead of
terminal. Use the MustGather reconciliation code and its Secret existence check
to align the behavior and wording.
In `@docs/security-guidelines.md`:
- Around line 70-72: Add a language tag to the fenced example in the docs so the
Markdown lint rule is satisfied; update the fence around the kubebuilder
validation snippet to include a Go language hint using the nearby markdown
example block, and keep the content unchanged in the security-guidelines
example.
- Around line 112-118: `build/bin/upload` is building the remote upload path
from user-controlled `caseid` and, for internal uploads, `username` without
validation; update the upload path assembly to normalize and allowlist these
components before they are combined into `REMOTE_FILENAME`. Use the existing
upload flow in `build/bin/upload` and the internal path construction logic to
reject unsafe characters or path traversal sequences, then only concatenate
sanitized values so the remote SFTP target stays predictable.
In `@docs/testing-guidelines.md`:
- Around line 153-160: The async assertions guidance is out of sync with the e2e
tests: the job-completion timeout recommendation no longer matches the behavior
in must_gather_operator_test. Update the wording in the testing guidelines near
the Async Assertions section to reflect the current job-completion wait used by
the e2e suite, or explicitly state that 5 minutes is only an upper bound for
exceptional cases. Make sure the guidance stays aligned with the job-related
assertions in must_gather_operator_test.
In `@README.md`:
- Around line 78-103: The local development example is missing the runtime
environment variable required by the controller, so the startup recipe is
incomplete. Update the local-run steps in README.md near the local development
workflow to also export OPERATOR_IMAGE alongside DEFAULT_MUST_GATHER_IMAGE
before invoking operator-sdk run, matching the deployment contract documented by
the operator. Use the local-dev section and the operator-sdk run example as the
reference points so the example remains consistent with the controller’s
expected environment.
---
Nitpick comments:
In `@docs/api-contracts-guidelines.md`:
- Around line 110-126: Clarify the Finalizer Pattern cleanup flow by explicitly
stating that the controller must first verify the target object's
OwnerReferences against the current MustGather instance UID, then delete the
Job's Pods before deleting the Job itself. Update the wording around the cleanup
steps in the MustGather contract so the ownership check and delete order are
unambiguous and tied to the MustGather UID and job cleanup logic.
- Line 33: The documentation currently describes Storage like a union, but it
should be documented as a simple enum plus required nested config instead.
Update the Storage guidance in the api-contracts section to remove any
union/discriminator framing and make it clear that StorageType is the only enum
involved today, with PersistentVolume as the current value. Keep the reference
to Storage and StorageType so the correct section is easy to find.
🪄 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: 10a7761f-b2d5-4587-a9d7-1602743eb487
📒 Files selected for processing (12)
.coderabbit.yamlAGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mddocs/ARCHITECTURE.mddocs/api-contracts-guidelines.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
| ``` | ||
| api/v1alpha1/ # CRD types, deepcopy, OpenAPI generation | ||
| controllers/mustgather/ # Controller, Job template, predicates, SFTP validation | ||
| mustgather_controller.go # Main reconcile loop | ||
| template.go # Job/container spec generation | ||
| predicates.go # Event filters for controller-runtime | ||
| validation.go # SFTP credential and proxy validation | ||
| constant.go # Shared constants (validation types, env var names) | ||
| config/ # Operator name, OLM constants | ||
| pkg/localmetrics/ # Prometheus counter definitions | ||
| pkg/k8sutil/ # Namespace detection utility | ||
| build/bin/upload # Shell script: compress + SFTP upload (runs in container) | ||
| deploy/ # Kubernetes manifests (Deployment, RBAC, CRD) | ||
| test/e2e/ # E2E tests (Ginkgo/Gomega, build tag: e2e) | ||
| test/library/ # E2E test helpers (dynamic resource loader, kube clients) | ||
| examples/ # Sample MustGather CR YAML files | ||
| boilerplate/ # OpenShift-eng convention system (do not edit directly) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language tags to the fenced Markdown blocks.
These two non-code fences trigger the markdownlint MD040 warning. Mark them as text (or another appropriate language) so the docs stay lint-clean.
Suggested fix
-```
+```textApply the same change to the owner-reference block below.
Also applies to: 66-69
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@AGENTS.md` around lines 30 - 47, Add language tags to the two fenced Markdown
blocks in AGENTS.md so markdownlint MD040 is satisfied; update the unlabeled
fences in the repository layout section and the owner-reference block to use an
explicit language such as text, keeping the existing content unchanged. Use the
surrounding markdown list and the nearby owner-reference section to locate both
fences.
Source: Linters/SAST tools
| ``` | ||
| MG-NNN: Short description of the change | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label this fenced example for markdownlint.
Add a language tag like text to the fence so MD040 doesn't flag the file.
Suggested fix
-```
+```text
MG-NNN: Short description of the change
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| MG-NNN: Short description of the change | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@CONTRIBUTING.md` around lines 100 - 102, The fenced example in the
CONTRIBUTING markdown needs a language tag so markdownlint MD040 does not flag
it. Update the fenced block containing “MG-NNN: Short description of the change”
to use a text language label, and keep the example otherwise unchanged; locate
the snippet by its fenced markdown example rather than line numbers.
Source: Linters/SAST tools
| **Decision**: The operator runs in a single namespace (`must-gather-operator`) but watches MustGather CRs across all namespaces. | ||
|
|
||
| **Rationale**: | ||
| - Jobs are created in the CR's namespace, not the operator's namespace | ||
| - This allows users to leverage namespace-scoped RBAC for must-gather Jobs | ||
| - The operator needs cluster-scoped RBAC only for watching CRs and creating Jobs | ||
| - Avoids requiring elevated permissions in every namespace | ||
|
|
||
| **Trade-off**: More complex RBAC setup (cluster-scoped ClusterRole) vs. simpler namespace-scoped deployment. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Don't hardcode the operator namespace.
must-gather-operator reads like a deployment default, not an architectural invariant. Rephrase this as a configurable operator namespace instead of a fixed name.
Based on learnings, OPERATOR_NAMESPACE is required at startup and only falls back to config.OperatorName in local mode.
♻️ Proposed fix
- The operator runs in a single namespace (`must-gather-operator`) but watches MustGather CRs across all namespaces.
+ The operator runs in a single deployment-defined namespace but watches MustGather CRs across all namespaces.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Decision**: The operator runs in a single namespace (`must-gather-operator`) but watches MustGather CRs across all namespaces. | |
| **Rationale**: | |
| - Jobs are created in the CR's namespace, not the operator's namespace | |
| - This allows users to leverage namespace-scoped RBAC for must-gather Jobs | |
| - The operator needs cluster-scoped RBAC only for watching CRs and creating Jobs | |
| - Avoids requiring elevated permissions in every namespace | |
| **Trade-off**: More complex RBAC setup (cluster-scoped ClusterRole) vs. simpler namespace-scoped deployment. | |
| **Decision**: The operator runs in a single deployment-defined namespace but watches MustGather CRs across all namespaces. | |
| **Rationale**: | |
| - Jobs are created in the CR's namespace, not the operator's namespace | |
| - This allows users to leverage namespace-scoped RBAC for must-gather Jobs | |
| - The operator needs cluster-scoped RBAC only for watching CRs and creating Jobs | |
| - Avoids requiring elevated permissions in every namespace | |
| **Trade-off**: More complex RBAC setup (cluster-scoped ClusterRole) vs. simpler namespace-scoped deployment. |
🤖 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 `@docs/ARCHITECTURE.md` around lines 17 - 25, The architecture note hardcodes
the operator namespace as a fixed value, but it should describe a configurable
operator namespace instead. Update the wording in the decision/rationale around
the operator namespace to refer to OPERATOR_NAMESPACE as the required startup
setting, and mention that config.OperatorName is only a local-mode fallback.
Keep the rest of the RBAC and watch/job placement explanation intact while
removing the impression that must-gather-operator is an invariant.
Source: Learnings
| **Decision**: Each MustGather Job uses two containers sharing a process namespace: | ||
| 1. **gather** - Runs the must-gather image, writes to `/must-gather` | ||
| 2. **upload** - Runs the operator image, waits for gather completion, compresses and uploads via SFTP | ||
|
|
||
| **Rationale**: | ||
| - **Separation of concerns**: Gather container runs user-specified images (potentially untrusted), upload container runs operator-controlled code | ||
| - **Permission isolation**: Gather runs with cluster-admin via ServiceAccount, upload runs with minimal permissions | ||
| - **Process detection**: `ShareProcessNamespace: true` allows upload to detect gather completion via `pgrep -a gather` | ||
| - **No gather image modification**: Must-gather images don't need built-in upload logic |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced section with line numbers
sed -n '1,120p' docs/ARCHITECTURE.md | cat -n
# Search for related permission / service account wording in the repo
rg -n "ServiceAccount|cluster-admin|ShareProcessNamespace|pgrep -a gather|must-gather|SFTP|upload container|gather container" docs . -g '!**/.git/**'Repository: openshift/must-gather-operator
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the job template and the lines that set service account / container fields
rg -n "ServiceAccountName|serviceAccountName|upload container|gather container|ShareProcessNamespace|automountServiceAccountToken|must-gather-admin|OperatorImage" -- glob '*.go' -- glob '*.md' -- glob '*.yaml'
# Inspect the most likely template/controller files around the relevant logic
fd -a "template.go|mustgather_controller.go" .Repository: openshift/must-gather-operator
Length of output: 526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant source files by name
fd -a 'template.go' .
fd -a 'mustgather_controller.go' .
# Search for the job template construction and service account settings
rg -n "ServiceAccountName|automountServiceAccountToken|ShareProcessNamespace|OperatorImage|upload container|gather container|must-gather-admin" -- ./api ./controllers ./pkg ./internal ./test ./docs 2>/dev/null || trueRepository: openshift/must-gather-operator
Length of output: 18236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the job template construction around the ServiceAccount and containers
sed -n '1,320p' controllers/mustgather/template.go | cat -n
# Show the controller validation that references the operator's ServiceAccount
sed -n '130,190p' controllers/mustgather/mustgather_controller.go | cat -n
# Read the security guidance that mentions the upload container's trust/permissions model
sed -n '1,170p' docs/security-guidelines.md | cat -nRepository: openshift/must-gather-operator
Length of output: 25400
Tighten the container permission model. The upload container shares the pod’s ServiceAccount with the gather container, so it doesn’t have a separate Kubernetes identity. Describe it as trusted operator-controlled code that doesn’t need API access, rather than as isolated permissions.
🤖 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 `@docs/ARCHITECTURE.md` around lines 31 - 39, The architecture note in the
MustGather Job description overstates permission isolation for the upload
container; it shares the same ServiceAccount as gather and does not have a
separate Kubernetes identity. Update the wording around the gather/upload
container split to emphasize that upload is trusted operator-controlled code
that relies on process namespace sharing for completion detection and does not
need Kubernetes API access, rather than claiming distinct pod-level permissions.
| **Why not a sidecar?**: Sidecars would require gather to signal completion. Process namespace sharing avoids modifying gather images. | ||
|
|
||
| **Why not sequential containers?**: Kubernetes doesn't support sequential container execution within a single pod. Init containers can't share volumes with main containers effectively for this use case. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== architecture excerpt ==\n'
sed -n '30,55p' docs/ARCHITECTURE.md
printf '\n== line count ==\n'
wc -l docs/ARCHITECTURE.mdRepository: openshift/must-gather-operator
Length of output: 1841
🌐 Web query:
Kubernetes init containers share volumes with app containers process namespace init containers run before app containers documentation
💡 Result:
Init containers in Kubernetes run before application containers and can share volumes, network resources, and process namespaces with them, depending on the Pod configuration [1][2][3]. Init Containers and Execution Order Init containers are specialized containers that run during Pod startup, before any application containers start [2][3]. If multiple init containers are defined, they run sequentially in the order specified in the Pod's configuration [4][3]. Each init container must complete successfully before the next one starts, and all init containers must finish successfully before the application containers can begin [4][3]. Shared Volumes Init containers and application containers can share volumes [5][6]. By mounting the same volume in both the init container and the application container, you can use the init container to perform pre-processing tasks, such as generating configuration files, setting permissions, or pulling data, which are then accessed by the application container [7][6]. Process Namespace Sharing Kubernetes allows containers within a Pod to share a process namespace [8]. When this feature is enabled (by setting shareProcessNamespace: true in the Pod spec), all containers in the Pod—including init containers and regular application containers—share a single process namespace [9][10]. This allows processes in one container to see and interact with processes in other containers [8]. Resource Sharing Init containers share the same network, resource limits (CPU/memory), and, if configured, process namespace as the main application containers [1]. However, unlike sidecar containers (which are initialized using the restartPolicy: Always configuration), init containers are not intended to run continuously alongside the main application; they are designed to run to completion [2][3].
Citations:
- 1: https://kubernetes.io/docs/concepts/workloads/pods/init-containers/
- 2: https://kubernetes.io/docs/concepts/workloads/pods/
- 3: https://github.com/kubernetes/website/blob/main/content/en/docs/concepts/workloads/pods/init-containers.md
- 4: https://developers.redhat.com/articles/2025/05/19/container-starting-and-termination-order-pod
- 5: https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-initialization/
- 6: https://oneuptime.com/blog/post/2026-02-09-init-containers-shared-volume/view
- 7: https://kubernetes.io/docs/tasks/access-application-cluster/communicate-containers-same-pod-shared-volume/
- 8: https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/
- 9: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/495-pod-pid-namespace/README.md
- 10: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/495-pod-pid-namespace
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('docs/ARCHITECTURE.md')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 38 <= i <= 46:
print(f"{i}: {line}")
PYRepository: openshift/must-gather-operator
Length of output: 783
🌐 Web query:
Kubernetes shareProcessNamespace init containers include init containers same process namespace documentation
💡 Result:
Yes, when the shareProcessNamespace field is set to true in a Pod specification, all containers in the Pod, including init containers, share the same process namespace [1][2]. When this setting is enabled: - All containers in the Pod can see the processes of all other containers, including processes that were started by init containers and are still running (if any), as well as those in the main application containers [1][3]. - Processes are visible to other containers in the Pod, and users can view process information in /proc, signal processes (if they have the necessary permissions), and access other containers' filesystems via the /proc/$pid/root link [1]. It is important to note that because init containers run to completion before the main application containers start, an init container's processes will only be visible to subsequent containers if they continue to run after the init container has terminated (e.g., if the init container spawns a background process that persists) [3]. Technical documentation and design specifications confirm that the kubelet translates the shareProcessNamespace setting into a single namespace mode for the pod sandbox and all regular and init containers [2].
Citations:
- 1: https://github.com/kubernetes/website/blob/master/content/en/docs/tasks/configure-pod-container/share-process-namespace.md
- 2: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/495-pod-pid-namespace
- 3: https://stackoverflow.com/questions/65576193/how-to-keep-a-process-running-throughout-pod-lifecycle
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "shareProcessNamespace|init containers|sequential containers|gather completion" docs/ARCHITECTURE.mdRepository: openshift/must-gather-operator
Length of output: 683
Clarify the init-container limitation.
Init containers do share volumes, and shareProcessNamespace applies to them too; the real limitation is that they run before app containers, so they can’t wait for gather completion or observe the long-running gather process this design depends on.
🤖 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 `@docs/ARCHITECTURE.md` around lines 41 - 43, Update the explanation in the
architecture doc to reflect the real init-container limitation: in the section
near the sequential containers discussion, clarify that init containers do share
volumes and can use shareProcessNamespace, but they run before app containers so
they cannot wait for gather to finish or observe the long-running gather process
this design relies on. Keep the reference to the sidecar vs. sequential
container rationale in ARCHITECTURE.md aligned with that distinction.
| - **No cross-namespace secret copy.** The Secret must exist in the same namespace as the MustGather CR. The Job references it via `SecretKeyRef` env vars. | ||
| - **Required fields:** `username` and `password`, both must be non-empty byte slices. | ||
| - **Validation order:** Check Secret exists -> check fields present and non-empty -> validate SFTP credentials -> create Job. | ||
| - **Secret not-found is terminal;** transient Get errors requeue with `Requeue: true`. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the Secret-not-found behavior with the controller.
controllers/mustgather/mustgather_controller.go:141-241 still routes a missing Secret through ManageError(...), so calling it terminal here is inaccurate. Either update the reconcile logic too, or rewrite this bullet as a retried runtime error.
Suggested doc fix
-- **Secret not-found is terminal;** transient Get errors requeue with `Requeue: true`.
+- **Secret not-found is retried via `ManageError`;** transient Get errors requeue with `Requeue: true`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **No cross-namespace secret copy.** The Secret must exist in the same namespace as the MustGather CR. The Job references it via `SecretKeyRef` env vars. | |
| - **Required fields:** `username` and `password`, both must be non-empty byte slices. | |
| - **Validation order:** Check Secret exists -> check fields present and non-empty -> validate SFTP credentials -> create Job. | |
| - **Secret not-found is terminal;** transient Get errors requeue with `Requeue: true`. | |
| - **No cross-namespace secret copy.** The Secret must exist in the same namespace as the MustGather CR. The Job references it via `SecretKeyRef` env vars. | |
| - **Required fields:** `username` and `password`, both must be non-empty byte slices. | |
| - **Validation order:** Check Secret exists -> check fields present and non-empty -> validate SFTP credentials -> create Job. | |
| - **Secret not-found is retried via `ManageError`;** transient Get errors requeue with `Requeue: true`. |
🤖 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 `@docs/integration-guidelines.md` around lines 50 - 54, The Secret-not-found
handling described in the integration guidelines does not match the actual
reconcile flow in MustGatherController. Update the MustGather controller logic
around ManageError and the Secret lookup path so a missing Secret is treated
consistently with the documented behavior, or change the doc bullet to describe
it as a retried runtime error instead of terminal. Use the MustGather
reconciliation code and its Secret existence check to align the behavior and
wording.
| ``` | ||
| +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || self.spec == oldSelf.spec" | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to this fenced example.
Markdownlint already flags this block (MD040). A language hint keeps docs checks green.
Proposed fix
-```
+```go📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || self.spec == oldSelf.spec" | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/security-guidelines.md` around lines 70 - 72, Add a language tag to the
fenced example in the docs so the Markdown lint rule is satisfied; update the
fence around the kubebuilder validation snippet to include a Go language hint
using the nearby markdown example block, and keep the content unchanged in the
security-guidelines example.
Source: Linters/SAST tools
| The upload script (`build/bin/upload`) has these security-relevant behaviors: | ||
|
|
||
| - `StrictHostKeyChecking=no` with a dedicated `UserKnownHostsFile` at `/tmp/must-gather-operator/.ssh/known_hosts`. The SSH directory and known_hosts file are created by the upload container via `mkdir -p` and `touch` commands with restricted permissions (`chmod 700` for the directory, `chmod 600` for known_hosts) in `template.go`. | ||
| - Password is passed via `SSHPASS` env var with `sshpass -e`, never on the command line. | ||
| - Proxy credentials may appear in `ProxyCommand` arguments for HTTP proxies (via `--proxy-auth`). For HTTPS proxies, credentials are passed via exported env vars to `https-proxy-connect-util`. | ||
| - Internal users upload to a different SFTP path (`username/caseID_filename`). This path construction is not sanitized beyond the SFTP server's own controls. | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant docs section ==\n'
nl -ba docs/security-guidelines.md | sed -n '100,130p'
printf '\n== Locate upload path construction ==\n'
rg -n --hidden --glob '!**/.git/**' 'caseID|username/caseID|SFTP|upload path|ProxyCommand|sshpass|known_hosts' .
printf '\n== Candidate files around upload logic ==\n'
fd -a 'upload|template\.go|bin' .Repository: openshift/must-gather-operator
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Relevant docs section ==\n'
cat -n docs/security-guidelines.md | sed -n '100,130p'
printf '\n== Search for path construction and upload logic ==\n'
rg -n --hidden --glob '!**/.git/**' 'caseID|username/|upload.*sftp|SFTP|ProxyCommand|sshpass|known_hosts|ProxyCommand|proxy-auth|https-proxy-connect-util' .
printf '\n== File names that look relevant ==\n'
fd -a 'template.go|upload|security-guidelines.md' .Repository: openshift/must-gather-operator
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Outline template.go ==\n'
ast-grep outline controllers/mustgather/template.go --view expanded || true
printf '\n== Relevant slices from template.go ==\n'
cat -n controllers/mustgather/template.go | sed -n '130,240p'
printf '\n== Outline upload script if present ==\n'
fd -a 'upload' build controllers test || true
printf '\n== Search for remote path assembly details ==\n'
rg -n --hidden --glob '!**/.git/**' 'caseID.*filename|filename.*caseID|username.*filename|InternalUser|remote.*path|sftp.*Put|Put\(|Rename\(|Mkdir\(|Create\(' build controllers testRepository: openshift/must-gather-operator
Length of output: 11538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== getUploadContainer section ==\n'
cat -n controllers/mustgather/template.go | sed -n '318,430p'
printf '\n== upload script ==\n'
cat -n build/bin/upload | sed -n '1,260p'
printf '\n== caseID validation rules ==\n'
cat -n api/v1alpha1/mustgather_types.go | sed -n '118,170p'Repository: openshift/must-gather-operator
Length of output: 11206
Normalize the path components before upload. build/bin/upload concatenates the user-controlled caseid (and username for internal uploads) directly into REMOTE_FILENAME; the tarball name itself is generated locally. Allowlist/normalize those inputs before assembling the remote path instead of relying on SFTP server-side rejection.
🤖 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 `@docs/security-guidelines.md` around lines 112 - 118, `build/bin/upload` is
building the remote upload path from user-controlled `caseid` and, for internal
uploads, `username` without validation; update the upload path assembly to
normalize and allowlist these components before they are combined into
`REMOTE_FILENAME`. Use the existing upload flow in `build/bin/upload` and the
internal path construction logic to reject unsafe characters or path traversal
sequences, then only concatenate sanitized values so the remote SFTP target
stays predictable.
| ### Async Assertions | ||
|
|
||
| ```go | ||
| Eventually(func() bool { ... }). | ||
| WithTimeout(2 * time.Minute).WithPolling(5 * time.Second).Should(BeTrue()) | ||
| ``` | ||
|
|
||
| Timeouts: 1-2 min for resource creation, 5 min for job completion, 30s for `Consistently`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the job-completion timeout with the current e2e suite.
test/e2e/must_gather_operator_test.go:491-532 currently waits 2 minutes for job creation/completion, so the “5 min for job completion” guidance is out of sync. Either update the recommendation or call out that 5 minutes is only an upper bound for exceptional cases.
🛠️ Possible wording
- Timeouts: 1-2 min for resource creation, 5 min for job completion, 30s for `Consistently`.
+ Timeouts: 1-2 min for resource creation, 2 min for job completion in the current suite, 30s for `Consistently`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Async Assertions | |
| ```go | |
| Eventually(func() bool { ... }). | |
| WithTimeout(2 * time.Minute).WithPolling(5 * time.Second).Should(BeTrue()) | |
| ``` | |
| Timeouts: 1-2 min for resource creation, 5 min for job completion, 30s for `Consistently`. | |
| ### Async Assertions | |
🤖 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 `@docs/testing-guidelines.md` around lines 153 - 160, The async assertions
guidance is out of sync with the e2e tests: the job-completion timeout
recommendation no longer matches the behavior in must_gather_operator_test.
Update the wording in the testing guidelines near the Async Assertions section
to reflect the current job-completion wait used by the e2e suite, or explicitly
state that 5 minutes is only an upper bound for exceptional cases. Make sure the
guidance stays aligned with the job-related assertions in
must_gather_operator_test.
| ## Local Development | ||
|
|
||
| Execute the following steps to develop the functionality locally. It is recommended that development be done using a cluster with `cluster-admin` permissions. | ||
|
|
||
| In the operator's `Deployment.yaml` [file](deploy/99_must-gather-operator.Deployment.yaml), add a variable to the deployment's `spec.template.spec.containers.env` list called `OPERATOR_IMAGE` and set the value to your local copy of the image: | ||
| ```shell | ||
| env: | ||
| - name: OPERATOR_IMAGE | ||
| value: "registry.example/repo/image:latest" | ||
| ``` | ||
| Then run: | ||
| ```shell | ||
| go mod download | ||
| ``` | ||
| 1. In the operator's `Deployment.yaml` [file](deploy/99_must-gather-operator.Deployment.yaml), add a variable to the deployment's `spec.template.spec.containers.env` list called `OPERATOR_IMAGE` and set the value to your local copy of the image: | ||
|
|
||
| Using the [operator-sdk](https://github.com/operator-framework/operator-sdk), run the operator locally: | ||
| ```yaml | ||
| env: | ||
| - name: OPERATOR_IMAGE | ||
| value: "registry.example/repo/image:latest" | ||
| ``` | ||
|
|
||
| ```shell | ||
| oc apply -f deploy/crds/operator.openshift.io_mustgathers_crd.yaml | ||
| oc new-project must-gather-operator | ||
| export DEFAULT_MUST_GATHER_IMAGE='quay.io/openshift/origin-must-gather:latest' | ||
| OPERATOR_NAME=must-gather-operator operator-sdk run --verbose --local --namespace '' | ||
| 2. Download dependencies: | ||
|
|
||
| ```shell | ||
| go mod download | ||
| ``` | ||
|
|
||
| 3. Using the [operator-sdk](https://github.com/operator-framework/operator-sdk), apply the CRD and run the operator locally: | ||
|
|
||
| ```shell | ||
| oc apply -f deploy/crds/operator.openshift.io_mustgathers_crd.yaml | ||
| oc new-project must-gather-operator | ||
| export DEFAULT_MUST_GATHER_IMAGE='quay.io/openshift/origin-must-gather:latest' | ||
| OPERATOR_NAME=must-gather-operator operator-sdk run --verbose --local --namespace '' | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Export OPERATOR_IMAGE in the local-dev example.
Based on learnings, the controller still needs OPERATOR_IMAGE at startup, but this recipe only exports DEFAULT_MUST_GATHER_IMAGE, so the local-run example stops short of the documented runtime contract.
Suggested fix
export DEFAULT_MUST_GATHER_IMAGE='quay.io/openshift/origin-must-gather:latest'
+export OPERATOR_IMAGE='quay.io/openshift/origin-must-gather-operator:latest'
OPERATOR_NAME=must-gather-operator operator-sdk run --verbose --local --namespace ''📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Local Development | |
| Execute the following steps to develop the functionality locally. It is recommended that development be done using a cluster with `cluster-admin` permissions. | |
| In the operator's `Deployment.yaml` [file](deploy/99_must-gather-operator.Deployment.yaml), add a variable to the deployment's `spec.template.spec.containers.env` list called `OPERATOR_IMAGE` and set the value to your local copy of the image: | |
| ```shell | |
| env: | |
| - name: OPERATOR_IMAGE | |
| value: "registry.example/repo/image:latest" | |
| ``` | |
| Then run: | |
| ```shell | |
| go mod download | |
| ``` | |
| 1. In the operator's `Deployment.yaml` [file](deploy/99_must-gather-operator.Deployment.yaml), add a variable to the deployment's `spec.template.spec.containers.env` list called `OPERATOR_IMAGE` and set the value to your local copy of the image: | |
| Using the [operator-sdk](https://github.com/operator-framework/operator-sdk), run the operator locally: | |
| ```yaml | |
| env: | |
| - name: OPERATOR_IMAGE | |
| value: "registry.example/repo/image:latest" | |
| ``` | |
| ```shell | |
| oc apply -f deploy/crds/operator.openshift.io_mustgathers_crd.yaml | |
| oc new-project must-gather-operator | |
| export DEFAULT_MUST_GATHER_IMAGE='quay.io/openshift/origin-must-gather:latest' | |
| OPERATOR_NAME=must-gather-operator operator-sdk run --verbose --local --namespace '' | |
| 2. Download dependencies: | |
| ```shell | |
| go mod download | |
| ``` | |
| 3. Using the [operator-sdk](https://github.com/operator-framework/operator-sdk), apply the CRD and run the operator locally: | |
| ```shell | |
| oc apply -f deploy/crds/operator.openshift.io_mustgathers_crd.yaml | |
| oc new-project must-gather-operator | |
| export DEFAULT_MUST_GATHER_IMAGE='quay.io/openshift/origin-must-gather:latest' | |
| OPERATOR_NAME=must-gather-operator operator-sdk run --verbose --local --namespace '' | |
| ``` | |
| ## Local Development | |
| Execute the following steps to develop the functionality locally. It is recommended that development be done using a cluster with `cluster-admin` permissions. | |
| 1. In the operator's `Deployment.yaml` [file](deploy/99_must-gather-operator.Deployment.yaml), add a variable to the deployment's `spec.template.spec.containers.env` list called `OPERATOR_IMAGE` and set the value to your local copy of the image: |
🤖 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 `@README.md` around lines 78 - 103, The local development example is missing
the runtime environment variable required by the controller, so the startup
recipe is incomplete. Update the local-run steps in README.md near the local
development workflow to also export OPERATOR_IMAGE alongside
DEFAULT_MUST_GATHER_IMAGE before invoking operator-sdk run, matching the
deployment contract documented by the operator. Use the local-dev section and
the operator-sdk run example as the reference points so the example remains
consistent with the controller’s expected environment.
Source: Learnings
|
@praveencodes: The following tests 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. |
3 participants
Compared the below 2 tools/plugins and created the agentic docs with Gwen's agent-readiness tool.
Summary by CodeRabbit
Documentation
Chores