Support ExternalArtifacts #7
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps Go toolchain and many dependencies; refactors Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI
participant Cmd as cmdFactory / Command
participant Handler as Command Handler
participant Resolver as resolveChartRef / getChartInfo
participant K8s as Kubernetes API (client.Client)
participant Merger as mergedValues
participant Helm as upgradeRelease / helmaction
participant History as markSuccess
CLI->>Cmd: invoke command (apply/diff/show/...)
Cmd->>Handler: run(ctx, cl, cfg, hr, chartDir)
Handler->>Resolver: resolveChartRef(ctx, cl, hr)
Resolver->>K8s: fetch ExternalArtifact / HelmChart / OCIRepository / sources
K8s-->>Resolver: chart metadata (name, version, valuesFiles)
Handler->>Merger: mergedValues(ctx, cl, hr, chartDir)
Merger->>K8s: load values files / inline values
K8s-->>Merger: values contents
Handler->>Helm: upgradeRelease(cfg, hr, chartDir, vals, takeOwnership)
Helm-->>Handler: result
Handler->>History: markSuccess(ctx, cl, recorder, hr, chartVer, cfgDigest)
History->>K8s: update status / emit events
K8s-->>History: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🪛 OSV Scanner (2.2.4)go.mod[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content in helm.sh/helm (GO-2025-3802) [HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion in helm.sh/helm (GO-2025-3887) [HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content in helm.sh/helm (GO-2025-3888) [HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content [HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion [HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content 🔇 Additional comments (14)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's ability to manage Helm releases by introducing support for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for ExternalArtifacts in HelmReleases, which involves significant refactoring of the command logic. It also adds a --take-ownership option for Helm upgrades. The changes are well-structured, but there are a few areas where error handling and code clarity could be improved. My review includes suggestions to make the code more robust, especially in handling different API versions and potential failures during Kubernetes client interactions. I've also pointed out a few places where readability could be enhanced.
main.go
Outdated
| prefixes := []string{"system-", "apps-"} | ||
| for _, prefix := range prefixes { | ||
| if strings.HasPrefix(refName, prefix) { | ||
| chartName = strings.TrimPrefix(refName, prefix) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to trim prefixes from ExternalArtifact names is hardcoded to system- and apps-. This is not very flexible and might be surprising to users who don't follow this specific convention. Consider making this configurable via a flag, or at least add a comment to explain that this is an expected naming convention for ExternalArtifacts in your environment.
| if err != nil { | ||
| artifactGenGVR = schema.GroupVersionResource{ | ||
| Group: "source.watcher.fluxcd.io", Version: "v2", Resource: "artifactgenerators", | ||
| } | ||
| list, err = dyn.Resource(artifactGenGVR).Namespace(extArtifactNS).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list ArtifactGenerators: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling for the first attempt to list ArtifactGenerators is too broad. It catches any error and proceeds to try the second GVR. This could mask important errors like authentication failures or network issues. It's better to specifically check if the error is because the resource kind is not found (meta.IsNoMatchError) before trying the alternative.
if err != nil {
if !meta.IsNoMatchError(err) {
return nil, fmt.Errorf("failed to list ArtifactGenerators with GVR %s: %w", artifactGenGVR, err)
}
artifactGenGVR = schema.GroupVersionResource{
Group: "source.watcher.fluxcd.io", Version: "v2", Resource: "artifactgenerators",
}
list, err = dyn.Resource(artifactGenGVR).Namespace(extArtifactNS).List(ctx, metav1.ListOptions{})
if err != nil {
return nil, fmt.Errorf("failed to list ArtifactGenerators with GVR %s: %w", artifactGenGVR, err)
}
}
main.go
Outdated
| cl, _ := client.New(rc, client.Options{}) | ||
| return fn(ctx, cl, cfg, stub, ".") |
There was a problem hiding this comment.
The error returned by client.New is being ignored. While the client may not be heavily used in plain mode, client.New can still fail (e.g., due to an invalid scheme setup), and it's best practice to handle all errors.
// Create a dummy client for plain mode
cl, err := client.New(rc, client.Options{})
if err != nil {
return err
}
return fn(ctx, cl, cfg, stub, ".")
main.go
Outdated
| // newHistoryEntry creates a v2.Snapshot for status.history. | ||
| func newHistoryEntry(hr *v2.HelmRelease, chartVersion, cfgDigest string) *v2.Snapshot { | ||
| func newHistoryEntry(ctx context.Context, cl client.Client, hr *v2.HelmRelease, chartVersion, cfgDigest string) *v2.Snapshot { | ||
| chartName, _, _ := getChartInfo(ctx, cl, hr) |
There was a problem hiding this comment.
The error from getChartInfo is ignored. If getChartInfo fails, chartName will be an empty string, resulting in an incomplete or incorrect history entry. The error should be handled, for example by logging it and falling back to a more generic name like the HelmRelease name.
chartName, _, err := getChartInfo(ctx, cl, hr)
if err != nil {
// Log the error and fall back to the HelmRelease name.
log.Printf("could not get chart info for %s/%s: %v", hr.Namespace, hr.Name, err)
chartName = hr.Name
}
main.go
Outdated
| // markSuccess sets Ready=True and emits a normal event. | ||
| func markSuccess(ctx context.Context, cl client.Client, rec record.EventRecorder, hr *v2.HelmRelease, chartVer, cfgDigest string) { | ||
| msg := fmt.Sprintf("Helm upgrade succeeded for %s/%s with chart %s@%s", hr.Namespace, hr.Name, hr.Spec.Chart.Spec.Chart, chartVer) | ||
| chartName, _, _ := getChartInfo(ctx, cl, hr) |
There was a problem hiding this comment.
The error from getChartInfo is ignored. If it fails, chartName will be empty, which leads to a confusing success message. The error should be handled, for example by falling back to the HelmRelease name.
chartName, _, err := getChartInfo(ctx, cl, hr)
if err != nil {
// Log the error and fall back to the HelmRelease name for the message.
log.Printf("could not get chart info for %s/%s: %v", hr.Namespace, hr.Name, err)
chartName = hr.Name
}
main.go
Outdated
| sourceNS = "" | ||
| sourceName = "" | ||
| sourceGVR = schema.GroupVersionResource{} // Clear GVR to skip annotation |
There was a problem hiding this comment.
Using sourceNS = "" and clearing other variables to signal that source reconciliation is complete and to skip a later block of code reduces readability. It would be clearer to use a dedicated boolean flag, for example sourceReconciliationDone, to control the flow.
For example, you could declare var sourceReconciliationDone bool at the top of the if withSource block, set it to true here, and then check if !sourceReconciliationDone for the next block.
Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
external-artifacts
branch
from
673f6c9 to
3816886
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
main.go (6)
252-258: Hardcoded prefix trimming lacks flexibility.The logic to trim prefixes from ExternalArtifact names uses hardcoded values ("system-", "apps-"), which may not be flexible for all naming conventions.
This concern was already raised in previous reviews.
307-315: Error handling is too broad.The error handling catches any error from the first GVR attempt and proceeds to try an alternative. This could mask important errors like authentication failures or network issues.
This concern was already raised in previous reviews, which suggested checking specifically for
meta.IsNoMatchError.
567-568: Ignoring error from client.New.The error returned by
client.Newis ignored. While the client may not be heavily used in plain mode, errors should still be handled.This concern was already raised in previous reviews.
1074-1074: Error from getChartInfo is ignored.If
getChartInfofails,chartNamewill be empty, resulting in incomplete history entries.This concern was already raised in previous reviews, which suggested logging the error and falling back to the HelmRelease name.
1090-1090: Error from getChartInfo is ignored.If
getChartInfofails,chartNamewill be empty, leading to a confusing success message.This concern was already raised in previous reviews, which suggested logging the error and falling back to the HelmRelease name.
1428-1430: Using empty string as a control flag reduces readability.Setting
sourceNS = ""to signal that source reconciliation is complete makes the control flow harder to follow.This concern was already raised in previous reviews, which suggested using a dedicated boolean flag like
sourceReconciliationDone.
🧹 Nitpick comments (1)
main.go (1)
1346-1430: Consider extracting ExternalArtifact source reconciliation logic.The nested logic for handling ExternalArtifact sources (finding ArtifactGenerator, iterating sources, determining GVR, annotating and waiting) is deeply nested and could benefit from extraction into a helper function for improved readability and testability.
For example:
func reconcileExternalArtifactSources(ctx context.Context, cl client.Client, dyn dynamic.Interface, refNS, refName string, waitByWatch func(...) error) error { // Extract the entire ExternalArtifact handling logic here artifactGen, err := findArtifactGeneratorForExternalArtifact(ctx, cl, refNS, refName) if err != nil { return fmt.Errorf("failed to find ArtifactGenerator for ExternalArtifact %s/%s: %w", refNS, refName, err) } sources, found, _ := unstructured.NestedSlice(artifactGen.Object, "spec", "sources") if !found || len(sources) == 0 { return fmt.Errorf("ArtifactGenerator %s has no sources", artifactGen.GetName()) } // ... rest of the logic return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(5 hunks)main.go(18 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content in helm.sh/helm
(GO-2025-3802)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion in helm.sh/helm
(GO-2025-3887)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content in helm.sh/helm
(GO-2025-3888)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content
🔇 Additional comments (7)
main.go (6)
35-35: LGTM!The import and scheme registration for Flux source-controller v1 types are correctly implemented to support the new ExternalArtifact functionality.
Also applies to: 78-78
336-351: LGTM!The
getChartInfofunction cleanly abstracts chart information retrieval from bothChartRefandChartsources with appropriate error handling.
354-400: LGTM!The
mergedValuesfunction correctly integrates context and client parameters to support dynamic resolution of values files from both ChartRef and Chart sources. The merge logic is preserved and error handling is appropriate.
738-738: LGTM! New --take-ownership flag added.The
--take-ownershipflag has been properly implemented as mentioned in the PR objectives, allowing users to take ownership of existing Helm resources during install/upgrade operations.
1270-1273: LGTM! Suspension check prevents unnecessary work.Adding a check to fail early if the HelmRelease is suspended is a good defensive practice that prevents wasted reconciliation attempts.
1403-1404: LGTM! Source suspension checks prevent cascading errors.The suspension checks for source resources (GitRepository, OCIRepository, Bucket, HelmChart) are appropriate and prevent attempting reconciliation on suspended resources.
Also applies to: 1472-1474
go.mod (1)
3-3: Go 1.25.0 is released and available.Go 1.25.0 was released on August 12, 2025 with binaries and source available on the official download page. The version is stable and ready to use.
| github.com/opencontainers/go-digest v1.0.0 | ||
| github.com/spf13/cobra v1.9.1 | ||
| github.com/spf13/cobra v1.10.1 | ||
| helm.sh/helm/v3 v3.18.2 |
There was a problem hiding this comment.
🧩 Analysis chain
Address HIGH severity vulnerabilities in Helm.
Multiple HIGH severity vulnerabilities are reported in helm.sh/helm/v3 v3.18.2:
- Code Injection through malicious chart.yaml (GO-2025-3802, GHSA-557j-xg8c-q2mm)
- Memory Exhaustion from JSON Schema values (GO-2025-3887, GHSA-9h84-qmv7-982p)
- Panic due to incorrect YAML content (GO-2025-3888, GHSA-f9f8-9pmf-xv68)
🏁 Script executed:
#!/bin/bash
# Check for the latest Helm version and security advisories
echo "Checking latest Helm v3 version..."
curl -s https://api.github.com/repos/helm/helm/releases/latest | jq -r '.tag_name'
echo -e "\nChecking for security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: GO, package: "helm.sh/helm/v3") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 2434
Upgrade Helm to v3.18.5 or later to address security vulnerabilities.
The review comment's severity assessment was inaccurate. v3.18.2 is actually affected by 1 HIGH severity and 2 MODERATE severity vulnerabilities, not 6 HIGH as claimed. The HIGH severity issue is Code Injection through malicious chart.yaml (patched in v3.18.4), and the MODERATE issues are Panic and Memory Exhaustion from YAML/JSON content (patched in v3.18.5). Upgrade to v3.18.5 or higher to address all reported vulnerabilities. Latest available is v4.0.0.
🧰 Tools
🪛 OSV Scanner (2.2.4)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content in helm.sh/helm
(GO-2025-3802)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion in helm.sh/helm
(GO-2025-3887)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content in helm.sh/helm
(GO-2025-3888)
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm vulnerable to Code Injection through malicious chart.yaml content
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm Charts with Specific JSON Schema Values Can Cause Memory Exhaustion
[HIGH] 14-14: helm.sh/helm/v3 3.18.2: Helm May Panic Due To Incorrect YAML Content
Signed-off-by: Andrei Kvapil <[email protected]>
kvaps
force-pushed
the
external-artifacts
branch
from
3816886 to
6bba540
Compare
Summary by CodeRabbit
New Features
Infrastructure