Skip to content

add suppressIPDiff and canonicalizeIP helpers -- use in resourceVultrFirewallRule#715

Merged
optik-aper merged 2 commits intovultr:masterfrom
wiggels:fix-subnet-normalization
Apr 20, 2026
Merged

add suppressIPDiff and canonicalizeIP helpers -- use in resourceVultrFirewallRule#715
optik-aper merged 2 commits intovultr:masterfrom
wiggels:fix-subnet-normalization

Conversation

@wiggels
Copy link
Copy Markdown
Contributor

@wiggels wiggels commented Apr 1, 2026

…FirewallRule (#714)

Description

Fix IPv6 subnet normalization drift on firewall rules. The Vultr API normalizes IPv6 addresses on return by stripping leading zeros per RFC 5952 (e.g., 05ff becomes 5ff). This causes Terraform to detect a perpetual diff and attempt to recreate the rule on every plan.

Adds a DiffSuppressFunc that compares parsed IP bytes instead of strings, and a StateFunc that canonicalizes the subnet on write so state always matches the API response. Handles both v4 and v6 transparently.

Related Issues

Fixes #714

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally?

@wiggels wiggels requested a review from optik-aper as a code owner April 1, 2026 00:25
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

714 - Partially compliant

Compliant requirements:

  • Added DiffSuppressFunc (suppressIPDiff) that compares parsed IP bytes instead of strings
  • Added StateFunc (canonicalizeIP) that canonicalizes subnet on write to match API response
  • Both IPv4 and IPv6 are handled transparently
  • Tests verify leading zero stripping, compression, mixed case hex, and different address detection

Non-compliant requirements:

  • None identified

Requires further human verification:

  • None - the fix is fully contained in the PR and testable via unit tests
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add safe type assertion to prevent panic

The type assertion val.(string) will panic if val is not a string (e.g., if
Terraform passes nil or a different type). Use a type assertion with comma-ok idiom
to safely handle unexpected types.

vultr/utility.go [69-76]

 func canonicalizeIP(val interface{}) string {
-	raw := val.(string)
+	raw, ok := val.(string)
+	if !ok {
+		return ""
+	}
 	ip := net.ParseIP(raw)
 	if ip == nil {
 		return raw
 	}
 	return ip.String()
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential panic scenario from unsafe type assertion. While this is a defensive improvement, Terraform's StateFunc typically receives the correct type based on the schema definition, so the risk is low. The fix is valid and improves robustness.

Low

Copy link
Copy Markdown
Contributor

@optik-aper optik-aper left a comment

Choose a reason for hiding this comment

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

Good to merge

@optik-aper optik-aper merged commit 9c2311e into vultr:master Apr 20, 2026
1 of 3 checks passed
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] - vultr_firewall_rule: IPv6 subnet normalization causes forced replacement on every plan

2 participants