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(server): migration to explicit primary IPs fails #597

Closed
wants to merge 1 commit into from

Conversation

apricote
Copy link
Member

When migrating a server from the previously used implicit primary IPs to using the hcloud_primary_ip resources and specifying those in the hcloud_server.public_net argument failed, because the state for the public_net was not correctly populated. This caused the servers to require an unnecessary re-attach of the primary ips.

Fixes #577

When migrating a server from the previously used implicit primary IPs to
using the `hcloud_primary_ip` resources and specifying those in the
`hcloud_server.public_net` argument failed, because the state for
the `public_net` was not correctly populated. This caused the servers to
require an unnecessary re-attach of the primary ips.

Fixes #577
@apricote apricote added the bug label Nov 23, 2022
@apricote apricote self-assigned this Nov 23, 2022
@github-actions
Copy link

This PR has been marked as stale because it has not had recent activity. The bot will close the PR if no further action occurs.

@github-actions github-actions bot added the stale label Oct 10, 2023
@apricote
Copy link
Member Author

Closing as I have not managed to fix the bugs that came with my changes. This will be easier to implement after #752

@apricote
Copy link
Member Author

apricote commented Sep 4, 2024

A more robust implementation of publicNetToTerraformPublicNet that avoids empty sets:

func publicNetToTerraformPublicNet(publicNet hcloud.ServerPublicNet) map[string]interface{} {
	tfPublicNet := make(map[string]interface{})

	ipv4Enabled := !publicNet.IPv4.IsUnspecified()
	tfPublicNet["ipv4_enabled"] = ipv4Enabled
	if ipv4Enabled {
		tfPublicNet["ipv4"] = publicNet.IPv4.ID
	}

	ipv6Enabled := !publicNet.IPv6.IsUnspecified()
	tfPublicNet["ipv6_enabled"] = ipv6Enabled
	if ipv6Enabled {
		tfPublicNet["ipv6"] = publicNet.IPv6.ID
	}

	return tfPublicNet
}

@apricote
Copy link
Member Author

apricote commented Sep 4, 2024

Adding computed: true to the attribute causes issues with the transition from an explicitly defined public_net to a removed public_net.

  • when the user removes the public_net block, the default should be restored. The default is generated random IPv4 and IPv6
  • if we mark the field as computed, Terraform will not mark the fields as changed and keep the values we previously wrote to the state.
  • Because of this, we can not detect that the user has removed the attribute, and can not trigger the logic to generate random IPs

This can only be solved by the plugin framework, which gives us access to the precise configuration that the user specified: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-framework/resource#UpdateRequest

@jooola jooola added the requires: plugin-framework The issue can be fixed after the resource has be migrated to the plugin framework. label Dec 13, 2024
@jooola
Copy link
Member

jooola commented Dec 13, 2024

Reopening as the issue is still relevant, we are just being blocked by the migration to the plugin framework.

@jooola jooola reopened this Dec 13, 2024
@jooola jooola requested a review from a team as a code owner December 13, 2024 08:36
@jooola
Copy link
Member

jooola commented Dec 13, 2024

Nevermind, I thought it was an issue, not a PR.

@jooola jooola closed this Dec 13, 2024
@jooola jooola removed the requires: plugin-framework The issue can be fixed after the resource has be migrated to the plugin framework. label Dec 13, 2024
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@06139fb). Learn more about missing BASE report.

Files with missing lines Patch % Lines
internal/server/resource.go 0.00% 19 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #597   +/-   ##
=======================================
  Coverage        ?   30.56%           
=======================================
  Files           ?       66           
  Lines           ?     9956           
  Branches        ?        0           
=======================================
  Hits            ?     3043           
  Misses          ?     6873           
  Partials        ?       40           
Flag Coverage Δ
unit 30.56% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: importing primary IPs generally buggy
2 participants