-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow helm values overrides to inject cluster labels via go templates #507
Conversation
Signed-off-by: Greg Sidelinger <gate@ilive4code.net>
Signed-off-by: Greg Sidelinger <gate@ilive4code.net>
Signed-off-by: Greg Sidelinger <gate@ilive4code.net>
Signed-off-by: Greg Sidelinger <gate@ilive4code.net>
This would be really nice for when you want to expose a service on all clusters. Currently, you can use variables like global.fleet.clusterLabels.management.cattle.io/cluster-display-name in your values but you can't append anything to the variable. |
@@ -72,6 +72,10 @@ helm: | |||
# All labels on Rancher clusters are available using global.fleet.clusterLabels.LABELNAME | |||
# These can now be accessed directly as variables | |||
variableName: global.fleet.clusterLabels.LABELNAME | |||
# They can also be access using go template logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small grammatical fixes:
In addition, they can be accessed using Go template logic
pkg/target/target_test.go
Outdated
templateName, ok := bundle.Helm.Values.Data["templateName"] | ||
if !ok { | ||
t.Fatal("key templateName not found") | ||
} | ||
|
||
if templateName != "kubernetes.io/cluster/local" { | ||
t.Fatal("unable to assert correct template") | ||
} | ||
|
||
templateMissing, ok := bundle.Helm.Values.Data["templateMissing"] | ||
if !ok { | ||
t.Fatal("key templateMissing not found") | ||
} | ||
|
||
if templateMissing != "{{ .missingValue }}" { | ||
t.Fatal("unable to assert correct templateMising") | ||
} | ||
|
||
templateLogic, ok := bundle.Helm.Values.Data["templateLogic"] | ||
if !ok { | ||
t.Fatal("key templateLogic not found") | ||
} | ||
|
||
if templateLogic != "Non Prod" { | ||
t.Fatal("unable to assert correct templateLogic") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's remove blank lines between the same template comparisons
pkg/target/target.go
Outdated
@@ -520,6 +529,18 @@ func processLabelValues(valuesMap map[string]interface{}, clusterLabels map[stri | |||
} else { | |||
return fmt.Errorf("invalid_label_reference %s in key %s", valStr, key) | |||
} | |||
} else { | |||
switch val.(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already gather valStr
on line 523. Perhaps, we can refactor this code to use it instead. I'd prefer that we handle this scenario once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have an updated PR I was going to send in that changes how a lot of this stuff is done and does not have any extra string conversions in that area of the code.
After playing around with the configmap stuff in 0.3.6 I thought it would be fun to allow the value substitution to come from any overridden values like the ones imported from a configmap, not just the ones in helm.vlaues from the fleet.yaml. So I moved this code into the helm install function to do that. I also added basic cluster annotation support on top of the cluster one to deal with values that can not be stored in an label. It's a bigger PR but adds a lot more flexibility that has been preventing us from using fleet for more deployments.
#528 supersedes this PR |
@gregsidelinger why are these PRs separate? I'd personally prefer one PR with discrete commits (or squashed, if small enough) in order to reduce context switching. However, it's probable that there's a perspective here that I am missing. |
@nickgerace Because I'm not 100% sure if there are any major design issues with where/how I did things in #528. It was a much bigger change than this small PR which has more lines of testing than actual code. I would say look at #528 and if nothing major jumps out with the approach I will close this one and focus on it. |
Cleanup extra string convirsion Cleanup extra whitespace in tests Signed-off-by: Greg Sidelinger <gate@ilive4code.net>
Thanks @gregsidelinger for the clarification. I'll pause my review here and jump to the other one. |
So after testing this branch and the one in #528 I think this is the PR that should be merged. I updated this PR with annotation support and added a few more test cases. As well as trapping errors from calling the go-templating that causes panics. Docs could get some for TLC as accessing labels and annotations with dashes in their name requires using index which I added a test case for. But I'm not sure if all of this should end up in the github-structure docs as that is starting to look full and out of place if there are too many examples of using go tempaltes. The only real difference between this one and #528 is this supports only fleet.yaml values and valuesfiles. Configmaps and secrets in the destination cluster are not processed, which may have been overkill. |
Hi @gregsidelinger thanks for this PR, its exactly what i am looking for. I noticed there is a comment on your PR that need to be resolved before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR @gregsidelinger! Apologies for the late re-review, had some comments on this approach though.
variableName: "kubernetes.io/cluster/{{ .global.fleet.clusterLabels.LABELNAME }}" | ||
variableName: "{{ if eq .global.fleet.clusterLabels.LABELNAME \"production\" }}Production Workload{{ else }}Non Prod{{ end }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the values provided here are intended to be Go templates that are parsed by the Helm chart? For example, values like this one found in kube-prometheus-stack that is expected to be a Go template supplied to the chart, as seen by the tpl
call made here
Based on this approach, we would no longer be able to support Go template encoded strings provided via the values.yaml to be interpreted by Helm instead of Fleet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an excellent point. I would think we could make this behavior opt in. So something like add parseValuesValues: true (that's a bad name fyi). Then if somebody wants to templatize something that has templates in it they would escape the template characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is true. An early version of the code actually would leave any lines alone that returned an error from the go templating call allowing fleet to still handle values files with go templating strings in them. This however would not return errors to the user if they did something like this variableName: "kubernetes.io/cluster/{{ .global.fleet.clusterLabels.MISSING_LABEL }}"
so I changed it as I forgot why I did it in the first place till you mentioned it.
Assuming I leave all clusterLabels prefixed with .global.fleet. a simple approach would be to only call to go templating if the string contained {{
, }}
and .global.fleet.
which should in theory be safe. for most use cases. Or only support the templating on stuff in fleet.yaml and not values files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of helm.preProcessValues
as the setting name to turn the templating on. I actually submitted a patch to @rajiteh for his PR #671 instead of more updates to my code. Granted I do have all the changes requested for this PR sitting locally but I think his approach is better you are should all review his PR.
func processValues(valuesMap map[string]interface{}, clusterLabels map[string]string, clusterAnnotations map[string]string) (err error) { | ||
labelPrefix := "global.fleet.clusterLabels." | ||
annotationPrefix := "global.fleet.clusterAnnotations." | ||
scopedClusterLables := map[string]interface{}{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scopedClusterLables := map[string]interface{}{ | |
scopedClusterLabels := map[string]interface{}{ |
nit: incorrect spelling
return fmt.Errorf("invalid_label_reference %s in key %s", valStr, key) | ||
} | ||
} else if strings.HasPrefix(valStr, annotationPrefix) { | ||
annoation := strings.TrimPrefix(valStr, annotationPrefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annoation := strings.TrimPrefix(valStr, annotationPrefix) | |
annotation := strings.TrimPrefix(valStr, annotationPrefix) |
nit: typo
} | ||
} else if strings.HasPrefix(valStr, annotationPrefix) { | ||
annoation := strings.TrimPrefix(valStr, annotationPrefix) | ||
annoationVal, annoationPresent := clusterLabels[annoation] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annoationVal, annoationPresent := clusterLabels[annoation] | |
annotationVal, annotationPresent := clusterLabels[annotation] |
nit: typo
defer func() { | ||
if r := recover(); r != nil { | ||
err = fmt.Errorf("Failed to process template substitution for key '%s' with value '%s': [%v]", key, valStr, err) | ||
} | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this? Is there a part of our code that panics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, a bad go template can actually raise a panic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the panic introduced with this change or is it already in master and you just added the logging ?
bundle.Helm.Values.Data["error"] = "{{ .missingValue }}" | ||
err = processValues(bundle.Helm.Values.Data, clusterLabels, clusterAnnotations) | ||
if err == nil { | ||
t.Fatalf("failed to error on {{ .missingValue }}") | ||
} | ||
|
||
bundle.Helm.Values.Data["error"] = "{{ .missingValue-panic }}" | ||
err = processValues(bundle.Helm.Values.Data, clusterLabels, clusterAnnotations) | ||
if err == nil { | ||
t.Fatalf("failed to error on {{ .missingValue-panic }}") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are these different tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is designed to raise a panic from the go templating. I ran into a panic at one point in time so I wrote a test for it. If I recall correctly the -
in the variable name actually causes the issue.
@xMAC94x @ibuildthecloud @aiyengar2
So from a long term design perspective which approach is preferred? |
Agree, a special syntax is prob a better solution than put everything in labels, this would also get around of some restrictions as label values cannot contain all UTF-8 characters, right ? |
@gregsidelinger Thank you for this PR and also the follow up design questions. I took a look at this PR and the conversations before where the problem of "how do we support templating for the charts that already have Go templates to be parsed by Helm engine". I see that you tried out the setting "helm.preProcessValues" method to turn on the Fleet templating when required. So I think when the setting "helm.preProcessValues" is "true" it would indicate that Fleet needs to parse the templated values before passing down to Helm. Is that correct? How would this make Fleet ignore processing of the templated charts intended to be processed by Helm engine? Are for such charts the setting should always be off? I will go over the second PR #671 to understand the second approach and raise any questions I face. |
@prachidamle author of PR #671 here. I have adopted the same config except inversed the config key. Essentially the config value and its default is However, a user could still utilize fleet-level helm templates and retain downstream templating ability by wrapping it in an escape block |
@rajiteh so essentially when the gotpls are wrapped in an escape block {{ |
Yes. The template string |
@gregsidelinger @rajiteh - are we good to close this PR in favor of #671 and pursue that avenue? |
I'm fine with focusing on #671. I did submit a PR to @rajiteh over the weekend to address the things brought up so far in the review so hopefully he is ok to merge those changes in. |
Thanks @gregsidelinger. I'm going to close this ticket then so we focus on #671. |
Signed-off-by: Greg Sidelinger gate@ilive4code.net