starhawking / python-terrascript

Create Terraform files using Python scripts.
BSD 2-Clause "Simplified" License
515 stars 76 forks source link

Work-around for data/JSON bug #3

Closed mjuenema closed 6 years ago

mjuenema commented 7 years ago

This relates to https://github.com/hashicorp/terraform/issues/13911.

The work-around suggested in the issue does not work. The underlying problem seems to be that at least with data sources the data source must have more than one argument.

Fails

{
  "data": {
    "aws_instance": {
      "NAME": {
        "first": {
          "a": 1,
          "b": "2",
          "c": true
        }
      }
    }
  }
}

Works

{
  "data": {
    "aws_instance": {
      "NAME": {
        "first": {
          "a": 1,
          "b": "2",
          "c": true
        },
        "second": "string"          <----- second argument.
      }
    }
  }
}

The work-around is to ensure that the data source always has a type (defaulting to string) and an (empty) description.

jbscare commented 6 years ago

Hi! I found https://github.com/mjuenema/python-terrascript/blob/0ee7ef4f1f16683f2c4443f3c78538b595819406/terrascript/__init__.py#L169 which doesn't seem to work with aws_iam_policy_document data sources: It produces JSON that includes "description": "" and "type": "string", which causes terraform plan to say

2 error(s) occurred:

* data.aws_iam_policy_document.foo: : invalid or unknown key: description
* data.aws_iam_policy_document.foo: : invalid or unknown key: type

I can send along more details if that isn't enough to reproduce this.

austin-millan commented 6 years ago

Is there a work-around coming soon? Running into the same problem. Not all data sources have description or type.

mjuenema commented 6 years ago

The original bug hashicorp/terraform#13911 is still open. What I did in my code is a work-around ... and I hate work-arounds because they usually cause other problems as proven by @jbscare. The next step could be to distinguish between different data sources and determine which ones support description and type and which ones don't. This would make python-terrascript a lot more complicated. So far I tried to avoid implementing any actual Terraform semantics into python-terrascript. I need to spend some more time on this.

mjuenema commented 6 years ago

@jbscare are you able to send more details so I can reproduce your exact situation? Thanks. Markus

jbscare commented 6 years ago

Yeah, I should be able to come up with something -- I don't have the exact case any more, because I worked around it by realizing that I could just define my policy documents as Python dicts, and use them directly in Terraform objects, rather than creating the policy documents as separate Terraform objects, which seems somewhat cleaner anyway. (When I started this, I was converting existing HCL config, and wanted to keep it as identical as possible for regression-testing purposes.)

Anyway, lemme look at what I'd been doing, and see if I can come up with an easy reproducer.

jbscare commented 6 years ago

Ok, so this works:

#!/usr/bin/env python3

import json
import terrascript
import terrascript.aws.d
import terrascript.aws.r

ts = terrascript.Terrascript()

ts += terrascript.provider\
      ("aws",
       region = "us-east-1")

policydict = {
    "Version": "2012-10-17",
    "Statement": [{"Action": "s3:*", "Resource": "*"}]}

ts += terrascript.aws.r.iam_policy\
      ("jbstest_works",
       name = "jbstest_works",
       policy = json.dumps(policydict, sort_keys=True))

#ts += terrascript.aws.d.aws_iam_policy_document\
#      ("jbstest",
#       statement = {
#           "actions": ["s3:*"],
#           "resources": ["*"]
#           }
#       )
#
#ts += terrascript.aws.r.iam_policy\
#      ("jbstest_fails",
#       name = "jbstest_fails",
#       policy = "${data.aws_iam_policy_document.jbstest.json}")

with open('jbstest.tf.json', 'w') as fh:
  fh.write(ts.dump())

That creates this JSON:

{
  "provider": {
    "aws": {
      "__DEFAULT__": {
        "region": "us-east-1"
      }
    }
  },
  "resource": {
    "aws_iam_policy": {
      "jbstest_works": {
        "name": "jbstest_works",
        "policy": "{\"Statement\": [{\"Action\": \"s3:*\", \"Resource\": \"*\"}], \"Version\": \"2012-10-17\"}"
      }
    }
  }
}

A terraform plan on that works fine.

If you uncomment the commented-out parts, you get this JSON:

{
  "data": {
    "aws_iam_policy_document": {
      "jbstest": {
        "description": "",
        "statement": {
          "actions": [
            "s3:*"
          ],
          "resources": [
            "*"
          ]
        },
        "type": "string"
      }
    }
  },
  "provider": {
    "aws": {
      "__DEFAULT__": {
        "region": "us-east-1"
      }
    }
  },
  "resource": {
    "aws_iam_policy": {
      "jbstest_fails": {
        "name": "jbstest_fails",
        "policy": "${data.aws_iam_policy_document.jbstest.json}"
      },
      "jbstest_works": {
        "name": "jbstest_works",
        "policy": "{\"Statement\": [{\"Action\": \"s3:*\", \"Resource\": \"*\"}], \"Version\": \"2012-10-17\"}"
      }
    }
  }
}

terraform plan then fails, saying

Error: data.aws_iam_policy_document.jbstest: : invalid or unknown key: description
Error: data.aws_iam_policy_document.jbstest: : invalid or unknown key: type

That's presumably because the JSON for data.aws_iam_policy_document has those "description" and "type" keys, which are what https://github.com/mjuenema/python-terrascript/blob/0ee7ef4f1f16683f2c4443f3c78538b595819406/terrascript/__init__.py#L169 adds.

nielsonsantana commented 6 years ago

@mjuenema, python-terrascript is awesome!!!!! Everyone that use terraform would use this tools. I wrote a script to deploy pgbouncer in a environment of high-availability and fail-over on AWS with less time than using terraform directly. Incredible!!! I had the same problem of invalid or unknown key: description and invalid or unknown key: type. The problem appears on data resources: aws_subnet_ids, aws_vpc, aws_subnet and aws_route53_zone.
From 10 resources that I imported from terrascript, 4 presented the problem. Then, should the attributes type and discriptioin be default? To be able to use terrascript I needed add a parameter default_attrs to disable the default attributes.

#!/usr/bin/env python3
class _data(_base):
    """Base class for data sources."""
    _class = 'data'

    # TODO: Work-around for https://github.com/mjuenema/python-terrascript/issues/3
    def __init__(self, name, default_attrs=True, **kwargs):
        if kwargs and default_attrs:
            if 'type' not in kwargs:
                kwargs['type'] = 'string'

Then, just call passing default_attrs=False.

vpc = aws_vpc('selected', id=vpc_id, default_attrs=False)

However, I think that it's not a good practice.

mjuenema commented 6 years ago

Thanks @jbscare and @nielsonsantana. This gives me examples to test with in tests/test_issues.py. (https://github.com/mjuenema/python-terrascript/blob/master/tests/test_issues.py). I'll keep you updated.

mjuenema commented 6 years ago

@nielsonsantana and @jbscare. I possibly fixed the problem. The code and test case are currently in the feature/issue3 branch (https://github.com/mjuenema/python-terrascript/tree/feature/issue3). Please check the tests/test_issue3*.py scripts for your cases. The underlying problem and work-around are described in this Terrafom issue https://github.com/hashicorp/terraform/issues/13037 (check the first reply).

nielsonsantana commented 6 years ago

@mjuenema, Worked nicely! Thanks! Great Job! I checked the issue on terraform, but the unique solution was remove the attributes type and description.

mjuenema commented 6 years ago

The delete argument in ts.validate(delete=False) determines whether the temporary Teraform JSON file the method creates will be deleted or not. This is really just for debugging and not meant for general use. It defaults to True.

Markus Juenemann markus@juenemann.net

On Mon, 30 Apr 2018, at 3:13 PM, Nielson Santana wrote:

@mjuenema[1], Worked nicely! Thanks! Great Job! I checked the issue on terraform, but the unique solution was remove the attributes type and description.> I don't understood was delete is False in test_issue33( https://github.com/mjuenema/python-terrascript/blob/issue33/tests/test_issue33.py#L45)> assert ts.validate(delete=False) == True — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[2], or mute the thread[3].>

Links:

  1. https://github.com/mjuenema
  2. https://github.com/mjuenema/python-terrascript/issues/3#issuecomment-385314668
  3. https://github.com/notifications/unsubscribe-auth/AGng8Jcu3qOLgMIBc6KD4s1j9h3c3wdJks5ttp1pgaJpZM4Oe8CV
mjuenema commented 6 years ago

Let @jbscare check whether this solves the problem he reported. Then I can merge the Github branch and create release 0.5.1.

mjuenema commented 6 years ago

@jbscare Please note that the policy 'statement' has to be a list.

    ts += terrascript.aws.d.aws_iam_policy_document\
          ("jbstest",
           statement = [{      # <--- dict inside a list!!!
               "actions": ["s3:*"],
               "resources": ["*"]
               }]
           )
jbscare commented 6 years ago

Huh, interesting about the list thing; that wasn't the cause of my problem, but would have been if not for the other problem. :^ ) With the correction, the relevant part of the JSON now looks like this:

  "data": {
    "aws_iam_policy_document": {
      "jbstest": {
        "description": "",
        "statement": [
          {
            "actions": [
              "s3:*"
            ],
            "resources": [
              "*"
            ]
          }
        ],
        "type": "string"
      }
    }
  },

and terraform plan still fails in the same way.

I'll need to set up a virtualenv to test the fix, will try to get to that soon!

mjuenema commented 6 years ago

Yes, I found that hint in a comment to an Ansible open bug. You'd have to test against the feature/issue3 branch of Github repository. The 0.5.1 (candidate) version of python-terrascript is not on PyPi yet.

jbscare commented 6 years ago

Hey, I finally got a chance to try this out, and it seems to work. Thanks!