azurerm_nginx_deployment - support NGINX App Protect WAF #27454

arpith-f5 · 2024-09-20T22:15:29Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

NGINXaaS now supports NGINX App Protect WAF. In order to use WAF, customers need to explicitly enable it while creating/update their NGINXaaS deployment. This commit includes changes to support a new block nginx_app_protect in azurerm_nginx_deployment resource to enable/disable WAF while creation or updating an NGINXaaS deployment.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Manually tested create and update of a NGINXaaS deployment with WAF enabled/disabled

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_nginx_deployment - support NGINX App Protect WAF

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

NGINXaaS now supports NGINX App Protect WAF. This commit includes changes to enable/disable WAF while creation or updating an NGINXaaS deployment.

puneetsarna · 2024-09-25T00:41:48Z

Looks like some vendor checks are failing?

website/docs/r/nginx_deployment.html.markdown

internal/services/nginx/nginx_deployment_resource.go

internal/services/nginx/nginx_deployment_resource_test.go

internal/services/nginx/nginx_deployment_data_source.go

puneetsarna · 2024-10-04T22:48:32Z

I like the newly flattened structure (less nested) to invoke this new feature as well 👍

arpith-f5 · 2024-10-14T14:21:18Z

Hi @katbyte @stephybun, when you get time would you be able to take a look at this PR. Thanks in advance!

katbyte

Hi, we have a test failure needing fixing:

