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_kubernetes_cluster: Support for CSI arguments #19396

Merged
merged 11 commits into from
Dec 7, 2022

Conversation

CorrenSoft
Copy link
Contributor

May solve #19235

Added 3 optional properties and an optional block (because the disk driver has an additional parameter to specify the version).

Example of implementation:

resource "azurerm_kubernetes_cluster" "aks" {
   
  ...

  storage_disk_driver {
    enabled = true
    version = "V1"
  }
  storage_blob_driver         = true
  storage_file_driver         = false
  storage_snapshot_controller = false
}

Considering the SDK's Object Model, I have two questions on my mind, and I would like to have some feedback:
1- Should all be blocks instead of properties, to allow easy expansion if in the future extra fields are added?
2- Should be all this encapsulated in a single block instead of individual components?

Note: The docs are yet to be updated, will do so upon receiving feedback on the code.

@stevehipwell
Copy link

@CorrenSoft I can confirm that this PR would resolve #19235.

My suggestion would be to use blocks and to potentially use a primary storage block to wrap the other ones.

Also is there a reason why you're using V1 instead of v1 for the version?

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @CorrenSoft, this is off to a great start!

As suggested by @stevehipwell, I think these would benefit from being grouped into a block called storage_profile. I also left a few more comments in-line regarding the property names and the property enabled which we don't typically expose in Terraform.

Other than that as you've mentioned the docs need to be updated, and we also need to add some tests around this.

@CorrenSoft
Copy link
Contributor Author

@stevehipwell, I've used V1 because that was the value that I got when I read it from an existing cluster, and also because in any reference that I could find about the valid values they were in uppercase.

@stephybun, thanks for the feedback :)
Just to confirm that I am properly understanding, you are saying that the implementation should be like this:

storage_profile {
  blob__driver_enabled        = true
  disk_driver_enabled         = true
  disk_driver_version         = "V1"
  file_driver_enabled         = false
  snapshot_controller_enabled = false
}

@stephybun
Copy link
Member

@CorrenSoft that's correct 🙂

@stevehipwell
Copy link

@CorrenSoft I'd suggest that v1 works better for the Terraform API version as it's the industry wide idiomatic representation of a version. @stephybun is there a convention for this provider or Terraform in general which applies to this scenario?

@CorrenSoft CorrenSoft marked this pull request as draft December 5, 2022 15:29
@CorrenSoft CorrenSoft marked this pull request as ready for review December 5, 2022 21:59
@CorrenSoft
Copy link
Contributor Author

@stevehipwell I changed the values to v1 and v2. In any case, values are not case-sensitive.

@stephybun I completed the refactor, updated the docs and added a new test (creates cluster with values different from default). Let me know if anything else is needed.

Note: Still running the Acc Tests.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes @CorrenSoft and for touching up the docs! The tests are currently running but I don't expect any issues, aside from the few comments left in-line I have one more question which is, are these properties actually updateable?

website/docs/d/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@stephybun
Copy link
Member

stephybun commented Dec 6, 2022

@stevehipwell the convention in the AzureRM provider is to use whatever casing is specified in the API spec/enums provided by Azure. Since there are none for these and the API seems to accept both upper and lower casing I would agree with you that we should go with the industry wide convention for representing versions.

As an aside in the swagger they used lower case v1 in the description.

@stevehipwell
Copy link

@stevehipwell the convention in the AzureRM provider is to use whatever casing is specified in the API spec/enums provided by Azure. Since there are none for these and the API seems to accept both upper and lower casing I would agree with you that we should go with the industry wide convention for representing versions.

As an aside in the swagger they used lower case v1 in the description.

They used lowercase in the docs too but we've all been caught out by mixed case sensitivity in Azure.

@CorrenSoft
Copy link
Contributor Author

I have one more question which is, are these properties actually updateable?

Yes, all of them. The only catch is if you try to use the v2 for the disk version because a preview feature is required (already included a note in the doc).

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Great work on this, thanks @CorrenSoft. LGTM 💯

@stephybun stephybun linked an issue Dec 7, 2022 that may be closed by this pull request
1 task
@stephybun stephybun merged commit 526b61d into hashicorp:main Dec 7, 2022
@github-actions github-actions bot added this to the v3.35.0 milestone Dec 7, 2022
@CorrenSoft CorrenSoft deleted the issue-19235 branch December 7, 2022 13:42
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

This functionality has been released in v3.35.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!

@github-actions
Copy link

github-actions bot commented Jan 9, 2023

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 Jan 9, 2023
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.

Support for azurerm_kubernetes_cluster CSI arguments
4 participants