Added new resource "WasmPlugin" #12275
Conversation
- Fixed NetworkServices product being configured to use v1 endpoints for the beta providers;
matheusaleixo-cit
changed the title
modular-magician
added
the
awaiting-approval
|
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. |
modular-magician
added
service/networkservices-traffic-director
service/networkservices-media-cdn
and removed
awaiting-approval
|
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;
modular-magician
added
the
awaiting-approval
|
@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
modular-magician
removed
the
awaiting-approval
|
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
|
|
🟢 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. |
modular-magician
added
the
awaiting-approval
There was a problem hiding this comment.
Looks good, just waiting on the test run to finish successfully now.
@krz-filip PTAL
There was a problem hiding this comment.
I see that all tests defines the single version configuration. Could you please extend at least one of them to define at least 2 versions? The perfect would updating "update" test to modify WasmPlugin from having v1(main)+v2 versions into v2(main)+v3?
There was a problem hiding this comment.
WasmPlugin is in Public Preview phase (beta), should reach GA in Q1 2025.
Is it valid to add it here?
There was a problem hiding this comment.
Looks like the answer is no, as the only product that I can find that exists in beta but not in ga is firebasestorage and it does not have an entry in the ga file.
| @@ -546,6 +546,11 @@ var ServicesListGa = mapOf( | |||
| "displayName" to "Networkservices", | |||
| "path" to "./google/services/networkservices" | |||
| ), | |||
| "networkservicesplugins" to mapOf( | |||
There was a problem hiding this comment.
Is this "networkservicesplugins" value customer visible?
Can it be safely changed in the future?
There was a problem hiding this comment.
It is customer visible, the name of the resource will be google_network_services_plugins_wasm_plugin (instead of google_network_services_wasm_plugin) and the documentation for the resource will land in a section called "Network Services Plugins" (though I think the docs section might be changeable so that it lands in the main "Network Services" section instead).
It can't be changed later, but I'm not at all stuck on the name - I suggested "Network Services Plugins" as it needed a new name and that seemed like a fitting description. If there's a better name, let's change it here.
There was a problem hiding this comment.
Thanks, I need to discuss it with the product owner (next week). I suppose that he would prefer something like "service_extensions" instead of "network_services_plugins".
AFAIK the main reason of moving it to the new product were failing tests for other network_services resources.
Do you think that there is a chance to make them passing? What should change on networkservices API?
modular-magician
removed
the
awaiting-approval
|
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.
|
| @@ -546,6 +546,11 @@ var ServicesListGa = mapOf( | |||
| "displayName" to "Networkservices", | |||
| "path" to "./google/services/networkservices" | |||
| ), | |||
| "networkservicesplugins" to mapOf( | |||
There was a problem hiding this comment.
It is customer visible, the name of the resource will be google_network_services_plugins_wasm_plugin (instead of google_network_services_wasm_plugin) and the documentation for the resource will land in a section called "Network Services Plugins" (though I think the docs section might be changeable so that it lands in the main "Network Services" section instead).
It can't be changed later, but I'm not at all stuck on the name - I suggested "Network Services Plugins" as it needed a new name and that seemed like a fitting description. If there's a better name, let's change it here.
There was a problem hiding this comment.
Looks like the answer is no, as the only product that I can find that exists in beta but not in ga is firebasestorage and it does not have an entry in the ga file.
Tests analyticsTotal tests: 4326 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 11 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. |
|
@matheusaleixo-cit, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
modular-magician
added
the
awaiting-approval
…lugins is in beta;
modular-magician
removed
the
awaiting-approval
|
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: 4357 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
Action takenFound 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. |
|
@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
There was a problem hiding this comment.
Looks good, just one final question to enable the tests - @krz-filip do you know if any of the examples available in a publicly accessible registry? Some of TPG's other resource tests rely on Docker containers, but we're able to use the service's own example containers in our tests (like Cloud Run has us-docker.pkg.dev/cloudrun/container/hello)
Adds new resource
google_network_services_wasm_plugin.Also creates new product
networkservicespluginsas a subset ofnetworkservicesto allow this resource to use the v1beta1 API for the beta provider.Fixes: hashicorp/terraform-provider-google#20220
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.