------- Stdout: -------
=== RUN   TestAccNginxDeploymentDataSource_nginxappprotect
=== PAUSE TestAccNginxDeploymentDataSource_nginxappprotect
=== CONT  TestAccNginxDeploymentDataSource_nginxappprotect
    testcase.go:173: Step 1/1 error: Error running apply: exit status 1
        
        Error: scaling is required for `sku` 'standardv2_Monthly', please provide `capacity` or `auto_scale_profiles`
        
          with azurerm_nginx_deployment.test,
          on terraform_plugin_test.tf line 75, in resource "azurerm_nginx_deployment" "test":
          75: resource "azurerm_nginx_deployment" "test" {
        
        scaling is required for `sku` 'standardv2_Monthly', please provide `capacity`
        or `auto_scale_profiles`
--- FAIL: TestAccNginxDeploymentDataSource_nginxappprotect (92.49s)
FAIL

puneetsarna · 2024-10-26T04:05:04Z

Hi @katbyte!! I see @arpith-f5 pushed a fix for the test. Can you please re-run the tests and see if the issue persists?

katbyte

Thanks for the pr @arpith-f5 - i've given it a quick review and made some schema suggestions. let me know what you think

katbyte · 2024-10-28T06:35:24Z

internal/services/nginx/nginx_deployment_data_source.go

@@ -194,10 +196,74 @@ func (m DeploymentDataSource) Attributes() map[string]*pluginsdk.Schema {
 			Computed: true,
 		},

+		"web_application_firewall_settings": {


settings is redundant, so we can remove it to also be more consistent with the rest of the provider

Suggested change

"web_application_firewall_settings": {

"web_application_firewall": {

I think this should be fine but the swagger calls it WebApplicationFirewallSettings and so that translates here fine in the client.

We can also check what we are doing for other client tools and keep the provider consistent with that as well to provide a uniform experience.

@katbyte I have been following the approach of keeping the provider consistent with the swagger spec which is the reason for using web_application_firewall_settings, activation_state and having the activation_state be an enum instead of a bool as per the swagger spec. I think deviating from the swagger spec will cause more confusion for users so would prefer to use the same naming. Do let me know if provider has a different set of naming convention that it needs to follow.

katbyte · 2024-10-29T17:35:42Z

internal/services/nginx/nginx_deployment_resource.go

@@ -262,6 +285,24 @@ func (m DeploymentResource) Arguments() map[string]*pluginsdk.Schema {
 				}, false),
 		},

+		"web_application_firewall_settings": {


settings is redundant, so we can remove it to also be more consistent with the rest of the provider

Suggested change

"web_application_firewall_settings": {

"web_application_firewall": {

internal/services/nginx/nginx_deployment_resource.go

katbyte · 2024-10-29T17:36:43Z

internal/services/nginx/nginx_deployment_resource.go

+			Optional: true,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"activation_state": {


if the block becomes web_application_firewall this would be better as activated or enabled? which would make it more consistent with the rest of the provided

addtionally is this the only setting that will be here or will there be further items

I am fine with either naming convention but would prefer what's already there as the WAF experience sounds fine to me. Will sync up with @arpith-f5 once to see if we want to change this.

katbyte · 2024-10-29T17:38:03Z

internal/services/nginx/nginx_deployment_resource.go

@@ -351,6 +392,19 @@ func (m DeploymentResource) Attributes() map[string]*pluginsdk.Schema {
 			Type:     pluginsdk.TypeString,
 			Computed: true,
 		},
+
+		"web_application_firewall_status": {


if the above block becomes web_application_firewall this could go inside as web_application_firewall.status.x ?

it could even be web_application_firewall.package_status.x and then web_application_firewall .component_versions

I initially wanted to follow this approach of nesting the status inside the web_application_firewall block but the problem I encountered is that the status needs to be an attribute but settings is an argument. I didn't find a good way to have them both in the same block. Do you have any suggestions on how to do that?

katbyte · 2024-10-29T17:39:46Z

internal/services/nginx/nginx_deployment_resource.go

+						Type:     pluginsdk.TypeString,
+						Required: true,
+						ValidateFunc: validation.StringInSlice(
+							[]string{
+								"Enabled",
+								"Disabled",
+							}, false),


this is a bool? so we should

Suggested change

Type: pluginsdk.TypeString,

Required: true,

ValidateFunc: validation.StringInSlice(

[]string{

"Enabled",

"Disabled",

}, false),

Type: pluginsdk.TypeBool,

Required: true,

and then in the provider code translate true/false to Enabled Disabled, unless there is going to be a 3rd state here?

The state is an enum.

internal/services/nginx/nginx_deployment_resource.go

katbyte

Thanks for the pr @arpith-f5 - i've given it a quick review and made some schema suggestions. let me know what you think

NGINXaaS App Protect

c2be86c

NGINXaaS now supports NGINX App Protect WAF. This commit includes changes to enable/disable WAF while creation or updating an NGINXaaS deployment.

arpith-f5 changed the title ~~NGINXaaS App Protect~~ azurerm_nginx_deployment - support NGINX App Protect WAF Sep 20, 2024

github-actions bot added documentation service/nginx size/L waiting-response labels Sep 20, 2024

puneetsarna reviewed Sep 25, 2024

View reviewed changes

website/docs/r/nginx_deployment.html.markdown Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

website/docs/r/nginx_deployment.html.markdown Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

internal/services/nginx/nginx_deployment_resource_test.go Outdated Show resolved Hide resolved

puneetsarna reviewed Sep 25, 2024

View reviewed changes

internal/services/nginx/nginx_deployment_data_source.go Outdated Show resolved Hide resolved

fix comments

9ad26b2

github-actions bot added size/XL and removed waiting-response size/L labels Sep 27, 2024

Merge branch 'hashicorp:main' into nginxaas_nap

795f4cb

arpith-f5 requested a review from puneetsarna September 27, 2024 21:51

github-actions bot added the waiting-response label Sep 27, 2024

update docs

4f6d8ec

arpith-f5 force-pushed the nginxaas_nap branch from c4c0a47 to 4f6d8ec Compare September 27, 2024 21:57

github-actions bot added waiting-response and removed waiting-response labels Sep 27, 2024

puneetsarna approved these changes Oct 4, 2024

View reviewed changes

github-actions bot removed the waiting-response label Oct 14, 2024

rcskosir added the enhancement label Oct 14, 2024

katbyte requested changes Oct 17, 2024

View reviewed changes

github-actions bot added the waiting-response label Oct 17, 2024

fix test

a9c6ce8

github-actions bot removed the waiting-response label Oct 17, 2024

arpith-f5 requested a review from katbyte October 17, 2024 03:39

katbyte requested changes Oct 29, 2024

View reviewed changes

github-actions bot added the waiting-response label Oct 29, 2024

fix comments

bd06e95

arpith-f5 requested a review from a team as a code owner November 5, 2024 19:24

github-actions bot removed the waiting-response label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_nginx_deployment - support NGINX App Protect WAF #27454

azurerm_nginx_deployment - support NGINX App Protect WAF #27454

arpith-f5 commented Sep 20, 2024

puneetsarna commented Sep 25, 2024

puneetsarna commented Oct 4, 2024

arpith-f5 commented Oct 14, 2024

katbyte left a comment

puneetsarna commented Oct 26, 2024

katbyte left a comment

katbyte Oct 28, 2024

puneetsarna Nov 5, 2024 •

edited

Loading

arpith-f5 Nov 5, 2024 •

edited

Loading

katbyte Oct 29, 2024

katbyte Oct 29, 2024

katbyte Oct 29, 2024

puneetsarna Nov 5, 2024

katbyte Oct 29, 2024

katbyte Oct 29, 2024

arpith-f5 Nov 5, 2024

katbyte Oct 29, 2024

puneetsarna Nov 5, 2024

katbyte left a comment

	"web_application_firewall_settings": {
	"web_application_firewall": {

azurerm_nginx_deployment - support NGINX App Protect WAF #27454

Are you sure you want to change the base?

azurerm_nginx_deployment - support NGINX App Protect WAF #27454

Conversation

arpith-f5 commented Sep 20, 2024

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

puneetsarna commented Sep 25, 2024

puneetsarna commented Oct 4, 2024

arpith-f5 commented Oct 14, 2024

katbyte left a comment

Choose a reason for hiding this comment

puneetsarna commented Oct 26, 2024

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puneetsarna Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

arpith-f5 Nov 5, 2024 • edited Loading

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

katbyte left a comment

Choose a reason for hiding this comment

puneetsarna Nov 5, 2024 •

edited

Loading

arpith-f5 Nov 5, 2024 •

edited

Loading