[Discussion] Optional recipe context attributes in TF module config and Recipe versioning

[youngbupark]

Problem

In PR #5999, we support recipe context in Terraform module config. User's recipe TF module should contain the following context variable. We found that TF threw the error if Radius didn't provide one of the attributes in module json config.

variable "context" {
  description = "This variable contains Radius recipe context."

  type = object({
    resource = object({
      name = string
      id = string
      type = string
    })

    application = object({
      name = string
      id = string
    })

    environment = object({
      name = string
      id = string
    })

    runtime = object({
      kubernetes = optional(object({
        namespace = string
        environmentNamespace = string
      }))
    })

    azure = object({
      resourceGroup = object({
        name = string
        id = string
      })
      subscription = object({
        subscriptionId = string
        id = string
      })
    })

    aws = object({
      region = string
      account = string
    })
  })
}

We found attributes \"aws\" and \"azure\"\nare required. error in the functional test because Radius didn't set aws and azure in module config. Terraform provides optional keyword for such optional attributes. In PR #5999, we add optional keyword to azure and aws objects to address the problem.

Functional test log - https://github.com/project-radius/radius/actions/runs/5774669455/job/15651905021

"code": "Internal",
    cli.go:382: [rad]           "message": "terraform apply failure: exit status 1\n\nError: Invalid value for input variable\n\n  on main.tf.json line 4, in module.default:\n   
4:       \"context\": {\n   5:         \"resource\": {\n   6:           \"name\": \"corerp-resources-terraform-context\",\n   7:           \"id\": \"/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Link/extenders/corerp-resources-terraform-context\",\n   8:           \"type\": \"Applications.Link/extenders\"\n   9:         },\n  10:         \"application\": {\n  11:           \"name\": \"corerp-resources-terraform-context\",\n  12:           \"id\": \"/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Core/applications/corerp-resources-terraform-context\"\n  
13:         },\n  14:         \"environment\": {\n  15:           \"name\": \"corerp-resources-terraform-context\",\n  16:           \"id\": \"/planes/radius/local/resourcegroups/kind-radius/providers/Applications.Core/environments/corerp-resources-terraform-context\"\n  17:         },\n  18:         \"runtime\": {\n  19:           \"kubernetes\": {\n  20:             \"namespace\": \"corerp-resources-terraform-context-app\",\n  21:             \"environmentNamespace\": \"\"\n  
22:           }\n  23:         }\n  
24:       },\n\nThe given value is not suitable for module.default.var.context declared at\n.terraform/modules/default/variables.tf:1,1-19: attributes \"aws\" and \"azure\"\nare required.\n"

Possible solutions

Option 1

In PR #5999 , I added optional keyword to make azure and aws optional. So terraform will ignore the properties.

variable "context" {
  description = "This variable contains Radius recipe context."
  type = object({
    resource = object({
      name = string
      id = string
      type = string
    })

    application = object({
      name = string
      id = string
    })

    environment = object({
      name = string
      id = string
    })

    runtime = object({
      kubernetes = optional(object({
        namespace = string
        environmentNamespace = string
      }))
    })

    azure = optional(object({  # <--- HEREHEREHERE
      resourceGroup = object({
        name = string
        id = string
      })
      subscription = object({
        subscriptionId = string
        id = string
      })
    }))
    
    aws = optional(object({  # <--- HEREHEREHERE
      region = string
      account = string
    }))
  })
}

Option 2

We don't add any optional keyword to attributes but instead, Radius can pass empty properties like below.

{
    // ... omit attributes ...
    "azure": {
          "resourceGroup": {
                "name": "",
                "id": ""
          }
          "subscription": {
                "subscriptionId": "",
                "id": ""
          }
    }
    "aws": {
          "region": "",
          "account": ""
    }
}

However, this will still cause the same problem when we introduce new attributes in recipe context.

Let's say there are User 1 and User 2.

User 1 uses Radius v0.30.0 and context v2 schema which includes new gcp attribute and publish it to the module repository.
User 2 uses Radius v0.20.0 and context v1 schema which doesn't contain gcp attribute and try to use User 1's module.
And then User 2 will face the same problem because Radius v0.20.0 would not populate gcp.

Other questions

1. Do we have a plan to introduce the verioning for Recipe Context schema?

AB#8905

AB#9511

AB#10014

[rynowak]

So does this go in the generated wrapper module? I'm trying to understand from bug whether this is about:

• Generated code that is used as plumbing to invoke the user's module (an implementation detail of Radius) VS
• Guidance for how users write recipes in terraform

If it's an implementation detail of Radius are there any downsides to option 1? Channeling my inner language designer here: I would strongly prefer using the type system over using sentinel values as a workaround.

[youngbupark]

So does this go in the generated wrapper module? I'm trying to understand from bug whether this is about:

• Generated code that is used as plumbing to invoke the user's module (an implementation detail of Radius) VS
• Guidance for how users write recipes in terraform

Does wrapper module mean main.tf.json template Radius creates to use recipe module? Still confusing the terminology :) Then yes your points are right.

If it's an implementation detail of Radius are there any downsides to option 1? Channeling my inner language designer here: I would strongly prefer using the type system over using sentinel values as a workaround.

Actually, I tried option 1 and 2. Apart from the downsides above in option 2, populating empty properties give user the wrong impression like a bug. so I prefer option 1 too.

[kachawla]

In PR #5999, we add optional keyword to azure and aws objects to address the problem.

I think there is room for confusion in how the problem and solutions are phrased. Adding optional field in this test file https://github.com/project-radius/radius/blob/a2d1ebe786aee71e2bf4215e0e3daf5671e6ae1d/pkg/recipes/terraform/testdata/.terraform/modules/test-module-recipe-context/variables.tf will not fix real user scenario. In real scenario this file is controlled by the user in the module/module wrapper they are using as a Recipe, so to support option 1, we are expecting that we can require users to always mark certain fields as optional which is not realistic.

As I mentioned on the PR, Terraform does not throw an error if certain fields are not defined at all but are passed - so if aws and azure were not included in the variables.tf but were passed by Radius, it will not fail. So users can very well just not include these fields in the their variable definition.

If it's an implementation detail of Radius

@rynowak This is not controlled by Radius.

[youngbupark]

In PR #5999, we add optional keyword to azure and aws objects to address the problem.

I think there is room for confusion in how the problem and solutions are phrased. Adding optional field in this test file https://github.com/project-radius/radius/blob/a2d1ebe786aee71e2bf4215e0e3daf5671e6ae1d/pkg/recipes/terraform/testdata/.terraform/modules/test-module-recipe-context/variables.tf will not fix real user scenario. In real scenario this file is controlled by the user in the module/module wrapper they are using as a Recipe, so to support option 1, we are expecting that we can require users to always mark certain fields as optional which is not realistic.

Just to be clear - I am pretty new to TF work so my question could be irrelevant or already discussed.

I am still learning our TF design/implementation, so I might have the missing discussion or conversation. But I still don't understand why optional is not realistic because optional is officially supported keyword in TF. I see that real users use this keyword in their TF templates in GitHub and stackoverflow communities...

As I mentioned on the PR, Terraform does not throw an error if certain fields are not defined at all but are passed - so if aws and azure were not included in the variables.tf but were passed by Radius, it will not fail. So users can very well just not include these fields in the their variable definition.

If it's an implementation detail of Radius

@rynowak This is not controlled by Radius.

From what I understand from Ryan's comment, there are two personas:

1. TF Recipe author: who creates TF module and defines and consumes radius recipe context variable in their module -- User will follow our docs or copy context variable sample definition to their own module.
2. Radius who creates wrapper module: Radius can populate recipe context parameters based on recipe context schema spec.

We do not have the full-control for the first one even though they will follow how to write recipe module and our sample, but we have our control for the second one. I thought that the second one was controllable in Ryan's comment.

Working on my first PR for TF work, my initial thought was that we would provide the sharable TF file which included recipe context schema, so recipe authors would just copy the sharable TF file to their TF recipe instead of authoring context variable by themselves.

@kachawla Based on your comment, is your expectation that we will guide TF recipe author that user would define only what they use in their module?

In bicep template, we don't define the entire recipe schema, which is free-form style object. But I thought I must define the exact schema for TF in the beginning. I learn today that TF also has any type, which is free-form style object like bicep template. The only downside is that this is loosely typed variable. I think this option is also good to be consistent with bicep behavior. @rynowak @kachawla WDYT?

variable "context" {
  description = "This variable contains Radius recipe context."
  type = any
}

[shalabhms]

@youngbupark , @kachawla - did we have the information needed here on this issue , any action item to be taken on this issue?

[radius-triage-bot]

👍 We've reviewed this issue and have agreed to add it to our backlog. Please subscribe to this issue for notifications, we'll provide updates when we pick it up.

We also welcome community contributions! If you would like to pick this item up sooner and submit a pull request, please visit our contribution guidelines and assign this to yourself by commenting "/assign" on this issue.

For more information on our triage process please visit our triage overview

[shalabhms]

triage: need to have more discussion on this and decide on the priority.

[kachawla]

But I still don't understand why optional is not realistic because optional is officially supported keyword in TF.

The reason why I mentioned it not being realistic is, because we don't control what users define in their variables.tf file, we can give them guidance to mark azure/aws attributes as optional since they will not be passed if not configured, but we can't enforce this since Radius doesn't have control over it.

TF also has any type, which is free-form style object like bicep template. The only downside is that this is loosely typed variable. I think this option is also good to be consistent with bicep behavior.

This is what we have documented in the recipe authoring page https://docs.radapp.io/guides/recipes/howto-author-recipes/