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

APIM - use existing pip #1960

Closed
wants to merge 2 commits into from
Closed

Conversation

mark-gronow
Copy link
Contributor

@mark-gronow mark-gronow commented Apr 8, 2024

PR Checklist

  • I have added example(s) inside the [./examples/] folder
  • I have added the example(s) to the integration test list for long runner >30 minutes
  • I have checked the coding conventions as per the wiki
  • I have checked to ensure there aren't other open Pull Requests for the same update/change? PR 1788 is still open as requested changes (Oct 2nd 2023) haven't been completed.

Description

APIM does not currently support using an existing Public IP, which is required for deployment/upgrade of the APIM Platform to version stv2.*. stv1 is going EOL 2025. This PR adds support for using a Public IP in the APIM module.

Does this introduce a breaking change

  • YES
  • NO

Testing

Run example examples/apim/118-api_management_platform_stv2/configuration.tfvars
I was able to successfully deploy it in my testing.

Copy link
Member

@arnaudlh arnaudlh left a comment

Choose a reason for hiding this comment

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

hi @mark-gronow thanks for your PR!

As Terraform is lazy at plan-time to evaluate the value with try statements, the public_ip_address_id as it is wouldnt be assesed at plan time. Meaning if you add a public ip address, even if not related to config file, it would create a "known at apply" message which could make ops teams anxious, as showed here: #1952 (review) . Do you think we could modify the statement in the same shape as described in the other PR? (a-la: disk_encryption_set_id = can(each.value.os_disk.disk_encryption_set_id) ? each.value.os_disk.disk_encryption_set_id : can(each.value.os_disk.disk_encryption_set_key) ? var.disk_encryption_sets[try(each.value.os_disk.lz_key, var.client_config.landingzone_key)][each.value.os_disk.disk_encryption_set_key].id : null)

Can you please also check your example file for:

  1. The right resource group keys.
  2. The right vnet and subnet address space blocks.
  3. Resource group for the NSG.
  4. Remove the nsg flow logs since your example does not have diagnostics.

@arnaudlh arnaudlh added the enhancement New feature or request label Apr 22, 2024
@arnaudlh arnaudlh added this to the 5.7.11 milestone Apr 22, 2024
@arnaudlh arnaudlh changed the title apm - use existing pip APIM - use existing pip Apr 22, 2024
@arnaudlh arnaudlh added the apim label Apr 22, 2024
@arnaudlh
Copy link
Member

arnaudlh commented May 2, 2024

Closing this as updated in #1971 - thanks for the PR @mark-gronow

@arnaudlh arnaudlh closed this May 2, 2024
arnaudlh added a commit that referenced this pull request May 23, 2024
* apm - use existing pip

* add to long runner integration test

* Refactor API Management module and example

Refactor API Management module and update example configuration for better clarity and functionality.

- Modify the `public_ip_address_id` assignment in `modules/apim/api_management/module.tf` to use `can` statements for conditional checks, enhancing clarity and reducing potential confusion during plan-time evaluation.
- Update the example configuration in `examples/apim/118-api_management_platform_stv2/configuration.tfvars` to correct resource group keys, vnet and subnet address spaces, ensuring they align with task requirements.
- Remove NSG flow logs configurations from the example file to simplify the setup and adhere to the task's request for removal.


---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/aztfmod/terraform-azurerm-caf/pull/1960?shareId=73fef528-9148-4db9-8557-478779a850ce).

* Update module.tf

* Update to use public_ip_address

---------

Co-authored-by: mark.gronow@shapingcloud.com <mark.gronow@shapingcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apim enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants