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

gohcl: Unable to decode block to *hcl.Block field. #505

Closed
jmalloc opened this issue Jan 14, 2022 · 6 comments · Fixed by #507
Closed

gohcl: Unable to decode block to *hcl.Block field. #505

jmalloc opened this issue Jan 14, 2022 · 6 comments · Fixed by #507
Assignees

Comments

@jmalloc
Copy link
Contributor

jmalloc commented Jan 14, 2022

I think I have discovered a bug in gohcl (I am using v2.11.1). Could you please confirm if this is a legitimate issue, or if I am misusing the package.

The gohcl package docs state:

"block" fields may be of type *hcl.Block or hcl.Body, in which case the corresponding raw value is assigned ... [etc]

However, attempting to decode a block into a field of type *hcl.Block produces an error (unless the block is empty).

Analysis

As best I can tell the issue is with the interplay between these two lines, though there are likely other culprits:

  • decode.go#L266 in which decodeBlockToValue() performs the check to see if the field can be assigned a *hcl.Block value
  • decode.go#L221 which calls decodeBlockToValue() for fields that are pointers. Specifically, it dereferences the pointer using v.Elem(), hence passing a value of type hcl.Block not *hcl.Block, causing the conditional on line 266 to fail.

The net result is that the field is treated like any other user-defined struct that has no hcl field tags, which is why this error only occurs if the block in the .hcl file is not empty.

Reproducing the issue

The following code can be used to reproduce the issue:

package main

import (
	"github.com/hashicorp/hcl/v2"
	"github.com/hashicorp/hcl/v2/hclsimple"
)

func main() {
	var output struct {
		TestBlock *hcl.Block `hcl:"my_block,block"`
	}

	err := hclsimple.Decode(
		"reproduce.hcl",
		[]byte(
			`my_block {
				some_attr = true
			}`,
		),
		nil, &output,
	)

	if err != nil {
		panic(err)
	}
}

It produces the following output:

go run main.go
panic: reproduce.hcl:2,5-14: Unsupported argument; An argument named "some_attr" is not expected here.

goroutine 1 [running]:
main.main()
	/tmp/hcl-test/main.go:24 +0xf0
exit status 2

This archive contains the main.go file above and go.mod / go.sum files to help reproduce the issue.


Thanks in advance for your help, and for making this fantastic system open source 👍

@apparentlymart
Copy link
Contributor

Hi @jmalloc! Thanks for reporting this.

Your theory as to why this didn't work makes sense to me. Honestly, this decoding into *hcl.Block functionality is something I added early on while we were prototyping and then forgot about later because nothing I've worked on so far has needed it, but if it's relatively non-invasive to make it work again then it would be nice to support it for completeness.

Other software I've built or seen built with gohcl has typically used hcl.Body instead, sometimes wrapped in a struct like this to use with a block-annotated field in another struct:

type Example struct {
    Foo string `hcl:"foo,label"`
    Bar string `hcl:"bar,label"`
    Config hcl.Body `hcl:",remain"`
}

The above then tells gohcl that the block ought to have two labels, named "foo" and "bar" in this case, but that it should defer decoding the body of the block and just save it in to Config for later processing.

I think the above raises a reason why decoding into *hcl.Block might not be as easy as it initially appears: gohcl is built around the Content or PartialContent method of hcl.Body -- the so-called "low-level API" -- and that requires a BodySchema which describes, among other things, which block labels each block type requires. The JSON variant of HCL syntax relies on this in order to resolve the inherent ambiguity in the JSON mapping to HCL, and so it's mandatory to ensure that all HCL-based languages will have a mapping to JSON.

Off the top of my head then I don't think decoding directly into *hcl.Block is feasible, because it doesn't give gohcl enough information to produce the schema. It may be better instead to update the documentation to no longer claim that this works, and then hopefully you can meet your use-case using a struct-based block representation as I described above.

@jmalloc
Copy link
Contributor Author

jmalloc commented Jan 15, 2022

Thanks for the detailed response!

I don't quite understand the difference in feasibility between decoding into an hcl.Block vs hcl.Body, as the schema is not known in either case. That said, I see now that there doesn't appear to be any equivalent of gohcl.DecodeBody() for blocks, anyway.

To offer some insight into my use case: I have a block with a known schema that contains a "sub" block with a schema defined by an as-yet-un-loaded plugin. I was hoping to defer validation of that inner block (including its labels) until a later phase when the plugin can be loaded. Perhaps I'm just running up against the limitations of gohcl and moving to a lower level API would be better?

I will happily come up with another solution if it turns out that decoding into hcl.Block is not feasible or undesirable. Conversely, if you think it's something worthwhile I'd be happy to attempt a PR. I can imagine this is very low priority :)

Thanks again!

@apparentlymart
Copy link
Contributor

Hi @jmalloc,

Thanks for that extra information. Given the use-case you described, I think an approach like I described in my previous comment could work for you today, as long as the number of labels on the nested block will be known before loading the plugin. This would be a similar design as in various other HCL-based languages, like Terraform and Packer's languages, where the block labels specify (directly or indirectly) which plugin to use.

This would also be possible using the lower-level API, and that's how Terraform in particular does it. But I don't think the low-level API should be needed in this case, because of gohcl's ability to decode a block into a struct with a hcl.Body field annotated as ,remain like I showed above. There's a real-world example of this for HashiCorp Waypoint's use blocks.

@jmalloc
Copy link
Contributor Author

jmalloc commented Jan 19, 2022

Thanks again, you have been very helpful! Based on this I've reengineered so that the plugin is only responsible for the schema of the body.

Would you a PR for correction of the docs (and removal of that unreachable code path, assuming it is indeed unreachable)?

@apparentlymart
Copy link
Contributor

Hi @jmalloc! Thanks for following up about what worked.

If you would be willing, I'd be happy to review and merge a change which changes "may be of type *hcl.Block or hcl.Body" to just say "may be of type hcl.Body", and adjust the grammatical number of the rest of the sentence to refer to "this type" rather than "these types".

If you can also see some codepaths that are easy to delete without significant restructuring then that'd be great too! I've not looked too closely yet but I'd agree that it seems like removing the case blockType.AssignableTo(ty) you indicated seems like a low-risk tidyup since I don't see any way a block value could work in that position given the other context we discussed earlier in this issue.

@jmalloc
Copy link
Contributor Author

jmalloc commented Jan 20, 2022

It seems that blocks can not actually be decoded to hcl.Body either.

I noticed there were no tests for this, so I added one (to ensure my changes didn't break this behaviour) and it fails quite early in ImpliedBodySchema() with the error hcl 'block' tag kind cannot be applied to hcl.Body field.

Upon reflection it makes sense that this is not allowed; doing so would discard label information entirely. Unless you feel I've overlooked something I'll remove both parts from the documentation along with the the obviously unused paths from decodeBlockToValue().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants