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

azurerm_windows_web_app, azurerm_windows_web_app_slot, azurerm_linux_web_app, azurerm_linux _web_app_slot - fix auto heal slow request with path issue #20049

Merged
merged 13 commits into from
May 7, 2024

Conversation

xiaxyi
Copy link
Contributor

@xiaxyi xiaxyi commented Jan 17, 2023

In current provider, we only allow user to set the slow request rule with one path, but slow request rule with multiple paths is also supported by api, checking the property - slowRequestsWithPath:

The previous PR #18227 was closed but the issue was not fixed.

fix #19256
fix #22492

Local Acc test:

--- PASS: TestAccLinuxWebAppSlot_withAutoHealSlowRequestWithPath (428.62s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice    429.724s
--- PASS: TestAccWindowsWebAppSlot_withAutoHealSlowRequestWithPath (420.55s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice    421.585s
--- PASS: TestAccLinuxWebApp_withAutoHealRulesSlowRequestWithPath (399.84s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice    400.520s
--- PASS: TestAccWindowsWebApp_withAutoHealRulesSlowRequestWithPath (403.73s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/appservice    404.528s

image

@pietersap
Copy link

I can confirm that the linked issue 17862 is not fixed yet, even though the previous PR was closed because it was supposedly superseded by another one.

Tried today in Azure provider 3.39.1, the autoheal settings (sub_status and win32_status) are not set, and behaviour is still the same as described in the original issue.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @xiaxyi - Thanks for this and apologies again for the delayed review.

I think we have a design issue here. The two properties are present in the swagger, and as far as I can tell, both should be maintained / supported? Which means we should be able to simply add this as a new property? possibly as ConflictsWith on slow_requests if appropriate to only have one? WDYT?

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Apr 7, 2023

Thanks @jackofallops for the suggestion, I didn't remove the slow_request property and I did added the slow_request_with_path as a new property. Not sure what do you mean by "add it as a new property"?

@pietersap
Copy link

Looking forward to this merge

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @xiaxyi - Thanks for this. I've left some comments below if you can take a look?

Thanks!

internal/services/appservice/helpers/auto_heal.go Outdated Show resolved Hide resolved
internal/services/appservice/helpers/auto_heal.go Outdated Show resolved Hide resolved
internal/services/appservice/helpers/auto_heal.go Outdated Show resolved Hide resolved
internal/services/appservice/helpers/auto_heal.go Outdated Show resolved Hide resolved
count = "10"
interval = "00:10:00"
time_taken = "00:00:10"
path = null
Copy link
Member

Choose a reason for hiding this comment

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

This is an Optional field, so should be omitted. It will also cause the test to fail when the 4.0 flag is set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

website/docs/r/windows_web_app.html.markdown Show resolved Hide resolved
@xiaxyi
Copy link
Contributor Author

xiaxyi commented May 15, 2023

Thanks @jackofallops for the review, I updated the code, added the validation and deprecation message.

Regarding the documentation, shall we still remove the path field even though I added the deprecation message right below the property?

@jackofallops
Copy link
Member

Hi @xiaxyi - Apologies for the delay in re-review. I have found there's a crash behind the feature flag here, which I don't believe can be addressed in this PR as the schema and the underlying go structs will no longer be compatible when the feature flag is set to true, so I'm going to mark this as blocked for now whilst the team investigates how to mitigate this.

@jackofallops jackofallops added this to the Blocked milestone Sep 6, 2023
@tombuildsstuff tombuildsstuff removed this from the Blocked milestone Dec 7, 2023
@xiaxyi
Copy link
Contributor Author

xiaxyi commented Jan 15, 2024

@jackofallops I don't see any crash when running the tests, what about from your side? I see Tom removed the Block tag, would you like to share some updates with me? Thanks!

@katbyte
Copy link
Collaborator

katbyte commented Feb 6, 2024

Now that the API has been updated and resources migrated can you please resolve the conflicts here?

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Hi @xiaxyi - Thanks for this PR, and sorry it fell between the cracks for so long. There's some changes to take a look at below, but if you can get those sorted, I think this will be good to go.

Thanks!

type AutoHealSlowRequestWithPath struct {
TimeTaken string `tfschema:"time_taken"`
Interval string `tfschema:"interval"`
Count int `tfschema:"count"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Count int `tfschema:"count"`
Count int64 `tfschema:"count"`

@@ -47,6 +49,13 @@ type AutoHealSlowRequest struct {
Path string `tfschema:"path"`
Copy link
Member

Choose a reason for hiding this comment

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

To be able to remove this from the schema below, the struct needs to be tagged as below or we'll get a error in decoding.

Suggested change
Path string `tfschema:"path"`
Path string `tfschema:"path,removedInNextMajorVersion"`

trigger := webapps.SlowRequestsBasedTrigger{
TimeTaken: pointer.To(sr.TimeTaken),
TimeInterval: pointer.To(sr.Interval),
Count: pointer.To(int64(sr.Count)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Count: pointer.To(int64(sr.Count)),
Count: pointer.To(sr.Count),

sr := AutoHealSlowRequestWithPath{
TimeTaken: pointer.From(v.TimeTaken),
Interval: pointer.From(v.TimeInterval),
Count: int(pointer.From(v.Count)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Count: int(pointer.From(v.Count)),
Count: pointer.From(v.Count),

trigger := webapps.SlowRequestsBasedTrigger{
TimeTaken: pointer.To(sr.TimeTaken),
TimeInterval: pointer.To(sr.Interval),
Count: pointer.To(int64(sr.Count)),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Count: pointer.To(int64(sr.Count)),
Count: pointer.To(sr.Count),

@@ -790,7 +804,9 @@ A `trigger` block supports the following:

* `requests` - (Optional) A `requests` block as defined above.

* `slow_request` - (Optional) One or more `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` block as defined above.

@@ -773,6 +773,20 @@ A `slow_request` block supports the following:

* `path` - (Optional) The path for which this slow request rule applies.

~> **NOTE:** `path` in `slow_request` block will be deprecated in 4.0 provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
~> **NOTE:** `path` in `slow_request` block will be deprecated in 4.0 provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.
~> **NOTE:** The `path` property in the `slow_request` block is deprecated and will be removed in 4.0 of provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.

@@ -821,7 +835,9 @@ A `trigger` block supports the following:

* `requests` - (Optional) A `requests` block as defined above.

* `slow_request` - (Optional) One or more `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` block as defined above.

@@ -770,6 +770,20 @@ A `slow_request` block supports the following:

* `path` - (Optional) The path for which this slow request rule applies.

~> **NOTE:** `path` in `slow_request` block will be deprecated in 4.0 provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
~> **NOTE:** `path` in `slow_request` block will be deprecated in 4.0 provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.
~> **NOTE:** The `path` property in the `slow_request` block is deprecated and will be removed in 4.0 of provider. Please use `slow_request_with_path` to set a slow request trigger with path specified.

@@ -810,7 +824,9 @@ A `trigger` block supports the following:

* `requests` - (Optional) A `requests` block as defined above.

* `slow_request` - (Optional) One or more `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `slow_request` - (Optional) A `slow_request` blocks as defined above.
* `slow_request` - (Optional) A `slow_request` block as defined above.

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Mar 29, 2024

@jackofallops Thanks for the review, I updated the code as comments. Let me know if you have any other feedbacks.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @xiaxyi - Apologies for the very long tail on this one. LGTM now 👍

@jackofallops
Copy link
Member

Tests looking fine, failures are transient errors:

image

@jackofallops jackofallops merged commit c3e020f into hashicorp:main May 7, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.103.0 milestone May 7, 2024
jackofallops added a commit that referenced this pull request May 7, 2024
Copy link

github-actions bot commented Jun 8, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants