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

feat: new lacework_query resource #266

Merged
merged 12 commits into from
Feb 23, 2022
Merged

feat: new lacework_query resource #266

merged 12 commits into from
Feb 23, 2022

Conversation

dmurray-lacework
Copy link
Collaborator

@dmurray-lacework dmurray-lacework commented Feb 14, 2022 •
edited
Loading

Issue: https://lacework.atlassian.net/browse/ALLY-870

Description:
Add new resource lacework_query

Usage:

 resource "lacework_query" "example" {
    query_id       = var.query_id
    evaluator_id   = "Cloudtrail"
    query          = var.query
  }

@dmurray-lacework
feat: new lacework_query resource
564b3dd 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
fix: query update
8a053f5 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
refactor: update example query
3def356 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
test: add update test to query terratest
7f0e2e8 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
hazedav
hazedav reviewed Feb 16, 2022
View reviewed changes
examples/resource_lacework_query/main.tf Outdated
resource "lacework_query" "example" {
query_id = var.query_id
evaluator_id = "Cloudtrail"
query = var.query
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. should we call this query_text to be consistent with Lacework APIs and SDK structs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to change it. I based the naming on @afiune's design doc. @afiune do you have any thoughts?
I think either would make sense to a user.

hazedav
hazedav approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@hazedav hazedav left a comment

Choose a reason for hiding this comment

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

Approved w/ comments.

afiune
afiune suggested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

Unfortunately, when we do not specify an evaluator ID, the platform returns the string "<<IMPLICIT>>" which causes a constant drift.

Screen Shot 2022-02-16 at 11 50 57 PM

How do we address this @dmurray-lacework @hazedav ?

@dmurray-lacework
Copy link
Collaborator Author

dmurray-lacework commented Feb 17, 2022 •
edited
Loading

Unfortunately, when we do not specify an evaluator ID, the platform returns the string "<<IMPLICIT>>" which causes a constant drift.

Screen Shot 2022-02-16 at 11 50 57 PM

How do we address this @dmurray-lacework @hazedav ?

We could set a default value of "<//<IMPLCIT//>>" in the schema.
Or a diffsupressfunc
Or check if the response is "<//<IMPLCIT//>>" and not set state on read

I'll take a look at this today

@dmurray-lacework
fix: suppressdiff if evalID is empty or <<IMPLICIT>>
c57845b 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
fix: suppressdiff if evalID is <<IMPLICIT>>
62fafd7 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
style: formatting
825b494 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
style: formatting
386a82c 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
afiune
afiune suggested changes Feb 17, 2022
View reviewed changes
lacework/resource_lacework_query.go Outdated Show resolved Hide resolved
lacework/resource_lacework_query.go Outdated Show resolved Hide resolved
website/docs/r/query.html.markdown Outdated Show resolved Hide resolved
website/docs/r/query.html.markdown Show resolved Hide resolved
@dmurray-lacework
refactor: address code review comments
aedebe3 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework
refactor: return error if query_id is changed on update
8f5aea1 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
afiune
afiune reviewed Feb 22, 2022
View reviewed changes
lacework/resource_lacework_query.go
lacework = meta.(*api.Client)
)

if d.HasChange("query_id") {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do this during validation? why? because then the user can run plan or a partial apply and fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't find a way to check the diff between old and new on validation.

afiune
afiune approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

Two comments but out of that...

tenor-128082851

lacework/resource_lacework_query.go
Optional: true,
Description: "The query evaluator id",
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
return old == "<<IMPLICIT>>" || new == "<<IMPLICIT>>"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that after this we could change the evaluator ID but I also think those are edge cases where the user wants to repurpose a query, so let us ignore this for now. 😅

dmurray-lacework and others added 2 commits February 23, 2022 12:40
@dmurray-lacework
Merge branch 'main' into dmurray-lacework/ALLY-870
17e0118
@dmurray-lacework
style: formatting
82eee3f 
Signed-off-by: Darren Murray <darren.murray@lacework.net>
@dmurray-lacework dmurray-lacework merged commit 37377bb into main Feb 23, 2022
@dmurray-lacework dmurray-lacework deleted the dmurray-lacework/ALLY-870 branch February 23, 2022 15:41
@lacework-releng lacework-releng mentioned this pull request Mar 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hazedav hazedav hazedav approved these changes

@afiune afiune afiune approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmurray-lacework @afiune @hazedav