add apigee non-vpc peering mode support #1648

g-greatdevaks · 2023-09-05T10:37:21Z

Apigee recently introduced the Private Service Connect (Non-VPC Peering) Provisioning Mode for Apigee X provisioning; the feature is in Preview; as of the time of raising this PR. When using this provisioning mode, there is no requirement for holding any PSA IP CIDR Ranges in the Authorized Network.

This PR focuses introducing the support for Private Service Connect (Non-VPC Peering) Provisioning Mode Provisioning Mode for Apigee X provisioning.
The PR also fixes a lifecycle issue with Apigee Environment resource. The issue was occurring because try function was being used for setting the name of the Apigee Environment. lifecycle ignore_changes configuration was added to overcome the issue. The issue force replaces the Apigee Environment and the same, if not fixed, will end up causing outage on each terraform apply.
The Apigee module has been tested for both VPC Peering and Non-VPC Peering mode before raising the PR.

Checklist
I acknowledge that I have:

Read the contributing guide
Ran terraform fmt on all modified files
Regenerated the relevant README.md files using tools/tfdoc.py
Made sure all relevant tests pass

Reference: [Apigee Non-VPC Peering Mode](https://cloud.google.com/apigee/docs/api-platform/get-started/networking-options#non-vpc-peering-architecture-overview)

…-peering-mode

juliocc · 2023-09-05T13:48:40Z

this is marked as fract. Is this ready for review?

g-greatdevaks · 2023-09-05T15:56:39Z

@juliocc Fixed broken tests and added some more. Kindly review.

g-greatdevaks · 2023-09-05T16:13:26Z

There seems to be some issue with one test case.
Will work on it and close in some time.

g-greatdevaks · 2023-09-05T18:09:15Z

The PR is ready for review, Julio 🙂

juliocc · 2023-09-05T19:19:34Z

modules/apigee/README.md

@@ -135,14 +135,24 @@ module "apigee" {
 module "apigee" {
  source     = "./fabric/modules/apigee"
  project_id = "my-project"
+  organization = {


can you add more examples and keep this one as it was?

juliocc · 2023-09-05T19:22:22Z

tests/modules/apigee/tftest.yaml

@@ -21,7 +21,9 @@ tests:
  env_only_with_api_proxy_type:
  env_only_with_deployment_type:
  envgroup_only:
-  instance_only:
+  instance_only_psc_mode:


Can you add this additional tests as examples in the readme?

juliocc · 2023-09-05T19:25:19Z

modules/apigee/main.tf

@@ -23,11 +23,12 @@ resource "google_apigee_organization" "organization" {
  count                                = var.organization == null ? 0 : 1
  analytics_region                     = var.organization.analytics_region
  project_id                           = var.project_id
-  authorized_network                   = var.organization.authorized_network
+  authorized_network                   = var.organization.disable_vpc_peering ? null : var.organization.authorized_network


Instead of changing the value here I'd add validation in the variable ensuring var.organization.authorized_network = null when var.organization.disable_vpc_peering is truee

juliocc · 2023-09-05T19:26:07Z

modules/apigee/main.tf

@@ -91,7 +92,7 @@ resource "google_apigee_instance" "instances" {
  description              = each.value.description
  location                 = each.key
  org_id                   = local.org_id
-  ip_range                 = "${each.value.runtime_ip_cidr_range},${each.value.troubleshooting_ip_cidr_range}"
+  ip_range                 = var.organization.disable_vpc_peering ? null : "${each.value.runtime_ip_cidr_range},${each.value.troubleshooting_ip_cidr_range}"


Same as before, I'd add a validation to variable (or maybe here you need a precondition check since it's s different variable)

juliocc · 2023-09-05T19:26:20Z

modules/apigee/main.tf

@@ -117,6 +118,9 @@ resource "google_apigee_instance_attachment" "instance_attachments" {
  instance_id = google_apigee_instance.instances[each.value.region].id
  environment = try(google_apigee_environment.environments[each.value.environment].name,
  "${local.org_id}/environments/${each.value.environment}")
+  lifecycle {


why is ignore_changes needed here?

There is an issue which is occurring because try function is being used for setting the name of the Apigee Environment. Because of this, the actual value for the environment argument cannot be determined until runtime and terraform plan shows force replaces comment for corresponding Apigee Environment. terraform apply force replaces the resource as well. If the same is not fixed, it will end up causing outage on each terraform apply.

lifecycle ignore_changes configuration has been added to overcome the issue.

I had some doubts about this too:

why do we need try here (to make destroy work? because otherwise it should always evaluate properly)

is indeed try the issue here, and not the for_each that does the transformations

But in the end it will work as expected, because if you change the environment name, you change the key in the resource, thus forcing replacement.

I'd love to have a comment here in the code about one and the other (so save my future-self from thinking this over again)

The try function is actually not even required here as if var.environments is {} then the instance attachment resource won't even be getting created.

And on another note, environment attribute requires the name of the Apigee Environment and not the id.
So usage of "${local.org_id}/environments/${each.value.environment}" is not right and instead it should just be each.value.environment. However, since we don't require the try function here, environment = google_apigee_environment.environments[each.value.environment].name should be sufficient.

…-peering-mode

g-greatdevaks · 2023-09-07T11:27:22Z

Closing the PR as an alternate implementation has been worked on: #1653

g-greatdevaks added 2 commits September 5, 2023 15:49

add apigee non-vpc peering mode support

f456754

Reference: [Apigee Non-VPC Peering Mode](https://cloud.google.com/apigee/docs/api-platform/get-started/networking-options#non-vpc-peering-architecture-overview)

Merge remote-tracking branch 'primary/master' into add-apigee-non-vpc…

9451dde

…-peering-mode

g-greatdevaks requested a review from juliocc September 5, 2023 10:37

g-greatdevaks self-assigned this Sep 5, 2023

github-actions bot added the on:modules label Sep 5, 2023

add and fix tests for apigee module

36c707f

g-greatdevaks force-pushed the add-apigee-non-vpc-peering-mode branch from 3011685 to 36c707f Compare September 5, 2023 15:55

g-greatdevaks marked this pull request as ready for review September 5, 2023 15:56

fix apigee module readme tftests

7259c0f

juliocc requested changes Sep 5, 2023

View reviewed changes

Merge remote-tracking branch 'primary/master' into add-apigee-non-vpc…

28fea9e

…-peering-mode

juliocc mentioned this pull request Sep 7, 2023

Fixes to the apigee module #1653

Merged

4 tasks

add preconditions and checks

4af6ac0

g-greatdevaks closed this Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add apigee non-vpc peering mode support #1648

add apigee non-vpc peering mode support #1648

g-greatdevaks commented Sep 5, 2023

juliocc commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

juliocc Sep 5, 2023

g-greatdevaks Sep 7, 2023

juliocc Sep 5, 2023

g-greatdevaks Sep 7, 2023

juliocc Sep 5, 2023

juliocc Sep 5, 2023

juliocc Sep 5, 2023

g-greatdevaks Sep 6, 2023

wiktorn Sep 6, 2023

g-greatdevaks Sep 7, 2023

g-greatdevaks commented Sep 7, 2023

add apigee non-vpc peering mode support #1648

add apigee non-vpc peering mode support #1648

Conversation

g-greatdevaks commented Sep 5, 2023

juliocc commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

g-greatdevaks commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-greatdevaks commented Sep 7, 2023