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.

Sign up for GitHub

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

Jump to bottom

Added WAF ACL Resource #8852

Merged
merged 1 commit into from
Oct 27, 2016
Merged

Added WAF ACL Resource #8852

merged 1 commit into from
Oct 27, 2016

Conversation

sharmaansh21
Copy link
Contributor

No description provided.

jen20
jen20 suggested changes Sep 15, 2016
View reviewed changes
Copy link
Contributor

@jen20 jen20 left a comment

Choose a reason for hiding this comment

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

Hi @optimisticanshul! This looks like a good start, but there are a couple of review comments inline. In addition, we'll need to add a couple of acceptance tests and documentation prior to being able to merge this - but I notice you have it marked as a WIP so this may be in progress. Thanks for opening a pull request early in your work!

builtin/providers/aws/resource_aws_waf_web_acl.go

// ChangeToken
var ct *waf.GetChangeTokenInput
res, _ := conn.GetChangeToken(ct)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this fail? We should probably not be ignoring this error.

builtin/providers/aws/resource_aws_waf_web_acl.go
conn := meta.(*AWSClient).wafconn
// ChangeToken
var ct *waf.GetChangeTokenInput
resp, err := conn.GetChangeToken(ct)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we assign this error we should also check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

builtin/providers/aws/resource_aws_waf_web_acl.go
log.Printf("[INFO] Deleting WAF ACL")
_, err = conn.DeleteWebACL(req)

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should likely test here for whether the return code is a 404-equivalent and remove from state rather than erroring out if that is the case - this allows for us to track ACLs removed via the console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in read func

@sharmaansh21 sharmaansh21 force-pushed the waf-web-acl-resource branch 2 times, most recently from 1049e69 to 33283da Compare September 15, 2016 19:49
@sharmaansh21
Copy link
Contributor Author

@jen20 @radeksimko Ready for the review. I will squash the commits once review is done.

@sharmaansh21 sharmaansh21 changed the title [WIP] Added WAF ACL Resource Added WAF ACL Resource Sep 16, 2016
@sharmaansh21
Copy link
Contributor Author

@stack72 bump

stack72
stack72 suggested changes Sep 30, 2016
View reviewed changes
Copy link
Contributor

@stack72 stack72 left a comment

Choose a reason for hiding this comment

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

@optimisticanshul I have left a more indepth review of these now - sorry it took me so long. You can see the pattern I am following, please can you apply the same pattern to all of the resources (i only reviewed 2)

builtin/providers/aws/resource_aws_waf_ipset.go

// ChangeToken
var ct *waf.GetChangeTokenInput
time.Sleep(10 * time.Second) // To Prevent WAFStaleDataException
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe we should do this - what is the WAFStateDataException?

Copy link
Contributor Author

@sharmaansh21 sharmaansh21 Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@stack72 if i don't wait for sometime i get this exception because of ChangeToken: res.ChangeToken i am not sure how to handle in a better way.

WAFStaleDataException: The operation failed because you tried to create, update, or delete an object by using a change token that has already been used.

builtin/providers/aws/resource_aws_waf_ipset.go
if err != nil {
return err
}
d.SetId(*resp.IPSet.IPSetId)
Copy link
Contributor

Choose a reason for hiding this comment

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

If there a chance that resp.IPSet can be nil? I think it would be worth checking for the nil response here to prevent any potential pointer deference errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think so it will be nil. Until/unless there is an error but in that case error is already handled

if err != nil {
    return err
 }

builtin/providers/aws/resource_aws_waf_ipset.go

for _, IPSetDescriptor := range resp.IPSet.IPSetDescriptors {
IPSet := map[string]interface{}{
"type": *IPSetDescriptor.Type,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we guaranteed to have both of these values each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

builtin/providers/aws/resource_aws_waf_ipset.go
}

func updateIPSetResource(d *schema.ResourceData, meta interface{}, ChangeAction string) error {
conn := meta.(*AWSClient).wafconn
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should only do this when we detect d.HasChange("ip_set_descriptors")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we can't reason behind it is because this method is used for deleting as well as updating and in deletion case there will be no change in ip_set_descriptors.

builtin/providers/aws/resource_aws_waf_ipset.go

// ChangeToken
var ct *waf.GetChangeTokenInput
time.Sleep(5 * time.Second) // To Prevent WAFStaleDataException
Copy link
Contributor

Choose a reason for hiding this comment

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

Again - we need to work out a better way of doing this than sleep

builtin/providers/aws/resource_aws_waf_rule.go
if err != nil {
return err
}
d.SetId(*resp.Rule.RuleId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance Rule can be nil? If so, we should guard against this being a dereferencing error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already handle in that in err.

builtin/providers/aws/resource_aws_waf_rule.go
}

return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check for Rule being nil here before using it

builtin/providers/aws/resource_aws_waf_rule.go
}
// ChangeToken
var ct *waf.GetChangeTokenInput
time.Sleep(5 * time.Second) // To Prevent WAFStaleDataException
Copy link
Contributor

Choose a reason for hiding this comment

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

No sleep please :)

builtin/providers/aws/resource_aws_waf_rule.go

// ChangeToken
var ct *waf.GetChangeTokenInput
time.Sleep(5 * time.Second) // To Prevent WAFStaleDataException
Copy link
Contributor

Choose a reason for hiding this comment

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

No Sleep please :)

builtin/providers/aws/resource_aws_waf_rule.go
return nil
}

func updateWafRuleResource(d *schema.ResourceData, meta interface{}, ChangeAction string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check for change in the parameter before making a call to the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To delete a rule we need to remove related dependencies. updateWafRuleResource is used for updating the resource as well as in deleting the resource. So we can't check for change.

@stack72
Copy link
Contributor

stack72 commented Sep 30, 2016

@optimisticanshul thanks for this work so far - I feel we need to do a little more work here :)

@stack72 stack72 added the waiting-response An issue/pull request is waiting for a response from the community label Sep 30, 2016
@sharmaansh21
Copy link
Contributor Author

@stack72 sure i will update 👍

@stack72
Copy link
Contributor

stack72 commented Oct 7, 2016

ping @optimisticanshul :)

@sharmaansh21
Copy link
Contributor Author

sharmaansh21 commented Oct 8, 2016 •
edited
Loading

@stack72 I added/removed things that you asked wherever required, so now its ready for another review.

➜  terraform git:(waf-web-acl-resource) make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSWafWebAcl_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/09 00:17:36 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSWafWebAcl_ -timeout 120m
=== RUN   TestAccAWSWafWebAcl_basic
--- PASS: TestAccAWSWafWebAcl_basic (80.84s)
=== RUN   TestAccAWSWafWebAcl_disappears
--- PASS: TestAccAWSWafWebAcl_disappears (73.24s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    154.435s
➜  terraform git:(waf-web-acl-resource) make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSWafIPSet_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/09 00:21:08 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSWafIPSet_ -timeout 120m
=== RUN   TestAccAWSWafIPSet_basic
--- PASS: TestAccAWSWafIPSet_basic (39.85s)
=== RUN   TestAccAWSWafIPSet_disappears
--- PASS: TestAccAWSWafIPSet_disappears (38.94s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    79.205s
➜  terraform git:(waf-web-acl-resource) make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSWafRule_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/09 00:23:17 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSWafRule_ -timeout 120m
=== RUN   TestAccAWSWafRule_basic
--- PASS: TestAccAWSWafRule_basic (59.82s)
=== RUN   TestAccAWSWafRule_disappears
--- PASS: TestAccAWSWafRule_disappears (54.15s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    114.314s

@stack72
Copy link
Contributor

stack72 commented Oct 26, 2016

Hi @optimisticanshul

Sorry i didn't get back to you on this sooner - i have been away. So the changes you had made look good. I think it needs 1 more thing - a test in each resource for Update. I can't see any testing of the Update funcs. Can you add something there?

P.

@sharmaansh21
Copy link
Contributor Author

Sure

On Wed 26 Oct, 2016, 2:30 PM Paul Stack, [email protected] wrote:

Hi @optimisticanshul https://github.com/optimisticanshul

Sorry i didn't get back to you on this sooner - i have been away. So the
changes you had made look good. I think it needs 1 more thing - a test in
each resource for Update. I can't see any testing of the Update funcs. Can
you add something there?

P.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8852 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEViYYx6fp4gnKi53RXmKMsAsGATOCGQks5q3xaigaJpZM4J9uFx
.

Sent From Handheld Device, Ignore Typos.

@sharmaansh21
Copy link
Contributor Author

@stack72 Hi ready for another review added update test cases 👍

@stack72
Copy link
Contributor

stack72 commented Oct 27, 2016

@optimisticanshul this now LGTM! The tests are green :) I thank you for all the work here!

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSWaf'                                                                    ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/27 12:47:31 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSWaf -timeout 120m
=== RUN   TestAccAWSWafIPSet_basic
--- PASS: TestAccAWSWafIPSet_basic (20.35s)
=== RUN   TestAccAWSWafIPSet_disappears
--- PASS: TestAccAWSWafIPSet_disappears (19.28s)
=== RUN   TestAccAWSWafIPSet_changeNameForceNew
--- PASS: TestAccAWSWafIPSet_changeNameForceNew (36.49s)
=== RUN   TestAccAWSWafRule_basic
--- PASS: TestAccAWSWafRule_basic (28.66s)
=== RUN   TestAccAWSWafRule_changeNameForceNew
--- PASS: TestAccAWSWafRule_changeNameForceNew (51.76s)
=== RUN   TestAccAWSWafRule_disappears
--- PASS: TestAccAWSWafRule_disappears (27.13s)
=== RUN   TestAccAWSWafWebAcl_basic
--- PASS: TestAccAWSWafWebAcl_basic (37.73s)
=== RUN   TestAccAWSWafWebAcl_changeNameForceNew
--- PASS: TestAccAWSWafWebAcl_changeNameForceNew (69.12s)
=== RUN   TestAccAWSWafWebAcl_changeDefaultAction
--- PASS: TestAccAWSWafWebAcl_changeDefaultAction (68.67s)
=== RUN   TestAccAWSWafWebAcl_disappears
--- PASS: TestAccAWSWafWebAcl_disappears (35.67s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    394.881s

@stack72 stack72 merged commit bc42229 into hashicorp:master Oct 27, 2016
@stack72 stack72 mentioned this pull request Oct 27, 2016
Closed
8 tasks
@sharmaansh21 sharmaansh21 deleted the waf-web-acl-resource branch October 28, 2016 05:16
mathieuherbert pushed a commit to mathieuherbert/terraform that referenced this pull request Oct 30, 2016
@sharmaansh21
Added WAF ACL Resource (hashicorp#8852)
ca4ba7c
gusmat pushed a commit to gusmat/terraform that referenced this pull request Dec 6, 2016
@sharmaansh21
Added WAF ACL Resource (hashicorp#8852)
de40efd
@ghost
Copy link

ghost commented Apr 21, 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 Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stack72 stack72 stack72 requested changes

@jen20 jen20 jen20 requested changes

Assignees
No one assigned
Labels
enhancement provider/aws waiting-response An issue/pull request is waiting for a response from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sharmaansh21 @stack72 @jen20