Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding ServiceConnectionPolicies resource in NetworkConnectivity. #8273

Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ffc9883
adding both resources, basic test for serviceClass it's working, need…
diogoEsteves42 Jul 6, 2023
4eb449d
fix the test scenario and the service class resource properties
diogoEsteves42 Jul 7, 2023
5dac289
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 11, 2023
5230861
adding a testing update, just adding the labels for the resource
diogoEsteves42 Jul 11, 2023
4240e1d
added the resource service connection map, and it's basic test, need …
diogoEsteves42 Jul 13, 2023
6460bfb
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 13, 2023
ccd09fe
Revert "added the resource service connection map, and it's basic tes…
diogoEsteves42 Jul 13, 2023
5a413b5
removing the service class in one test scenario and using a static value
diogoEsteves42 Jul 14, 2023
8b53efc
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 17, 2023
110baa6
fix a typo
diogoEsteves42 Jul 18, 2023
a4edf35
removing serviceClass and fix the tests
diogoEsteves42 Jul 19, 2023
be29545
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 19, 2023
d1b4d94
removing ga tag and updatinga etag for fingerprint type
diogoEsteves42 Jul 21, 2023
eae5967
updating the code, to remove the static value from the docs, need to …
diogoEsteves42 Jul 26, 2023
886ab72
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 26, 2023
677dc31
adding a file to tpgtools override at product level
diogoEsteves42 Jul 28, 2023
d18ffd2
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Jul 28, 2023
b60527f
adding function to help with env var, but the patch on tests it's fai…
diogoEsteves42 Jul 29, 2023
f914190
adding the variable to be used in the docs
diogoEsteves42 Jul 29, 2023
9ba4921
fixing the test, the network field cannot be immutable
diogoEsteves42 Jul 31, 2023
28a5cdc
removing the env_var service class and adding a static value
diogoEsteves42 Jul 31, 2023
03ea524
updating the value used from tests and documentation
diogoEsteves42 Jul 31, 2023
bf298e1
removing unused resources for docs and tests
diogoEsteves42 Jul 31, 2023
f82d55b
fix typos, not used variables and run the fmt
diogoEsteves42 Aug 1, 2023
d6af2e8
Merge branch 'main' into create-service-automation-connection-policy
diogoEsteves42 Aug 1, 2023
f68acb8
adding description to the basic test
diogoEsteves42 Aug 1, 2023
1387c0b
fixed yaml variable override
diogoEsteves42 Aug 1, 2023
b06a8c7
removing unused resources for tests, and making the test fails with n…
diogoEsteves42 Aug 1, 2023
a448bf9
adding encoder to help network field be immutable and be sent on patc…
diogoEsteves42 Aug 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions mmv1/products/networkconnectivity/ServiceConnectionPolicies.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
# Copyright 2021 Google Inc.
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
# Licensed under the Apache License, Version 2.0 (the 'License');
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an 'AS IS' BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

--- !ruby/object:Api::Resource
name: 'ServiceConnectionPolicy'
base_url: 'projects/{{project}}/locations/{{location}}/serviceConnectionPolicies'
create_url: 'projects/{{project}}/locations/{{location}}/serviceConnectionPolicies?serviceConnectionPolicyId={{name}}'
self_link: 'projects/{{project}}/locations/{{location}}/serviceConnectionPolicies/{{name}}'
update_verb: :PATCH
update_mask: true
description:
'Manage Service Connection Policies.'
references: !ruby/object:Api::Resource::ReferenceLinks
api: 'https://cloud.google.com/secure-web-proxy/docs/reference/networkconnectivity/rest/v1/projects.locations.networkConnectionPolicies'
guides:
'About Service Connection Policies': 'https://cloud.google.com/vpc/docs/about-service-connection-policies#service-policies'
async: !ruby/object:Api::OpAsync
operation: !ruby/object:Api::OpAsync::Operation
path: 'name'
base_url: '{{op_id}}'
wait_ms: 1000
timeouts: !ruby/object:Api::Timeouts
insert_minutes: 30
update_minutes: 30
delete_minutes: 30
result: !ruby/object:Api::OpAsync::Result
path: 'response'
status: !ruby/object:Api::OpAsync::Status
path: 'done'
complete: true
allowed:
- true
- false
error: !ruby/object:Api::OpAsync::Error
path: 'error'
message: 'message'
autogen_async: true
import_format:
['projects/{{project}}/locations/{{location}}/serviceConnectionPolicies/{{name}}']
examples:
- !ruby/object:Provider::Terraform::Examples
name: 'network_connectivity_policy_basic'
primary_resource_id: 'default'
vars:
resource_name: 'my-network-connectivity-policy'
consumer_network_name: 'consumer-net'
consumer_subnetwork_name: 'consumer-subnet'
producer_network_name: 'producer-net'
producer_subnetwork_name: 'producer-subnet'
parameters:
- !ruby/object:Api::Type::String
name: 'name'
required: true
immutable: true
url_param_only: true
description: |
The name of a ServiceConnectionPolicy. Format: projects/{project}/locations/{location}/serviceConnectionPolicies/{service_connection_policy} See: https://google.aip.dev/122#fields-representing-resource-names
- !ruby/object:Api::Type::String
name: 'location'
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
required: true
url_param_only: true
description: |
The location of the ServiceConnectionPolicy.
properties:
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
- !ruby/object:Api::Type::Time
name: 'createTime'
description: |
The timestamp when the resource was created.
output: true
- !ruby/object:Api::Type::Time
name: 'updateTime'
description: |
The timestamp when the resource was updated.
output: true
- !ruby/object:Api::Type::String
name: 'serviceClass'
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
description: |
The service class identifier for which this ServiceConnectionPolicy is for. The service class identifier is a unique, symbolic representation of a ServiceClass.
It is provided by the Service Producer. Google services have a prefix of gcp. For example, gcp-cloud-sql. 3rd party services do not. For example, test-service-a3dfcx.
- !ruby/object:Api::Type::String
name: 'description'
description: |
Free-text description of the resource.
- !ruby/object:Api::Type::String
name: 'network'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, is it possible to create this resource without specifying a network?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not, thanks for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other fields look good, but I believe network here still needs to be marked as immutable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roaks3 I was testing this scenario, if I add the network as immutable, the test fails because the API requires the network in the payload.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's strange. Could you provide any more info on that? I think we should be able to configure this property in a way that works.

Alternatively, you could submit it with the immutable: true and then I can see the failure in the logs in VCR.

description: |
The resource path of the consumer network. Example: - projects/{projectNumOrId}/global/networks/{resourceId}.
- !ruby/object:Api::Type::NestedObject
name: 'pscConfig'
description: |
Configuration used for Private Service Connect connections. Used when Infrastructure is PSC.
properties:
- !ruby/object:Api::Type::Array
name: 'subnetworks'
required: true
item_type: Api::Type::String
description: |
IDs of the subnetworks or fully qualified identifiers for the subnetworks
- !ruby/object:Api::Type::String
name: 'limit'
description: |
Max number of PSC connections for this policy.
- !ruby/object:Api::Type::Fingerprint
name: 'etag'
description: |
The etag is computed by the server, and may be sent on update and delete requests to ensure the client has an up-to-date value before proceeding.
- !ruby/object:Api::Type::Array
name: 'pscConnections'
output: true
item_type: Api::Type::String
description: |
Information about each Private Service Connect connection.
- !ruby/object:Api::Type::String
name: 'infrastructure'
output: true
description: |
The type of underlying resources used to create the connection.
- !ruby/object:Api::Type::KeyValuePairs
name: "labels"
description: |
User-defined labels.
27 changes: 27 additions & 0 deletions mmv1/products/networkconnectivity/product.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Copyright 2020 Google Inc.
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

--- !ruby/object:Api::Product
name: NetworkConnectivity
display_name: Network Connectivity
scopes:
- https://www.googleapis.com/auth/cloud-platform
versions:
- !ruby/object:Api::Product::Version
name: ga
base_url: https://networkconnectivity.googleapis.com/v1/
apis_required:
- !ruby/object:Api::Product::ApiReference
name: Network Connectivity API
url: https://console.cloud.google.com/apis/library/networkconnectivity.googleapis.com/

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
resource "google_compute_network" "consumer_net" {
name = "<%= ctx[:vars]['consumer_network_name'] %>"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "consumer_subnet" {
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
name = "<%= ctx[:vars]['consumer_subnetwork_name'] %>"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.consumer_net.id
}

resource "google_compute_network" "producer_net" {
name = "<%= ctx[:vars]['producer_network_name'] %>"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "producer_subnet" {
name = "<%= ctx[:vars]['producer_subnetwork_name'] %>"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.producer_net.id
}

resource "google_network_connectivity_service_connection_policy" "default" {
name = "<%= ctx[:vars]['resource_name'] %>"
location = "us-central1"
service_class = "gcp-memorystore-redis"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you briefly explain how this value is working? This will show up in the official documentation for this resource, and it will be used for testing in a number of different environments, so I'd like to understand if gcp-memorystore-redis is intended to be used in all of those places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, great, I just forgot that.
Yeah this should not be public. I've talked with a person for the serviceConnectionPolicy and mentioned that I need a way to make the tests work. So he gave me this resource to use. So, it's not for all those places, just here, temporarily while we don't support service_class.

How can I set this one as an env var?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this part of the code, maybe the test start to fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this comment on the prior pass.

Could you share any more about this value? Is it the name of an actual resource, or is it some sort of fake value? If it is an actual resource, is it publicly accessible (for anyone who chooses to run these tests)? If any sort of access is needed for it, we will need someone to make sure that access is set up for all of our internal environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roaks3 according to someone from the ServiceConnectionPolicies, this is a real resource, it's not a fake one, that it will be used while we don't have the serviceClass public available. It's not publicly accessible and I don't know how to make sure that all the envs will be added to have access. I will ask him to talk with you about that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good. Depending on that conversation, we may want to hold this from being merged, or skip the tests right before we merge, but in the meantime:

  • We will definitely want the docs to have some sort of example value. You can do this by adding a new field under vars that specifies the service class value that we would use in the docs (like example-class). Then you can also add a test_vars_overrides block with the value that will be used for tests only (you can search for some existing examples of this).
  • We tend to avoid environment variables when possible, because we need to manage them centrally, and it would become overwhelming if they were needed by many teams/resources. Normally in cases like this, we would keep the test value as you have it (a hardcoded value), unless there is some sort of concern around doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, I got the idea.

I've added some more code to help with env_var.

network = google_compute_network.producer_net.id
psc_config {
subnetworks = [google_compute_subnetwork.producer_subnet.id]
limit = 2
}
}
1 change: 0 additions & 1 deletion mmv1/third_party/terraform/fwmodels/provider_model.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ type ProviderModel struct {
CloudResourceManagerCustomEndpoint types.String `tfsdk:"cloud_resource_manager_custom_endpoint"`
EventarcCustomEndpoint types.String `tfsdk:"eventarc_custom_endpoint"`
FirebaserulesCustomEndpoint types.String `tfsdk:"firebaserules_custom_endpoint"`
NetworkConnectivityCustomEndpoint types.String `tfsdk:"network_connectivity_custom_endpoint"`
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
<% if version == "ga" -%>
OrgPolicyCustomEndpoint types.String `tfsdk:"org_policy_custom_endpoint"`
<% end -%>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package google

import (
"fmt"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
"github.com/hashicorp/terraform-provider-google/google/acctest"
)

func TestAccNetworkConnectivityServiceConnectionPolicy_update(t *testing.T) {
t.Parallel()

networkName := fmt.Sprintf("tf-test-network-%s", RandString(t, 10))
networkProducerName := fmt.Sprintf("tf-test-network-%s", RandString(t, 10))
subnetworkConsumerName := fmt.Sprintf("tf-test-subnet-consumer-%s", RandString(t, 10))
subnetworkProducerName := fmt.Sprintf("tf-test-subnet-producer-%s", RandString(t, 10))
serviceConnectionPolicyName := fmt.Sprintf("tf-test-service-connection-policy-%s", RandString(t, 10))

VcrTest(t, resource.TestCase{
PreCheck: func() { acctest.AccTestPreCheck(t) },
ProtoV5ProviderFactories: ProtoV5ProviderFactories(t),
CheckDestroy: testAccCheckNetworkConnectivityServiceConnectionPolicyDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccNetworkConnectivityServiceConnectionPolicy_basic(networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName),
},
{
ResourceName: "google_network_connectivity_service_connection_policy.default",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccNetworkConnectivityServiceConnectionPolicy_update(networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName),
},
{
ResourceName: "google_network_connectivity_service_connection_policy.default",
ImportState: true,
ImportStateVerify: true,
},
{
Config: testAccNetworkConnectivityServiceConnectionPolicy_basic(networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName),
},
{
ResourceName: "google_network_connectivity_service_connection_policy.default",
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func testAccNetworkConnectivityServiceConnectionPolicy_basic(networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName string) string {
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Sprintf(`
resource "google_compute_network" "consumer_net" {
name = "%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "consumer_subnet" {
name = "%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.consumer_net.id
}

resource "google_compute_network" "producer_net" {
name = "%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "producer_subnet" {
name = "%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.producer_net.id
}

resource "google_network_connectivity_service_connection_policy" "default" {
name = "%s"
location = "us-central1"
description = "my basic service connection policy"
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
service_class = "gcp-memorystore-redis"
diogoEsteves42 marked this conversation as resolved.
Show resolved Hide resolved
network = google_compute_network.producer_net.id
psc_config {
subnetworks = [google_compute_subnetwork.producer_subnet.id]
limit = 2
}
}
`, networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName)
}

func testAccNetworkConnectivityServiceConnectionPolicy_update(networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName string) string {
return fmt.Sprintf(`
resource "google_compute_network" "consumer_net" {
name = "%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "consumer_subnet" {
name = "%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.consumer_net.id
}

resource "google_compute_network" "producer_net" {
name = "%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "producer_subnet" {
name = "%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-central1"
network = google_compute_network.producer_net.id
}

resource "google_network_connectivity_service_connection_policy" "default" {
name = "%s"
location = "us-central1"
description = "my basic service connection policy"
service_class = "gcp-memorystore-redis"
network = google_compute_network.producer_net.id
psc_config {
subnetworks = [google_compute_subnetwork.producer_subnet.id]
limit = 2
}
labels = {
foo = "bar"
}
}
`, networkName, subnetworkConsumerName, networkProducerName, subnetworkProducerName, serviceConnectionPolicyName)
}