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

provider/aws: Add support for AWS IoT #6961

Closed
wants to merge 52 commits into
base: master
from

Conversation

Projects
None yet
@jhedev

jhedev commented Jun 1, 2016

This is my very first PR to a go project so please be patient with me :)

This will add the following resources:

  • aws_iot_certificate
  • aws_iot_thing
  • aws_iot_policy
  • aws_iot_policy_attachment
  • aws_iot_topic_rule

I already added acceptance tests for all resources except aws_iot_policy_attachment (I have a test locally but it is still failing...).

Still todo:

  • Update function of aws_iot_policy_attachment
  • Update function of aws_iot_topic_rule
  • More tests for most resources
  • Clean up code
  • Documentation

Also see #6138

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Jun 1, 2016

The aws_iot_certificate acceptance tests require a csr.pem file. This is still missing in the repo as I didn't know where to put it. Maybe someone can help me?

jhedev commented Jun 1, 2016

The aws_iot_certificate acceptance tests require a csr.pem file. This is still missing in the repo as I didn't know where to put it. Maybe someone can help me?

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Jun 25, 2016

Added some basic documentation for all new resources.

jhedev commented Jun 25, 2016

Added some basic documentation for all new resources.

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Jun 27, 2016

Added the csr.pem file to the test-fixtures directory.

jhedev commented Jun 27, 2016

Added the csr.pem file to the test-fixtures directory.

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Jun 28, 2016

Rebased onto master. Unfortunately this messed up the commits a bit.

jhedev commented Jun 28, 2016

Rebased onto master. Unfortunately this messed up the commits a bit.

@jhedev jhedev referenced this pull request Jun 28, 2016

Closed

AWS IoT support #6138

@vrcsix

This comment has been minimized.

Show comment
Hide comment
@vrcsix

vrcsix Aug 23, 2016

Contributor

Thanks for putting work into this @jhedev. I've rebased our fork of this PR against current master, and made some other (minor) changes to get the tests running again. Feel free to pull that into yours.

As for aws_iot_policy_attachment, perhaps it'd be less surprising if it follows the interface of aws_iam_policy_attachment? That is: rather than attaching multiple policies to one principal, it attaches one policy to multiple principals. I'm not exactly sure why I had it the other way around, but I think it's more convenient also. You'd usually create one policy for a group of principals, so having the ability to perform this mapping in one go:

resource "aws_iot_policy_attachment" "pubsub_bulbs" {
  name = "pubsub-bulbs"
  policy = "${aws_iot_policy.pubsub.name}"
  principals = ["${aws_iot_certificate.bulb.*.arn}"]
}

seems easier to me.

Contributor

vrcsix commented Aug 23, 2016

Thanks for putting work into this @jhedev. I've rebased our fork of this PR against current master, and made some other (minor) changes to get the tests running again. Feel free to pull that into yours.

As for aws_iot_policy_attachment, perhaps it'd be less surprising if it follows the interface of aws_iam_policy_attachment? That is: rather than attaching multiple policies to one principal, it attaches one policy to multiple principals. I'm not exactly sure why I had it the other way around, but I think it's more convenient also. You'd usually create one policy for a group of principals, so having the ability to perform this mapping in one go:

resource "aws_iot_policy_attachment" "pubsub_bulbs" {
  name = "pubsub-bulbs"
  policy = "${aws_iot_policy.pubsub.name}"
  principals = ["${aws_iot_certificate.bulb.*.arn}"]
}

seems easier to me.

@jwestboston

This comment has been minimized.

Show comment
Hide comment
@jwestboston

jwestboston Oct 5, 2016

Looking forward to this being supported within Terraform!

jwestboston commented Oct 5, 2016

Looking forward to this being supported within Terraform!

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Oct 7, 2016

Thanks @protomouse! I pulled your changes in.

I remember wondering about your proposed interface of the aws_iot_policy_attachment, too. Not sure why I didn't ask, though.

I also agree that this should be kept similar to the aws_iam_policy_attachment. So I'll update the code.

jhedev commented Oct 7, 2016

Thanks @protomouse! I pulled your changes in.

I remember wondering about your proposed interface of the aws_iot_policy_attachment, too. Not sure why I didn't ask, though.

I also agree that this should be kept similar to the aws_iam_policy_attachment. So I'll update the code.

@jhedev jhedev changed the title from [WIP] provider/aws: Add support for AWS IoT to provider/aws: Add support for AWS IoT Oct 7, 2016

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Oct 7, 2016

Rebased onto master and updated the interface of aws_iot_policy_attachment as discussed above.

If there are no other remarks this can be merged now :)

jhedev commented Oct 7, 2016

Rebased onto master and updated the interface of aws_iot_policy_attachment as discussed above.

If there are no other remarks this can be merged now :)

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Nov 2, 2016

Anything else I can do to speed up this PR getting merged?

jhedev commented Nov 2, 2016

Anything else I can do to speed up this PR getting merged?

@vrcsix

This comment has been minimized.

Show comment
Hide comment
@vrcsix

vrcsix Nov 3, 2016

Contributor

Pinging @catsby for review.

Contributor

vrcsix commented Nov 3, 2016

Pinging @catsby for review.

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Nov 14, 2016

Any chance you have some time to review, @catsby ? :)

jhedev commented Nov 14, 2016

Any chance you have some time to review, @catsby ? :)

@vrcsix

This comment has been minimized.

Show comment
Hide comment
@vrcsix

vrcsix Dec 5, 2016

Contributor

I've resolved the merge conflicts in our fork, as well as brought the IoT dependency level with the other vendor'd AWS packages. Could you please @jhedev pull those changes into your branch, and then ping some more HashiCorp folk? (phinze, stack72, mitchellh)

Contributor

vrcsix commented Dec 5, 2016

I've resolved the merge conflicts in our fork, as well as brought the IoT dependency level with the other vendor'd AWS packages. Could you please @jhedev pull those changes into your branch, and then ping some more HashiCorp folk? (phinze, stack72, mitchellh)

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Dec 5, 2016

@protomouse Done. Thanks for your help!

Any chance one of you can review, @catsby @phinze @stack72 @mitchellh ?

jhedev commented Dec 5, 2016

@protomouse Done. Thanks for your help!

Any chance one of you can review, @catsby @phinze @stack72 @mitchellh ?

@stack72

Hi @jhedev

Thanks for the work here - I have gone through the first few resources and made some comments about what will need to be fixed up to make it conform to our standard way of writing a resource :)

Please have a read through and then apply the same changes across all of the resources

Thanks for the work here

Paul

Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_certificate.go
Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_certificate.go
Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_certificate.go
Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_certificate.go
Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_certificate.go
Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_policy_attachment.go
conn := meta.(*AWSClient).iotconn
for _, p := range d.Get("principals").(*schema.Set).List() {
_, err := conn.AttachPrincipalPolicy(&iot.AttachPrincipalPolicyInput{

This comment has been minimized.

@stack72

stack72 Dec 5, 2016

Contributor

Is there a potential eventual consistency issue here? Do we need to wait and check the attachment has happened before we move on to the next?

@stack72

stack72 Dec 5, 2016

Contributor

Is there a potential eventual consistency issue here? Do we need to wait and check the attachment has happened before we move on to the next?

Show outdated Hide outdated builtin/providers/aws/resource_aws_iot_policy_attachment.go
for _, p := range d.Get("principals").(*schema.Set).List() {
log.Printf("[INFO] %+v", p)
_, err := conn.DetachPrincipalPolicy(&iot.DetachPrincipalPolicyInput{

This comment has been minimized.

@stack72

stack72 Dec 5, 2016

Contributor

Is there a potential time to wait for the detachment to happen?

@stack72

stack72 Dec 5, 2016

Contributor

Is there a potential time to wait for the detachment to happen?

}
func testAccCheckAWSIoTPolicyAttachmentDestroy_basic(s *terraform.State) error {
return nil

This comment has been minimized.

@stack72

stack72 Dec 5, 2016

Contributor

why have we got a func that returns nil?

@stack72

stack72 Dec 5, 2016

Contributor

why have we got a func that returns nil?

This comment has been minimized.

@jhedev

jhedev Apr 24, 2017

The problem here is that I'm not sure how to test if the attachment was destroyed and just copied this idea from the resource_aws_iam_policy_attachment_test.go which does the same:

func testAccCheckAWSPolicyAttachmentDestroy(s *terraform.State) error {
	return nil
}
@jhedev

jhedev Apr 24, 2017

The problem here is that I'm not sure how to test if the attachment was destroyed and just copied this idea from the resource_aws_iam_policy_attachment_test.go which does the same:

func testAccCheckAWSPolicyAttachmentDestroy(s *terraform.State) error {
	return nil
}
@fnouama

This comment has been minimized.

Show comment
Hide comment
@fnouama

fnouama Jan 12, 2017

👍 Looking forward to have this feature available through terraform

fnouama commented Jan 12, 2017

👍 Looking forward to have this feature available through terraform

@KoenR3

This comment has been minimized.

Show comment
Hide comment
@KoenR3

KoenR3 Feb 21, 2017

It is one of the downsides we currently have with terraform that we cannot provision IoT. I hope this feature will be available soon!

KoenR3 commented Feb 21, 2017

It is one of the downsides we currently have with terraform that we cannot provision IoT. I hope this feature will be available soon!

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Feb 22, 2017

@KoenR3 Same goes for me. Unfortunately I was pretty short on time the last couple months and couldn't make the requested changes. I hope to find some time in the next two weeks to make the required changes. So we can get this merged eventually :)

jhedev commented Feb 22, 2017

@KoenR3 Same goes for me. Unfortunately I was pretty short on time the last couple months and couldn't make the requested changes. I hope to find some time in the next two weeks to make the required changes. So we can get this merged eventually :)

@vrcsix

This comment has been minimized.

Show comment
Hide comment
@vrcsix

vrcsix Feb 27, 2017

Contributor

Cool @jhedev. I'm working on covering some of the more recent IoT functionality not covered by this PR, such as thing types and BYOC. I'll submit a separate PR for these. For now though, I'd like to suggest a small API change to aws_iot_certificate - namely, replacing the boolean attribute active with an enum status (ACTIVE, INACTIVE). This is because a certificate can also (irreversibly) be marked as REVOKED. Feel free to do as you wish w.r.t. actually implementing certificate revocation in this PR, but this API change will at least make adding this functionality in the future a bit smoother.

Contributor

vrcsix commented Feb 27, 2017

Cool @jhedev. I'm working on covering some of the more recent IoT functionality not covered by this PR, such as thing types and BYOC. I'll submit a separate PR for these. For now though, I'd like to suggest a small API change to aws_iot_certificate - namely, replacing the boolean attribute active with an enum status (ACTIVE, INACTIVE). This is because a certificate can also (irreversibly) be marked as REVOKED. Feel free to do as you wish w.r.t. actually implementing certificate revocation in this PR, but this API change will at least make adding this functionality in the future a bit smoother.

@alessandroben

This comment has been minimized.

Show comment
Hide comment
@alessandroben

alessandroben Mar 7, 2017

Hi guys, do you have any news about that? I'm looking to implement aws_iot on my terraform project and I'm stuck since there is no support at the moment :(
Let me know about the ETA of this feature, thanks you very much.

alessandroben commented Mar 7, 2017

Hi guys, do you have any news about that? I'm looking to implement aws_iot on my terraform project and I'm stuck since there is no support at the moment :(
Let me know about the ETA of this feature, thanks you very much.

@ajlanghorn

This comment has been minimized.

Show comment
Hide comment
@ajlanghorn

ajlanghorn Mar 10, 2017

Contributor

IoT resources in Terraform would be awesome! Kudos for the great work on this so far.
Let me know if you need any help; happy to contribute, if I can!

Contributor

ajlanghorn commented Mar 10, 2017

IoT resources in Terraform would be awesome! Kudos for the great work on this so far.
Let me know if you need any help; happy to contribute, if I can!

@theherk

This comment has been minimized.

Show comment
Hide comment
@theherk

theherk Apr 12, 2017

Contributor

@jhedev, you are doing a great thing for many of us. If there were anything I could do to help I would, but you're in the driver's seat. How far do you believe you are from this being ready for merge. I want very much not to write a CloudFormation template for our IoT bits.

Contributor

theherk commented Apr 12, 2017

@jhedev, you are doing a great thing for many of us. If there were anything I could do to help I would, but you're in the driver's seat. How far do you believe you are from this being ready for merge. I want very much not to write a CloudFormation template for our IoT bits.

@stephencoe

This comment has been minimized.

Show comment
Hide comment
@stephencoe

stephencoe Apr 20, 2017

Contributor

@jhedev thanks for the great work on this so far! I have been doing some testing and running your branch to build IOT within AWS. Everything has been great up to this point, I found some small inconsistencies with the dynamodb topic rule

Missing fields hash_key_type & range_key_type. These are specified in the resource doc but were missed in the go file.

The payload field shouldn't be Required. In the console / cli you are able to create this without applying the field. Having it required and setting it to an empty string causes the rule to error when writing the the stream (this part is a strange behaviour in AWS)

example iot_rule.json:

{
    "sql": "<QUERY>",
    "ruleDisabled": false,
    "actions": [{
        "dynamoDB": {
            "tableName": "<TABLE>",
            "roleArn": "<ROLE ARN>",
            "hashKeyField": "unique_id",
            "hashKeyValue": "${id}",
            "rangeKeyField": "timestamp",
            "rangeKeyValue": "${timestamp()}"
        }
    }]
}

aws iot create-topic-rule --rule-name dynamo_rule_test --topic-rule-payload file://iot_rule.json

Let us know if there is anything we can do to help

Contributor

stephencoe commented Apr 20, 2017

@jhedev thanks for the great work on this so far! I have been doing some testing and running your branch to build IOT within AWS. Everything has been great up to this point, I found some small inconsistencies with the dynamodb topic rule

Missing fields hash_key_type & range_key_type. These are specified in the resource doc but were missed in the go file.

The payload field shouldn't be Required. In the console / cli you are able to create this without applying the field. Having it required and setting it to an empty string causes the rule to error when writing the the stream (this part is a strange behaviour in AWS)

example iot_rule.json:

{
    "sql": "<QUERY>",
    "ruleDisabled": false,
    "actions": [{
        "dynamoDB": {
            "tableName": "<TABLE>",
            "roleArn": "<ROLE ARN>",
            "hashKeyField": "unique_id",
            "hashKeyValue": "${id}",
            "rangeKeyField": "timestamp",
            "rangeKeyValue": "${timestamp()}"
        }
    }]
}

aws iot create-topic-rule --rule-name dynamo_rule_test --topic-rule-payload file://iot_rule.json

Let us know if there is anything we can do to help

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Apr 20, 2017

@stephencoe Thanks for testing! That helps a lot.

Sorry for being so quiet lately. I didn't have much time to work on this but hope to find some time to work on it during the following weekend.

Reviewing code and testing is always very much appreciated :)

jhedev commented Apr 20, 2017

@stephencoe Thanks for testing! That helps a lot.

Sorry for being so quiet lately. I didn't have much time to work on this but hope to find some time to work on it during the following weekend.

Reviewing code and testing is always very much appreciated :)

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Apr 24, 2017

I made some of the changes mentioned above and rebased on master again. However, there are still a few things that need to be change but I'm not sure how. So it would be great to get some input how these things are handled in terraform and if possible a pointer to some source code where this is done:

aws_iot_policy_attachment

  • Eventual consistency issue when attaching principals? see discussion
  • Wait some time when detaching? see discussion

aws_iot_thing

  • Same issue as above when attaching principals?
  • Same issue as above when detaching principals?

jhedev commented Apr 24, 2017

I made some of the changes mentioned above and rebased on master again. However, there are still a few things that need to be change but I'm not sure how. So it would be great to get some input how these things are handled in terraform and if possible a pointer to some source code where this is done:

aws_iot_policy_attachment

  • Eventual consistency issue when attaching principals? see discussion
  • Wait some time when detaching? see discussion

aws_iot_thing

  • Same issue as above when attaching principals?
  • Same issue as above when detaching principals?
@stephencoe

This comment has been minimized.

Show comment
Hide comment
@stephencoe

stephencoe Apr 25, 2017

Contributor

@jhedev this is awesome! I don't have a great deal of experience with the code base but looking at other examples where this has been implemented it looks like it's wrapped in a resource.Retry.

Some examples I found of this were in lambda_function, api_gateway_account, aws_launch_configuration.

Contributor

stephencoe commented Apr 25, 2017

@jhedev this is awesome! I don't have a great deal of experience with the code base but looking at other examples where this has been implemented it looks like it's wrapped in a resource.Retry.

Some examples I found of this were in lambda_function, api_gateway_account, aws_launch_configuration.

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Apr 25, 2017

@stephencoe Thanks for your help. I'll have a look at the implementations.

jhedev commented Apr 25, 2017

@stephencoe Thanks for your help. I'll have a look at the implementations.

@rob-smallshire

This comment has been minimized.

Show comment
Hide comment
@rob-smallshire

rob-smallshire Jun 22, 2017

Is this being worked on currently by anybody? No activity for a few months now...

rob-smallshire commented Jun 22, 2017

Is this being worked on currently by anybody? No activity for a few months now...

@rob-smallshire rob-smallshire referenced this pull request Jun 23, 2017

Open

AWS IoT support #143

4 of 7 tasks complete
@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Jun 23, 2017

Hey @rob-smallshire, I always planned to get this finished at some point. However, my free time is very rare at the moment and since I don't use AWS IoT at work anymore there is very little motivation to get this done.

Long story short: I do not work on it actively at the moment and don't think I will in the near future.

Feel free to take over and use any of my code if you like :)

jhedev commented Jun 23, 2017

Hey @rob-smallshire, I always planned to get this finished at some point. However, my free time is very rare at the moment and since I don't use AWS IoT at work anymore there is very little motivation to get this done.

Long story short: I do not work on it actively at the moment and don't think I will in the near future.

Feel free to take over and use any of my code if you like :)

@rob-smallshire

This comment has been minimized.

Show comment
Hide comment
@rob-smallshire

rob-smallshire Jun 23, 2017

@jhedev Thanks for a prompt response. It's good to have some clarity on the status of this feature. I'm still evaluating AWS IoT for my application, but if it works out for us I can dedicate some effort towards resurrecting your code. It looks like your were pretty close!

rob-smallshire commented Jun 23, 2017

@jhedev Thanks for a prompt response. It's good to have some clarity on the status of this feature. I'm still evaluating AWS IoT for my application, but if it works out for us I can dedicate some effort towards resurrecting your code. It looks like your were pretty close!

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Aug 8, 2017

I guess we should close this PR now?

jhedev commented Aug 8, 2017

I guess we should close this PR now?

@rob-smallshire

This comment has been minimized.

Show comment
Hide comment

rob-smallshire commented Aug 8, 2017

@jhedev

This comment has been minimized.

Show comment
Hide comment
@jhedev

jhedev Aug 8, 2017

Yeah, I've seen that. Thanks for taking over :)

jhedev commented Aug 8, 2017

Yeah, I've seen that. Thanks for taking over :)

@jhedev jhedev closed this Aug 8, 2017

@abingham abingham referenced this pull request Oct 11, 2017

Merged

New Resource: aws_iot_topic_rule #1858

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment