Missing mandatory update_policy attribute in 3-networks-hub-and-spoke/modules/transitivity/main.tf

mromascanu123 commented 5 months ago

TL;DR

terraform plan fails as per below

[myself@linuxbox 3-networks-hub-and-spoke]# ./tf-wrapper.sh plan shared * TERRAFORM PLAN ***** At environment: envs/shared

Error: Invalid value for input variable

on ../../modules/transitivity/main.tf line 76, in module "migs": 76: update_policy = [

The given value is not suitable for module.base_transitivity.module.migs.var.update_policy declared at ../../../terraform-google-modules/vm/google/modules/mig/variables.tf:100,1-25: element 0: attribute "most_disruptive_allowed_action" is required.

Expected behavior

terraform plan should succeed

The attribute should either be provided or marked as optional in terraform-google-modules/vm/google/modules/mig/variables.tf : variable "update_policy" { ... type = list(object({ ...

Observed behavior

terraform plan fails [myself@linuxbox 3-networks-hub-and-spoke]# ./tf-wrapper.sh plan shared * TERRAFORM PLAN ***** At environment: envs/shared

Error: Invalid value for input variable

on ../../modules/transitivity/main.tf line 76, in module "migs": 76: update_policy = [

The given value is not suitable for module.base_transitivity.module.migs.var.update_policy declared at ../../../terraform-google-modules/vm/google/modules/mig/variables.tf:100,1-25: element 0: attribute "most_disruptive_allowed_action" is required.

Terraform Configuration

N/A

Terraform Version

Terraform v1.6.0
on linux_amd64

Your version of Terraform is out of date! The latest version
is 1.7.5. You can update by downloading from https://www.terraform.io/downloads.html

Additional information

Very easy (tested) fix - simply add the attribute in 3-networks-hub-and-spoke/modules/transitivity/main.tf

update_policy = [ { max_surge_fixed = 4 max_surge_percent = null instance_redistribution_type = "NONE" max_unavailable_fixed = 4 max_unavailable_percent = null min_ready_sec = 180 minimal_action = "RESTART" type = "OPPORTUNISTIC" replacement_method = "SUBSTITUTE" most_disruptive_allowed_action = null <<<=== added }

fmichaelobrien commented 4 months ago

stale bot timer restart - https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/.github/workflows/stale.yml#L21

daniel-cit commented 3 months ago

Thanks for your report of this issue.

The Terraform configuration is valid as validated by terraform validate

The key point is that in the original code in this repository, the mig module is pinned to a version: version = "~> 10.0" https://github.com/terraform-google-modules/terraform-example-foundation/blob/master/3-networks-hub-and-spoke/modules/transitivity/main.tf#L66C1-L68C23

and in this version most_disruptive_allowed_action is not part of the object used in the variable update_policy https://github.com/terraform-google-modules/terraform-google-vm/blob/v10.0.0/modules/mig/variables.tf#L100C1-L114C2

most_disruptive_allowed_action was added in version 11.1.0 https://github.com/terraform-google-modules/terraform-google-vm/blob/v11.1.0/modules/mig/variables.tf#L100C1-L115C2

The version attribute should not be removed from the module usage.

The version for the modules used from the registry is very important to be able to have a code base in which the same results can be reproduced every time the configuration is deployed.

Removing the version atribute can lead to your pipeline failing unexpectedly when a new release for the modules is created.

The safe way to update the modules used in the foundation to use the newest version is to do a step by step approach:

got to the directory with the configuration
run terraform init
run terraform validate to check that the configuration is valid
go to terraform register and find the most recent version of the module
update the code to the new version e.g.: version = "~> 10.0" => version = "~> 11.0"
run terraform init
run terraform validate to check if the configuration is still valid and do the appropriate fixes if needed

The modules use Semantic Versioning for the creation of the versions and Terraform has a rich way to define version constraints

If you need a feature that is in the main branch but is not yet in a publish release it is OK to ask for a new release in the repository for the module

mromascanu123 commented 3 months ago

Thank you for the detailed explanation, however:

I might be wrong but relying on default parameter values, especially when not (at least) documented in the calling module, might lead to nasty surprises, e.g. when the version constraint is along the lines of " version = ">= xy.z" and something changed upstream.
Second (and again I might be wrong) once a particular version of code passes QA and UAT it should get into "code freeze" including the modules which in current approach are coming from the TF registry, in most cases as "latest" version.
Also from a security standpoint, a low-risk-tolerance organization (e.g. government, defense) might not want to be exposed by automatically importing and deploying code from github or TF registry. In theory it's safe until no more... (see the links)
Ideally upon code freeze the vetted and security-scanned versions of the TF modules from the TF registry should be "frozen" i.e. stored in a local, private registry or even together with the TEF code and no longer import on the fly. Especially when deploying in secure environments with limited Internet access.

In this respect (modules part of the distribution) a simple script can automatically adjust the "source" references to point to the local versions of the modules under the folders terraform-google-modules and GoogleCloudPlatform

Here are the scary links: https://howtofix.guide/malware-in-the-github-repositories/ https://www.bleepingcomputer.com/news/security/trojanized-dnspy-app-drops-malware-cocktail-on-researchers-devs/ https://www.itworldcanada.com/article/cyber-security-today-jan-18-2023-data-hacked-of-nissan-owners-a-github-vulnerability-alert-holes-in-gitlab-found-and-more/522553

daniel-cit commented 3 months ago

Hi @mromascanu123 thanks for your reply

I think maybe we are discussing two different issues.

A) The bug opened was:

Missing mandatory update_policy attribute in 3-networks-hub-and-spoke/modules/transitivity/main.tf

Which is a bug reported against the code that is in the public version of the code in this repository and that is not an issue because the code in this repository is using a version of the mig module that does not have the extra parameter.

B) Regarding the code management for a low-risk-tolerance organization, I agree with you 100%.

The Terraform Example Foundation repository should be forked internally in your version control system together with the relevant dependencies so that all necessary changes in the code to meet your specific requirements/constraints can be made, tracked internally, and follow your internal review process.

eeaton commented 3 months ago

Thanks Daniel, I agree with the explanation.

I think an improvement we can make on this repo is to document point B explicitly. The intent for this repo is to be forked and maintained into the user's own version control system, not used as a remote reference. I will add documentation throughout the repo to make it more clear about what we intend to support (the reference code is semantically valid, pinned to versions, and passes CI tests that we know it successfully deploys resources aligned to best practice recommendations) and what we do not (remote reference, support for all versions and customizations that a user might tweak in their own environment).

More context:

We maintain a changelog of major version releases, generated by the releasepleasebot and based on conventionalcommits. This states that there is no in-place upgrade path between major versions.
However, even for minor version changes on this repo, it is not a realistic goal to support in-place upgrade paths for customers who deploy the foundation and then build their own resources on top of it. We assume that each customer makes non-trivial customizations for their own projects, networks, etc. We as repo maintainers don't have visibility on what customization the end user makes, or which resources they deploy on top that will have breaking dependencies on which configuration, etc.
So, the changelog can help somebody identify the new features that have been released after they adopted an earlier version of the repo, but it will be their responsibility to use the references to modify their own code with an awareness of all the ways their custom deployment differs from the public sample.

I'll close this comment, but please re-open if there are further concerns.

mromascanu123 commented 3 months ago

Almost any open source project provides a functional baseline that one can use as is and obviously, if need be, customize. But in most cases there is no need to customize because the consumers audience of such a OSS distro find the baseline functionality more than sufficient. TEF is the exception and it seems the project is geared at development teams rather than end-users. Because as-is a TEF deployment is unusable for real-life use scenarios. This is a major issue for would-be adopters of the Google Landing Zone - very few (and large) organizations have the capacity to re-develop TEF or engage PSO to do it. Most organizations would simply go with another provider's landing zone, be it Azure, AWS or Oracle which have much lower barriers to adoptions and can be used as-is.

Need to think in the short term to adapting TEF to become usable as-is and customizable if needed, via a mechanism of configurability.

@.***

Marian Romascanu Conseiller en architecture technologique infonuagique Ministère de la Cybersécurité et du Numérique 1685, boulevard Wilfrid-Hamel,1er étage, porte 120A, Québec (Québec) G1N 3Y7 Tél. : 418 999-9999 | Téléc. : 418 643-0998 @.**@.>

De : eeaton @.> Envoyé : 16 mai 2024 07:34 À : terraform-google-modules/terraform-example-foundation @.> Cc : Romascanu, Marian (Consultant) @.>; Mention @.> Objet : Re: [terraform-google-modules/terraform-example-foundation] Missing mandatory update_policy attribute in 3-networks-hub-and-spoke/modules/transitivity/main.tf (Issue #1167)

*ATTENTION : Ce courriel provient de l'extérieur de votre organisation. Évitez de cliquer sur un hyperlien, d'ouvrir une pièce jointe ou de transmettre des informations personnelles si vous ne connaissez pas l'expéditeur du courriel. En cas de doute, communiquez verbalement avec lui.

Thanks Daniel, I agree with the explanation.

I think an improvement we can make on this repo is to document point B explicitly. The intent for this repo was always to be forked and maintained into the user's own version control system, not used as a remote reference. I will add documentation throughout the repo to make it more clear about what we intend to support (the reference code is semantically valid, pinned to versions, and passes CI tests that we know it successfully deploys resources aligned to best practice recommendations) and what we do not (remote reference, support for all versions and customizations that a user might tweak in their own environment).

More context:

We maintain a changeloghttps://github.com/terraform-google-modules/terraform-example-foundation/blob/0a69eac4be4cdc05e190d11f9c058f60c9aa3897/docs/upgrading_to_v4.0.md of major version releases, based on convential commits and. This states that there is no in-place upgrade path between major versions.
However, even for minor version changes on this repo, it is not a realistic goal to support in-place upgrade paths for customers who deploy the foundation and then build their own resources on top of it. We assume that each customer makes non-trivial customizations for their own projects, networks, etc. We as repo maintainers don't have visibility on what customization the end user makes, or which resources they deploy on top that will have breaking dependencies on which configuration, etc.

I'll close this comment, but please re-open if there are further concerns.

- Reply to this email directly, view it on GitHubhttps://github.com/terraform-google-modules/terraform-example-foundation/issues/1167#issuecomment-2114985008, or unsubscribehttps://github.com/notifications/unsubscribe-auth/BGZOME5Z4EI7BFT3EMRXNE3ZCSKUDAVCNFSM6AAAAABE4ADWCKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJUHE4DKMBQHA. You are receiving this because you were mentioned.Message ID: @.**@.>>

mromascanu123 commented 3 months ago

Almost any open source project provides a functional baseline that one can use as is and obviously, if need be, customize. But in most cases there is no need to customize because the consumers audience of such a OSS distro find the baseline functionality more than sufficient. TEF is the exception and it seems the project is geared at development teams rather than end-users. Because as-is a TEF deployment is unusable for real-life use scenarios. This is a major issue for would-be adopters of the Google Landing Zone – very few (and large) organizations have the capacity to re-develop TEF or engage PSO to do it. Most organizations would simply go with another provider’s landing zone, be it Azure, AWS or Oracle which have much lower barriers to adoptions and can be used as-is.

Maybe need to think in the short term to adapting TEF to become usable as-is and customizable if needed, and this via a mechanism of configurability making the code immutable and deployment for common use scenarios data-driven e.g. via yaml config files just like AWS LZA.

terraform-google-modules / terraform-example-foundation