azurerm_netapp_volume - support NFSv4.1, vol sizing issue

[paulomarquesc]

Hi Terraform team,

I'm part of Azure NetApp team at Microsoft and I'm providing this PR that adds support for NFSv4.1 since we have a partner that needs this support to work internally and with their own customers.

This is a summary of changes I made:

• Bumped up the ANF's Go SDK version to the latest (2019-10-01)
• Implemented an attribute name change for the ExportPolicyRule that got renamed to Nfsv41 since api version 2019-07-01 and applied the same renaming concept to Go's own object properties to avoid confusion.
• Implemented the property called protocol_types as additional volume property since this is the one that controls what protocol the volume will support (it is defined as a list following our API definition but only one is supported) and included validation.
• Implemented some changes on Tests so we can check if a NFSv4.1 volume gets created, plus testing for NFSv3 value on basic volume test
• Stepped down the ServiceLevel from Premium to Standard on Tests so developer incur in less costs while working on ANF resource.
• Fixed a bug related to volume size that was limiting a volume to 4TB while the correct maximum size is 100TB.

The acceptance tests are working fine as you can see the results below:

Please let me know if this PR looks good or if any changes are needed.

Regards

Paulo

[katbyte]

Thanks for the PR @paulomarquesc,

In addition to the comments i've left inline the docs will need to be updated to reflect the changes.

[katbyte]

Thanks for the changes @paulomarquesc,
i've address your comments inline

[paulomarquesc]

Hi @katbyte , I completed the changes, please let me know if they're ok now or if I need to change anything else.

[paulomarquesc]

Hey @paulomarquesc, these changes are looking off to a good start but I've done some local testing and some of the constraints aren't quite right. I think we can squeeze this into 2.0 so we should be able to remove all the deprecated fields

@mbfrahry, awesome, I'll work on removing all the things that makes it non-breaking changes.

[paulomarquesc]

Hi @mbfrahry, @katbyte, just saw that v2 was released, can I still remove all extra code for non-breaking changes? And I also still have a last question that needs clarification regarding adding back the ForceNew attribute above, can you please take a look and clarify?

[mbfrahry]

Hey @paulomarquesc. Unfortunately, we still have to keep the breaking changes in until the next major release. For your ForceNew question, I saw we just have ForceNew there. Are there any options that you can swap it to other than NFSv3 or NFSv4.1? If so, we could write a CustomizeDiff that will ForceNew when those values are being changed from but I don't think it's necessary

[paulomarquesc]

Hey @paulomarquesc. Unfortunately, we still have to keep the breaking changes in until the next major release. For your ForceNew question, I saw we just have ForceNew there. Are there any options that you can swap it to other than NFSv3 or NFSv4.1? If so, we could write a CustomizeDiff that will ForceNew when those values are being changed from but I don't think it's necessary

Hi @mbfrahry, there is no migration path from one protocol to another, the volume needs to be recreated but we need to block the customer to inadvertently make this change and end up deleting production volumes, the only way I see is to remove ForceNew or implement an error if this change is coming, I looked at other parts of the AzureRm code and it looks like there is a way to raise the error.

About the breaking changes, ok, i'll keep those,

[paulomarquesc]

@mbfrahry, I need you guys help here, I tried using the following code change to do a pre-validation but it looks like it does not work the way I was envisioned:

	if d.HasChange("protocols") {
		return fmt.Errorf("`protocols` cannot be changed to a different protocol, a new volume needs to be created. `ProtocolTypesCanNotBeChanged`")
	}

When I added back the ForceNew = true, I ended up with my volume deleted and the error message came after it, so no use for this option.

As I mentioned before to @katbyte, unless there is a clear solution you can help me with, where there is zero chance that the customer volume will be deleted before any sort of validation, we will need to leave the ForceNew option out and let Azure bubble up the error.

This review process is already taking a long time and this is impacting one of the largest Azure customer that requires this change. I'm open for a joint session or any other solution that can speed up this PR.

Please let me know how we can make this happen.

[katbyte]

@paulomarquesc,

ForceNew is already on the name and many other resources where accidental deletion would be less than ideal. I don't see any reason to not mark this attribute as force new as the plan will show it to the user and it is inline with all other terraform resources. For users who are worried about accidental deletion.

resource "resource" "name" {
  lifecycle {
    prevent_destroy = true
  }
}

for further information see https://www.terraform.io/docs/configuration/resources.html#prevent_destroy

[paulomarquesc]

@katbyte, thanks, that is really helpful. I'll even add to the example and make a note in the documentation.

I still have another question pending here: #5485 (comment)

Can you please check? The issue is that terraform plan always states that a change will happen even without any changes in the TF file or in the environment as follows:

Terraform will perform the following actions:

  ~ azurerm_netapp_volume.example
      export_policy_rule.3167648489.allowed_clients.#:            "1" => "0"
      export_policy_rule.3167648489.allowed_clients.3397213214:   "10.0.0.0/20" => ""
      export_policy_rule.3167648489.rule_index:                   "1" => "0"
      export_policy_rule.3167648489.unix_read_only:               "false" => "false"
      export_policy_rule.3167648489.unix_read_write:              "true" => "false"
      export_policy_rule.3976225008.allowed_clients.#:            "0" => "1"
      export_policy_rule.3976225008.allowed_clients.3397213214:   "" => "10.0.0.0/20"
      export_policy_rule.3976225008.cifs_enabled:                 "" => <computed>
      export_policy_rule.3976225008.nfsv3_enabled:                "" => <computed>
      export_policy_rule.3976225008.nfsv4_enabled:                "" => <computed>
      export_policy_rule.3976225008.protocols_enabled.#:          "0" => "1"
      export_policy_rule.3976225008.protocols_enabled.2676449260: "" => "NFSv3"
      export_policy_rule.3976225008.rule_index:                   "" => "1"
      export_policy_rule.3976225008.unix_read_only:               "" => "false"
      export_policy_rule.3976225008.unix_read_write:              "" => "true"


Plan: 0 to add, 1 to change, 0 to destroy.

[katbyte]

@paulomarquesc, that looks like it may have to do with the export policies being a type set.. it might go away if you use a type list? might not be possibleto do the deprecations with typeset =/

[paulomarquesc]

@katbyte, yes, I didn't have this issue when originally I made this as TypeList.

[katbyte]

@paulomarquesc, the protocols or the export policy?

[katbyte]

Also if you hope onto our community slack we can have a real time discussion there

[paulomarquesc]

@paulomarquesc, the protocols or the export policy?

@katbyte, The issue is with Export policy. Protocols is working fine.

[katbyte]

Thanks for the revisions @paulomarquesc! this LGTM now 🚀

[ghost]

This has been released in version 2.1.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.1.0"
}
# ... other configuration ...

[ghost]

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!