Enable Flow Logs deprecated

[joshua9519]

Hi, Not sure if this has been caught with another issue (I had a look but couldn't find anything - apologies if my search was lacking...) The enable_flow_logs setting in Terraform has been deprecated in favour of log_config and therefore when using this module, you get a warning. If this hasn't been captured yet, I'd be happy to help with updating this. :+1: Thanks

[morgante]

Thanks for the catch, looks like you're the first to notice!

I'd say this is what we should do:

1. Make a dynamic log_config block which is only added if enable_flow_logs is true.
2. Add a flow_logs_interval variable
3. Add a flow_logs_sampling variable

We should base this work on the 2.0 branch: https://github.com/terraform-google-modules/terraform-google-network/pull/86

[joshua9519]

Hi @morgante Thanks for the quick reply. According to the warning:

Warning: "enable_flow_logs": [DEPRECATED] This field is being removed in favor of log_config. If log_config is present, flow logs are enabled.

So I think it should be more like a dynamic log_config and we just remove enable_flow_logs? What do you think? Also, could the variable for log_config look like:

variable "log_config" {
  type = object({
    aggregation_interval = string
    flow_sampling = number
    metadata = string
  })
}

and remove the need for the other variables? Let me know your thoughts :) Thanks Josh

[morgante]

@joshua9519 I'd actually like to avoid changing the module interface. Internally, we can generate a dynamic log_config but keep the outside variables the same.

[joshua9519]

Ah okay! I'm happy with that then, just thought I'd offer the suggestion. Would you like to keep enable_flow_logs too and use the logic you described above? My only issue with that is that the warning will still exist as we are still using that key.

[morgante]

Would you like to keep enable_flow_logs too and use the logic you described above? My only issue with that is that the warning will still exist as we are still using that key.

To be clear, we'd keep enable_flow_logs as a variable on the module but not set it on the resource. Instead, enable_flow_logs would drive the creation of the log_config block (and thus the warning would disappear).

[joshua9519]

Ah! Thanks for the clarity! That sounds great. Want me to have a crack at it?

[morgante]

Sure! If you're able to, very happy to look at a PR.

[joshua9519]

Branching off #86?

[morgante]

Ideally yes.

[joshua9519]

Hi @morgante, I'm unfamiliar with the testing setup here. I am following the contributing guide and am at the stage of running make docker_test_prepare and I am getting the following error:

Warning: google_project_services is deprecated - many users reported issues with dependent services that were not resolvable.  Please use google_project_service or the https://github.com/terraform-google-modules/terraform-google-project-factory/tree/master/modules/project_services module.  It's recommended that you use a provider version of 2.13.0 or higher when you migrate so that requests are batched to the API, reducing the request rate. This resource will be removed in version 3.0.0.

  on .terraform/modules/project/terraform-google-modules-terraform-google-project-factory-f93d3cd/modules/core_project_factory/main.tf line 165, in resource "google_project_services" "project_services_authority":
 165: resource "google_project_services" "project_services_authority" {

So I tried upgrading the project_factory module in the test/setup/main.tf to 4.0 and got this error:

Error: Error in function call

  on .terraform/modules/project/terraform-google-modules-terraform-google-project-factory-46e55ae/modules/project_services/outputs.tf line 19, in output "project_id":
  19:   value       = var.enable_apis ? element([for v in google_project_service.project_services : v.project], 0) : var.project_id
    |----------------
    | google_project_service.project_services is object with no attributes

I then tried 5.0 and got the same error as above. Is this a common issue? Is there a fix for it?

[morgante]

The first message is a warning, not an error, so you should actually be able to ignore it.

[joshua9519]

It bombs out after the warning unfortunately...

[morgante]

Interesting. Can you share the full log and what command you used?

[joshua9519]

Sorry for the delay - had a busy day at work. This is what is happening

make docker_test_prepare
Makefile:46: warning: overriding recipe for target 'docker_test_prepare'
Makefile:34: warning: ignoring old recipe for target 'docker_test_prepare'
docker run --rm -it \
        -e SERVICE_ACCOUNT_JSON \
        -e TF_VAR_org_id \
        -e TF_VAR_folder_id \
        -e TF_VAR_billing_account \
        -v /home/josh_hill/Projects/CTS/terraform-google-network:/workspace \
        gcr.io/cloud-foundation-cicd/cft/developer-tools:0.1.0 \
        /usr/local/bin/execute_with_credentials.sh cleanup_environment
Updated property [core/pass_credentials_to_gsutil].
Updated property [core/pass_credentials_to_gsutil].
Initializing modules...

Initializing the backend...

Initializing provider plugins...

The following providers do not have any version constraints in configuration,
so the latest version was installed.

To prevent automatic upgrades to new major versions that may contain breaking
changes, it is recommended to add version = "..." constraints to the
corresponding provider blocks in configuration, with the constraint strings
suggested below.

* provider.google: version = "~> 2.19"
* provider.google-beta: version = "~> 2.19"
* provider.null: version = "~> 2.1"
* provider.random: version = "~> 2.2"

Terraform has been successfully initialized!

You may now begin working with Terraform. Try running "terraform plan" to see
any changes that are required for your infrastructure. All Terraform commands
should now work.

If you ever set or change modules or backend configuration for Terraform,
rerun this command to reinitialize your working directory. If you forget, other
commands will detect it and remind you to do so if necessary.
module.project.module.gsuite_group.data.google_organization.org[0]: Refreshing state...

Warning: google_project_services is deprecated - many users reported issues with dependent services that were not resolvable.  Please use google_project_service or the https://github.com/terraform-google-modules/terraform-google-project-factory/tree/master/modules/project_services module.  It's recommended that you use a provider version of 2.13.0 or higher when you migrate so that requests are batched to the API, reducing the request rate. This resource will be removed in version 3.0.0.

  on .terraform/modules/project/terraform-google-modules-terraform-google-project-factory-f93d3cd/modules/core_project_factory/main.tf line 165, in resource "google_project_services" "project_services_authority":
 165: resource "google_project_services" "project_services_authority" {

Destroy complete! Resources: 0 destroyed.

[joshua9519]

I just realized tat in the Makefile, there are two "docker_test_prepare"s and so when I was running it, it was running a script called cleanup_environment rather than a script called prepare_environment Apologies!

[morgante]

Interesting, was this something unique to your environment? If it's a common bug we should fix it, but looking at the Makefile I'm not quite sure how how docker_test_prepare could trigger the cleanup script.

[joshua9519]

I branched from the 2.0 branch. So this is the Makefile I was working with.

[morgante]

Ah, that explains it. We'll make sure to fix before merging in 2.0.0.

[joshua9519]

Do I need some permissions to write to this repo?

[morgante]

Do I need some permissions to write to this repo?

You should fork and push to your fork/pull request: https://guides.github.com/activities/forking/