support GKE in terraform-mapper with some kind of generated code (terraform-google-conversion)

[yukinying]

Release Note Template for Downstream PRs (will be copied)

Adding GKE support for terraform-google-conversion.

[modular-magician]

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@SirGitsalot, please review this PR or find an appropriate assignee.

[yukinying]

The code has been tested manually in terraform-google-conversion and terraform-validator repository.

I have discussed with @morgante and @rileykarson previously and it is preferred to write terraform-google-conversion code manually rather than backporting a terraform.yaml that would work for both terraform and terraform-google-conversion. Yet I find that it is possible to generate the code in a way that works with terraform-google-conversion but not touching terraform related code, thus creating this CL.

[morgante]

One potential concern I have with this is the potential for drift. If new fields are added to the provider, they won't necessarily be added to this code. @rileykarson Do we have a plan for when GKE resources might be autogenerated? If it's relatively soon, my concern with drift is lessened.

[rileykarson]

We have no plan for when GKE resources will be generated. Personally I consider it a non-goal at this time, although my thoughts aren't universal.

My concern is drift as well- we have no way to test that the schemas are identical, and I don't think we can realistically gate GKE changes on updating this definition. The resource undergoes a lot of change, and much of that is from community contributors where I wouldn't expect them to update this schema in addition to a standard contribution. Even for 1P contributors on our team, adding additional complexity to adding GKE features will delay adding them, and we already tend to be behind what features are offered in the API.

/cc @chrisst we were talking about this earlier. Do you have any thoughts?

[yukinying]

I have concerns about the drift and I can also offer my perspective how this may be minimized.

Currently the code generation of my code is based on two files: 1. terraform.yaml and 2. api.yaml.

Let's talk about the first part. The file terraform.yaml is required when the TF directive mapping to API is not straightforward. There are 3 examples: fields get renamed, fields need to be removed, and fields that needs to be expanded. Any thing other than that would automatically generated. Currently I used unit test to make sure changes upstream can be reflected downstream (as CAI object in terraform-validator).

The second part is interesting as well. There is already an api.yaml that is used for generate ansible and Inspec code. However that api.yaml missed a few new api so I have cloned it and added those back. Ideally this should not be required and changes should go to the original api.yaml directly. However I don't have full knowledge on the ansible / inspec code, and feel safer to do that in an isolated manner. A simple diff for the api.yaml files could reveal the changes so keeping that file up-to-date is not hard, just that it will be a manual process.

[chrisst]

Apologies fat-fingered the close button.

In regards to your second point, you can add new fields to the API.yaml and use exclude to remove them from inspec and ansible. There are many examples where we have only built partial support for a resource in 1 provider and different support in another.

However I'm in line with @morgante and @rileykarson that a fully generated validator resource will drift from the terraform resource realistically within a matter of weeks. This resource has some of the highest churn of any resources that we support. I also agree that a fully generated GKE resource is definitely not something we will be able to tackle in the near/medium term.

Riley and I were talking yesterday about turning GKE into a hybrid of generated/handwritten code so that new fields could be added into MM and have their expanders/flatteners generated. But this also wouldn't help with the scenario where new field support is added directly to the handwritten resources.

[yukinying]

I totally understand the concern. Since magic-module would be the repository that stores the hand written code for GKE terraform, would it be a fair statement that every time there is a fair amount of GKE changes, we should issue a tracker to update terraform-mapper, and then we can measure the amount of drift? If you look at the code in this pull request, the amount of custom expand is fairly small. They are only 8 of those in container.go and the logic is very straighforward (e.g. adding network parsing logic). Also ideally these common expand logic should have been made available as template.

[morgante]

Given that there aren't plans to proceed with turning GKE into an auto-generated resource and Validator support for GKE is urgently needed (for both internal and external customers), I'd suggest we proceed with this approach while being careful to update validator as we GKE is updated.

One ask: let's make sure to update the warning script so anyone who makes changes to GKE will be warned to update Validator.

[chrisst]

I meant to chat with @yukinying but couldn't yesterday due to my daughter getting sick and requiring daycare evac. Since we missed each other by phone and time is of the essence I'll clarify what I think is happening here and my thoughts on it.

It looks like the api.yaml has been forked for container and that a terraform.yaml has been added. Running MM with those two new files placed into the code path will generate the container_cluster and node_pool files that are checked in (those full of expanders). The goal here appears to be getting a 'thin' version of container cluster for the validator to use but not wholesale copying across the existing hand written resource.

My concerns for the PR in its current state:

• Adding container_cluster to both the generation step and copying the files manually will result in an error in the MM core logic that's meant to stop race conditions (just like this). So the instructions would need to be updated with something like "comment out the copy files" for it to run. This is a lot of manual hacking, that's very bespoke that will need to be done each time there are additions to GKE. Which of all the resources in GCP sees the most fields added on a regular basis.
• The fork of api.yaml has a subtle difference already and will continue to diverge from the existing api.yaml without constant attention. I think you can remove the forked version of the file and edit the existing file to add the new fields but mark them all as exclude: true in the api.yaml and exclude: false in the terraform.yaml. This will make sure that the other downstreams don't accidentally pick up those changes but won't require a separate copy maintained.
• This code is completely disconnected from the current hand written resources. I'm not confident that the expanded output will look the same in all of the edge cases of the resource configuration. As mentioned above, if new fields are added to the handwritten resource, they will need to be added to the terraform.yaml, api.yaml and any custom logic introduced in those fields will not get ported without adding new custom expanders.

Even though the other manual resources aren't handled in a great way I would prefer handling GKE consistently with those other resources if we aren't creating a better way of handling resources.

Possible alternatives:

1. Handle container_cluster similar to the other handwritten resources (such as compute_instance) by stripping the flatteners and other extraneous methods and checking that file into third_party/validator.
2. Make changes to MM to allow validator to override expanders using its own terraform.yaml style config. It will remove the need for manual steps when generating these files, but will continue to incur the drift. This shouldn't be much effort and I will be able to take it on in the next week or two.
3. Get smarter about copying handwritten resources by parsing the source and only copying the methods that are needed for the validator. This is the largest effort up front but I think it will be a better long term approach as it will speed up the adoption of new handwritten resources for validator.

If I'm missing context on anything here or got my assumptions wrong, please let me know.

[rileykarson]

I'm pretty strongly against #2. As the person who currently edits the GKE resources the most, anything that makes me maintain a second copy of the google_container_cluster schema will cause me to not want to make those changes. Additionally, the whole point of thin providers (oics/mapper) is that they don't have their own config so that they're basically free to maintain.

Our goal is to generate more resources, and imo eng effort spent on making more handwritten resources work is counterproductive. If I remember right, original assumption when adding these handwritten resources was that it was roughly a fixed-size list. GKE is clearly exceptional, so #1 feels the most appropriate to me.

Edit: Honestly, for GKE specifically I'm not in love with #1 either. Manually editing the files is annoying too because we need to evaluate every change, and making GKE changes even harder than they already are will make us lag (even more) in features. The existing handwritten resources in validator are acceptable because they're low-effort due to not being edited often.

[morgante]

@rileykarson What would you suggest then? I don't have a strong opinion on how we do it, but we need to add validator support for GKE somehow soon.

3 sounds like the best option if we can actually do it effectively. Failing that, I'd suggest 1 (or perhaps start with 1 and then move to 3).

[rileykarson]

I think #1 is still my most preferred, I'm just hesitant about the eng cost of keeping it up to date.

#3 would be nice, but feels like it would be hard to do and still end up fairly finicky. Imo it wouldn't be worth the upfront cost + maintenance, although that's not an informed opinion and @chrisst has probably given it more thought.

[yukinying]

Discussed with @chrisst , we will go with both 1st and 3rd approach.

To make 1st approach happen, I will remove all the instruction on how these Go files are generated so we will not make others getting into this maintenance burden. The Go file will be flatten into one. And a disclaimer need to added that the file may have small difference with the terraform version (as it is not generated from the same source). This can be done quickly. Please expect an update in this PR for such.

@chrisst will look at the 3rd approach, which is hard and expected to take much more time to complete. But that approach will give us a more reliable way for making the logic in each library stay consistent.

[yukinying]

I created a new pull request #2564 which follows the recommendation in 1st approach. I am closing this.