From 3ee8cfda8ae62d8ac573a642049fa999f8d7ba06 Mon Sep 17 00:00:00 2001 From: Wayne Starr Date: Mon, 3 Feb 2025 13:44:51 -0700 Subject: [PATCH 1/4] ZEP-0015: Helm Value Style Variable Interfaces Signed-off-by: Wayne Starr --- .../README.md | 208 ++++++++++++++++++ .../zep.yaml | 25 +++ 2 files changed, 233 insertions(+) create mode 100644 0015-helm-value-style-variable-interfaces/README.md create mode 100644 0015-helm-value-style-variable-interfaces/zep.yaml diff --git a/0015-helm-value-style-variable-interfaces/README.md b/0015-helm-value-style-variable-interfaces/README.md new file mode 100644 index 0000000..b3f1c2a --- /dev/null +++ b/0015-helm-value-style-variable-interfaces/README.md @@ -0,0 +1,208 @@ + + +# ZEP-0015: Helm Value Style Variable Interfaces + + + + + + +- [Summary](#summary) +- [Motivation](#motivation) + - [Goals](#goals) + - [Non-Goals](#non-goals) +- [Proposal](#proposal) + - [User Stories (Optional)](#user-stories-optional) + - [Story 1](#story-1) + - [Story 2](#story-2) + - [Risks and Mitigations](#risks-and-mitigations) +- [Design Details](#design-details) + - [Test Plan](#test-plan) + - [Prerequisite testing updates](#prerequisite-testing-updates) + - [Unit tests](#unit-tests) + - [e2e tests](#e2e-tests) + - [Graduation Criteria](#graduation-criteria) + - [Upgrade / Downgrade Strategy](#upgrade--downgrade-strategy) + - [Version Skew Strategy](#version-skew-strategy) +- [Implementation History](#implementation-history) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) +- [Infrastructure Needed (Optional)](#infrastructure-needed-optional) + + +## Summary + +This ZEP proposes to expand variables to more than just `string` values and instead to accept an `interface{}` instead. This would change `map[string]string` for variables into `map[string]interface{}` in addition to changing how variables are inputted and handled internally. + +## Motivation + +The motivation for this centers around aligning Zarf Variables closer to Helm Values which use the type `map[string]interface{}` over `map[string]string`. Pull Request [#2132](https://github.com/zarf-dev/zarf/pull/2131) pulled this point more into focus by allowing Zarf variables to be directly passed as Helm values, and there has been desire from the Zarf community to treat Zarf Variables more like Helm Values [[1](https://kubernetes.slack.com/archives/C03B6BJAUJ3/p1706175082741539)], [[2](https://kubernetes.slack.com/archives/C03B6BJAUJ3/p1702400472208839)], [[3](https://kubernetes.slack.com/archives/C03B6BJAUJ3/p1706299407255849?thread_ts=1706175134.116329&cid=C03B6BJAUJ3)]. + +### Goals + +- Simplify the use of Zarf Variables for members of the community familiar with Helm + +### Non-Goals + +- Entirely redesign Zarf Variables, Constants and Templates + - (while this is a non-goal for _this_ proposal it is worth discussion if this would better serve the goal above) + +## Proposal + +The proposed solution is to change the internal tracking of Zarf Variables from a `map[string]string` to a `map[string]interface{}` as well as allowing the `map[string]interface{}` to be loaded from a `zarf-config` file (including all of the existing formats that the file accepts). + +Additionally `--set` on the CLI would align to the [Helm `--set` syntax](https://helm.sh/docs/intro/using_helm/#the-format-and-limitations-of---set) with the values on the right-hand side of the `=` being handled as described there instead of as a `string`. + +### User Stories (Optional) + +#### Story 1 + +**As** Jacquline **I want** to be able to set full Helm value objects in a Zarf config variable **so that** I can simplify package configurations and rely on my existing familiarity with Helm. + +**Given** I have a Zarf Package with a Helm value override in a chart +**When** I deploy that package with a `zarf-config.yaml` like the below*: +```yaml +package: + deploy: + set: + MY_OVERRIDE_VARIABLE_NAME: + key: value + bool: true + num: 100 + arr: [] + obj: {} +``` +**Then** Zarf will apply the entire interface to the Helm chart override + +> [!NOTE] +> *This would apply to all `zarf-config` formats not just YAML + +#### Story 2 + +**As** Jacquline **I want** to be able to set full Helm value objects from the CLI **so that** I can simplify package configurations and rely on my existing familiarity with Helm. + +**Given** I have a Zarf Package with a Helm value override in a chart +**When** I deploy that package with a `--set` like the below: +```yaml +zarf package deploy zarf-package-test.tar.zst --set MY_OVERRIDE_VARIABLE_NAME.key=value,MY_OVERRIDE_VARIABLE_NAME.bool=true,MY_OVERRIDE_VARIABLE_NAME.num=100,MY_OVERRIDE_VARIABLE_NAME.arr=[] +``` +**Then** Zarf will apply the entire interface to the Helm chart override + +> [!NOTE] +> *This should follow the [Helm `--set` syntax](https://helm.sh/docs/intro/using_helm/#the-format-and-limitations-of---set) + +### Risks and Mitigations + +This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. + +The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no monger represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. + +## Design Details + +This design proposal keeps changes to a minimum to align Zarf Variables with Helm Values with the largest changes being the change from `string` to `interface` and the changes to loading a `zarf-config` file and handling `--set` as described above. + +### Test Plan + +[X] I/we understand the owners of the involved components may require updates to +existing tests to make this code solid enough prior to committing the changes necessary +to implement this proposal. + +##### Prerequisite testing updates + +NA - This is a modification of existing behavior that should not require prerequisite testing updates. + +##### Unit tests + +Variable interfaces and libraries should be updated to ensure that interfaces are properly handled as opposed to strings. + +##### e2e tests + +Additional E2E tests should be added to ensure that `zarf-config` interfaces and Helm-style `--set`s are passed through appropriately to Helm on chart install / upgrade. + +### Graduation Criteria + +Pending review / community input these changes could be made either with known breakages (pending perceived impact of the --set changes) or as a feature flag for a series of releases eventually becoming the default behavior. + +### Upgrade / Downgrade Strategy + +NA - There would be no upgrade / downgrade of cluster installed components + +### Version Skew Strategy + +NA - This proposal doesn't impact how Zarf's components interact + +## Implementation History + + + +## Drawbacks + +This will introduce a breakage that will need to be mitigated for existing users either expressly as a breaking change or with a feature flag. It also pushes out/builds upon the existing Variables/Constants/Templates paradigm which has been in need of some rethinking for a while. This doesn't preclude doing that work eventually but it does build upon / patch a paradigm that will likely need to be rethought. + +## Alternatives + +We could instead spend more effort redesigning the entire Variables/Constants/Templates paradigm to simplify this for Zarf users as mentioned in the drawbacks above. This would likely look like collapsing Templates/Constants together and changing Variables to Values or something similar to wholly align them with Helm paradigms (since that is what many Zarf users would be familiar with). + +## Infrastructure Needed (Optional) + +NA - This change requires no additional infrastructure as it is internal to Zarf's operation. diff --git a/0015-helm-value-style-variable-interfaces/zep.yaml b/0015-helm-value-style-variable-interfaces/zep.yaml new file mode 100644 index 0000000..cec6074 --- /dev/null +++ b/0015-helm-value-style-variable-interfaces/zep.yaml @@ -0,0 +1,25 @@ +schema-version: 1.0.0 + +title: Helm Value Style Variable Interfaces +zep-number: 0015 +authors: + - "@racer159" +status: implementable +creation-date: 2025-02-03 +reviewers: + - @zarf-dev +approvers: + - @zarf-dev + +# The target maturity stage in the current dev cycle for this ZEP. +stage: alpha + +# The most recent milestone for which work toward delivery of this ZEP has been +# done. This can be the current (upcoming) milestone, if it is being actively +# worked on. +latest-milestone: NA + +# The milestone at which this feature was, or is targeted to be, at each stage. +milestone: + alpha: "TBD" + stable: "v1.0" From 99cc648b8fba03c744622b63b1209ddb408caa3d Mon Sep 17 00:00:00 2001 From: Wayne Starr Date: Mon, 3 Feb 2025 15:35:57 -0700 Subject: [PATCH 2/4] fix yaml Signed-off-by: Wayne Starr --- 0015-helm-value-style-variable-interfaces/zep.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/0015-helm-value-style-variable-interfaces/zep.yaml b/0015-helm-value-style-variable-interfaces/zep.yaml index cec6074..439393f 100644 --- a/0015-helm-value-style-variable-interfaces/zep.yaml +++ b/0015-helm-value-style-variable-interfaces/zep.yaml @@ -7,9 +7,9 @@ authors: status: implementable creation-date: 2025-02-03 reviewers: - - @zarf-dev + - "@zarf-dev" approvers: - - @zarf-dev + - "@zarf-dev" # The target maturity stage in the current dev cycle for this ZEP. stage: alpha From 2e609738030fb787a08918e898f8490a1c184303 Mon Sep 17 00:00:00 2001 From: Wayne Starr Date: Tue, 4 Feb 2025 08:51:37 -0700 Subject: [PATCH 3/4] update ZEP for comments after community meetup Signed-off-by: Wayne Starr --- .../README.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/0015-helm-value-style-variable-interfaces/README.md b/0015-helm-value-style-variable-interfaces/README.md index b3f1c2a..ae6089b 100644 --- a/0015-helm-value-style-variable-interfaces/README.md +++ b/0015-helm-value-style-variable-interfaces/README.md @@ -144,13 +144,20 @@ zarf package deploy zarf-package-test.tar.zst --set MY_OVERRIDE_VARIABLE_NAME.ke ### Risks and Mitigations -This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. +This will require some additional processing of the `zarf-config` files to allow them to be processed properly because of Viper's long-running [case-insensitive keys issue](https://github.com/spf13/viper/issues/1014). This could be performed similar to UDS CLI's implementation of this feature and / or by deprecating some of the existing less-used formats for configuration. (TODO: (@AustinAbro321) - link this to the other ZEP for viper reconsideration) -The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no monger represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. +The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no longer represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. + +As we implement these changes tehre are risks around opening a `string` to an `interface{}` and we should strongly look at adopting many of the [Helm helpers](https://github.com/helm/helm/blob/main/pkg/chartutil/values.go#L71) from their `chartutil` package to ensure that potential security and stability issues are minimized. Also called out below we should implement fuzz testing to catch unanticipated issues and provide an additional layer of assurance to the implementation. ## Design Details -This design proposal keeps changes to a minimum to align Zarf Variables with Helm Values with the largest changes being the change from `string` to `interface` and the changes to loading a `zarf-config` file and handling `--set` as described above. +This design proposal seeks to keep changes to a minimum to align Zarf Variables with Helm Values with the largest changes being the change from `string` to `interface` and the changes to loading a `zarf-config` file and handling `--set` as described above. + +This change will also impact other aspects of Zarf variables as described below: + +- `default` values would now accept an interface - this would affect packages in the same way as zarf-config values for backwards compatibility +- `setVariables` would still set variables to strings with no additional processing - if more is desired here later (i.e. expanding the `type` field) that would be a separate ZEP ### Test Plan @@ -160,7 +167,7 @@ to implement this proposal. ##### Prerequisite testing updates -NA - This is a modification of existing behavior that should not require prerequisite testing updates. +For additional safety when implementing this feature we should look at adding fuzz testing to the existing unit / e2e tests to ensure that panics and other potential security issues are handled appropriately. As mentioned above Helm does have some utility functions to help address some of these concerns but this would ensure that those functions were being used properly within Zarf and any additional concerns from Zarf's additional functionality were handled correctly as well. ##### Unit tests From 8c3907767a83a9e1755b63c22198848dbb560f86 Mon Sep 17 00:00:00 2001 From: Wayne Starr Date: Thu, 13 Feb 2025 20:45:38 -0700 Subject: [PATCH 4/4] Update 0015-helm-value-style-variable-interfaces/README.md Co-authored-by: Brandt Keller <43887158+brandtkeller@users.noreply.github.com> --- 0015-helm-value-style-variable-interfaces/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/0015-helm-value-style-variable-interfaces/README.md b/0015-helm-value-style-variable-interfaces/README.md index ae6089b..a1411fd 100644 --- a/0015-helm-value-style-variable-interfaces/README.md +++ b/0015-helm-value-style-variable-interfaces/README.md @@ -148,7 +148,7 @@ This will require some additional processing of the `zarf-config` files to allow The `--set` syntax will change somewhat how variables are interpreted on the CLI (i.e. `--set VAR=100` will no longer represent `"100"` and instead will just be `100` internally). For `###` templates this can be mitigated by simply representing the value as a string for backwards compatibility though this will likely need to either be marked as breaking for Helm overrides or have additional flag changes / opt ins for this specific feature. -As we implement these changes tehre are risks around opening a `string` to an `interface{}` and we should strongly look at adopting many of the [Helm helpers](https://github.com/helm/helm/blob/main/pkg/chartutil/values.go#L71) from their `chartutil` package to ensure that potential security and stability issues are minimized. Also called out below we should implement fuzz testing to catch unanticipated issues and provide an additional layer of assurance to the implementation. +As we implement these changes there are risks around opening a `string` to an `interface{}` and we should strongly look at adopting many of the [Helm helpers](https://github.com/helm/helm/blob/main/pkg/chartutil/values.go#L71) from their `chartutil` package to ensure that potential security and stability issues are minimized. Also called out below we should implement fuzz testing to catch unanticipated issues and provide an additional layer of assurance to the implementation. ## Design Details