Added new resource "WasmPlugin" #12275
Conversation
- Fixed NetworkServices product being configured to use v1 endpoints for the beta providers;
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SirGitsalot, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_wasm_plugin" "primary" {
versions {
plugin_config_data = # value needed
plugin_config_uri = # value needed
}
}
|
| type: ResourceRef | ||
| resource: 'WasmPluginVersion' | ||
| description: | | ||
| Optional. The ID of the WasmPluginVersion resource that is the currently serving one. The version referred to must be a child of this WasmPlugin resource and should be listed in the "versions" field. |
There was a problem hiding this comment.
Is there a google_network_services_wasm_plugin_version resource on its way?
There was a problem hiding this comment.
I am currently trying to implement a version resource but I am having some issues since main_version_id appears to be required for creation (despite the API docs describing it as optional) and needs to refer to a version listed in this plugin resource during request. Trying to list a initial version in the plugin resource and config additional versions as resources inevitably causes permadiffs.
Either way, I think this field would be better off as a String since it may not necessarily be used with a future versions resource.
There was a problem hiding this comment.
I just checked the API proto definition, and main_version_id is annotated with both (google.api.field_behavior) = OPTIONAL and a validator.rule that makes it required (and the latter wins).
The main reason I asked about google_network_services_wasm_plugin_version is if that resource can manage versions and so can a writeable versions field in google_network_services_wasm_plugin then there's no single canonical place to configure versions, which doesn't play well with Terraform.
Thinking about it a little more, I'm not sure that google_network_services_wasm_plugin_version would be possible, at least not without a lot of custom code or a sketchy user experience. Since the parent google_network_services_wasm_plugin needs to exist before creating a google_network_services_wasm_plugin_version child, the configuration sequence would be:
- Configure a
google_network_services_wasm_pluginwith an emptymain_version_id(assuming that thevalidator.ruleis incorrect and can be removed from the API) - Create a
google_network_services_wasm_plugin_version - Update the plugin from step 1 to set
main_version_idto the version from step 2.
As you've discovered, step 3 is what makes things a PITA. There's a couple of ways to work around the problem:
- The easiest is not to implement
google_network_services_wasm_plugin_versionand make theversionsfield ofgoogle_network_services_wasm_pluginrequired instead. - If
main_version_idreally is optional, remove thevalidator.rule(and wait for the subsequent API rollout). Then, implementgoogle_network_services_wasm_plugin_versionwith an additional virtual boolean field, something likemain_version. When agoogle_network_services_wasm_plugin_versionis configured withmain_version = truea custom post-create or post-update step wouldPATCHthe parent plugin resource to setmain_versionto the version in question.
With the latter, main_version_id and versions in google_network_services_wasm_plugin can be output only or omitted entirely. You'll need custom encoder for main_version_id and versions for updates so that they don't get zeroed out when updating the other fields in the resource.
I think the separate version resource option is a more idiomatically Terraform approach and a better user experience, but it will be a fair amount of additional work to create and maintain. I'll leave it up to you which you prefer.
There was a problem hiding this comment.
main_version_id being required seems to be the intended behavior for the API, with versions being managed together with the WasmPlugin, so I will be keeping the versions implementation on the google_network_services_wasm_plugin resource.
Tests analyticsTotal tests: 4288 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 63 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
| @@ -16,7 +16,7 @@ name: 'NetworkServices' | |||
| display_name: 'Network services' | |||
| versions: | |||
| - name: 'beta' | |||
| base_url: 'https://networkservices.googleapis.com/v1/' | |||
| base_url: 'https://networkservices.googleapis.com/v1beta1/' | |||
There was a problem hiding this comment.
This change is what's causing the new test failures. I think (based on looking at your API service config) that the wasm endpoints are being served on /v1/ so you should be able to revert.
There was a problem hiding this comment.
/v1/ may not be available until Q1 2025. We need to stay with v1beta1.
There was a problem hiding this comment.
The discovery doc says that the wasm plugin endpoints are available under /v1/ so it might be worth trying. If it works, it'll save you from having to fix all the tests that are broken when connecting to /v1beta1/
There was a problem hiding this comment.
Running the v1 endpoints for wasmPlugins causes a 403 error: "WasmPlugins v1 api is not yet available. Please use the v1beta1 API." so I guess there is no way around that.
Most of the broken tests are related to NetworkServicesEdgeCache* resources, and the docs lists only v1 and v1alpha1 endpoints for these, so maybe we could use v1alpha1 endpoints for the beta provider?
There was a problem hiding this comment.
It's OK to use a higher-versioned endpoint (like using GA for beta as has been used here) but going the other way is not. This is the first time I've encountered a split where one resource is available in GA but not beta, while another resource is available in beta but not GA. I'll talk to the team and see if there's a precedent for how to handle the situation.
There was a problem hiding this comment.
Most of the broken tests are related to
NetworkServicesEdgeCache*resources, and the docs lists onlyv1andv1alpha1endpoints for these, so maybe we could usev1alpha1endpoints for the beta provider?
I can't image how NetworkServicesEdgeCache tests may be impacted, as it would require WasmAction resource (a connection between WasmPlugin and EdgeCacheService) which stays in v1alpha and is not a part of the current task (you have access to WasmAction API because your test projects are allowlisted).
At this moment WasmPlugin is intented to be used with LbTrafficExtension and LbRouteExtensions only.
There was a problem hiding this comment.
I can't image how NetworkServicesEdgeCache tests may be impacted
Those tests are unchanged, but Magic Modules only supports a single base URL per version (beta or GA) and this PR changes the base URL for the beta version from /v1/ to /v1beta/.
@matheusaleixo-cit after discussing this with the rest of the team the solution is to move this new resource to a new product (say, networkservicesplugins). That should be a quick change:
- Create the new
mmv1/products/networkservicespluginsdirectory - Move
WasmPlugin.yamlto it - Copy
mmv1/products/networkservices/product.yamlto it, with the beta base url set to/v1beta - Revert the change to
mmv1/products/networkservices/product.yaml
- Changed mainVersionId type to String;
|
@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 4319 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 46 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
I'm reopening this PR since WasmPlugins v1 API seems to now be open to the public. |
|
@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @SirGitsalot This PR has been waiting for review for 1 week. Please take a look! Use the label |
|
Hello @SirGitsalot , any chance we could give this PR priority now that the API is now available? No changes to the code has been made since last review and this has been in our backlog for quite a while. |
| references: | ||
| guides: | ||
| 'Configure a route extension': 'https://cloud.google.com/service-extensions/docs/create-plugin' | ||
| api: 'https://cloud.google.com/service-extensions/docs/reference/rest/v1beta1/projects.locations.wasmPlugins' |
There was a problem hiding this comment.
|
@GoogleCloudPlatform/terraform-team @SirGitsalot This PR has been waiting for review for 2 weeks. Please take a look! Use the label |
|
@GoogleCloudPlatform/terraform-team @SirGitsalot This PR has been waiting for review for 3 weeks. Please take a look! Use the label |
|
@modular-magician reassign-reviewer |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_wasm_plugin" "primary" {
location = # value needed
used_by {
name = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels. |
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 67 Click here to see the affected service packages
Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_wasm_plugin" "primary" {
location = # value needed
used_by {
name = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels. |
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 67 Click here to see the affected service packages
Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
| transformed := make(map[string]interface{}) | ||
|
|
||
| // Ensure we don't send empty versions | ||
| if tpgresource.IsEmptyValue(reflect.ValueOf(original["version_name"])) { |
There was a problem hiding this comment.
Why do we not want to send empty versions? Like, can we just require version_name to be specified?
| @@ -805,3 +806,374 @@ resource "google_compute_region_backend_service" "callouts_backend_2" { | |||
| } | |||
| `, context) | |||
| } | |||
|
|
|||
| func TestAccNetworkServicesLbTrafficExtension_crossRegionInternalPluginExtension(t *testing.T) { | |||
| acctest.SkipIfVcr(t) // Test requires a existing container image that contains the plugin code, published in an Artifact Registry repository. | |||
There was a problem hiding this comment.
Is it possible to add the container image as part of the test? Or how are we supposed to set this up in general?
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
- Removed location and main_version_id fields from tests ImportStateVerifyIgnore;
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_network_services_wasm_plugin" "primary" {
used_by {
name = # value needed
}
}
Missing service labelsThe following new resources do not have corresponding service labels:
If you believe this detection to be incorrect please raise the concern with your reviewer. Googlers: This error is safe to ignore once you've completed go/fix-missing-service-labels. |
Non-exercised tests🔴 Tests were added that are skipped in VCR:
Tests analyticsTotal tests: 68 Click here to see the affected service packages
Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Adds new resource
google_network_services_wasm_plugin.Fixes: hashicorp/terraform-provider-google#20220
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.