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_function_app: Support for "client_cert_mode" and "client_cert_enabled" for Function Apps #11161

Merged
merged 6 commits into from
Apr 8, 2021

Conversation

howlowck
Copy link
Contributor

@howlowck howlowck commented Mar 30, 2021

PR closes #5006

Changes:

  • Implements client_cert_mode that takes in one of three possible values: "Required", "Optional", "Ignore"
    • Depending on the value, it will properly set the values of client_cert_enabled and client_cert_mode on the ARM client

TestResult:

❯ go test -timeout 30m -run ^TestAccFunctionApp_clientCertMode$ github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web -v
2021/03/30 15:31:30 [DEBUG] not using binary driver name, it's no longer needed
2021/03/30 15:31:30 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccFunctionApp_clientCertMode
=== PAUSE TestAccFunctionApp_clientCertMode
=== CONT  TestAccFunctionApp_clientCertMode
--- PASS: TestAccFunctionApp_clientCertMode (284.05s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web 284.133s

@howlowck howlowck changed the title azurerm_function_app: Support for "client_cert_mode" and "client_cert_enabled" for Function Apps azurerm_function_app: Support for "client_cert_mode" and "client_cert_enabled" for Function Apps Mar 31, 2021
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @howlowck

Thanks for this PR.

Taking a look through on the whole this looks pretty good - I've left some comments inline but if we can fix those up (and the tests pass) then this should be good to go 👍

Thanks!

azurerm/internal/services/web/function_app_resource.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/function_app_resource.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/function_app_resource.go Outdated Show resolved Hide resolved
azurerm/internal/services/web/function_app_resource.go Outdated Show resolved Hide resolved
website/docs/r/function_app.html.markdown Outdated Show resolved Hide resolved
@howlowck
Copy link
Contributor Author

howlowck commented Apr 7, 2021

Thank you @tombuildsstuff for the review! I committed your suggestions. I need to update the code a bit from there. One issue is the dereferencing of the *props.ClientCertMode, the construct for that is a enum of web.ClientCertMode.

@tombuildsstuff
Copy link
Contributor

@howlowck apologies, in which case we can do string(props.ClientCertMode) != "" instead of props.ClientCertMode != nil which should be fine 👍

@ghost ghost removed the waiting-response label Apr 7, 2021
@howlowck
Copy link
Contributor Author

howlowck commented Apr 7, 2021

@tombuildsstuff thanks for the guidance! I made the change regarding the enum to string conversion. I also fixed some minor logic and syntax issues. All tests now pass for data_source and resource:

Data Source Test:

❯ go test -timeout 30m -run ^TestAccFunctionAppDataSource_clientCertMode$ github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web -v
2021/04/07 16:05:32 [DEBUG] not using binary driver name, it's no longer needed
2021/04/07 16:05:32 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccFunctionAppDataSource_clientCertMode
=== PAUSE TestAccFunctionAppDataSource_clientCertMode
=== CONT  TestAccFunctionAppDataSource_clientCertMode
--- PASS: TestAccFunctionAppDataSource_clientCertMode (259.92s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web 260.054s

Resource Test:

❯ go test -timeout 30m -run ^TestAccFunctionApp_clientCertMode$ github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web -v
2021/04/07 16:16:09 [DEBUG] not using binary driver name, it's no longer needed
2021/04/07 16:16:09 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccFunctionApp_clientCertMode
=== PAUSE TestAccFunctionApp_clientCertMode
=== CONT  TestAccFunctionApp_clientCertMode
--- PASS: TestAccFunctionApp_clientCertMode (284.67s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web 284.807s

@howlowck
Copy link
Contributor Author

howlowck commented Apr 7, 2021

Data Source Test from latest push:

❯ go test -timeout 30m -run ^TestAccFunctionAppDataSource_clientCertMode$ github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web -v
2021/04/07 16:35:30 [DEBUG] not using binary driver name, it's no longer needed
2021/04/07 16:35:30 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccFunctionAppDataSource_clientCertMode
=== PAUSE TestAccFunctionAppDataSource_clientCertMode
=== CONT  TestAccFunctionAppDataSource_clientCertMode
--- PASS: TestAccFunctionAppDataSource_clientCertMode (265.58s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web 265.702s

@howlowck
Copy link
Contributor Author

howlowck commented Apr 7, 2021

I want to be clear on what this terraform client_cert_mode is doing to both ARM client_cert_enabled and ARM client_cert_mode.

Terraform client_cert_mode ARM client_cert_enabled ARM client_cert_mode
"" false not set
"Optional" true Optional
"Required" true Required

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for pushing those changes @howlowck

@tombuildsstuff tombuildsstuff added this to the v2.55.0 milestone Apr 8, 2021
@tombuildsstuff
Copy link
Contributor

Tests look good 👍

@tombuildsstuff tombuildsstuff merged commit a3891f9 into hashicorp:master Apr 8, 2021
tombuildsstuff added a commit that referenced this pull request Apr 8, 2021
@ghost
Copy link

ghost commented Apr 9, 2021

This has been released in version 2.55.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.55.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented May 8, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Function App support for client_cert_enabled for turning on Incoming client certificates
3 participants