ROX-27677: Eliminate Python dependency from operator build#18947
ROX-27677: Eliminate Python dependency from operator build#18947GrimmiMeloni merged 159 commits intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
/test all |
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 2 other issues
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `operator/bundle_helpers/cmd/fix_descriptors.go:63-64` </location>
<code_context>
+// This handles formatting quirks (quote styles, line wrapping, etc.) while keeping
+// all business logic in Go.
+func normalizeYAMLOutput(goYAML []byte, w io.Writer) error {
+ wd, _ := os.Getwd()
+ normalizerPath := filepath.Join(wd, "bundle_helpers", "yaml-normalizer.py")
+
+ // Run the normalizer
</code_context>
<issue_to_address>
**issue (bug_risk):** YAML normalizer path is likely incorrect when run via dispatch.sh (ends up with bundle_helpers/bundle_helpers/...)
Because `dispatch.sh` runs `go run` from `.../bundle_helpers`, `os.Getwd()` will already be `.../bundle_helpers`. Joining `bundle_helpers/yaml-normalizer.py` produces `.../bundle_helpers/bundle_helpers/yaml-normalizer.py`, which doesn’t exist, so normalization will always fail.
Resolve the script path relative to the source file or executable (e.g., via `os.Executable()` or an embed-based approach). If you can rely on the current working directory being `bundle_helpers`, just join `yaml-normalizer.py` directly instead of adding the extra directory level.
</issue_to_address>
### Comment 2
<location> `operator/bundle_helpers/pkg/rewrite/rewriter_test.go:19` </location>
<code_context>
+ {
+ name: "bare string cannot be modified",
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test where nested structures are traversed but no replacements occur to assert modified=false
Adding a case where nested structures are traversed but no values match `old`, and asserting `RewriteStrings` returns `false` and leaves the structure unchanged, would help ensure `modified` isn’t incorrectly set just because traversal occurred.
</issue_to_address>
### Comment 3
<location> `operator/bundle_helpers/cmd/fix_descriptors.go:67` </location>
<code_context>
cmd := exec.Command(normalizerPath)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 6b43bf6. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18947 +/- ##
==========================================
+ Coverage 49.28% 49.37% +0.08%
==========================================
Files 2735 2742 +7
Lines 206170 206921 +751
==========================================
+ Hits 101617 102159 +542
- Misses 97012 97176 +164
- Partials 7541 7586 +45
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:
|
|
/test all |
|
/retest |
1 similar comment
|
/retest |
|
/retest operator-bundle-on-push |
|
/test gke-nongroovy-e2e-tests |
|
/test gke-ui-e2e-tests |
1 similar comment
|
/test gke-ui-e2e-tests |
|
/test gke-upgrade-tests |
GrimmiMeloni
left a comment
There was a problem hiding this comment.
LGTM from my side
@porridge your are up next
5802001 to
d9c63cf
Compare
All other Konflux Dockerfiles already use rhel_9_golang_1.25. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The rhel_9 golang builder has an openssl-devel conflict with python3.12-libs. Use ubi9-minimal (like master) and install golang alongside python3.12-pyyaml via microdnf instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3d3f010 to
18279ce
Compare
|
/retest |
|
/konflux-retest operator-bundle-on-push |
The setup-go action's built-in cache uses go.sum as key without job-name scoping, which can race with other workflows. Since this workflow is temporary, caching is not worth the risk. Addresses review comment from janisz on PR #18947. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of just disabling caching, use the repo's standard cache-go-dependencies composite action which properly scopes cache keys by job name and separates module/build caches. Partially generated by AI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/konflux-retest operator-bundle-on-push |
Description
Ports the operator bundle preparation tools from Python to Go, making the Go implementation the default while keeping the Python scripts to verify equivalence via CI.
What changes:
operator/bundle_helpers/main.go) implementing two subcommands:fix-spec-descriptor-order— fix specDescriptor ordering in the operator bundlepatch-csv— patch the ClusterServiceVersion file (CSV version handling, image replacement, related images logic foromit/downstream/konfluxmodes)dispatch.shwrapper replaces direct Python script invocations inprepare-bundle-manifests.shand the Makefile. It routes to the Go binary by default (USE_GO_BUNDLE_HELPER=true) and falls back to Python when explicitly disabled..github/workflows/test-bundle-helpers.yaml) that runs both implementations and verifies byte-by-byte output equivalence, ensuring the Go port is correct.RELATED_IMAGES_MODEMakefile variable (omit/downstream/konflux) to control how image references are handled in the CSV.User-facing documentation
Testing and quality
Automated testing
How I validated my change
Local validation:
go test ./...)make golangci-lint)CI validation:
Scope:
Internal build tooling only.