carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.68k stars 137 forks source link

[data/values] data values are parsed as strings by default #179

Closed NoiSek closed 2 years ago

NoiSek commented 4 years ago

Summary: Passing integer values in through the use of --data-values-env will cause errors due to ytt rendering them later on as strings.

Tangentially related to #103 and the accompanying typing proposal.

Expected Behavior: The parser should preserve the integer state of parsed environment variables and / or the renderer should support the ability to cast a given value to an integer at runtime.

(i.e. #@ int:data.values.foobar or similar)

Actual Behavior: Values set through environment variables such as CFG_http_port=8080 will be resolved as "8080" when used, which causes errors later on against tools that validate the types of fields that use those values.

Steps to reproduce:

foo/values.yaml:

#@data/values
---
test:

foo/bar.yaml:

#@ load("@ytt:data", "data")
casted-value: #@ data.values.test

foo/nginx.yaml:

#@ load("@ytt:data", "data")
apiVersion: apps/v1
kind: Deployment
metadata:
  name: my-nginx
spec:
  selector:
    matchLabels:
      run: my-nginx
  replicas: 2
  template:
    metadata:
      labels:
        run: my-nginx
    spec:
      containers:
      - name: my-nginx
        image: nginx
        ports:
        - containerPort: #@ data.values.test

Building an actual container through Kapp will give you an error similar to:

Invalid value: "string": spec.datacenter.racks.members in body must be of type integer: "string" (reason: Invalid)

Or:

Service in version "v1" cannot be handled as a Service: v1.Service.Spec: v1.ServiceSpec.Ports: []v1.ServicePort: v1.ServicePort.Port: readUint32: unexpected character: �, error found in #10 byte of ...|","port":"8080","tar|..., bigger context ...|IP":"10.105.7.57","ports":[{"name":"http","port":"8080","targetPort":"8080"}],"selector":{"app":"dja|... (reason: BadRequest)
ewrenn8 commented 4 years ago

Hey @NoiSek,

Thanks for the detailed report! By default, ytt treats variables provided via the command line and environment as strings, but it does expose the --data-value-yaml and --data-values-env-yaml flags to interpret the values as yaml. There are some docs about this here

Eli

NoiSek commented 4 years ago

Ah, so it's intentional then. But what is the value in stringifying variables by default in an environment where types matter?

I can't think of a situation where that would actually be expected behavior, especially given bash itself makes a distinction between types.

aaronshurley commented 4 years ago

Hi @NoiSek,

Thank you for the feedback!

But what is the value in stringifying variables by default in an environment where types matter?

Consistency comes to mind for me. And I don't think that bash environment variables are typed (source) so bash variables don't seem to be as expansive as yaml types (doubles come to mind). Does that help?

NoiSek commented 4 years ago

I'm referring more to the idea that despite representing those values as strings, Bash still makes an effort to interpret values as integers where appropriate rather than treating them as straight string to string conversions.

i.e. adding integers:

$ export counter="1"
$ echo $((counter + 1))
> 2

And again with two disparate types, where Bash still prefers to process them as integers rather than concatenate them:

$ export counter="foo"
$ echo $((counter + 1))
> 1

Speaking to your point with consistency, it seems to me that these sorts of mishaps are handled transparently more often than not and that doing otherwise is actually the inconsistent behavior.

Parsing as YAML comes with more baggage than just converting integer values to actual integers as I understand it ("true" strings come to mind), but it's sort of a lose-lose situation to run into this behavior and then have to convert anyway. What purpose does this idea of consistency serve if it generally leads to you deviating from your original schema regardless?

Would it not make more sense to make explicit YAML conversion the default and attach a disclaimer?

cppforlife commented 4 years ago

@NoiSek

Tangentially related to #103 and the accompanying typing proposal.

i think you've found what our direction here is. once we implement schema feature, it should give us type information for data values which would in turn allows us to be more accurate about interpreting data values.

as eli mentioned we do support allowing users to switch to yaml parsing via --data-value-yaml and --data-value-env-yaml flags, so it's really a decision point for user running ytt.

we did long time ago parse values as yaml by default, but switched away to strings by default because it was also surprising why things were parsed by default (e.g. export MY_TOKEN=23473257294259054045 should be a string not an integer).

cari-lynn commented 2 years ago

The feature mentioned above has been implemented and should alleviate some concerns about flags that parse data values as strings.

When using a data values schema, if a data value flag like --data-values-env parses a value as a string, but the schema expects an int, then feedback on the error will be provided right away and the value will not end up incorrectly as a string in the output.

If not using schema, then using a flag that parses the value as yaml is recommended.

Feel free to reopen this issue if there is more to talk about with regard to this.