feat(ip) - add ovh_ip_move resource to move an IP to a provided service name or park it if no service name given #510

julianguinard · 2023-12-05T11:24:34Z

Description

This adds a new add ovh_ip_move resource to move an IP to a provided service name or park it if empty service is given.

It relies on the following APIV6 routes:

taken resource ID is the move task ID if available, or the IP itself otherwise

Moving/parking process:

if there is an IpTask recorded in the resource state, we try to wait for it to be finished until it is considered expired
after having waited until previous task expiration or completion, do the move only if the current service is different from the one given in the inputs

Type of change

New feature (non-breaking change which adds functionality)
Documentation update

How Has This Been Tested?

Test :

make testacc TESTARGS="-run TestAccIpMove_basic"`
=== RUN   TestAccIpMove_basic
--- PASS: TestAccIpMove_basic (108.21s)
PASS

Test Configuration:

Terraform version: terraform version: Terraform v1.2.9
Existing HCL configuration you used:

Attaching an IP to a service

resource "ovh_ip_move" "move" {
    ip = "X.X.X.X/32"
    routed_to {
      service_name = "loadbalancer-YYYYYYYY"
    }
}

Then parking the same IP

resource "ovh_ip_move" "move" {
    ip = "X.X.X.X/32"
    routed_to {
      service_name = ""
    }
}

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added acceptance tests that prove my fix is effective or that my feature works
New and existing acceptance tests pass locally with my changes

Stoakes · 2023-12-06T08:44:36Z

Personal opinion, but I feel that naming could be improved. Move is an action verb. It doesn't feel right once it is applied.

What about something like ovh_ip_association ? Disclaimer, 100% inspired by aws_eip_association

scraly · 2023-12-06T08:54:41Z

The naming of the resource and datasource depends of the API endpoint you call :-)

endpoint := fmt.Sprintf("/ip/%s/move",

So, depending of the API scheme, ovh_ip_move is a good name.
If we want another name, we have to think it when we create the API endpoint :).

amstuta · 2024-03-12T12:21:01Z

ovh/resource_ip_move.go

+const taskExpiresAfter = 300 * time.Second
+
+// WaitingTimeInSecondsBeforeRefreshState number if seconds to wait before making a new API call to refresh ip task state
+const WaitingTimeInSecondsBeforeRefreshState = 10


Suggested change

const WaitingTimeInSecondsBeforeRefreshState = 10

const waitingTimeInSecondsBeforeRefreshState = 10

I think it doesn't need to be exported

amstuta · 2024-03-12T12:50:25Z

ovh/resource_ip_move.go

+	config := meta.(*Config)
+	ip := d.Get("ip").(string)
+
+	// no need to update if ip is already routed to the appropriate service


comment does not seem related to the code ? (from what I understand FromResource func doesn't check that)

right, check is done later down the road.
will move comment to l163

amstuta · 2024-03-12T13:03:16Z

ovh/resource_ip_move.go

+
+	endpoint := fmt.Sprintf("/ip/%s/task/%d",
+		url.PathEscape(d.Get("ip").(string)),
+		ipTask.TaskId,


needs to be url.PathEscape'd

but ipTask.TaskId is an int64, not a string (see %d formatter)

my bad, read too fast :)

amstuta · 2024-03-12T13:18:28Z

ovh/resource_ip_move.go

+	if timeDiff < taskExpiresAfter {
+		log.Printf("[DEBUG] ipTask.Status is currently: %s. Waiting %d second (we allow %f more seconds for the task to complete)", ipTask.Status, WaitingTimeInSecondsBeforeRefreshState, taskExpiresAfter.Seconds()-timeDiff.Seconds())
+		time.Sleep(WaitingTimeInSecondsBeforeRefreshState * time.Second)
+		err = resourceIpTaskRead(d, ipTask, meta)


I think this error should be checked and we should return immediately if it's not nil: if the task doesn't exist anymore for example, I think we shouldn't retry.

What do you think ?

in some instances such as a gone task it might make sense to indeed return error immediately. We can possibly do this by verifying ipTask nillity at the very beginning of recursiveWaitTaskFinished, which would achieve a similar behavior

In other cases though (network error, temporary server error, etc...) stopping the whole plan is not relevant nor wanted, and we would rather retry recursively

seems ok as a workaround, but IMO a better solution is to verify the error response like it's done in func resourceIpServiceReadByServiceName (with type assertion err.(*ovh.APIError) + status check)

amstuta · 2024-03-12T13:23:17Z

ovh/resource_ip_move.go

+func waitForTaskFinished(d *schema.ResourceData, meta interface{}, ipTask *IpTask, ip string, opts *IpMoveOpts) (finishedWithSuccess *bool, err error) {
+	defer func() {
+		errSet := d.Set("task_status", ipTask.Status)
+		if errSet == nil {


shouldn't it be != nil instead ? Otherwise you will hide the underlying error, for example in case your task has a status IpTaskStatusOvhError.

Additionally, it would be preferable to avoid this defer to ease readability :)

I will replace the whole method body to the following as I realize that go 1.20 introduced a way of joining errors, which is actually what I was ultimately willing to achieve so we are aware of both potential Set error and/or recursiveWaitTaskFinished() error

finishedWithSuccess, err = recursiveWaitTaskFinished(d, meta, ipTask, ip, opts) errSet := d.Set("task_status", ipTask.Status) return finishedWithSuccess, errors.Join(err, errSet)

amstuta · 2024-03-12T13:57:50Z