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.

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

GKE Autopilot module: add network tags #1675

Conversation

olliefr
Copy link
Collaborator

@olliefr olliefr commented Sep 14, 2023

This PR adds support for network tags to gke-cluster-autopilot module.

There was a nullable input variable tags defined before I started but for some reason it was not used by the module.

In addition to making use of the variable, I made it non-nullable and added validation as well.

Potential impact area

Estimated risk: minimal.

  • I did not change the name or the type of the variable.
  • It was previously declared but was not being used so there's unlikely to be much code out there that depends on it.

Feedback wanted

I'm still new to Fabric so please don't hesitate to tell me if my validation is excessive 🥲 and I will trim it.

Another question I have is about node_pool_auto_config block in google_container_cluster. The Terraform resource docs say it's in "beta" although I can see the corresponding JSON block not only in v1beta API but also in v1. Is it just the case of Terraform Google provider lagging behind the release cadence of the API or am I missing something more fundamental here? From a practical point of view, this module was already using google-beta provider so it's not really an issue, I'm just curious.

Lastly, I have a FIXME comment in the code where I had to use a dynamic block to work around a glitch in diff-ing the empty list of network tags. My understanding is that the API does allow an empty list but Terraform somehow can't cope with it and considers it to be a diff. My question is should I proceed with the dynamic block or should I try to report this upstream into the provider so that this unnecessary empty diff does not happen?

I hope this makes sense. Any other feedback is welcome as well... :shipit:

Checklist

I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

@olliefr
Copy link
Collaborator Author

olliefr commented Sep 14, 2023

Oh, one last thing!

I do wonder if the input variable should be called network_tags rather than tags to avoid confusion with Tags? In my line of work I often come across people who are confused by this dual use of the term "tag" on Google Cloud so I thought maybe it's an opportunity to make it a bit easier for them. Please let me know what you think...

@olliefr
Copy link
Collaborator Author

olliefr commented Sep 14, 2023

Spotted a few references to GKE Standard clusters in the README. Fixed that and expanded Backup for GKE example section. Added a TOC as well as the README is getting longer

Removed a reference to Standard clusters and updated the section to include a warning because the new versions of Autopilot clusters can only use Cloud DNS and it is pre-configured by default so the example in the README does not apply to them.
@olliefr olliefr requested review from juliocc and ludoo September 14, 2023 06:03
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

Very nice, thanks for this. A couple areas are a bit overkill vs our traditional approach. README changes are great!

modules/gke-cluster-autopilot/README.md Outdated Show resolved Hide resolved
modules/gke-cluster-autopilot/variables.tf Outdated Show resolved Hide resolved
modules/gke-cluster-autopilot/variables.tf Outdated Show resolved Hide resolved
@olliefr
Copy link
Collaborator Author

olliefr commented Sep 14, 2023

Added a tftest because I don't think that network tags warrant their own example in README.

Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

thanks :)

@ludoo ludoo enabled auto-merge (squash) September 14, 2023 07:11
@ludoo ludoo removed the request for review from juliocc September 14, 2023 07:11
@ludoo ludoo merged commit 05c0195 into GoogleCloudPlatform:master Sep 14, 2023
@olliefr olliefr deleted the olliefr/gke-cluster-autopilot-validate-network-tags branch September 14, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants