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

Refactor testing to leverage core chart-testing capabilities and enable multiple root level charts #324

Merged
merged 9 commits into from
Jul 19, 2023

Conversation

marcofranssen
Copy link
Contributor

@marcofranssen marcofranssen commented May 30, 2023

This refactor leverages the chart-testing ci values feature (https://github.com/helm/chart-testing/blob/main/pkg/chart/chart.go#L571-L605).

Doing so we can also support multiple root level charts.

Some of the tests should in that case move into the specific root level chart once we start to break out those charts.

This also allows to run the tests locally on a kubecluster of your choice (active current-context). Could be Kind, Rancher, EKS, anything.

make test
make test-examples
make test-example-production
make test-example-tornjak
…

To ignore certain tests in the chart ci folder, you can rename them to anything that doesn't match the pattern *-values.yaml. e.g. oidc-values-ignore.yaml. That eases debugging single tests.

@marcofranssen marcofranssen marked this pull request as draft May 30, 2023 07:55
@marcofranssen marcofranssen added this to the 0.9.0 milestone May 30, 2023
@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch 22 times, most recently from 332dff5 to 00eb816 Compare May 30, 2023 12:03
@kfox1111
Copy link
Contributor

kfox1111 commented Jul 5, 2023

Discussion of something that affects this PR started here:
https://spiffe.slack.com/archives/C04BURZP1KK/p1688575164862469

@kfox1111
Copy link
Contributor

kfox1111 commented Jul 5, 2023

If this change goes forward, I will lose the ability to migrate my customers onto this project. I've mentioned before, they see Helm as a package manager of an Application, and minimally they see the "Server, Agent, Container Manager / Registrar, and SPIFFE CSI Drivers" as part of the core application. They don't mind seeing Tornjak as a sub-chart, and they are considering possibly using sub-charts for their database deployments (which are currently heavily managed by terraform as they are Athena databases).

We should not make the server an Application, and the agent a different application and then require that they both be tied together by a sub-chart style deployment. I would ask that we reconsider moving the server and the agent, minimally, together into the root chart.

This style of application breakup within a chart is not uncommon. For example, gitlab on prem is often used by enterprise customers and is architected with subcharts:
https://gitlab.com/gitlab-org/charts/gitlab/-/tree/master/charts
Same with kube-prometheus-stack:
https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/Chart.yaml#L41-L57

Is the pushback related to a feeling that it shouldn't be done that way, or is there evidence that harm is done when structured that way? Would it help to get feedback from the helm dev team itself, speaking to the legitimacy or illegitimacy of using nested charts for this sort of application?

@faisal-memon
Copy link
Contributor

If this change goes forward, I will lose the ability to migrate my customers onto this project. I've mentioned before, they see Helm as a package manager of an Application, and minimally they see the "Server, Agent, Container Manager / Registrar, and SPIFFE CSI Drivers" as part of the core application. They don't mind seeing Tornjak as a sub-chart, and they are considering possibly using sub-charts for their database deployments (which are currently heavily managed by terraform as they are Athena databases).

We should not make the server an Application, and the agent a different application and then require that they both be tied together by a sub-chart style deployment. I would ask that we reconsider moving the server and the agent, minimally, together into the root chart.

@edwbuck We will still support the aggregated chart, just provide options for people to deploy components separately for advanced deployments. Functionally it will still be the same, implementation details would be different. Anything specific that would prevent adoption by your customers?

@edwbuck
Copy link
Contributor

edwbuck commented Jul 6, 2023

@edwbuck We will still support the aggregated chart, just provide options for people to
deploy components separately for advanced deployments. Functionally it will still be the
same, implementation details would be different. Anything specific that would prevent
adoption by your customers?

Basically they see a cluster as an Application. The only exception to this is their root clusters (they run on nested spire) where the root cluster is also an Application, but the child clusters are again single Applications with an additional (optional) Agent that is part of the root cluster. For their standalone clusters, they use the child cluster with the "upstream" agent turned off.

They rely heavily on simple / easy to interpret templates, and they are not keen on having to dig into one Chart's file to ensure it coordinates with another Chart's file, but they are more inclined to grep across multiple .tpl files in the same Chart.

Part of that is because they also have monitoring and management through other tooling that would over-report each component as a different application, preventing them from getting the single view of the entire cluster. That also impacts their DataDog collection system, as the multiple deployments for a single conceptual cluster would affect their auto-labeling system such that the metrics would be more disjoint.

In short, my requests to at least have the core components of SPIRE be delivered in one Helm Chart (the extras can easily be sub-charted, they'll likely deploy them independently anyway) are not just interesting design ideas, it's what they want.

Currently, they are committed to maintaining their own charts, and their commentary on this project isn't worth repeating. Let's just say they're not fans.

@marcofranssen
Copy link
Contributor Author

@edwbuck We will still support the aggregated chart, just provide options for people to
deploy components separately for advanced deployments. Functionally it will still be the
same, implementation details would be different. Anything specific that would prevent
adoption by your customers?

Basically they see a cluster as an Application. The only exception to this is their root clusters (they run on nested spire) where the root cluster is also an Application, but the child clusters are again single Applications with an additional (optional) Agent that is part of the root cluster. For their standalone clusters, they use the child cluster with the "upstream" agent turned off.

They rely heavily on simple / easy to interpret templates, and they are not keen on having to dig into one Chart's file to ensure it coordinates with another Chart's file, but they are more inclined to grep across multiple .tpl files in the same Chart.

Part of that is because they also have monitoring and management through other tooling that would over-report each component as a different application, preventing them from getting the single view of the entire cluster. That also impacts their DataDog collection system, as the multiple deployments for a single conceptual cluster would affect their auto-labeling system such that the metrics would be more disjoint.

In short, my requests to at least have the core components of SPIRE be delivered in one Helm Chart (the extras can easily be sub-charted, they'll likely deploy them independently anyway) are not just interesting design ideas, it's what they want.

Currently, they are committed to maintaining their own charts, and their commentary on this project isn't worth repeating. Let's just say they're not fans.

This PR is not even breaking out charts at this stage. This chart is all about using the industry standard tooling instead of all our custom scripting. Lets have that discussion of breaking out charts somewhere else. Even if we are not breaking out charts this refactor cleans up already +-140 lines of custom plumbing code.

@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch from 87c877c to a503c4b Compare July 7, 2023 11:44
@marcofranssen
Copy link
Contributor Author

If this change goes forward, I will lose the ability to migrate my customers onto this project. I've mentioned before, they see Helm as a package manager of an Application, and minimally they see the "Server, Agent, Container Manager / Registrar, and SPIFFE CSI Drivers" as part of the core application. They don't mind seeing Tornjak as a sub-chart, and they are considering possibly using sub-charts for their database deployments (which are currently heavily managed by terraform as they are Athena databases).

We should not make the server an Application, and the agent a different application and then require that they both be tied together by a sub-chart style deployment. I would ask that we reconsider moving the server and the agent, minimally, together into the root chart.

So those same customers of yours, do they demand the same from any opensource project. Telling them on how to organize their Go packages, and if such a project to decide to move things into a separate library etc? Maybe we should ask the spire project to get rid of the spiffe module and put that in the same spire repo as well, because that would for me also be easier to find the code as opposed to pulling in a dependency managed in another repo 🤷‍♂️ .

What we are talking about here is internals. The public API for them will still be the same. They still install the spire Helm chart. They still provide the same values. We can't have others decide how we organize things internally for other usecases. So in terms of golang we are talking about things like. Put all in the same package vs having multiple packages vs having external dependencies.

@edwbuck
Copy link
Contributor

edwbuck commented Jul 7, 2023

If this change goes forward, I will lose the ability to migrate my customers onto this project. I've mentioned before, they see Helm as a package manager of an Application, and minimally they see the "Server, Agent, Container Manager / Registrar, and SPIFFE CSI Drivers" as part of the core application. They don't mind seeing Tornjak as a sub-chart, and they are considering possibly using sub-charts for their database deployments (which are currently heavily managed by terraform as they are Athena databases).
We should not make the server an Application, and the agent a different application and then require that they both be tied together by a sub-chart style deployment. I would ask that we reconsider moving the server and the agent, minimally, together into the root chart.

So those same customers of yours, do they demand the same from any opensource project. Telling them on how to organize their Go packages, and if such a project to decide to move things into a separate library etc? Maybe we should ask the spire project to get rid of the spiffe module and put that in the same spire repo as well, because that would for me also be easier to find the code as opposed to pulling in a dependency managed in another repo 🤷‍♂️ .

What we are talking about here is internals. The public API for them will still be the same. They still install the spire Helm chart. They still provide the same values. We can't have others decide how we organize things internally for other usecases. So in terms of golang we are talking about things like. Put all in the same package vs having multiple packages vs having external dependencies.

I will reach back to the customer with this messaging. Thank you. While the two charts appear very different, which is stalling them from adopting, if the differences are truly internal, this will help with future migration efforts.

@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch from f2d90d4 to 64e05b5 Compare July 13, 2023 09:41
@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch 3 times, most recently from 0413afb to a7dd86c Compare July 19, 2023 17:54
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

I still have concerns its taking so long to do runs, but since we think we can get it reduced in follow on patches, I'll go along with that.

The rest looks ok.

@kfox1111
Copy link
Contributor

Fixed the merge conflict

This also enables the refactor to have multiple root level charts.

Resolves #100

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
This allows more granular tasks and composition.

Also improved the documentation.

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch from b0183d9 to 4dc094e Compare July 19, 2023 20:49
helm/chart-testing#556
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Unfortunatily it fails on linting

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
@marcofranssen marcofranssen force-pushed the enable-testing-multiple-charts branch from 4dc094e to e426bc0 Compare July 19, 2023 20:54
@marcofranssen marcofranssen merged commit 94a2b72 into main Jul 19, 2023
@marcofranssen marcofranssen deleted the enable-testing-multiple-charts branch July 19, 2023 21:06
marcofranssen added a commit that referenced this pull request Jul 19, 2023
* 94a2b72 Merge pull request #324 from spiffe/enable-testing-multiple-charts
* e426bc0 Downgrade chart-testing tool to 3.8.0
* 09b4664 Utilize ct install --github-groups in ci workflow
* 42086bd Run example tests also on all k8s versions
* 4848c48 Improve Makefile help and implementation
* db06038 Increase some timeouts, trying to fix the tests
* 54ed71f Add back tests for examples
* 4b4cef1 Skip namespace-override test because of #330
* 380979c Prevent ci folder ending up in Helm package
* 06ddb7c Use chart-testing ci/*-values.yaml for testing
* a4c1de7 Add basic unit test framework (#390)
* 088b296 Improve tornjak service API to have object structure (#392)
* 522066e Align tornjak clientCA naming convention (#393)
* f05cb4f Add option to configure TLS/mTLS endpoint for Tornjak (#338)
* f461a01 Bump test chart dependencies (#386)
* ce39e82 Bump test chart dependencies (#382)
* 19d3208 Fix oidc provider config change not rolling out (#383)
* 0197621 Bump helm/kind-action from 1.7.0 to 1.8.0 (#384)
* 3ed1859 Add missing tolerations config to daemonsets (#381)
* 4ad68b1 Add namespace to spiffe-oidc-discovery-provider RBAC definitions (#379)
* c1b1dd3 Add additional domains to JWT issued items. (#230)
* 3405e13 Bump test chart dependencies
* 81452d5 Fix missing spiffe-csi-driver imagePullSecrets template (#376)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
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.

4 participants