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

lang/funcs: add jsonpath function #22460

Closed
wants to merge 3 commits into from

Conversation

bonifaido
Copy link
Contributor

@bonifaido bonifaido commented Aug 14, 2019

This is mostly a POC/proposal but I would be more than happy if this or something similar gets accepted. I'm highly open to any kind of feedback and/or ideas. JSONPath is a quite common object query language and could make the life of Terraform users easier, when looking up values in deeply nested objects/maps/lists. It is very similar to the lookup function but instead of a key it accepts a path parameter.

Usage example:

locals {
  mymap  = { "a" = { "b" = 3 } }
  mylist = [local.mymap]
}

output "jsonpath-found" {
  value = jsonpath(local.mymap, "$.a.b")
}


output "jsonpath-notfound" {
  value = jsonpath(local.mymap, "$.a.b.c", 12)
}

output "jsonpath-list" {
  value = jsonpath(local.mylist, "$[0]")
}
$ terraform apply

Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Outputs:

jsonpath-found = 3
jsonpath-list = {
  "a" = {
    "b" = 3
  }
}
jsonpath-notfound = 12

Some issues:

  • I use JSONEncode/JSONDecode to make a Go object (and transform it back) of type interface{} from the cty.Value, what is the official way to do this? :)

@apparentlymart
Copy link
Member

Hi @bonifaido! Thanks for working on this.

This is an interesting proposal for sure, but I feel a little unsure as to what this adds that isn't already possible using the native Terraform language syntax and existing functions. For example:

jsonpath existing functionality
jsonpath(local.mymap, "$.a.b") local.mymap.a.b
jsonpath(local.mymap, "$.a.b.c", 12) lookup(local.mymap.a.b, "c", 12)
jsonpath(local.mylist, "$[0]") mylist[0]

If you'd be willing, I'd prefer to take a step back here and talk about what the use-cases are, and then we can discuss this and other potential solutions to address those use-cases.

While I do very much appreciate you taking the time to work on this PR, I'm cautious about introducing another competing traversal syntax into Terraform since it is likely to cause confusion as to which approach is best for a given situation, and confusion for readers of configuration about whether the these two approaches have a significantly different meaning or purpose. If we can identify some specific use-cases to address, hopefully we can find a way to address them that doesn't also duplicate existing functionality along the way.

Thanks!

@apparentlymart apparentlymart added waiting-response An issue/pull request is waiting for a response from the community config enhancement labels Aug 16, 2019
@bonifaido
Copy link
Contributor Author

Hi @apparentlymart!

Yes, as I mentioned, this is just a POC/proposal, so I'm also not 100% sure about that we need this, and I have no issues if drop this entirely. I think that accessing deeply nested objects could have been easier, when some parts of the path didn't exist, for example in this example, what happens if a or b doesn't exist, it will fail, however with jsonpath you will get back the default value 12:

jsonpath terraform
jsonpath(local.mymap, "$.a.b.c", 12) lookup(local.mymap.a.b, "c", 12)

Of course, this can be solved with existing Terraform syntax as well, with a chain of "lookup with default as empty map" calls, but this version is hard to read:

lookup(lookup(lookup(local.mymap, "a", {}), "b", {}), "c", 12)

We mostly use this to parse YAML/JSON configuration files.

@ghost ghost removed the waiting-response An issue/pull request is waiting for a response from the community label Aug 22, 2019
@sagikazarmark
Copy link

sagikazarmark commented Oct 29, 2019

@apparentlymart as @bonifaido pointed out we lookup values in nested objects on various levels. At the moment that's cumbersome to say the least (see the example above).

We decided to propose jsonpath, because it's an established pattern for traversing nested JSON objects (eg. aws cli uses it as well), but we would be happy to work on any kind of solution that you think fits Terraform more.

@sagikazarmark
Copy link

Ping @apparentlymart

@jakebiesinger-onduo
Copy link

jakebiesinger-onduo commented Nov 12, 2019

+1 on lookup(lookup(lookup(local.mymap, "a", {}), "b", {}), "c", 12) being cumbersome.

In my use case, I have a series of config blocks that I filter down and then apply a for_each on a resource. I assumed that lookup worked exactly like this (e.g., lookup(config, "some.nested.value", false) and when I realized they didn't, I went through several frustrating iterations. For example, given this config:

locals { config {
    foo1 { feature1 = { enabled = true } }
    foo2 { feature2 = { enabled = true } }
} }

none of the following work:

  1. no paths in a lookup :(
locals {
  feature1_enabled = {
    for f, c in local.config:
      f => c if lookup(c, "feature1.enabled", false)
} }
  1. No short-circuiting on booleans in if filter expressions:
locals {
  feature1_enabled = {
    for f, c in local.config:
      f => c if contains(keys(c), "feature1") && lookup(c.feature1, "enabled", false)
} }

FWIW, it seems like a simple path traversal might catch a good chunk of the use cases without the full power/complexity of a JsonPath.

@apparentlymart
Copy link
Member

Hi again! Sorry for the silence here. Got distracted by some other work...

I think this boils down to a couple different language design tensions:

  • We generally encourage working with data structures that are strongly typed, which is why we provide type constraints for variables, aimed at ensuring that you are less often in a situation where you need to work with a value whose type is not known at all. The philosophy behind that is that it's better to require a specific type and produce an error if the type doesn't match (so the person who write the value can see it fix it) rather than to quietly do something unexpected.

    It sounds like the use-cases you have in mind for jsonpath are going in the other direction of this: accepting possibly-invalid or possibly-incomplete data and trying to recover by applying defaults, rather than returning an error.

    Since we've only been discussing things in the abstract so far it's hard to tell if we're just taking a different API design philosophy (which makes this purely a language design tradeoff) or if there are additional real-world use-cases that can be met by recovering from "invalid" input that would be hard or impossible to meet in situations where a specific data structure shape must be specified by the module developer.

    Wen I say "real-world use-case" I mean in terms of the final result we want to achieve, like "deploy arbitrary user-provided specification to Kubernetes", as opposed to abstract use-cases like "ability to process arbitrary JSON" without saying what this arbitrary JSON is for.

  • Putting that philosophical question aside, there's also a more practical design question I mentioned before: exposing JSONPath in the Terraform language effectively makes JSONPath part of the Terraform language, and that's a lot of extra stuff someone would need to learn before being able to fully understand an arbitrary Terraform configuration.

    As @jakebiesinger-onduo noted, one way we could mitigate that is to add a function that lets you use Terraform's own traversal syntax in a way that allows Terraform to detect when the traversal does something that's impossible for the given data structure and return a default value instead. If we move forward with this, I expect that's the path we'd want to take, since at least then we can refer to our existing documentation for references and thus avoid users having to become familiar with an entirely new syntax.

    The Terraform language is built on an application-agnostic foundation called HCL which doesn't have a mechanism for us to directly support a "try a traversal and produce a default if it fails" operation within the language itself, so if we were to do it we'd need to have the user provide the traversal to try inside a string and then parse it within the function:

    tryreference(obj, "a.b.c[\"foo\"]")
    

    The expression syntax isn't design to nest expressions inside strings like this, so it's kinda ugly but hopefully this sort of technique would be used infrequently enough, and usually in situations that don't require quoted strings in the traversals anyway, that it being a little ugly is okay when it happens. We have been making progress (from Terraform 0.12.0 onwards) at avoiding confusing situations where there are quoted strings containing what look like Terraform language expressions, so this would be a slight regression on that path, but perhaps an okay one if the real-world use-cases for it are compelling enough and it does not become a crutch to avoid more deliberate use of types in the language.

For these reasons, I still feel ambivalent about the idea of encouraging "try this and use a default if it doesn't work" as a common design pattern, but I can at least say that I'd rather not use JSONPath to meet that use-case if we do move forward with it, and would want to use Terraform's (HCL's) relative traversal syntax instead, matching what we see in the ignore_changes lifecycle argument, albeit in quotes rather than "naked".

If either of you are able to share some examples of situations where you've needed to use constructions like lookup(lookup(lookup(local.mymap, "a", {}), "b", {}), "c", 12) in real-world configurations (situations where using local.mymap.a.b.c and letting it return an error would not have been an option), that could help to make a decision about the first tension I described above. Thanks again!

@jakebiesinger-onduo
Copy link

jakebiesinger-onduo commented Nov 13, 2019

Thanks for the detailed and thoughtful response!

I don't know how common my use-case is, but it seems like something that may become more common with terraform's recent dynamic and for_each for blocks and resources.

We use terraform to provision a few dozen SFTP clients and their client-specific configurations in GCP, including the SFTP server they connect to, the cron job schedules that trigger data pulls, cloud storage + pubsub notifications associated with outgoing data, encryption keys for each client, etc.

We have a single .tf file that defines all of these configurations for these few-dozen partners, structured something like this:

locals = {
  config = {
    partner1 = {
      sftp-server = { hostname = ..., user = ... }
      pull-config = { 
        schedule = { cron = "0 6 * * * *" } 
        patterns = [ {src_dir = "incoming", regex = ".*\.csv$"} ]
        encryption = { pgp-private-key = "path-to-some-key" }
      }
      push-config = {
        trigger { 
           use_gcs_trigger= true 
           bucket = google_storage_bucket.outgoing["partner1"].name 
        }
        patterns = [ { dest_dir = "outgoing", regex = ".*" } ]
        encryption = { pgp-public-key = "some-key" }
      }
    }
    partner2 = {
      partner-gcs-bucket = { bucket = "some-partner-bucket" prefix="path/to/stuff" }
      pull-config = { 
        trigger { pubsub = google_pubsub_topic.incoming["partner2"].id }
        patterns = [ {src_dir = "incoming", regex = ".*\.csv$"} ]
        # no encryption for this partner.
      }
      push-config = {
       trigger { use_gcs_trigger = false bucket = google_storage_bucket.outgoing["partner2"].name }
       patterns = [ { dest_dir = "outgoing", regex = ".*" } ]
        # no encryption for this partner.
     }
  }
   partner3 = {
      partner-gcs-bucket = { ... }
      pull-config = {  ... }
      # no push config for this partner-- we only receive data.
  }
}

As you can see, we essentially have a collection of features available for each partner in several categories. For triggering, we offer pubsub, GCS, and scheduled jobs. For src/destination, we can connect to SFTP servers or directly read+write to partner GCS buckets. Since each option requires some additional config, we can't really switch on a single enum, or list out a bunch of features and have boolean on/off switches and I think that approach would be much less readable. Having this single file makes configuring a new partner dead simple-- folks without terraform experience can see what their options are by inspection. At the same time, we can interpolate values into the config (e.g., terraform.workspace, or various remote_state fields).

Our terraform resources and modules slice this config into pieces and use for_each on the relevant chunks in order to stand up the associated pubsub topics, GCS buckets + notifications, IAM permissions, cloud scheduler jobs, etc... hence our desire to have paths or their equivalent in the lookup method. You can imagine how convoluted some of lookup statements become when you're querying across partners and ragged datasets to, e.g., find the list of trigger-buckets thats you need to set up IAM permissions on. Ideally, we could instead use something like:

locals { 
  bucket-triggers-needing-iam = { 
    for partner, features in local.config:
      partner => features.push-config.trigger.bucket 
      if lookup(features, "push-config.trigger.use_gcs_trigger", false) 
  }
}
resource google_storage_bucket_iam_member read-buckets {
  for_each = local.bucket-triggers-needing-iam
  name = each.value
  ...
}

Instead, the filter step becomes even more convoluted:

if lookup(lookup(lookup(features, "push-config", {}), "trigger", {}), "use_gcs_trigger", false)))

I wholeheartedly agree with you about making sure the schema is configured properly. Unfortunately Terraform's native type system, even with TF .12 changes, is not strong enough for this kind of config. Instead, we validate the schema of this blob using an external provider that checks JsonSchema, allowing us to specify that, e.g., exactly one of sftp-server and partner-gcs-bucket must be specified. Terraform errors out immediately if that's not the case, preventing terraform apply until the schema is satisfied.

As I said, this may be an extreme usecase, or maybe not what terraform is intended for. I will say that this pattern of "uber config that get sliced into parts and divvied out to modules + resources" is becoming popular on our repos, especially since we recently upgraded and now allow for_each on resource blocks.

@sagikazarmark
Copy link

Thanks for the detailed answer!

It sounds like the use-cases you have in mind for jsonpath are going in the other direction of this: accepting possibly-invalid or possibly-incomplete data and trying to recover by applying defaults, rather than returning an error.

In our case it's rather having a default value and allowing the user to override it. Technically it's still an incomplete structure, but to add another detail: we decode YAML files and allow users to pass complex structures there.

The reason we do that is because those YAML structures actually end up in helm chart installations and it's easier for users to write a familiar syntax for helm chart values. Given the huge amount of parameters, terraform's flat variable design doesn't really work for us in these scenarios.

Some of those values though are used in other places as well which is when we want to traverse a nested structure and return a default value.

@apparentlymart
Copy link
Member

Thanks for sharing these use-case details, both!


@jakebiesinger-onduo, this sort of "mega-object" configuration style is not a configuration style we've seen a lot, but I can see the attractions to it. We've generally been encouraging a quite different strategy of separation of concerns via module composition, where the root module consists mainly of calls to various modules that each describe one part of the overall system, and data flows between them so that it's easier to see how the components are interconnected.

For the system you described here, I suppose that would end up looking like one module per partner where the module itself replaces the var.config element, and then a different module for each permutation of pulling and pushing so that each partner module can select which implementations of those ideas it will use. I'm not sure if this specific example makes sense in your system in practice, but I might for example try to represent "GCS trigger" as a component module that a particular partner module can either call or not call depending on whether that mechanism is required.

That is a variant of the idea discussed under "multi-cloud abstractions" in the Module Composition guide, albeit probably abstracting over a few different ways to achieve the same thing in the same "cloud" in your case.

My worry about providing a big, all-encompassing data structure like that and then pulling it apart into individual resources would be that it seems likely that it would be hard to predict exactly how a change to the data structure will impact the described infrastructure, and in turn folks making those changes are less likely to be able to confidently interpret Terraform's plan. One reason why we try to discourage "heavy" abstractions is that anyone making changes to a Terraform configuration ideally ought to be able to look at the resulting plan and confirm that it matches what they expected to happen.


@sagikazarmark in your case I feel like I'd want to approach it by first normalizing the data structure (applying the defaults, making sure all of the specified values are of the correct type, etc) all together in one place, and then in the rest of the configuration just assume that the data is already in the right shape. That would then keep all of the "ugly" (subjectively) dynamic type wrangling together in one place and let most of the configuration be straightforward, unconditional references into that normalized data structure.

With that said, it's not easy to write that sort of normalization in Terraform today either. I'm not sure if you were talking specifically about the Helm chart.yaml format or about some other Helm mechanism (I'm not super familiar with Helm myself) but to use the chart.yaml format as an example, normalization of an arbitrary YAML input might look like this:

locals {
  raw_chart = yamldecode(file("${path.module}/chart.yaml"))

  chart = {
    apiVersion  = raw_chart.apiVersion
    name        = raw_chart.name
    version     = raw_chart.version
    kubeVersion = lookup(raw_chart, "kubeVersion", ">= 0.0.0")
    description = lookup(raw_chart, "description", null)
    keywords    = lookup(raw_chart, "keywords", [])
    home        = lookup(raw_chart, "home", null)
    sources     = lookup(raw_chart, "sources", [])
    maintainers = [
      for m in lookup(raw_chart, "maintainers", []) : {
        name  = m.name
        email = lookup(m, "email", null)
        url   = lookup(m, "url", null)
      }
    ]
    engine        = lookup(raw_chart, "engine", "gotpl")
    icon          = lookup(raw_chart, "icon", null)
    appVersion    = lookup(raw_chart, "appVersion", null)
    deprecated    = lookup(raw_chart, "deprecated", false)
    tillerVersion = lookup(raw_chart, "tillerVersion", ">2.0.0")
  }
}

With that normalization in place then elsewhere in the configuration I could e.g. write local.chart.deprecated and have it either be what was specified in the file or the default value false automatically, without any further conditional logic. We can also look at the normalization expression above and see somewhat-clearly what data structure we're aiming to produce and what it might be possible to set in the YAML file, which would be harder to do if the lookup calls (or jsonpath calls, etc) were distributed throughout the configuration.

Maybe it would serve us better to work on making it easier to write normalization expressions like the above in a more readable way, so that you could meet your use-case in a way that would reduce the sprawling complexity of conditional lookups. I remember in another issue (which sadly I wasn't able to find quickly now to reference) we were discussing a possible deepmerge function that could allow providing a data structure full over overridable defaults to merge into a given object, though I had some concerns in that of how best to define merging of lists of objects like the maintainers in the example above; it's not clear how you'd express that a maintainer has a name, an email, and a url without writing out an actual default maintainer.


🤔 Lots of language design questions to noodle on here. Thanks again to both of you for sharing your use-cases. Both of them are somewhat novel approaches that I've not seen a lot in the wild, but I want to be clear that I'm not trying to tell you that they are invalid approaches, I'm just thinking aloud about how they compare to existing approaches I have seen before and whether there are different ways to meet these intents within existing language features.

I'm thinking maybe we should divert this discussion into a feature request issue rather than this PR, since this has become more of a design question than a review of a proposed implementation. I'm running out of time for the day today, so I won't be able to do this immediately, but I'm thinking maybe we record each of those slightly-different use-cases in its own issue and then discuss some different ways we could address them. Does that sound reasonable to you both? If so, I'm happy to do the paperwork of adapting what you already shared into the "Feature Request" template, since you already took the trouble of writing out your examples in some detail.

@sagikazarmark
Copy link

sagikazarmark commented Nov 14, 2019

Thanks for the detailed answer again!

Unfortunately we are talking about values.yaml which is by definition unstructured and nested. Also, this isn't really effective for a huge number of charts.

We have given it quite a few thoughts actually, and nested object traversing is the best we could come up with.

@jakebiesinger-onduo
Copy link

Thanks again for the detailed feedback!

What you describe here makes sense and we'e certainly contemplated it. The idea of having a module instantiation per partner certainly groups the associated resources more nicely (e.g., you see changes to module.partner1.google_storage_bucket.sftp-storage rather than google_storage_bucket.sftp-storage["partner1"). We have been planning on migrating in that direction anyway.

That said, the two approaches aren't incompatible. Lots more folks understand config files than understand terraform modules. The super-config presented here just needs a module for_each to generate the appropriate modules for each partner and we can keep our single-config super-object. Again, our readers + config writers are NOT tf experts. They do immediately grok the config file, while I think they'd struggle with a file full of modules and maybe resources etc. We have literally no other stuff in this config file-- in some ways, it is like a data-only modules in the module guide.

All that said, we've effectively reduced the depth by 1 here. Further modularization can help abstract stuff and in doing so reduce depth, but the general problem hasn't gone away IMHO. We want more and more things to be infra-as-code and those things are often nested and complex. It feels like maybe a false dichotomy to offer up modularization + decomposition as the solution-- I think we can recommend that path while still allowing an escape hatch for more complex cases?

As I mentioned before, I assumed that simple path-based traversal was a thing in terraform. It just seems odd to allow arbitrarily nested maps without a clean way to get to those values.

@jakebiesinger-onduo
Copy link

Aaaand sorry, yes, happy to move this conversation to a feature request. Probably should have done that before the novel of a response :)

@sagikazarmark
Copy link

I'm thinking maybe we should divert this discussion into a feature request issue rather than this PR, since this has become more of a design question than a review of a proposed implementation.

Yeah, same here, we don't want to force this solution at all, if it starts a conversation, we are more than happy.

@bonifaido
Copy link
Contributor Author

Closing this, for now, I hope it will reborn some time as a form a feature-request rather :)

@apparentlymart
Copy link
Member

Hi all! Thanks for the feedback.

I've created #23378 "Top-level Configuration Abstractions" (for want of a better name for the use-case) to represent the first of the break-out use-cases discussed above. I tried to elaborate there on some more of my thought process and collect some links to other issues that seem in a similar spot.

I had been planning to open a second one named something like "Data Structure Normalization" but it sounds like I misread what @sagikazarmark described and, reflecting on it again now with fresh eyes, I see that a summary like that would be describing a solution rather than a use-case anyway. I think I'd prefer to make it more specific to the problem at hand, so something like "Passing Helm Charts values.yaml through Terraform" maybe? But I don't know enough (really, anything) about Helm in order to confidently write up something like that. @sagikazarmark would you be willing to open a feature request issue with a summary like that (not necessarily exactly that, if it doesn't feel right for what you're trying to do), perhaps giving some specific examples of what these YAML files look like and how you're currently using them in Terraform, and then we can talk there about possible ways to get it done?

@apparentlymart
Copy link
Member

In the forthcoming release v0.12.20 we're planning to include a new function try which has some "special powers" when compared to most functions in that it's able to catch and handle dynamic errors that arise when evaluating its arguments. We're intending this as a compromise to allow concisely writing traversal expressions into data structures of unknown shape without introducing any new traversal syntax into the language, which you'll remember was one of the concerns I raised in an earlier comment.

locals {
  mymap  = { "a" = { "b" = 3 } }
  mylist = [local.mymap]
}

output "jsonpath-found" {
  value = try(local.mymap.a.b, 12)
}

output "jsonpath-notfound" {
  value = try(local.mymap.a.b.c, 12)
}

output "jsonpath-list" {
  value = try(local.mylist[0], {})
}

The try function tries evaluating each of its arguments in turn and returns the value of the first one that evaluates without error. By ensuring that the final argument is a constant (which therefore cannot fail) we can use try to concisely select default values to use in case of a failure of any of the traversal steps.

As is common with special language features like this, it will of course be possible to use try in ways that might make the configuration harder to read. In the interests of pragmatism though, we're introducing this function and merely recommending that it be used only with straightforward traversal expressions (similar to what you might've expressed in JSONPath with this PR) and not with more complex expressions that could fail in multiple different ways.

My other recommendation with try, which will also be reflected in the documentation once it's published, is to limit it only to special "normalization expressions" given in local values, like I showed with the Helm chart example in an earlier comment:

locals {
  raw_chart = yamldecode(file("${path.module}/chart.yaml"))

  chart = {
    apiVersion  = local.raw_chart.apiVersion
    name        = local.raw_chart.name
    version     = local.raw_chart.version
    kubeVersion = try(local.raw_chart.kubeVersion, ">= 0.0.0")
    description = try(local.raw_chart.description, null)
    keywords    = try(local.raw_chart.keywords, [])
    home        = try(local.raw_chart.home, null)
    sources     = try(local.raw_chart.sources, [])
    maintainers = [
      for m in lookup(local.raw_chart, "maintainers", []) : {
        name  = m.name
        email = try(m.email, null)
        url   = try(m.url, null)
      }
    ]
    engine        = try(local.raw_chart.engine, "gotpl")
    icon          = try(local.raw_chart.icon, null)
    appVersion    = try(local.raw_chart.appVersion, null)
    deprecated    = try(local.raw_chart.deprecated, false)
    tillerVersion = try(local.raw_chart.tillerVersion, ">2.0.0")
  }
}

My intent here is that all of the "trickery" for normalizing the value can be gathered together into a single spot, and the rest of the configuration can just have straightforward references into local.chart without a lot of conditional tests and other visual noise making the configuration harder to read.

I'm hoping that this new try function is the missing piece to ease the "Passing Helm Charts values.yaml through Terraform" part of this problem. I'm leaving #23378 to represent the other use-case we discussed here, because the try function is not intended to address that one, although it could potentially be used to make the interface of a module friendly enough that it's less annoying to express the top-level abstraction as a normal Terraform module in some cases.

Thanks again for sharing the use-cases here, and giving us some food for thought on how something like this might fit with the existing Terraform language features.

@ghost
Copy link

ghost commented Jan 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants