cli: save Helm charts to disk before running upgrades

[daniel-weisse]

Context

constellation upgrade apply upgrades and/or installs chart to the cluster.
What charts are used for this, and how the values look can be rather hard to determine for a user, making potential troubleshooting difficult.
Additionally, the values used to apply these upgrades are not shown or saved anywhere.

Proposed change(s)

• Save the charts, values and override values used during constellation upgrade apply to disk before running said operation
  • Adds a new SaveCharts method to the Helm interface
  • Charts are saved to constellation-upgrade/upgrade-<timestamp>-<uid>
  • Each action defines a SaveChart method that is called by the interface
    • install actions save their charts to <dir>/helm-install
    • upgrade actions save their charts to <dir>/helm-upgrade
  • override values (values generated by our CLI and are merged with the charts values) are saved as overrides.yaml in the chart directory

Additional info

• I copied and adjusted the SaveDir function from helm's chartutil package because the original function saved dependencies as tar files instead of writing chart files directly.
• AB#3409

Checklist

• Update docs:
  • See Azure boards ticket
• Add labels (e.g., for changelog category)
• Is PR title adequate for changelog?
• Link to Milestone

[github-actions]

Coverage report

Package	Old	New	Trend
cli/internal/cmd	54.20%	54.20%	↔️
cli/internal/helm	47.70%	43.80%	↘️

[elchead]

This could be a good chance to add a unit test with some default setups for the CSPs and to check that the chart is rendered as expected.
-> covers the feature
-> covers the helm rendering

Happy to help out

[elchead]

Can we document this behavior in https://docs.edgeless.systems/constellation/workflows/upgrade?

[elchead]

I think this feature should be released with #2310 to be able to actually skip the apply. Can we document this at the same time? (The user can review the helm chart files and skip the helm chart application through --skip-phases=helm)

[elchead]

lgtm besides open points

[daniel-weisse]

This could be a good chance to add a unit test with some default setups for the CSPs and to check that the chart is rendered as expected.

I'm not sure how well this would fit a unit test.
The helm code interacts with the file system directly using os.Create etc., which makes unit tests suboptimal.
Additionally, I am not really sure what this test should cover, as saving the chart to disk is mostly handled by an external dependency (even more so if we go with just saving charts as tar files).

-> covers the helm rendering

Saving the charts to disk does not actually perform any rendering of the charts. We do not template the charts, we simply save the chart we previously loaded from the embedded file system.
The only exceptions are updating the version numbers in Chart.yaml for charts we packaged, and generating an additional overrides.yaml with the values we loaded dynamically depending on the users config file.
At this point we could maybe check the values we write to disk as overrides.yaml, but again, this would simply check the output of a fileHandler.WriteYaml call on values we have previously loaded in other functions.

Can we document this behavior

Please view the linked ticket in the PR description

Can we document this at the same time? (The user can review the helm chart files and skip the helm chart application through --skip-phases=helm)

This is not what this feature implementation was made for. It is intended to allow for better troubleshooting in case of failing to upgrade helm charts during constellation upgrade apply.
An additional "review the proposed changes step" is something we have already talked about at some occasions, and will likely be implemented in the future.

[malt3]

LGTM and testing was successful.

[elchead]

Can we document this at the same time? (The user can review the helm chart files and skip the helm chart application through --skip-phases=helm)

This is not what this feature implementation was made for. It is intended to allow for better troubleshooting in case of failing to upgrade helm charts during constellation upgrade apply. An additional "review the proposed changes step" is something we have already talked about at some occasions, and will likely be implemented in the future.

Would it be valuable to add this in AB#3413 though since we now support this review?

[elchead]

This could be a good chance to add a unit test with some default setups for the CSPs and to check that the chart is rendered as expected.

I'm not sure how well this would fit a unit test. The helm code interacts with the file system directly using os.Create etc., which makes unit tests suboptimal. Additionally, I am not really sure what this test should cover, as saving the chart to disk is mostly handled by an external dependency (even more so if we go with just saving charts as tar files).

-> covers the helm rendering

Saving the charts to disk does not actually perform any rendering of the charts. We do not template the charts, we simply save the chart we previously loaded from the embedded file system. The only exceptions are updating the version numbers in Chart.yaml for charts we packaged, and generating an additional overrides.yaml with the values we loaded dynamically depending on the users config file. At this point we could maybe check the values we write to disk as overrides.yaml, but again, this would simply check the output of a fileHandler.WriteYaml call on values we have previously loaded in other functions.

Fair point, only real rendering (through dry-run) could probably provide any value and also OS makes the testing less ideal.
Let's defer it