spiffe / helm-charts-hardened

Apache License 2.0
12 stars 26 forks source link

Move common external bits to spire-lib chart #330

Closed kfox1111 closed 2 months ago

kfox1111 commented 2 months ago

Please consider restarting this effort, by creating a charts/spire/charts/server-environment chart, and moving the bundle-configmap.yaml, controller-manager-cluster-ids.yaml, the controller-manager-ftd.yaml, the controller-manager-roles.yaml and the controller-manager-static-entries.yaml into the the charts/spire/charts/server-environment chart, fixing the value references (if they even need fixed) and then making the spire-server chart depend on the charts/spire/charts/server-environment chart, so it gets all of the "server environment config" as well as defining the server instance yamls.

A chart can't move objects to a subchart, add it back as a dependency, and keep the same values api. I can't think of any way to structure things the way you'd like and still keep the chart fairly usable. Helm has a lot things that restrict how you can design charts, making for some difficult/unpleasant tradeoffs. :/

This pr though allows a second, external-spire-server chart to be created that calls the same macro's that limits support only pointing at an external spire-server with the appropriate objects still created, without the server deployment code as per your request. The code can at least be shared between the two charts that way, and values will look right.

edwbuck commented 2 months ago

Please consider restarting this effort, by creating a charts/spire/charts/server-environment chart, and moving the bundle-configmap.yaml, controller-manager-cluster-ids.yaml, the controller-manager-ftd.yaml, the controller-manager-roles.yaml and the controller-manager-static-entries.yaml into the the charts/spire/charts/server-environment chart, fixing the value references (if they even need fixed) and then making the spire-server chart depend on the charts/spire/charts/server-environment chart, so it gets all of the "server environment config" as well as defining the server instance yamls.

A chart can't move objects to a subchart, add it back as a dependency, and keep the same values api. I can't think of any way to structure things the way you'd like and still keep the chart fairly usable. Helm has a lot things that restrict how you can design charts, making for some difficult/unpleasant tradeoffs. :/

This pr though allows a second, external-spire-server chart to be created that calls the same macro's that limits support only pointing at an external spire-server with the appropriate objects still created, without the server deployment code as per your request. The code can at least be shared between the two charts that way, and values will look right.

This makes me wonder if we are actually doing stuff in the right order. Maybe we should prioritize the promotion of the charts to top-level charts first, and then make the changes without the need to redo them. (If we don't have the time to do it the right way once, we don't have the time to do it the wrong way once and then fix it later).

If you are up to it, would you get behind this idea of breaking up the charts into multiple top-level charts in the manner you have suggested multiple times? I am beginning to believe that the longer we put that task you desire off, the worse the refactoring will eventually become to structure the charts the way you want them.

kfox1111 commented 2 months ago

This change and a change breaking out the nested charts to the root level are orthogonal. The only ordering drawback I can think of is if we do the breakout first would create some slight merge conflicts in this pr due to renamed files that would have to be resolved. So its a bit less work to merge this first. Actually, getting the existing pr's merged and then break out the charts would be good for the same reason.

edwbuck commented 2 months ago

This change and a change breaking out the nested charts to the root level are orthogonal. The only ordering drawback I can think of is if we do the breakout first would create some slight merge conflicts in this pr due to renamed files that would have to be resolved. So its a bit less work to merge this first. Actually, getting the existing pr's merged and then break out the charts would be good for the same reason.

I really can't see how this is less work, as I can't see how:

is more work than

If the merge conflicts are a real concern, and work must be restructured to avoid merge conflicts, please use the standard approaches that git recommends for reducing merge conflicts.

kfox1111 commented 2 months ago

I think we have enough feedback to inform that this pr is not the right solution so I'm going to close this in favor of #303. Lets get that merged so we can make some progress. There is still time before 1.0. After spire nested support is merged, we can reevaluate. If that means #303 gets reverted later for something better, great.

@edwbuck Please make a pr with your preferred solution and I promise you it will get a fair review. I don't have the time/energy to do it myself. I've already done 2 implementations, spending a fair number of hours exploring implementations.