feat!: Update audit_config submodule to support multiple log types #108
Conversation
Update variable type definition to support multiple log types.
Use dynamic block on the resource.
|
@agnnn is it necessary to have each service only listed once? I'm wondering if the existing implementation would work with a config like this: |
|
@morgante So basically... each resource is authoritative and not additive. When we have log_type for a single service defined on multiple resources, it will apply only one configuration. Every time doing a terraform apply.. it goes into a loop applying changes for that particular service that got modified due to the last terraform apply. |
|
Got it, thanks. I think we could actually accomplish this without changing the module's variable interface, it will just require some more logic in the module. The logic would be something like:
I'd like to take the above approach for 2 reasons:
|
re: 1. - It is a breaking change, but on the other hand, the current implementation is broken. /the peanut gallery :) |
The current implementation isn't broken. It works fine for users who only want 1 consistent config—this PR is an enhancement, not a bug fix. |
Arguably, and I am not arguing particularly hard here, the initial implementation assumes an additive resource vs an authoritative one. Since the underlying resource supports multiple log types per service, and since the current implementation can't configure more than one log type per service, I consider it broken for a (not so arguably, since each service supports 3 different audit configs) very common use case. Functionally, the important part is addition of the dynamic block, and this is why I'm not going to argue too hard. I will note that the current implementation is released and can be pinned to preserve the simpler use case. |
|
@morgante Any updates on this ?? Or do you still wanna go with the approach you suggested earlier ?? |
|
@agnnn Yes, I'd like this to be updated to the approach I suggested (which will be backwards-compatible). |
|
So, I have done some investigation on how this could be implemented, and the conversion works with this pattern: ...resulting in the desired input format for the new implementation: |
|
@kpeder my understanding is that you can go ahead with your approach. Since you keep the variable the same and it still works with just one item, we are good. Feel free to tag me before asking for Morgante's final review. An additional note is, as you probably noticed, the permissions are not authoritative. It is a side effect because of the for loop overwriting the key. |
|
@morgante Hey, so I have updated the PR based on your requested changes and it is almost ready to be reviewed except for the failing check, could you inspect the logs and suggest what is the error on lint ?? |
|
hi @agnnn let me see if I can reproduce the error. |
There was a problem hiding this comment.
Thanks for the PR @agnnn
Please run make generate_docs to regenerate docs. You can also run make docker_test_lint to test link check locally.
Errors from CI
Checking for documentation generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=.git' '--exclude=autogen' '--exclude=*.tfvars' /workspace/modules/audit_config/README.md /tmp/tmp.K5k2UROctV/generate_docs/workspace/modules/audit_config/README.md
49,51c49,50
< | audit_log_config | List of objects to be added to audit log config | list(object({ service : string, log_type : string, exempted_members : list(string) })) | n/a | yes |
< | project | GCP Project ID | string | n/a | yes |
<
---
> | audit\_log\_config | List of objects to be added to audit log config | object | n/a | yes |
> | project | Project to add the IAM policies/bindings | string | n/a | yes |
57,58c56
< | audit_log_config | Map of log type and exempted members added to service |
<
---
> | audit\_log\_config | Map of log type and exempted members to be added to service |
Error: Documentation generation has not been run, please run the
'make docker_generate_docs' command and commit the above changes.
Checking for trailing whitespace
./modules/audit_config/README.md:11:
./modules/audit_config/README.md:14: service = "pubsub.googleapis.com"
./modules/audit_config/README.md:23: service = "storage.googleapis.com"
./modules/audit_config/README.md:32: service = "pubsub.googleapis.com"
Error: Trailing whitespace found in the lines above.
Checking for missing newline at end of file
Running shellcheck
Checking file headers
Running flake8
Running terraform fmt
Running terraform validate
terraform_validate ./examples/billing_account
Success! The configuration is valid.
|
@agnnn seems to be an issue in the CI, not related to your PR. Everything passes locally on my machine. Meanwhile, I'll try to help and give it my review. |
🤦 I ran tests from the master branch. My bad. |
|
@bharathkkb It looks good to me (just need to run make docker_generate_docs). I deployed with the latest release and then applied this branch and the plan shows no change as expected. (Sorry for previous mistakes. I'll do better on paying attention to the details) |
|
@bharathkkb Thanks for your quick reply. So I did run Everything looks good on my local system. But on the PR it is still failing the check. Any suggestions ?? |
Co-authored-by: Bharath KKB <bharathkrishnakb@gmail.com>
|
@bharathkkb Done, All checks are passing now. This Looks good to go for a review. |
There was a problem hiding this comment.
Overall LGTM
Since the keys are changing there will be a delete recreate which I think is okay for a minor release.
/cc @morgante
bharathkkb
changed the title
|
@bharathkkb the key is not changing. Just the logic.
…Sent from my iPhone
On 6 Nov 2020, at 19:34, Bharath KKB ***@***.***> wrote:
@bharathkkb approved this pull request.
Overall LGTM
Since the keys are changing there will be a delete recreate which I think is okay for a minor release.
/cc @morgante
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@thiagonache IIUC the change - key => val
+ val.service => val...will change the key from an array index like |
My understanding is since the value is variadic, the state won't change if you don't change the input. And I actually tested it out, if the input is the same there's no changes on the terraform plan. |
|
@thiagonache I just tested it and it produced a diff with the new keys; for instance destroy full diff: https://gist.github.com/bharathkkb/3f48d70a015c5c76ebc751259b601493 {
service = "storage.googleapis.com"
log_type = "DATA_READ"
exempted_members = ["serviceAccount:ci-iam-member-0-5da1@project-id.iam.gserviceaccount.com"]
},
{
service = "allServices"
log_type = "DATA_READ"
exempted_members = ["serviceAccount:ci-iam-member-0-5da1@project-id.iam.gserviceaccount.com"]
} |
@bharathkkb Idk what I've done and how you still don't send me to hell and stop bothering you. I'm bad at reviews :( I wanna do it more to get better, I hope you don't mind. |
|
@thiagonache np :) Regarding your modifications, I am still not sure why we need to keep the array indices as keys. Doing so will create unnecessary diffs if for instance I modify the order of the input list of objects? |
|
@bharathkkb Because I use the keys to group configs but keep the state as list by getting distinct apis and loop over it. It was suggested by Morgante |
|
@thiagonache ack, IIUC having the |
|
That was the initial idea but @morgante didn’t like it.
I would suggest to change it on the next major change and apply the feature now with the logic in my patch.
… On 7 Nov 2020, at 21:14, Bharath KKB ***@***.***> wrote:
@thiagonache ack, IIUC having the local.api_services as index => service serves mostly as a way to prevent that delete recreate. IMHO we should just delete recreate and transition to using services as keys as it would still keep the interface same and added benefit of not being dependent on order of objects in audit_log_config.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
The real point of the update is around ensuring more than one configuration can be applied per service api. I would suggest to update test inputs and assertions to ensure multiple log type configuration is happening properly. IMO the destroy/recreate isn't a big issue... even on destroy its only a configuration update under the hood; no resource is being removed. |
|
Well, it may be a big deal to be without audit in production even for a short time. If we need to force recreate it should be a breaking change
…Sent from my iPhone
On 7 Nov 2020, at 23:05, Kevin Pedersen ***@***.***> wrote:
The real point if the update is around ensuring more than one configuration can be applied per service api.
I would suggest to update test inputs and assertions to configure multiple log type configuration is happening properly.
IMO the destroy/recreate isn't a big issue... even on destroy its only a configuration update under the hood; no resource is being removed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
As a module user, I wouldn't be happy with my resources being recreated in a minor release upgrade |
@kpeder @bharathkkb this is why I coded my patch. |
|
Hey @bharathkkb from the above comments from @kpeder and @thiagonache what is your suggestion, if you're planning to do a major release, I assume it should be okay to recreate resources. |
I would endorse Thiago's patch; and open an issue to track and add the breaking change as is here on next major release? |
|
Historically we have done minor releases for some safe delete recreates like IAM. Ack that even a short time without audit is not ideal in prod so we can do a major release too. |
bharathkkb
changed the title
|
I'm fine with making this a breaking change if we think the delete/recreate is a major issue (even though it's only for a tiny amount of time). Can we add some test cases though? I'm really not sure if this implementation works the way I was looking for. |
Update modules/audit_config to support multiple log types for a specified service while defining audit logs config.
Make use of dynamic block on resource deployment.
Fixes: