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

Allow terraform module to specify complex variable structures #4797

Merged
merged 36 commits into from
Oct 3, 2022
Merged

Conversation

kosalaat
Copy link
Contributor

@kosalaat kosalaat commented Jun 7, 2022

  • Terrform variable types are mapped to ansible veriable types

  • Currently handles Dict, List, Str, Int, Bool types

  • Updated the documentation accordingly

  • Updated with an example.

SUMMARY

Added the capability to specify complex variables structures in the terraform module. With this change we can specify Terraform Object / Nested type variables via ansible:

For example, in TF:

variable "vm_additional_disks" {
  description = "Additional Disks"
  type = list(object({
    unit_number      = number
    size             = number
    label            = string
    thin_provisioned = bool
  }))
  default = []
}

Would translate to:

        vm_additional_disks:
          - label: "Third Disk"
            size: 40
            thin_provisioned: true
            unit_number: 2
          - label: "Fourth Disk"
            size: 22
            thin_provisioned: true
            unit_number: 3
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

terraform

ADDITIONAL INFORMATION

Changed the variable_args to be constructed via a recursive function process_args. It's capable of handling nested lists, dictionaries, bools, string and numbers.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added cloud feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Jun 7, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Jun 8, 2022
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Please add a changelog fragment.

Please also note that we had a similar proposal in the past (#4281) which had to be reverted for breaking backwards compatibility. It looks to me like your approach should work better, but I'm not using terraform so I cannot really judge.

plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
@kosalaat
Copy link
Contributor Author

Thanks for your contribution! Please add a changelog fragment.

Please also note that we had a similar proposal in the past (#4281) which had to be reverted for breaking backwards compatibility. It looks to me like your approach should work better, but I'm not using terraform so I cannot really judge.

Added the change log fragment.

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jun 21, 2022
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 21, 2022
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
@github-actions

This comment was marked as resolved.

@ansibullbot ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jun 26, 2022
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 4, 2022
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Outdated Show resolved Hide resolved
plugins/modules/cloud/misc/terraform.py Show resolved Hide resolved
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jul 9, 2022
@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 17, 2022
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jul 31, 2022
@@ -449,12 +480,72 @@ def main():
if state == 'present' and module.params.get('parallelism') is not None:
command.append('-parallelism=%d' % module.params.get('parallelism'))

def format_args(vars):
if isinstance(vars, str):
return '"{string}"'.format(string=vars)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should better use json.dumps() here, otherwise there will be problems with properly escaping quotes and other things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although json.dumps() do encode special characters, so is the shlex_quote(). Terraform did not like the double encapsulation, although the code/commands work without fail, it did not pass the correct string from ansible to terraform. I created a test case in https://github.com/kosalaat/terrafrom-tests using terraform null_resource, which now test all the scenarios, including checking whether string are passed in intact.

Is there a way we can make this part of the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above TF workfiles, the following ansible playbooks works:

---

- hosts: localhost
  gather_facts: false
  tasks:
    - community.general.terraform:
        project_path: "../../terraform/null-resource"
        state: present
        variables:
          dictionaries:
            name: kosala
            age: 44
          list_of_strings:
            - kosala
            - neloufer
            - punu
            - ari
          list_of_objects:
            - name: kosala
              age: 44
            - name: neloufer
              age: 44
            - name: punu
              age: 9
            - name: ari
              age: 1
          boolean: true
          string_type: "randomstring2&$%@"
          list_of_lists:
            - [ 1 ]
            - [ 11, 12, 13 ]
            - [ 2 ]
            - [ 3 ]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although json.dumps() do encode special characters, so is the shlex_quote(). Terraform did not like the double encapsulation,

I'm not sure what you mean. There is no shlex_quote() involved anywhere (with the variables you are touching).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if string happens to be a"b here? Then terraform will get passed "a"b". I'm pretty sure it will not be happy with that.

Copy link
Contributor Author

@kosalaat kosalaat Aug 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not. shlex_quote() inside run_command() will sort that out. More explanation in the next comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still wondering how strings containing " will be handled. Simply adding quotes around them is probably not ok?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to look at https://www.terraform.io/language/expressions/strings#escape-sequences to see how to correctly quote strings.

This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud feature This issue/PR relates to a feature request integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants