Add support GCE_VM_IP type for network endpoint group #10638
Conversation
There was a problem hiding this comment.
@irwanshofwan thanks for your contribution! It looks like there are a few small things it would be great to address.
| @@ -65,7 +65,7 @@ range).`, | |||
| }, | |||
| "port": { | |||
| Type: schema.TypeInt, | |||
| Required: true, | |||
| Optional: true, | |||
There was a problem hiding this comment.
From the docs it looks like "If not specified, the defaultPort for the network endpoint group will be used." Do you know if this means that this will be nil (but fall back to the network endpoint group's value) or if it will be set on creation to whatever the default is? In the latter case this would need to be computed as well as optional.
There was a problem hiding this comment.
I've try this changes to and I think any API bug in google, so as documentation if we set the Network Endpoint type as "GCE_VM_IP" we don't need port and will use default_port variable which set in Network Endpoint Group.
But In console, port input is required (Different with the docs).
The another thing when we implement the changes (modified provider) to infrastructure, the provider will set port to 0 and the network endpoint still created (it's diff with the spec of port which must greater than zero in console).
So we can conclude that GCP not filter the API when creation by API and have different behaviour with docs. The proven is when we want to destroy the resource via terraform, it will turns to error regarding zero port.
Here's the docs I refer to https://cloud.google.com/load-balancing/docs/negs/setting-up-zonal-negs
There was a problem hiding this comment.
Okay... I think this probably needs to be computed as well as optional.
There was a problem hiding this comment.
If you're upstreaming this to magic-modules, you would want to use default_from_api: true to indicate that it should be optional & computed. I believe that marking it as computed will probably / hopefully resolve the issue you're seeing with a 0 port? It could also be a problem with Terraform not being able to tell the difference between nil and zero.
| @@ -103,7 +103,7 @@ The following arguments are supported: | |||
| The instance must be in the same zone of network endpoint group. | |||
|
|
|||
| * `port` - | |||
| (Required) | |||
| (Optional) | |||
| Port number of network endpoint. | |||
There was a problem hiding this comment.
This should also include (from the API docs): If not specified, the defaultPort for the network endpoint group will be used.
There was a problem hiding this comment.
Here's the docs I refer to https://cloud.google.com/load-balancing/docs/negs/setting-up-zonal-negs
There was a problem hiding this comment.
In addition to the change specified above, this docs file also need to be updated to specify the new possible value of GCE_VM_IP.
|
It would be great to add this field to a test. It might be easier to make this change and add a test by contributing directly to https://github.com/GoogleCloudPlatform/magic-modules/ |
will look into this |
|
@irwanshofwan are you still working on this PR? |
|
Closing this PR for now. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Please review this pull request, the pull request to support GCE_VM_IP type for network endpoint group.
So we can setup internal lb using NEG as backend using terraform.