Skip to content

Conversation

@mondragonfx
Copy link
Contributor

@mondragonfx mondragonfx commented May 15, 2025

User description

Description

Related Issues

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?

PR Type

Enhancement


Description

  • Add Auto SSL support to load balancer resource

  • Expose auto_ssl_domain in data source and resource

  • Update documentation for auto SSL domain usage

  • Add domain parsing and validation helper


Diagram Walkthrough

flowchart LR
  LB["Load Balancer"] -- "adds" --> AS["AutoSSL Domain"]
  AS -- "computed" --> DS["Data Source"]
  AS -- "optional" --> RS["Resource"]
  DOC["Documentation"] -- "updated" --> AS
Loading

File Walkthrough

Relevant files
Enhancement
data_source_vultr_load_balancer.go
Expose auto SSL domain in data source                                       

vultr/data_source_vultr_load_balancer.go

  • Add auto_ssl_domain computed string field to schema
  • Read and set auto_ssl_domain from API response
+7/-1     
resource_vultr_load_balancer.go
Add Auto SSL domain to load balancer resource                       

vultr/resource_vultr_load_balancer.go

  • Add auto_ssl_domain optional string field to schema
  • Include AutoSSL in create and update requests
  • Add generateAutoSSL helper for domain parsing
  • Handle auto SSL enable/disable on updates
+61/-3   
Documentation
load_balancer.html.markdown
Document auto SSL domain in data source docs                         

website/docs/d/load_balancer.html.markdown

  • Document new auto_ssl_domain attribute
+2/-0     
load_balancer.html.markdown
Document auto SSL domain in resource docs                               

website/docs/r/load_balancer.html.markdown

  • Document new auto_ssl_domain argument
  • Document new auto_ssl_domain exported attribute
+2/-0     

@mondragonfx mondragonfx requested a review from optik-aper May 15, 2025 17:38
@optik-aper
Copy link
Member

optik-aper commented May 15, 2025

  • Add auto ssl to load balancer resource read
  • Add auto ssl to load balancer resource update
  • Add auto ssl to load balancer data source read
  • Document data source attributes

@mondragonfx mondragonfx requested a review from optik-aper May 21, 2025 17:34
Copy link
Member

@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.

This is currently getting a dirty plan on a basic load balancer that doesn't even define auto_ssl

vultr_load_balancer.foo: Refreshing state... [id=a9323cf7-df24-4ff2-9058-e783f9e0e278]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # vultr_load_balancer.foo will be updated in-place
  ~ resource "vultr_load_balancer" "foo" {
        id                  = "a9323cf7-df24-4ff2-9058-e783f9e0e278"
        # (12 unchanged attributes hidden)

      - auto_ssl {
            # (2 unchanged attributes hidden)
        }

        # (2 unchanged blocks hidden)
    }

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

Debug log:

2025-05-21T17:21:20.566-0400 [WARN]  Provider "vultr.com/dev/vultr" produced an unexpected new value for vultr_load_balancer.foo during refresh.
      - .auto_ssl: actual set element cty.ObjectVal(map[string]cty.Value{"domain_zone":cty.StringVal(""), "sub_domain":cty.StringVal("")}) does not correlate with any element in plan

How you're doing the reads is not producing a consistent terraform state for the auto_ssl schema

@mondragonfx mondragonfx requested a review from optik-aper May 29, 2025 13:34
@github-actions
Copy link

github-actions bot commented Aug 1, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Domain Validation

The generateAutoSSL helper only checks that the scheme is empty, but does not validate the domain format (e.g., punycode, length, allowed characters). Consider adding stricter validation to prevent malformed domains from being sent to the API.

func generateAutoSSL(domain string) (*govultr.AutoSSL, error) {
	parsedDomain, err := url.Parse(domain)
	if err != nil || parsedDomain.Scheme != "" {
		return nil, fmt.Errorf("domain format must not include URL scheme (http:// or https://)")
	}

	hostname := parsedDomain.Hostname()
	if hostname == "" {
		hostname = domain
	}

	subdomainParts := strings.Split(hostname, ".")
	if len(subdomainParts) <= 2 {
		return &govultr.AutoSSL{
			DomainZone: hostname,
			DomainSub:  "",
		}, nil
	}

	subDomain := subdomainParts[0]
	domainZone := strings.Join(subdomainParts[1:], ".")

	return &govultr.AutoSSL{
		DomainZone: domainZone,
		DomainSub:  subDomain,
	}, nil
}
Update Logic

When auto_ssl_domain is removed (empty string), the code only calls DeleteAutoSSL but does not set req.AutoSSL = nil. This may leave the previous value in the update request. Ensure the request struct is updated to reflect the removal.

if d.HasChange("auto_ssl_domain") {
	if autoSSLData, ok := d.GetOk("auto_ssl_domain"); ok && autoSSLData.(string) != "" {
		autoSSL, err := generateAutoSSL(autoSSLData.(string))
		if err != nil {
			return diag.Errorf("failed to parse auto SSL domain: %v", err)
		}
		req.AutoSSL = autoSSL
	} else {
		log.Printf("[INFO] Disabled load balancer auto SSL certificate (%v)", d.Id())
		if err := client.LoadBalancer.DeleteAutoSSL(ctx, d.Id()); err != nil {
			return diag.Errorf("error disabling load balancer auto SSL certificate (%v): %v", d.Id(), err)
		}
	}

@github-actions
Copy link

github-actions bot commented Aug 1, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix domain parsing and validation

The current logic incorrectly parses domains like "example.com" because url.Parse
without a scheme returns an empty Hostname(). Use strings.TrimPrefix to remove
schemes and validate the domain format directly. Also, validate that the domain has
at least one dot to avoid invalid inputs like "localhost".

vultr/resource_vultr_load_balancer.go [656-682]

 func generateAutoSSL(domain string) (*govultr.AutoSSL, error) {
-	parsedDomain, err := url.Parse(domain)
-	if err != nil || parsedDomain.Scheme != "" {
+	// Remove scheme if present
+	domain = strings.TrimPrefix(strings.TrimPrefix(domain, "http://"), "https://")
+	if strings.Contains(domain, "://") {
 		return nil, fmt.Errorf("domain format must not include URL scheme (http:// or https://)")
 	}
 
-	hostname := parsedDomain.Hostname()
-	if hostname == "" {
-		hostname = domain
+	// Validate basic domain format
+	if !strings.Contains(domain, ".") {
+		return nil, fmt.Errorf("invalid domain format: must contain at least one dot")
 	}
 
-	subdomainParts := strings.Split(hostname, ".")
+	subdomainParts := strings.Split(domain, ".")
 	if len(subdomainParts) <= 2 {
 		return &govultr.AutoSSL{
-			DomainZone: hostname,
+			DomainZone: domain,
 			DomainSub:  "",
 		}, nil
 	}
 
 	subDomain := subdomainParts[0]
 	domainZone := strings.Join(subdomainParts[1:], ".")
 
 	return &govultr.AutoSSL{
 		DomainZone: domainZone,
 		DomainSub:  subDomain,
 	}, nil
 }
Suggestion importance[1-10]: 8

__

Why: The current url.Parse approach fails for domains without schemes (returns empty hostname) and lacks validation for malformed domains. The suggested fix uses strings.TrimPrefix for scheme removal and adds explicit validation for dot-containing domains, which is critical for correct AutoSSL configuration.

Medium

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.

3 participants