Skip to content

Ignore soft dependencies of LCM and chart prioritization #559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dharmit wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Ignore soft dependencies of LCM and chart prioritization #559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f66143
Implicitly remove SUC and cert-manager dependencies for LCM chart
dharmit Aug 18, 2026
4dc6f47
Prioritize core chart over solution
dharmit Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions internal/config/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,18 @@ func (h *Helm) writeHelmSecrets(secrets []*helm.Secret, butaneCfg *butane.Config
func (h *Helm) retrieveHelmCharts(rm *resolver.ResolvedManifest, conf *image.Configuration) ([]*helm.CRD, []*helm.Secret, error) {
var crds []*helm.CRD

valueFiles := conf.Release.Components.HelmValueFiles()

err := evaluateLCMDeps(conf.Release.Components.HelmCharts, rm.CorePlatform, valueFiles, h.ValuesResolver)
if err != nil {
return nil, nil, fmt.Errorf("evaluating %s dependencies: %w", elementalLifecycleManager, err)
}

charts, repositories, err := enabledHelmCharts(rm, conf.Release.Components.HelmCharts, h.Logger)
if err != nil {
return nil, nil, fmt.Errorf("filtering enabled helm charts: %w", err)
}

valueFiles := conf.Release.Components.HelmValueFiles()

authMap, err := createAuthMap(charts, repositories, conf)
if err != nil {
return nil, nil, fmt.Errorf("creating helm chart auth map: %w", err)
Expand Down Expand Up @@ -288,17 +293,16 @@ func enabledHelmCharts(rm *resolver.ResolvedManifest, enabled []release.HelmChar
var addChart func(name string) error

// Add a chart and its direct dependencies, avoiding duplicates.
// Prioritize charts from solution releases over core ones.
addChart = func(name string) error {
source := "solution"
source := "core"

chart, ok := solutionCharts[name]
chart, ok := coreCharts[name]
if !ok {
chart, ok = coreCharts[name]
chart, ok = solutionCharts[name]
if !ok {
return fmt.Errorf("helm chart does not exist")
}
source = "core"
source = "solution"
}

if logger != nil {
Expand Down
57 changes: 57 additions & 0 deletions internal/config/helm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,4 +855,61 @@ spec:
Expect(repositories).To(BeNil())
})
})

Describe("prioritizing charts", func() {
var rm *resolver.ResolvedManifest

BeforeEach(func() {
rm = &resolver.ResolvedManifest{
CorePlatform: &core.ReleaseManifest{
Components: core.Components{
Helm: &api.Helm{
Charts: []*api.HelmChart{
{
Name: "Common Chart",
Chart: "common-chart",
Version: "core",
Namespace: "default",
Repository: "common-repo",
},
},
Repositories: []*api.HelmRepository{
{
Name: "common-repo",
URL: "https://charts.common-repo.lol",
},
},
},
},
},
SolutionExtension: &solution.ReleaseManifest{
Components: solution.Components{
Helm: &api.Helm{
Charts: []*api.HelmChart{
{
Name: "Common Chart",
Chart: "common-chart",
Version: "solution",
Namespace: "default",
Repository: "common-repo",
},
},
Repositories: []*api.HelmRepository{
{
Name: "common-repo",
URL: "https://charts.common-repo.lol",
},
},
},
},
},
}
})

It("should prioritze core over solution", func() {
charts, _, err := enabledHelmCharts(rm, []release.HelmChart{{Name: "common-chart"}}, logger)
Expect(err).ToNot(HaveOccurred())
Expect(charts[0].Version).To(Equal("core"))
})
})
})
127 changes: 127 additions & 0 deletions internal/config/lcm.go
View file
Open in desktop

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this file consists of a single function and is doing essentially a helm chart configuration, would it make sense to merge its logic and its _test.go with the helm.go file? That way we would have all the chart handling logic at a single place and will keep the number of files for this package to a minimum.

Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
Copyright © 2025-2026 SUSE LLC
SPDX-License-Identifier: Apache-2.0

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package config

import (
"fmt"
"slices"

"github.com/suse/elemental/v3/internal/image/release"
"github.com/suse/elemental/v3/pkg/helm"
"github.com/suse/elemental/v3/pkg/manifest/api"
"github.com/suse/elemental/v3/pkg/manifest/api/core"
"go.yaml.in/yaml/v3"
)

const (
elementalLifecycleManager = "elemental-lifecycle-manager"
rancher = "rancher"
systemUpgradeController = "system-upgrade-controller"
certManager = "cert-manager"
)

// lcmWebhookValues is used to marshal data from values file for LCM chart
type lcmWebhookValues struct {

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not foresee this type being every used outside of evaluateLCMDeps, as such would it make sense to include it as an anonymous struct inside the func?

Also we could maybe simplify the struct itself to something like:

var lcmWebhookValues struct {
		Webhook struct {
			Cert *struct {
				CreateDefault bool `yaml:"createDefault"`
			} `yaml:"cert"`
		} `yaml:"webhook"`
	}

And then check against the Cert struct pointer and the CreateDefault field themselves. That would be enough, as if the user has continuously provided the "cert:" field with a non-true createDefault, then it is up to them to define the other fields, in our eyes this should be enough for the trigger to disable the cert-manager dependency.

Webhook struct {
Cert struct {
CreateDefault bool `yaml:"createDefault"`
ExistingSecret string `yaml:"existingSecret"`
CABundle string `yaml:"caBundle"`
} `yaml:"cert"`
} `yaml:"webhook"`
}

// evaluateLCMDeps removes dependencies of LCM if they are satisfied separately, i.e.,
// - if Rancher chart is enabled in release.yaml, it removes dependency on system-upgrade-controller
// - if the values files contains custom certificate configuration, it removes dependency on cert-manager
func evaluateLCMDeps(enabled []release.HelmChart, corePlatform *core.ReleaseManifest, valueFiles map[string]string, valuesResolver helmValuesResolver) error {

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions here:

  1. I am not sure that the valueFiles map[string]string parameter is needed here. The values file location can be taken from the LCM entry in the enabled []release.HelmChart slice.
  2. This function seems to be doing too much, could we perhaps split out some of its logic in separate functions. For example:
    • We could extract the logic to find a chart from the core manifest in a separate generic function that could be reused for other use-cases in the future.
    • We could extract the logic to resolve a specific charts values (or a portion of it) in a separate generic function that could then be reused for other use-cases in the future.

var lcmChart *api.HelmChart

var (
lcmEnabled = false
rancherEnabled = false
)
for _, chart := range enabled {
if chart.Name == rancher {
rancherEnabled = true

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a personal preference, but instead of populating boolean values, would it make sense to convert the enabled slice to a map and check against the map keys. That way we could do the dependency removal in the same block that we check whether the chart is enabled.

continue
}
if chart.Name == elementalLifecycleManager {
lcmEnabled = true
}
}
if !lcmEnabled {
// nothing to do!
return nil

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we error out here? IIRC, we agreed that LCM is a required chart that should be specified by the user under release.yaml. By returning nil here, we do not have any gate that ensures that LCM will be on the cluster. Probably missing something.

}

coreCharts := corePlatform.Components.Helm
for _, chart := range coreCharts.Charts {
if chart.GetName() == elementalLifecycleManager {
lcmChart = chart
break
}
}

if lcmChart == nil {
// this could be the case if using core manifest that doesn't contain LCM charts which is the case currently
// TODO (dharmit): remove this check once the core manifest includes LCM chart by default
return nil // nothing to do!

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also error out here as well? My main thinking is that LCM will always be a part of the core manifest, so it not being present in the parsed corePlatform struct should be flagged.

Or did you want to make the elemental3 binary backwards compatible? If that is the case, I guess we need to decide how we want to handle this. IMO if LCM is going to be an expected component in our setup we need to be explicit here and guard against a missing LCM in release.yaml as well as in the core manifest. Let me know what you think.

}

if rancherEnabled {
// remove system-upgrade-controller from list of dependencies
for i, dep := range lcmChart.DependsOn {
if dep.Name == systemUpgradeController {
lcmChart.DependsOn = slices.Delete(lcmChart.DependsOn, i, i+1)

@ipetrov117 ipetrov117 Aug 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a personal preference, so feel free to ignore, but would it make sense to use the slices.DeleteFunc here and instead of this whole block, have something like:

lcmChart.DependsOn = slices.DeleteFunc(lcmChart.DependsOn,
			func(d api.HelmChartDependency) bool { return d.Name == systemUpgradeController })

break
}
}

}

_, ok := valueFiles[elementalLifecycleManager]
if !ok {
// values.yaml equivalent for LCM isn't provided; cert-manager dependency to be kept as-is
return nil
}

source := &helm.ValueSource{Inline: lcmChart.GetInlineValues(), File: valueFiles[lcmChart.GetName()]}
values, err := valuesResolver.Resolve(source)
if err != nil {
return fmt.Errorf("resolving values for chart %s: %w", lcmChart.GetName(), err)
}

var lcmValues lcmWebhookValues
err = yaml.Unmarshal(values, &lcmValues)
if err != nil {
return err
}

if !lcmValues.Webhook.Cert.CreateDefault && lcmValues.Webhook.Cert.ExistingSecret != "" {
// checking only createDefault and existingSecret because caBundle could be an empty string
for i, dep := range lcmChart.DependsOn {
if dep.Name == certManager {
lcmChart.DependsOn = slices.Delete(lcmChart.DependsOn, i, i+1)
break
}
}
}

return nil
}
Loading
Loading