Add support for alias_ip_range in google_compute_instance network interface

[rosbo]

Fixes #288, #311

1. The alias ip range can use addresses from the primary ip range or the secondary range defined in the subnetwork.
2. Updated api_versions.go to handle the case where the beta field is nested. For this case, alias_ip_range is nested under network_interface. Created a new notation for this.

[rosbo]

Added @rileykarson in case you have some free time to look at the notation I created for specifying nested beta fields.

[rileykarson]

I read through the changes quickly; makes sense to me!

Glad to see this feature getting closed out 👍

[rileykarson]

Is the DiffSuppressFunc not covering this one?

[rosbo]

The problem is that subnetwork can be specified using the name or the self-link.

If a user specify a name in the tf config, the diff is suppressed correctly using linkDiffSuppress.
However, if the user specify a self-link in the tf config, we get a diff because one link is beta and the other one is v1 and linkDiffSuppress doesn't work for that case.

The other option is to change the DiffSuppressFunc to something along these lines:

DiffSuppressFunc: func(...) bool { 
   return compareSelfLinkRelativePaths(...) || linkDiffSuppress(...)
}

We could factored this logic into its own method compareRegionalSelfLinkOrResourceName or something like that. The same way it was done for compareGlobalSelfLinkOrResourceName

[rileykarson]

That makes sense, thanks!

Looking at how linkDiffSuppress is used for both scopes, would we be able to generalise compareGlobalSelfLinkOrResourceName, and use that instead?

As I understand it, we are always going to store a full self link in state, so old will always be a full link; we can then split new on /, and check its length, where there are two cases:

• newParts has a length of 1: The user has specified a resource name, so we should split old into oldParts and check the last element of each.
• newParts has any other length: The user has not specified a name, and so they are using a relative self link (starting from projects/), or a full self link; we can compare by using compareSelfLinkRelativePaths on old and new.

That way we can cover both cases with the same diff function, and this should work for Zonal resources too if there is a similar case, without adding a third function for that as well.

[rosbo]

Done

[selmanj]

I have to say I'm not a huge fan of this syntax. I appreciate that you do have to make some sort of change however to be able to see if this feature is in use for any of the network_interface properties.

If I see a '*' I would expect it to be globbing like a regex rather than having it match a sublist. Anyway to make the behavior like that?

(Alternatively, if we could see if a feature is in use by using a more structural syntax I'd totally love that (similar to how we navigate the schema.ResourceData struct)

Also, can we document this syntax in the inUseBy method?

[rosbo]

There is no use case for globbing on a field. I don't expect why a beta field would be present under two different field. inUseBy is only use for api versions.

You can see the * as globbing on the indices. Usually, you would have field.5.nested_field.

I am not sure I follow your suggestion about structural syntax...

I will add more documentation once we agree on the syntax.

[selmanj]

But you're globbing on a field right now! (or at least, you're globbing on an integer)

I guess I'm saying is that if we introduce syntax like '*' but it is specific to integers, it's asking for a bug in the future when someone uses '*' like globbing. Given that globbing would work here, would it be too hard to add?

For a more structural syntax, I meant being able to receive and parse the actual schema.ResourceData struct using GetOk and other syntax to see if a given feature is in use, rather than using the serialize string data.

[rosbo]

Supporting globbing also on field names would definitely add complexity (and potentially bugs) when there is clearly no needs for this. However, I can make sure it fails if with a nice error message if someone uses the * on a field name instead of an indice. This way, we prevent somebody from misusing it without knowing they are misusing it.

[selmanj]

I don't want to block this whole PR on the syntax (especially since what's currently-implemented would work if * was a globbing symbol) but I'm starting to worry that this inUseBy method is an anti-pattern. Right now it depends on the serialized state and not the provided schema.ResourceData which provides a well-defined set of methods for dealing with the schema. My understanding is that inUseBy is used because it's convenient, but if we ever depend on values like optionals or validation then we'll regret using it here.

That said, we can probably have that discussion outside of this PR. If we document the '*' syntax I'm ok as-is.

[rileykarson]

I'm a bit confused - inUseBy uses the methods from schema.ResourceData; TerraformResourceData is just an interface for the two methods inUseBy needs, since we don't need the rest of the behaviour of the class and are able to unit test this code w/ a mock of that interface.

[selmanj]

OK, after looking at the code, you were right and I was confused; it IS operating on schema.ResourceData; I had no idea you could directly access attributes like foobar.# to get the size of a list for example using the Get or GetOk method. For some reason I assumed it was using something similar to the the attributes map that we constantly assert on in unit tests.

[rosbo]

Documentation added.

[selmanj]

Minor quibble; this is just a bit too terse for me (and maybe i"m wrong but I don't think it works for all nested fields right? Only integer-nested fields?)

Maybe something like:

 // - integer-nested field: If the Item field contains a '*', then the feature is considered to be in-use if it is present in any element of the list or set (e.g. "network_interface.*.alias_ip_range" is considered to be in-use if "network_interface.3.alias_ip_range" was set in the state

Hopefully you can come up with something better (also I like your name path better for the field name)

[rosbo]

I updated the documentation incorporating some of the bits you suggested. Hopefully it is clearer now.

Yes, it works for all nested fields (not just integer nested fields). The proof is that alias_ip_range is of TypeList and network_interface is of TypeList.

It doesn't support beta field nested under a set at the moment. It would add complexity and be slow since we would need to scan the whole state to find the elements in the set (we can't rely on zero-indexed incremental indices like we do for lists). If we ever need to support this, we can implement this efficiently with a trie.

Note: I will rename the Item for Path or FieldPath in a separate PR. Since updating the field touches a lot of files, I prefer to do it in a separate PR that can get merged quickly to avoid the rebase/merge hassle and also to avoid polluting this already long PR .

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!