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

Backwards compatible fix for Cluster Variable #13280

Conversation

apeschel
Copy link

@apeschel apeschel commented Sep 8, 2021

#13099

This default value causes clusters to be rebuilt on existing
deployments. Simply removing the default value should be sufficient for
preventing the rebuilds from occuring.

If someone needs to explicitly set this to false for some reason, they
can still do that manually. This is a preferable situation to existing
users not being able to upgrade without a complete cluster rebuild.

hashicorp#13099

This default value causes clusters to be rebuilt on existing
deployments. Simply removing the default value should be sufficient for
preventing the rebuilds from occuring.

If someone needs to explicitly set this to false for some reason, they
can still do that manually. This is a preferable situation to existing
users not being able to upgrade without a complete cluster rebuild.
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 for this @apeschel - i think we should instead swap this to be computed rather then remove the default entirely? WDYT?

@@ -592,7 +592,6 @@ func resourceKubernetesCluster() *pluginsdk.Resource {
"private_cluster_public_fqdn_enabled": {
Type: pluginsdk.TypeBool,
Optional: true,
Default: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably just make this computed otherwise it could result in cluster rebuilds for some users?

Copy link
Contributor

Choose a reason for hiding this comment

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

The default value is not causing the cluster to be re-created. I put a comment in the issue. I think the default must be kept to false and the forceNew set to false. This is forceNew causing the cluster to be re-created.
This happens as Microsoft are releasing a new feature on private clusters that give an option to expose the private ip of the cluster to a public fqdn. This feature is exposed on the REST API used by version 2.73+ hence causing the cluster to be re-created.
New clusters created from 2.73.0 are created with private_cluster_public_fqdn_enabled = false as per expected behavior with public aks documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this default will will set this to the default value for a bool - which is false - so this PR won't have any affect anyway, unfortunately

Is it now possible to change this field without destroying/re-creating the cluster via the API? This was historically the case (and is why this is ForceNew) - so we'd need to confirm that to be able to determine if we can remove ForceNew here

@mbfrahry
Copy link
Member

Hey @apeschel, I'm going to close this down since the parent issue was closed by #13413 which removed the ForceNew tag. Feel free to reopen this issue if you think this change is still necessary

@apeschel
Copy link
Author

@mbfrahry Works for me, thanks!

@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 Nov 26, 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.

5 participants