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/make azurerm_firewall_policy premium tier features available #12769

Merged

Conversation

gro1m
Copy link
Contributor

@gro1m gro1m commented Jul 28, 2021

Related Issues:

What could/has to be still improved:

  • Tests should be done a bit better, but help could be needed there,among others need to provision a key vault to read out the secret.
  • Update docs

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

thanks @gro1m - aside from one comment about schema and the docs need to be updated i think this looks good!

@gro1m gro1m requested a review from katbyte July 29, 2021 15:12
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @gro1m - looks good! just. need to fix up the tests and this should be good to go:

Test ended in panic.

------- Stdout: -------
=== RUN   TestAccFirewallPolicy_requiresImport
=== PAUSE TestAccFirewallPolicy_requiresImport
=== CONT  TestAccFirewallPolicy_requiresImport

------- Stderr: -------
panic: Invalid address to set: []string{"transport_security"}

goroutine 450 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc0001b4e80, 0x60b5bc9, 0x12, 0x56b5060, 0xc0022d9698, 0x0, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource_data.go:230 +0x371
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/firewall.resourceFirewallPolicyRead(0xc0001b4e80, 0x585b660, 0xc000a57880, 0x0, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/azurerm/internal/services/firewall/firewall_policy_resource.go:430 +0xb53
github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/firewall.resourceFirewallPolicyCreateUpdate(0xc0001b4e80, 0x585b660, 0xc000a57880, 0x0, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/azurerm/internal/services/firewall/firewall_policy_resource.go:377 +0xdb0
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0xc0002bfce0, 0x6837788, 0xc00118f8c0, 0xc0001b4e80, 0x585b660, 0xc000a57880, 0x0, 0x0, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource.go:318 +0x1ee
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0xc0002bfce0, 0x6837788, 0xc00118f8c0, 0xc001526b60, 0xc002076ce0, 0x585b660, 0xc000a57880, 0x0, 0x0, 0x0, ...)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/resource.go:456 +0x67b
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0xc0028b53e0, 0x6837788, 0xc00118f8c0, 0xc0032b25a0, 0xc00118f8c0, 0x5d50440, 0xc003661e00)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema/grpc_provider.go:955 +0x8ef
github.com/hashicorp/terraform-plugin-go/tfprotov5/server.(*server).ApplyResourceChange(0xc002112cc0, 0x6837830, 0xc00118f8c0, 0xc0015268c0, 0xc002112cc0, 0xc003661e90, 0xc001cd9ba0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-go/tfprotov5/server/server.go:332 +0xb5
github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5._Provider_ApplyResourceChange_Handler(0x5d50440, 0xc002112cc0, 0x6837830, 0xc003661e90, 0xc001b1c8a0, 0x0, 0x6837830, 0xc003661e90, 0xc0019fc400, 0x3a0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/github.com/hashicorp/terraform-plugin-go/tfprotov5/internal/tfplugin5/tfplugin5_grpc.pb.go:380 +0x214
google.golang.org/grpc.(*Server).processUnaryRPC(0xc000d4efc0, 0x6884018, 0xc002406a80, 0xc001902b40, 0xc0014ac390, 0x9e2d9a0, 0x0, 0x0, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/google.golang.org/grpc/server.go:1292 +0x52b
google.golang.org/grpc.(*Server).handleStream(0xc000d4efc0, 0x6884018, 0xc002406a80, 0xc001902b40, 0x0)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/google.golang.org/grpc/server.go:1617 +0xd0c
google.golang.org/grpc.(*Server).serveStreams.func1.2(0xc0035b01b0, 0xc000d4efc0, 0x6884018, 0xc002406a80, 0xc001902b40)
	/opt/teamcity-agent/work/a73be106926a7472/vendor/google.golang.org/grpc/server.go:940 +0xab
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/opt/teamcity-agent/work/a73be106926a7472/vendor/google.golang.org/grpc/server.go:938 +0x1fd

@katbyte katbyte added this to the v2.70.0 milestone Jul 29, 2021
@gro1m
Copy link
Contributor Author

gro1m commented Jul 29, 2021

@katbyte Adressed in latest commit :)

@gro1m gro1m requested a review from katbyte July 29, 2021 18:23
@katbyte
Copy link
Collaborator

katbyte commented Jul 29, 2021

Thanks @gro1m - checked the tests results and am now seeing :

------- Stdout: -------
=== RUN   TestAccFirewallPolicy_completePremium
=== PAUSE TestAccFirewallPolicy_completePremium
=== CONT  TestAccFirewallPolicy_completePremium
    testcase.go:88: Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Reference to undeclared resource
        
          on terraform_plugin_test.tf line 25, in resource "azurerm_key_vault" "test":
          25:     object_id = "${data.azuread_service_principal.test.object_id}"
        
        A data resource "azuread_service_principal" "test" has not been declared in
        the root module.
        
        Error: Reference to undeclared resource
        
          on terraform_plugin_test.tf line 86, in resource "azurerm_firewall_policy" "test":
          86:     key_vault_secret_id = data.azurem_key_vault.test.id
        
        A data resource "azurem_key_vault" "test" has not been declared in the root
        module.
--- FAIL: TestAccFirewallPolicy_completePremium (10.59s)
FAIL

@katbyte katbyte modified the milestones: v2.70.0, v2.71.0 Jul 30, 2021
@gro1m
Copy link
Contributor Author

gro1m commented Aug 19, 2021

Hi @manicminer @katbyte
I fixed some stuff, but I am stuck now at the following error:

make acctests SERVICE='firewall' TESTARGS='-run=TestAccFirewallPolicy_completePremium' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/firewall -run=TestAccFirewallPolicy_completePremium -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccFirewallPolicy_completePremium
=== PAUSE TestAccFirewallPolicy_completePremium
=== CONT  TestAccFirewallPolicy_completePremium
panic: Invalid address to set: []string{"intrusion_detection", "0", "signature_overrides", "0", "ID"}

Could you help me fix this please?
Also not sure how to fix the commented ConflictsWith lines 215 and 237 - also help appreciated there, as the uncommented line does not work...

@katbyte katbyte modified the milestones: v2.73.0, v2.74.0 Aug 20, 2021
@manicminer
Copy link
Contributor

Hi @gro1m, you're on the right track. The flattenFirewallPolicyIntrusionDetection needs to return a valid data structure for Terraform to set in state. Whilst the basic structure of the return variable looks ok, each value needs to be converted to its primitive type, e.g. string, int or bool. Where these are pointers in the model struct, they should be dereferenced accordingly, and a default value should always be set.

For example:

func flattenFirewallPolicyIntrusionDetection(input *network.FirewallPolicyIntrusionDetection) []interface{} {
	if input == nil {
		return []interface{}{}
	}

	signatureOverrides := make([]interface{}, 0)
	if overrides := input.Configuration.SignatureOverrides; overrides != nil {
		for _, override := range *overrides {
			id := ""
			if override.ID != nil {
				id = *override.ID
			}
			signatureOverrides = append(signatureOverrides, map[string]interface{}{
				"id": id,
				"state": string(override.Mode),
			})
		}
	}

	return []interface{}{
		map[string]interface{}{
			"mode":                string(input.Mode),
			"signature_overrides": signatureOverrides,
			// ...
		},
	}
}

(Noting that fields stored as pointers are first defaulted, then dereferenced if not nil)

@gro1m
Copy link
Contributor Author

gro1m commented Aug 23, 2021

Hi @manicminer
I tried to adjust the function with the latest commit, but at the moment I can not test it, so would be good if you could tell me if it is good like this or not. Thank you!

@manicminer
Copy link
Contributor

@gro1m Thanks, that seems to work. I removed the lifecycle blocks from the test configs and made the protocol case insensitive as the API casing is inconsistent with the SDK.

The azurerm_key_vault resource in the test config is problematic - the certificate doesn't match the resource properties. Is it necessary to use a pre-prepared certificate for this, or can we have key vault generate a cert/key instead?

@gro1m
Copy link
Contributor Author

gro1m commented Aug 24, 2021

Hi @manicminer
A colleague of mine tried to use a key vault generated certificate 3 months ago and at that point this did not work. Not sure how it is now. So not sure what we shall do here: can you maybe expand by what you mean with "the certificate does not match the resource properties"?

@manicminer
Copy link
Contributor

OK, I'll give it a try. The values in the x509_certificate_properties block don't match those in the certificate, e.g. the CN and the keyUsage fields are different; this seems to be causing a persistent diff since it looks like KV (correctly) uses the values from the certificate.

example output
          # azurerm_key_vault_certificate.test must be replaced
        -/+ resource "azurerm_key_vault_certificate" "test" {
              ~ certificate_attribute   = [
                  - {
                      - created        = "2021-08-24T13:10:00Z"
                      - enabled        = true
                      - expires        = "2031-04-20T19:58:10Z"
                      - not_before     = "2021-04-22T19:58:10Z"
                      - recovery_level = "Recoverable+Purgeable"
                      - updated        = "2021-08-24T13:10:00Z"
                    },
                ] -> (known after apply)
              ~ certificate_data        = "308203923082027A020900D8D40E1A52F4D9D1300D06092A864886F70D01010B050030818A310B3009060355040613024348310B300906035504080C025A48310B300906035504070C025A4831123010060355040A0C095465727261666F726D310E300C060355040B0C05417A7572653118301606035504030C0F7777772E636F6E746F736F2E636F6D3123302106092A864886F70D0109011614776861746576657240636F6E746F736F2E636F6D301E170D3231303432323139353831305A170D3331303432303139353831305A30818A310B3009060355040613024348310B300906035504080C025A48310B300906035504070C025A4831123010060355040A0C095465727261666F726D310E300C060355040B0C05417A7572653118301606035504030C0F7777772E636F6E746F736F2E636F6D3123302106092A864886F70D0109011614776861746576657240636F6E746F736F2E636F6D30820122300D06092A864886F70D01010105000382010F003082010A0282010100E6BC777D337451457592D7ADCCCD8010827864A42589BACBB553913D7D8B429D9597F9FBE033C3D917BF6603B636D8E18C8B583BAE594AA3A01B3DD1F040F8AF5D80A242C2166AA10679EBE08C9B79A428B3BA6B8CB2B0488CA3E4D6D5306BB334EE8FE795B5CD331F57E0ACB5B0AD5ED44F6BBCE2CD78D86D3DDD7E9C3C16024B87DB1559A8823B7E0DBBD3D617D351225CC87BF124DECB3F26736AB241FEC67C60C6AEBCB256996B8AA88C18E22E5FC8748B3803D0EDC899C577DF71BA1F274A1320E3291C70FAFDFFE6016B3F3F11FEFFCE65E3DE0F432D3AC86BB255EACF34579A1520D685ECA5EEF639A30FE6B4DD2B11FFC78A5FA2BEC4306DAB1A73F30203010001300D06092A864886F70D01010B050003820101000E19EDA49C25718DED3A8A17E1CE9AE0EE54CCEC60A0897A9AE1F742570AB4849018A4C836F45EC37F7384FF3543FE6CDD45665EDB2553638EDF60511F0CA3269067A982A202B34C137E105285A9BCBE4936C837284F4D4B6ABF889B7193A0FDC948CE5815BAB16A6D11FC056C904E5A1D0FAD20653072EC168638CA42E5AE6B9F6D2345128E019A2B7719E6DC853903DC137645CE7756337473A233471F9509393EF3B7845E6C1CC646970D76B6E43AD564455648C07D3025F9B8E3347E2F7E948695D477C5C62AFB9A28596A261C3FF14B58CD75D7E5BC076FF25390E2D46ECF20A1B1B6DE5161C571B9421AADC2A6271D7470F4985C96FF29C1E1209A19E3" -> (known after apply)
              ~ certificate_data_base64 = "MIIDkjCCAnoCCQDY1A4aUvTZ0TANBgkqhkiG9w0BAQsFADCBijELMAkGA1UEBhMCQ0gxCzAJBgNVBAgMAlpIMQswCQYDVQQHDAJaSDESMBAGA1UECgwJVGVycmFmb3JtMQ4wDAYDVQQLDAVBenVyZTEYMBYGA1UEAwwPd3d3LmNvbnRvc28uY29tMSMwIQYJKoZIhvcNAQkBFhR3aGF0ZXZlckBjb250b3NvLmNvbTAeFw0yMTA0MjIxOTU4MTBaFw0zMTA0MjAxOTU4MTBaMIGKMQswCQYDVQQGEwJDSDELMAkGA1UECAwCWkgxCzAJBgNVBAcMAlpIMRIwEAYDVQQKDAlUZXJyYWZvcm0xDjAMBgNVBAsMBUF6dXJlMRgwFgYDVQQDDA93d3cuY29udG9zby5jb20xIzAhBgkqhkiG9w0BCQEWFHdoYXRldmVyQGNvbnRvc28uY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA5rx3fTN0UUV1ktetzM2AEIJ4ZKQlibrLtVORPX2LQp2Vl/n74DPD2Re/ZgO2NtjhjItYO65ZSqOgGz3R8ED4r12AokLCFmqhBnnr4IybeaQos7prjLKwSIyj5NbVMGuzNO6P55W1zTMfV+CstbCtXtRPa7zizXjYbT3dfpw8FgJLh9sVWaiCO34Nu9PWF9NRIlzIe/Ek3ss/JnNqskH+xnxgxq68slaZa4qojBjiLl/IdIs4A9DtyJnFd99xuh8nShMg4ykccPr9/+YBaz8/Ef7/zmXj3g9DLTrIa7JV6s80V5oVINaF7KXu9jmjD+a03SsR/8eKX6K+xDBtqxpz8wIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQAOGe2knCVxje06ihfhzprg7lTM7GCgiXqa4fdCVwq0hJAYpMg29F7Df3OE/zVD/mzdRWZe2yVTY47fYFEfDKMmkGepgqICs0wTfhBSham8vkk2yDcoT01Lar+Im3GToP3JSM5YFbqxam0R/AVskE5aHQ+tIGUwcuwWhjjKQuWua59tI0USjgGaK3cZ5tyFOQPcE3ZFzndWM3RzojNHH5UJOT7zt4RebBzGRpcNdrbkOtVkRVZIwH0wJfm44zR+L36UhpXUd8XGKvuaKFlqJhw/8UtYzXXX5bwHb/JTkOLUbs8gobG23lFhxXG5QhqtwqYnHXRw9Jhclv8pweEgmhnj" -> (known after apply)
              ~ id                      = "https://tlskv210824140712311381.vault.azure.net/certificates/AzureFirewallPolicyCertificate/d1914baa2eb046acb40f3fccac5a427b" -> (known after apply)
                name                    = "AzureFirewallPolicyCertificate"
              ~ secret_id               = "https://tlskv210824140712311381.vault.azure.net/secrets/AzureFirewallPolicyCertificate/d1914baa2eb046acb40f3fccac5a427b" -> (known after apply)
              ~ thumbprint              = "89CD395A2E0952B6B0FF29A05074FF16F026C2B6" -> (known after apply)
              ~ version                 = "d1914baa2eb046acb40f3fccac5a427b" -> (known after apply)
                # (1 unchanged attribute hidden)
        
              ~ certificate {
                    # (1 unchanged attribute hidden)
                }
        
              ~ certificate_policy {
        
                  ~ key_properties {
                      + curve      = (known after apply)
                        # (4 unchanged attributes hidden)
                    }
        
        
                  ~ x509_certificate_properties {
                      ~ extended_key_usage = [ # forces replacement
                          + "1.3.6.1.5.5.7.3.1",
                          + "1.3.6.1.5.5.7.3.2",
                        ]
                      ~ key_usage          = [ # forces replacement
                          + "cRLSign",
                          + "dataEncipherment",
                          + "digitalSignature",
                          + "keyAgreement",
                          + "keyCertSign",
                          + "keyEncipherment",
                        ]
                      ~ subject            = "[email protected], CN=www.contoso.com, OU=Azure, O=Terraform, L=ZH, S=ZH, C=CH" -> "CN=api.pluginsdk.io" # forces replacement
                      ~ validity_in_months = 120 -> 1 # forces replacement
        
                        # (1 unchanged block hidden)
                    }
                    # (2 unchanged blocks hidden)
                }
            }

@gro1m
Copy link
Contributor Author

gro1m commented Aug 25, 2021

Hi @manicminer
I will try with importing PFX as documented here:

using

openssl pkcs12 -export -out certificate.pfx -inkey privateKey.key -in certificate.pem

And otherwise I will proceed as documented in the following and do not submit a certificate:

At the moment I did not do it correctly, as it seems :)

@gro1m gro1m requested a review from manicminer August 25, 2021 16:00
@gro1m
Copy link
Contributor Author

gro1m commented Aug 25, 2021

OK, I'll give it a try. The values in the x509_certificate_properties block don't match those in the certificate, e.g. the CN and the keyUsage fields are different; this seems to be causing a persistent diff since it looks like KV (correctly) uses the values from the certificate.

example output

@manicminer
I think this should be resolved now with commit e1062f5.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

@gro1m Great work! The tests are passing and the changes LGTM

Screenshot 2021-08-25 at 23 44 28

@manicminer manicminer merged commit d61302e into hashicorp:main Aug 25, 2021
manicminer added a commit that referenced this pull request Aug 25, 2021
@github-actions
Copy link

This functionality has been released in v2.74.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@gro1m gro1m deleted the fix/azurerm_firewall_policy_premium_tier branch August 29, 2021 11:04
@github-actions
Copy link

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 Sep 29, 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.

4 participants