Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Move spire-lib macro's to their own library chart #346

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

Conversation

kfox1111
Copy link
Contributor

No description provided.

Copy link
Contributor

@mrsabath mrsabath left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +25 to +27
- name: spire-lib
repository: file://./charts/spire-lib
version: 0.1.0
Copy link
Contributor

@drewwells drewwells Jun 22, 2023

Choose a reason for hiding this comment

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

This is not a standard pattern. I suggest putting this in the parent chart. https://helm.sh/docs/chart_best_practices/templates/#names-of-defined-templates

This set of subchart would be improved by collapsing into one chart.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a standard pattern. I suggest putting this in the parent chart. helm.sh/docs/chart_best_practices/templates/#names-of-defined-templates

This set of subchart would be improved by collapsing into one chart.

Agree, we should hold this off till we break out some charts like spire-server etc. To not waste time keeping this in sync probably better to recreate this PR later. Instead of nesting this chart it can then be a top level chart with its own release cycle.

Before we can breakout charts we first need to fix #324 so our CI can support testing multiple charts.

@drewwells what we are aiming for is something like this. https://github.com/sigstore/helm-charts this will allow us to do advanced usecases like nested spire and such more flexible.

This spire-lib chart would be similar to the sigstore-common chart to share some commonly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an attempt to get the library chart done so the pr that breaks out the subcharts to /charts can have as minimal changes as possible. adding all this code into that breakout pr just makes for a lot more possible merge issues. Its not ideal to have under charts/spire/charts, but is a piece to getting it broken out.

Copy link
Contributor

Choose a reason for hiding this comment

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

The breakout can be done chart by chart. Where the first breakout can be these lib functions. Next can be spiffe-csi, because that has very few dependencies, then probably spiffe-oidc.

Copy link
Contributor Author

@kfox1111 kfox1111 Jun 23, 2023

Choose a reason for hiding this comment

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

When I tried it last, it gets confused doing a helm dep up when you have subcharts directly in the charts/spire/charts/ dir. It tries to tar up each of the dependencies and then you get duplicates of the subcharts. I don't think having charts directly under another chart is actually officially supported by helm. So, I think all the charts need to be broken out all at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this is not a standard pattern.

I'm all for supporting sub-charts, but there should be better dividing lines. Claiming that an Agent is a sub-chart doesn't make sense, as it needs to coordinate strongly with a Server, possibly an SPIRE CSI component, and maybe a controller manager.

For those items, putting them into the top-level spire chart makes sense, as these are all core components of SPIRE. For the other items like Tornjak, possible co-database deployment, etc. Those are not (in my mind) core SPIRE components, and using sub-charts makes a lot of sense.

@marcofranssen I understand that you are attempting to clone sigstore structure into this project; but, sigstore is very much a different piece of software, with a different design, and a different set of issues. Splitting up the core SPIRE offerings, each into their own sub-chart, now creates more effort in trying to keep the sub-charts coordinated, where one item somewhere (controller manager, for example) will require another item be present in a different root chart. That robs Helm of it's primary duty, to ensure a installation of the entire application, across all microservices, by putting the burden of a working implementation on the deplorer to correctly pick all the needed sub-charts.

This has already hurt adoption, such that all of the customers I maintain have decided to continue the efforts of maintaining simpler chart setups. They want the one-application / one-chart distinction, and they are defining one application as a whole SPIRE cluster, not an isolated Agent or an isolated Server.

@faisal-memon faisal-memon added this to the 0.13.0 milestone Aug 8, 2023
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
@faisal-memon faisal-memon modified the milestones: 0.13.0, 0.14.0 Sep 6, 2023
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Signed-off-by: Kevin Fox <Kevin.Fox@pnnl.gov>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants