Skip to content
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

Template Stanza single quote and HCL2 cause parser error #9838

Open
idrennanvmware opened this issue Jan 17, 2021 · 11 comments
Open

Template Stanza single quote and HCL2 cause parser error #9838

idrennanvmware opened this issue Jan 17, 2021 · 11 comments
Labels
stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/hcl type/bug

Comments

@idrennanvmware
Copy link
Contributor

idrennanvmware commented Jan 17, 2021

Nomad 1.0.2

Issue

We make pretty extensive usage of file content rendering in our template stanza's but we have noticed that if the file contents have '' in them then the HCL renderer gives us this kind of message

Invalid character; Single quotes are not valid. Use double quotes (") to enclose strings.

The problem is that if we then replace the '' in the file with "" we then get a different error because we also have } and that error is

Extra characters after interpolation expression; Expected a closing brace to end the interpolation expression, but found extra characters

Here's a snippet from the job file

template {
        data = <<EOH
        [[ fileContents ( print .service_dir "/journalbeat.yml" ) ]]
        EOH

        destination = "local/journalbeat.yml"
      }

and the respective file it's trying loading in (redacted)

############################# Journalbeat #####################################

---
journalbeat.inputs:
  - paths: []
    include_matches:
      - "systemd.unit=vault.service"
    seek: cursor
    fields_under_root: true
    fields:
      type: control-plane-vault
      token: ${LOGZIO_TOKEN:''}
      logzio_codec: plain
      ipAddress: ${IP_ADDRESS:''}
      hostName: ${HOSTNAME:''}
      clusterName: ${CLUSTER_NAME:''}
      deploymentType: ${DEPLOYMENT_TYPE:''}

and with the single quote replacement

############################# Journalbeat #####################################

---
journalbeat.inputs:
  - paths: []
    include_matches:
      - "systemd.unit=vault.service"
    seek: cursor
    fields_under_root: true
    fields:
      type: control-plane-vault
      token: ${LOGZIO_TOKEN:""}
      logzio_codec: plain
      ipAddress: ${IP_ADDRESS:""}
      hostName: ${HOSTNAME:""}
      clusterName: ${CLUSTER_NAME:""}
      deploymentType: ${DEPLOYMENT_TYPE:""}

Neither of which are ok in the HCL2 parser. However, if we run this with the -hcl1 flag for deployment then it all works as expected

@tgross
Copy link
Member

tgross commented Jan 19, 2021

Thanks for reporting this @idrennanvmware!

@notnoop
Copy link
Contributor

notnoop commented Jan 19, 2021

Hi @idrennanvmware ! Thanks for raising this issue, and we'll look into how to address this behavior change.

I believe the issue to be hcl2 attempting variable expansion, so the workaround would be to to escape ${...}, e.g. ${LOGZIO_TOKEN:''} to be $${LOGZIO_TOKEN:''}. In HCL2, HCL2 parser will attempt to interpolate${LOGZIO_TOKEN:''} as variable reference but it's deemed invalid due to the quotation marks. Escaping $ will ensure that the template is read without any interpolation.

We'll document this gotcha in the upgrade guide for now. Also, we'll consider having a way to disable variable interpolation in a heredoc, potentially by quoting the delimiter like in bash (e.g. using <<'EOH' in your sample to disable HCL2 variable interpolation).

@idrennanvmware
Copy link
Contributor Author

idrennanvmware commented Jan 19, 2021

Hi @notnoop - thanks for the fast reply (and work around).

For now we can definitely add that work around but it raises a bigger problem for us in that our config files are now "hcl aware" and that may not always be the case. In fact, often it's not at all. I can see users wanting both types of options (inject the variables for HCL2 etc) but there are also definitely cases we would want the file to be "as is"

Interesting problem, thanks for taking the time to hear us out :)

For now we'll add the $${LOGZIO_TOKEN:''} approach but if we can keep this issue open as something to consider for functionality in the future?

Also, we'll consider having a way to disable variable interpolation in a heredoc, potentially by quoting the delimiter like in bash (e.g. using <<'EOH' in your sample to disable HCL2 variable interpolation).

I like it :)

@idrennanvmware
Copy link
Contributor Author

So I just checked - it would be a massive change impacting multiple project scripts for us to do the $$ approach. we will have to stick to the HCL1 parser for now :(

@tgross tgross added stage/accepted Confirmed, and intend to work on. No timeline committment though. and removed stage/needs-investigation labels Feb 3, 2021
@tgross tgross added this to Needs Triage in Nomad - Community Issues Triage via automation Feb 12, 2021
@tgross tgross moved this from Needs Triage to Needs Roadmapping in Nomad - Community Issues Triage Feb 12, 2021
@tgross tgross removed this from Needs Roadmapping in Nomad - Community Issues Triage Mar 4, 2021
@istarkov
Copy link

Disabling variable interpolation is really needed as of now behaviour is tricky.

i.e.

      template {
        data        = <<-EOH
          ${var.pause_mem} interpolated to var value

          ${NOMAD_UPSTREAM_ADDR_hasura} not interpolated and same as $${NOMAD_UPSTREAM_ADDR_hasura}
          {{ env "NOMAD_UPSTREAM_ADDR_hasura" }} interpolated

          ${1} interpolated to 1

          ${meta.connect.proxy_concurrency} not interpolated
          {{ env "meta.connect.proxy_concurrency" }} interpolated
        EOH
        destination = "/local/hasura/example.tpl"
      }

Result:

200 interpolated to var value

${NOMAD_UPSTREAM_ADDR_hasura} not interpolated and same as ${NOMAD_UPSTREAM_ADDR_hasura}
127.0.0.1:8080 interpolated

1 interpolated to 1

${meta.connect.proxy_concurrency} not interpolated
1 interpolated

Like in bash i.e. <<'EOH' would be the best.

@idrennanvmware
Copy link
Contributor Author

idrennanvmware commented May 21, 2021

Agreed @istarkov - we really are between a rock and a hard place and can't begin to practically migrate with out this ability We're stuck on HC1 for the foreseeable future or until we can escape 'EOH'

notnoop pushed a commit that referenced this issue Jul 26, 2021
Support the new post-1.0.0 job spec fields in the HCLv1 parser.

The HCLv1 parser is still the default (or only!) parser in many downstream tools, e.g. [Levant](https://github.com/hashicorp/levant/blob/e48c439f14def83b0cbe24c2553e90e959474f16/template/render.go#L13-L30), and [terraform-provider-nomad](https://github.com/hashicorp/terraform-provider-nomad/blob/bce32a783176c7943ac662a3eee48b76d92c4d6a/nomad/resource_job.go#L735-L743) .

While we initially intended to deprecate HCLv1 parser in 1.0.0, we never communicated that publicly. We did not fully anticipate the public usage of `jobspec` package (we assumed it's a private package), or the friction that HCLv2 introduced in some cases (e.g. #10777, #9838).

So moving forward we intend to ensure that new job spec fields are honored in both the HCLv1 and HCLv2 parser until we solidify the migration path and communicate it properly.
@tgross
Copy link
Member

tgross commented Jul 29, 2024

Just was taking a look at this briefly because of @42wim's comment here #20195 (comment) and decided to do some archaeology on the HCL repo.

It looks like the <<'EOT' heredoc syntax has been previously proposed for HCL and rejected (ref hashicorp/hcl#207). Nomad can support the workaround there via the file HCL2 function or using a remote source, but neither feel like particularly satisfactory solutions. That being said, Nomad currently uses a fork of HCL2 anyways so even if we had to implement it specially for Nomad, that wouldn't be impossible. I was sort of hoping I'd find a rejected PR doing just that, but alas we'll have to code it up ourselves 😁

@tgross
Copy link
Member

tgross commented Jul 29, 2024

Another thought I had was that we parse the HCL (ref parse.go#L94) before we interpolate it. So in theory we could identify the literal-heredoc syntax in Nomad and swap it out for a blank string and then swap it back in when we're done interpolating. But because parsing happens first, unfortunately that means that any "syntax errors" in the literal-heredoc (i.e. syntax that's fine in whatever the template data is supposed to be, but not for HCL) would get treated as HCL2 diagnostics and result in errors.

@tgross
Copy link
Member

tgross commented Aug 16, 2024

I did a little more poking at this and we can't even parse the AST first and then manipulate it before doing evaluation, because the heredoc sigil isn't included in the AST. Ex. a heredoc like this:

data = <<EOH
{{ env SOME_ENV }}
EOH

has an AST node shaped like this (via go-spew):

  Attributes: (hclsyntax.Attributes) (len=1) {
   (string) (len=4) "data": (*hclsyntax.Attribute)(0xc0000f8380)({
    Name: (string) (len=4) "data",
    Expr: (*hclsyntax.TemplateExpr)(0xc000024660)({
     Parts: ([]hclsyntax.Expression) (len=1 cap=1) {
      (*hclsyntax.LiteralValueExpr)(0xc000024600)({
       Val: (cty.Value) {
        ty: (cty.Type) {
         typeImpl: (cty.primitiveType) {
          typeImplSigil: (cty.typeImplSigil) {
          },
          Kind: (cty.primitiveTypeKind) 83
         }
        },
        v: (string) (len=27) "\t{{ env DOCKER_ENV }}: foo\n"
       },
       SrcRange: (hcl.Range) foo.hcl:3,1-4,1
      })
     },
     SrcRange: (hcl.Range) foo.hcl:2,11-4,5
    }),
    SrcRange: (hcl.Range) foo.hcl:2,4-4,5,
    NameRange: (hcl.Range) foo.hcl:2,4-8,
    EqualsRange: (hcl.Range) foo.hcl:2,9-10
   })
  },

@tgross
Copy link
Member

tgross commented Aug 16, 2024

Oh, interesting... if I parse just the heredoc with the 'EOT' syntax, I actually get an AST node like this (a literal and not a template value):

  Attributes: (hclsyntax.Attributes) (len=1) {
   (string) (len=4) "data": (*hclsyntax.Attribute)(0xc0000f8380)({
    Name: (string) (len=4) "data",
    Expr: (*hclsyntax.LiteralValueExpr)(0xc0000245a0)({
     Val: (cty.Value) {
      ty: (cty.Type) {
       typeImpl: (cty.pseudoTypeDynamic) {
        typeImplSigil: (cty.typeImplSigil) {
        }
       }
      },
      v: (*cty.unknownType)(0xdfe340)({
       refinement: (cty.unknownValRefinement) <nil>
      })
     },
     SrcRange: (hcl.Range) foo.hcl:2,11-12
    }),
    SrcRange: (hcl.Range) foo.hcl:2,4-10,
    NameRange: (hcl.Range) foo.hcl:2,4-8,
    EqualsRange: (hcl.Range) foo.hcl:2,9-10
   })
  },

That doesn't parse in a whole jobspec. But there might be some support for this in the parser we're just not taking advantage of... will keep digging.

Edit: it parses that as a literal but returns diagnostic errors, that's why.

I also tried adding a literal function to the HCL2 function set, but the args are interpolated before they ever get to the function (understandably). You can also set the heredoc as a HCL2 variable value but that doesn't help either, as it'll get rejected by the parser saying that "variables are not allowed here"

@tgross
Copy link
Member

tgross commented Aug 16, 2024

I did take a look at what it would take to get heredoc literals added to the HCL syntax, and unfortunately that in itself seems like it'd be a much larger lift than I expected, when compared to the decoder tweaks that Nomad has (ref hashicorp/hcl#620). But given the decoding step is where evaluation happens, there might be an opportunity to introduce some other behavior in the Nomad-specific decoding tweaks.

In the meantime, the test below exercises some workarounds using either the file function or passing the template as the contents of a variable.

jobspec2/parse_test.go
func TestHeredocInterpolation(t *testing.T) {

	templateHeredoc := `
variable "port" {
  type    = number
  default = 8001
}

job "example" {
  group "group" {
    task "task" {
      template {
        data = <<EOH
PORT=${var.port}
EOH

      }
    }
  }
}
`
	out, err := ParseWithConfig(&ParseConfig{
		Path:    "input.hcl",
		Body:    []byte(templateHeredoc),
		ArgVars: nil,
		AllowFS: false,
	})
	must.NoError(t, err)
	must.Eq(t, "PORT=8001\n", *out.TaskGroups[0].Tasks[0].Templates[0].EmbeddedTmpl)

	tmpdir := t.TempDir()
	path := filepath.Join(tmpdir, "foo.tpl")
	os.WriteFile(path, []byte("PORT=${var.port}\n"), 0777)

	literalFromFileContent := fmt.Sprintf(`variable "port" {
			  type    = number
			  default = 8001
			}

			job "example" {
			  group "group" {
			    task "task" {
			      template {
			        data = file("%s")
			      }
			    }
			  }
			}
			`, path)

	out, err = ParseWithConfig(&ParseConfig{
		Path:    "input.hcl",
		Body:    []byte(literalFromFileContent),
		ArgVars: nil,
		AllowFS: true,
	})
	must.NoError(t, err)
	must.Eq(t, "PORT=${var.port}\n", *out.TaskGroups[0].Tasks[0].Templates[0].EmbeddedTmpl)

	literalFromVariable := `variable "template" {
			  type    = string
			}

			job "example" {
			  group "group" {
			    task "task" {
			      template {
			        data = var.template
			      }
			    }
			  }
			}
			`
	out, err = ParseWithConfig(&ParseConfig{
		Path:    "input.hcl",
		Body:    []byte(literalFromVariable),
		ArgVars: []string{"template=PORT=${var.port}\n"},
		AllowFS: true,
	})
	must.NoError(t, err)
	must.Eq(t, "PORT=${var.port}\n", *out.TaskGroups[0].Tasks[0].Templates[0].EmbeddedTmpl)

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/accepted Confirmed, and intend to work on. No timeline committment though. theme/hcl type/bug
Projects
None yet
Development

No branches or pull requests

4 participants