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

feat: configure client certificate and public network access #215

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

Adnan029
Copy link
Contributor

We used this module internally and made some changes which need to be compliant as per internal compliance policy.

Below are the changes -

log_analytics_workspace_id was mandate before now we have added a variable to handle it if this can be used or can be skip using the following logs_analytics_enable.

We have added 2 more attributes to the webapp client_certificate_mode & client_certificate_enabled and we parameterized this attribute the public_network_access_enabled.

We have done a testing with these changes and it is working as expected.

@Adnan029 Adnan029 requested a review from a team as a code owner September 17, 2024 07:28
Copy link
Member

@hknutsen hknutsen left a comment

Choose a reason for hiding this comment

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

Changes related to client certificate and public network access look good 😄 🙌 Added a few suggestions for minor improvements. We'll also have to implement the same changes for the azurerm_windows_web_app.this resource.

In regards to the Log Analytics change: Is the goal to allow users to prevent the creation of the azurerm_monitor_diagnostic_setting.this resource? If yes, what's the reason for this request? (please note that this is a public repository, so no information related specifically to your company should be shared here. Feel free to contact me directly to explain your case.)

variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@hknutsen hknutsen changed the title Feat/logging public network feat: configure client certificate and public network access Sep 17, 2024
@hknutsen hknutsen added the enhancement New feature or request label Sep 17, 2024
@hknutsen hknutsen self-assigned this Sep 17, 2024
Adnan029 and others added 3 commits September 17, 2024 14:20
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
Co-authored-by: Henrik Simonsen Knutsen <[email protected]>
@Adnan029
Copy link
Contributor Author

Thanks for the swift response @hknutsen.

I have committed your suggestion and its looks good to me. Also, client certificate and public network access changes already applied to azurerm_windows_web_app.this.

In regards to the Log Analytics change: Is the goal to allow users to prevent the creation of the azurerm_monitor_diagnostic_setting.this resource? If yes, what's the reason for this request? (please note that this is a public repository, so no information related specifically to your company should be shared here. Feel free to contact me directly to explain your case.)

Yes, the goal is to allow users to prevent the creation of the azurerm_monitor_diagnostic_setting.this resource because we aim to provide flexibility in scenarios where logging is not required or desired.

@hknutsen
Copy link
Member

[...] client certificate and public network access changes already applied to azurerm_windows_web_app.this.

Didn't notice in my initial review, but yes, this looks to already be in place.

Yes, the goal is to allow users to prevent the creation of the azurerm_monitor_diagnostic_setting.this resource because we aim to provide flexibility in scenarios where logging is not required or desired.

I think it would be a good idea to remove the changes to related to the diagnostic setting in this PR, then create a separate PR where we can continue to discuss a potential implementation.

@hknutsen
Copy link
Member

hknutsen commented Sep 17, 2024

I'll create a quick PR with a suggested implementation for conditional diagnostic setting.

Update: see #216

@Adnan029
Copy link
Contributor Author

Really appreciate your prompt response @hknutsen. I have removed log diagnostic setting.

@hknutsen hknutsen merged commit f87655b into equinor:main Sep 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants