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

Fix/1975 azurerm logic app standard subnet #1978

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

arne21a
Copy link
Contributor

@arne21a arne21a commented May 17, 2024

1975

But also contains fixes for:
#1977
#1973

because those are required for this to work.

PR Checklist


Description

Does this introduce a breaking change

  • YES
  • NO

As described in my issue 1975, the proposed change will lead to a short downtime for existing deployments.

Testing

@JoDexsph
Copy link
Contributor

Hey @arne21a, I've tried your fix(we are using the logic app in our caf deployment), however, the Vnet integration still doesn't work.
Using both subnet_id or lz_key,vnet_key and subnet_key.

Private endpoint and identity works : -)

@arne21a
Copy link
Contributor Author

arne21a commented Jun 20, 2024

hi @JoDexsph thanks for your feedback! Would you be able to provide your configuration and your error messages?

One general consideration: Many CAF modules struggle with subnets created in an other state then the vnet. This might also be the issue here.

@JoDexsph
Copy link
Contributor

Sure thing @arne21a , kindly check the following gist : https://gist.github.com/JoDexsph/6804a46d6702007d92eaffa5a842689b

Not sure if it my place to ask, but can you please add the "public_network_access_enabled" and "https_only" terms to the azurerm block?
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/logic_app_standard#public_network_access_enabled
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/logic_app_standard#https_only

Thanks.

@arne21a
Copy link
Contributor Author

arne21a commented Jun 20, 2024

Hi @JoDexsph
The posted configuration looks good to me at first glance. Unfortunately i do not have a test environment where i can test the upstream version of this repo, as my project uses a slightly diverged version which is not fully compatible.
The error message you get could give us a hint where to start troubleshooting.

About your other requests:
https_only is already merged: public_network_access_enabled

public_network_access_enabled is also something i would like to add, but it is not supported in the used provider version https://registry.terraform.io/providers/hashicorp/azurerm/3.75.0/docs/resources/logic_app_standard

@JoDexsph
Copy link
Contributor

Hello @arne21a , I am trying to run it via the rover and get Debug / Trace logs to have more information.
with Error log level, the deployment is completed successfully, but in the plan I cant find anything related to the vnet integration.

@JoDexsph
Copy link
Contributor

Hi @JoDexsph The posted configuration looks good to me at first glance. Unfortunately i do not have a test environment where i can test the upstream version of this repo, as my project uses a slightly diverged version which is not fully compatible. The error message you get could give us a hint where to start troubleshooting.

About your other requests: https_only is already merged: public_network_access_enabled

public_network_access_enabled is also something i would like to add, but it is not supported in the used provider version https://registry.terraform.io/providers/hashicorp/azurerm/3.75.0/docs/resources/logic_app_standard

Anyway regarding the azurerm, today CAF/Rover are using azurerm v3.105.0.
https://registry.terraform.io/providers/hashicorp/azurerm/3.105.0/docs/resources/logic_app_standard

@arne21a
Copy link
Contributor Author

arne21a commented Jun 20, 2024

@JoDexsph As i said, i am working with a diverged version of this project which does not include that provider version update as of right now. Thats the reason why i can not test it.
I will not propose features i was not able to verify.
Feel free to implement, test and propose this feature. It is on my wishlist too.

Regarding the issue with the vnet integration:
I just double checked my test cases and they work with my environment and surrounding implementation. Your best option to proceed is debugging within your own environment.
I usually add outputs to the module to see if specific configurations actually arrive in the targeted module. So i would start by adding an output with the value lookup(var.settings, "vnet_integration", null) to the module and see if the value is populated correctly.
Make sure to expose this output through the complete chain (module call, landingzone), otherwise you wont see results in your plan.

@JoDexsph
Copy link
Contributor

Hey @arne21a,
I've tried various ways to get the output, couldn't get it to give any data.
I will try to create a lab with Terraform and not CAF to run the module as a standalone to see the behavior.

@arnaudlh arnaudlh modified the milestones: 5.7.13, 5.7.14 Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment