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

Port (metal) lateinitializer of conflicting vlanIds and vxlandIds fields prevents deletion #54

Open
displague opened this issue Jul 16, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@displague
Copy link
Collaborator

displague commented Jul 16, 2024

What happened?

Metal Port's lateinitializer is creating a conflicting definition with both vlanIds and vxlandIds. This is preventing deletion of the resource (possibly Ready status too).

How can we reproduce it?

https://community.equinix.com/t5/discussions/crossplane-failure-to-delete-port-resource-in-claim/m-p/2706/highlight/true#M1346

Similar reported in #50

The IP reservation is being successfully created. The problem seems to be at refresh, related to the quantity being set to 256 as a computed field despite the field conflicting with the other fields (cidr, vrf_id).

What environment did it happen in?

Crossplane version:
Provider version: 0.6.1

@displague displague added the bug Something isn't working label Jul 16, 2024
@displague
Copy link
Collaborator Author

#52 attempted to generalize the conditions in-which LateInititialization can be skipped. This change may not be complete, I suspect the names of the fields that were sent to the Canonical Ignore list need to be formatted differently. My evidence that this wasn't implemented correctly is the CRD changes between v0.7.0 and v0.8.0 (also those seen in #52). Looking at the Device resource, we see that the Facility vs Metro handling was removed without a suitable replacement.

With #49, there may be an alternate path:

	// THIS IS A BETA FIELD. It will be honored
	// unless the Management Policies feature flag is disabled.
	// InitProvider holds the same fields as ForProvider, with the exception
	// of Identifier and other resource reference fields. The fields that are
	// in InitProvider are merged into ForProvider when the resource is created.
	// The same fields are also added to the terraform ignore_changes hook, to
	// avoid updating them after creation. This is useful for fields that are
	// required on creation, but we do not desire to update them after creation,
	// for example because of an external controller is managing them, like an
	// autoscaler.
	InitProvider BGPSessionInitParameters `json:"initProvider,omitempty"`

@displague
Copy link
Collaborator Author

LateInitialization in mutually exclusive fields is discussed here: https://github.com/crossplane/upjet/blob/main/docs/configuring-a-resource.md#further-details-on-late-initialization

@displague
Copy link
Collaborator Author

displague commented Jul 22, 2024

I don't believe InitProvider will settle the vlan_ids vs vxlan_ids conflict since the fields are mutually exclusive and both would need to be defined (not possible) and ignored for InitProvider's ignore_changes to apply correctly.

https://github.com/equinix/terraform-provider-equinix/blob/main/internal/resources/metal/port/resource.go#L71-L86

It will need to be tested directly to see if this was covered in 80a4d74.

(testing is blocked by #56)

@displague displague mentioned this issue Jul 29, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant