Bump oxide.go and handle interface types. #598

jmcarp · 2026-01-16T18:36:31Z

Update to use oxidecomputer/oxide.go#359 to work through how the changes will affect the provider. Marking as a draft while we finish the upstream PR. Overall, some of the code got more verbose, because we have to switch over all known variant types, but we also dropped the json marshal/unmarshal hack, and type safety is improved.

Pull request checklist

Add changelog entry for this change.

jmcarp · 2026-01-16T18:44:44Z

internal/provider/resource_instance.go

-				continue
-			}
-			ip = stack.Ip
+		if v4, ok := nic.IpStack.Value.(*oxide.PrivateIpStackV4); ok {


Note that we can test the type of nic.IpStack.Value here rather than looking at the string value of nic.IpStack.Type. The behavior is the same, but we no longer have to rely on the assumption of a correspondence between type strings and types, and don't have to check errors from type casts.

I like this much better but am slightly struggling to understand why we can use the type assertion here at the top level rather than needing to create helper functions like shown in https://github.com/oxidecomputer/terraform-provider-oxide/pull/598/changes#r2700298578. This might just be late Friday brain where I'm missing something small.

This is a bit of a weird case because we're only handling the v4 case here, not v6 or dual-stack. Those will come soon, as part of our r18 compat work. So later on we'll also have to handle all possible variants here.

jmcarp · 2026-01-16T18:50:01Z

internal/provider/resource_instance.go

 	return ips
 }

-func ipStackAsIPv4Stack(ipStack oxide.PrivateIpStack) (oxide.PrivateIpv4Stack, error) {


This is the kind of hack we can dispense with using interface types. When we represent tagged unions as any, we can't cast values to the correct struct type, because they're unmarshalled to a map[string]any. Instead, we've been marshalling to json and unmarshalling to the struct type. We don't have to live like this!

Yes, be gone!

sudomateo

This looks much better! I have a comment about the need for the helper functions and whether we can consolidate those in to the Go SDK since we'll likely need those across downstream consumers.

sudomateo · 2026-01-16T23:12:07Z

internal/provider/data_source_vpc_router_route.go

+// routeDestinationValue extracts the string value from a RouteDestination variant.
+func routeDestinationValue(dest oxide.RouteDestination) (string, error) {
+	switch v := dest.Value.(type) {
+	case *oxide.RouteDestinationIp:
+		return v.Value, nil
+	case *oxide.RouteDestinationIpNet:
+		if s, ok := v.Value.(string); ok {
+			return s, nil
+		}
+		return fmt.Sprintf("%v", v.Value), nil
+	case *oxide.RouteDestinationVpc:
+		return string(v.Value), nil
+	case *oxide.RouteDestinationSubnet:
+		return string(v.Value), nil
+	default:
+		return "", fmt.Errorf("unknown route destination type: %T", dest.Value)
+	}
+}
+
+// routeTargetValue extracts the string value from a RouteTarget variant.
+// Returns empty string for "drop" targets which have no value.
+func routeTargetValue(target oxide.RouteTarget) (string, error) {
+	switch v := target.Value.(type) {
+	case *oxide.RouteTargetIp:
+		return v.Value, nil
+	case *oxide.RouteTargetVpc:
+		return string(v.Value), nil
+	case *oxide.RouteTargetSubnet:
+		return string(v.Value), nil
+	case *oxide.RouteTargetInstance:
+		return string(v.Value), nil
+	case *oxide.RouteTargetInternetGateway:
+		return string(v.Value), nil
+	case *oxide.RouteTargetDrop:
+		return "", nil
+	default:
+		return "", fmt.Errorf("unknown route target type: %T", target.Value)
+	}
+}


Could this code be put in the SDK behind some Value method or similar? I understand that it'll only really work if all the variants are the same type, as these are, and other values that have different types would need to return an interface and have type assertions. Just trying to see how we can balance the boilerplate between the Go SDK and its consumers. I don't dislike this code, I just know it'll be something we use across consumers.

sudomateo · 2026-01-16T23:16:31Z

internal/provider/resource_instance.go

-				continue
-			}
-			ip = stack.Ip
+		if v4, ok := nic.IpStack.Value.(*oxide.PrivateIpStackV4); ok {


I like this much better but am slightly struggling to understand why we can use the type assertion here at the top level rather than needing to create helper functions like shown in https://github.com/oxidecomputer/terraform-provider-oxide/pull/598/changes#r2700298578. This might just be late Friday brain where I'm missing something small.

sudomateo · 2026-01-16T23:16:44Z

internal/provider/resource_instance.go

 	return ips
 }

-func ipStackAsIPv4Stack(ipStack oxide.PrivateIpStack) (oxide.PrivateIpv4Stack, error) {


Yes, be gone!

jmcarp · 2026-01-21T15:23:19Z

internal/provider/resource_vpc_router_route.go

-		Type:  types.StringValue(string(vpcRouterRoute.Destination.Type)),
-		Value: types.StringValue(vpcRouterRoute.Destination.Value.(string)),
+		Type:  types.StringValue(string(vpcRouterRoute.Destination.Type())),
+		Value: types.StringValue(vpcRouterRoute.Destination.String()),


Note: we added String() methods to a select set of api types, all of whose variants either wrap or are type-defined to string.

jmcarp · 2026-01-21T15:24:04Z

internal/provider/resource_vpc_router_route.go

 }
+
+// newRouteDestination creates a RouteDestination from type and value strings.
+func newRouteDestination(typeStr, value string) (oxide.RouteDestination, error) {


We could think about moving these constructors to helpers in the sdk, like the String() methods we've already added, but this feels very specific to terraform. I'm leaving these constructors here for now.

jmcarp · 2026-01-21T15:31:25Z

I'm pretty happy with these changes now. We don't actually have a huge number of oneOf types in the api, so the changes only touch a few files in the terraform provider. The changes fall into a few categories:

Trivial changes, like s/Type/Type()/.
New constructors for oneOf types when preparing the body of an api request.
Error handling for those constructors, since they can return errors if we encounter an unexpected variant type.

jmcarp · 2026-01-22T18:16:31Z

Updated to use new String() and As$Variant() helpers in oxide.go.

lgfa29

Changes look like a nice improvement. I will rebase my WIP branches once this is merged to get a hands-on experience with the new SDK.

I also wondered about the generators, but couldn't think of a good way to handle them. I wouldn't say they're Terraform specific, but this case is somewhat particular because all variants have a similar structure, so it's easy to create a New*(string, string) function. If any of the variants were a little different, the constructor would start to become quite messy (like having arguments that are only used with specific types).

lgfa29 · 2026-01-22T18:59:59Z

internal/provider/resource_instance.go

-			}
-			ip = stack.Ip
+		if v4, ok := nic.IpStack.AsV4(); ok {
+			ip = v4.Value.Ip


We should probably keep the error handling in an else clause. This part of the code will probably change when we add IPv6 support, but keeping the error is a good reminder to double-check other conditions.

Which error handling? Do you mean we should log an error if we got a type other than oxide.PrivateIpStackTypeV4 for now?

Yup. Mostly just a gatekeeper to make sure we parse the IP properly when implementing IPv6. Without the error the instance IP may be set to an empty string in state .

lgfa29 · 2026-01-22T19:00:25Z

internal/provider/resource_instance.go

-		stack, err := ipStackAsIPv4Stack(nic.IpStack)
-		if err != nil {
-			diags.AddError(
-				"Unable to read instance network interface:",
-				"Error: "+err.Error(),
-			)
-			continue
-		}
-		n.IPAddr = types.StringValue(stack.Ip)
-


lgfa29 · 2026-01-22T19:06:18Z

internal/provider/resource_vpc_firewall_rules.go

-			Targets:      newTargetsModelFromResponse(rule.Targets),
+			Targets:      targets,


I'm necessarily opposed to the change, but it seems unnecessary in this case since the response from newTargetsModelFromResponse() is not being processed?

lgfa29 · 2026-01-22T19:06:51Z

internal/provider/resource_vpc_firewall_rules.go

 		}

 		filters, diags := newFiltersModelFromResponse(rule.Filters)
-		diags.Append(diags...)


Ops, good catch. I wonder if error messages were being duplicated because of this.

lgfa29 · 2026-01-22T19:12:00Z

internal/provider/resource_vpc_firewall_rules.go

+	switch typeStr {
+	case "vpc":


I think that, in general, the provider tries to use SDK values instead of direct strings, so this part would be something like:

switch oxide.VpcFirewallRuleHostFilterType(typeStr) { case oxide.VpcFirewallRuleHostFilterTypeVpc :

lgfa29 · 2026-01-22T19:12:47Z

internal/provider/resource_vpc_firewall_rules.go

+			return nil, fmt.Errorf("creating filters for rule %q: %w", ruleName, err)
+		}
+		targets, err := newTargetTypeFromModel(rule.Targets)
+		if err != nil {
+			return nil, fmt.Errorf("creating targets for rule %q: %w", ruleName, err)


Suggested change

return nil, fmt.Errorf("creating filters for rule %q: %w", ruleName, err)

}

targets, err := newTargetTypeFromModel(rule.Targets)

if err != nil {

return nil, fmt.Errorf("creating targets for rule %q: %w", ruleName, err)

return nil, fmt.Errorf("error creating filters for rule %q: %w", ruleName, err)

}

targets, err := newTargetTypeFromModel(rule.Targets)

if err != nil {

return nil, fmt.Errorf("error creating targets for rule %q: %w", ruleName, err)

Minor suggestion. These error messages read like log lines to me.

lgfa29 · 2026-01-22T19:25:35Z

internal/provider/resource_vpc_router_route.go

+// newRouteDestination creates a RouteDestination from type and value strings.
+func newRouteDestination(typeStr, value string) (oxide.RouteDestination, error) {
+	switch typeStr {
+	case "ip":


Same comment as above wrt using SDK values instead of strings.

jmcarp · 2026-01-23T16:54:11Z

I also wondered about the generators, but couldn't think of a good way to handle them. I wouldn't say they're Terraform specific, but this case is somewhat particular because all variants have a similar structure, so it's easy to create a New*(string, string) function. If any of the variants were a little different, the constructor would start to become quite messy (like having arguments that are only used with specific types).

I think it would be fine to add these constructors specifically for oneOfs whose variants are all string-like, similar to the String() methods in https://github.com/oxidecomputer/oxide.go/blob/main/oxide/helpers.go. Let me just add those while we're thinking about it.

Now that oxide.go uses interfaces to represent certain oneOf types in nexus, bump the sdk to latest, and update uses of those oneOf types.

* Error on ipv6 until supported. * Fix merge conflicts. * Run golangci-lint.

jmcarp force-pushed the jmcarp/interface-types branch from 75aa0cd to c82f79e Compare January 16, 2026 18:42

jmcarp commented Jan 16, 2026

View reviewed changes

jmcarp force-pushed the jmcarp/interface-types branch from c82f79e to 8a7d8aa Compare January 16, 2026 19:03

sudomateo reviewed Jan 16, 2026

View reviewed changes

jmcarp force-pushed the jmcarp/interface-types branch from 8a7d8aa to 265fa21 Compare January 21, 2026 15:12

jmcarp commented Jan 21, 2026

View reviewed changes

jmcarp mentioned this pull request Jan 21, 2026

Model single-discriminator, single-value enums as interfaces. oxidecomputer/oxide.go#359

Merged

jmcarp force-pushed the jmcarp/interface-types branch from 265fa21 to aed9781 Compare January 22, 2026 18:15

jmcarp marked this pull request as ready for review January 22, 2026 18:16

jmcarp requested a review from a team as a code owner January 22, 2026 18:16

lgfa29 approved these changes Jan 22, 2026

View reviewed changes

jmcarp force-pushed the jmcarp/interface-types branch from aed9781 to f90f8a9 Compare January 27, 2026 15:37

jmcarp added 2 commits January 27, 2026 10:38

Bump oxide.go and handle interface types.

42e9da4

Now that oxide.go uses interfaces to represent certain oneOf types in nexus, bump the sdk to latest, and update uses of those oneOf types.

Address comments from code review.

fe90d36

* Error on ipv6 until supported. * Fix merge conflicts. * Run golangci-lint.

jmcarp force-pushed the jmcarp/interface-types branch from f90f8a9 to fe90d36 Compare January 27, 2026 15:38

jmcarp merged commit b3fd554 into main Jan 27, 2026
3 checks passed

jmcarp deleted the jmcarp/interface-types branch January 27, 2026 16:04

		Targets: newTargetsModelFromResponse(rule.Targets),
		Targets: targets,

Bump oxide.go and handle interface types. #598

Bump oxide.go and handle interface types. #598

Uh oh!

Conversation

jmcarp commented Jan 16, 2026

Pull request checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sudomateo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmcarp commented Jan 21, 2026

Uh oh!

jmcarp commented Jan 22, 2026

Uh oh!

lgfa29 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmcarp commented Jan 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants