added veth interface and tests

[robbisso]

Added interface veth resource and tests to go with, it is missing testing for gateway6 field the code covers updating and handling this field just fine. If you have recommendations on how you would like this handled I will update accordingly

[maksym-nazarenko]

@robbisso thank you for this contribution,
I left some comments which should be addressed before approving so don't hesitate to ask for help, if needed.

[maksym-nazarenko]

@robbisso forgot to add to the review: tfplugindocs / tfplugindocs (pull_request) Failing after 45s
pr should contain updated docs, which can be generated with go generate and committed in this PR.

[maksym-nazarenko]

@ddelnano we have a major issue here - the latest Docker image for RouterOS has v7.1beta6 as version, but veth support appeared in v7.4 if I read changelog correctly.
We might need to handle this on our own as the upstream for Docker image seems to be abandoned. (I'll create an issue in some days)

[robbisso]

@maksym-nazarenko I'm using a CHR vm to do the testing in esxi, but I came to the same conclusion regarding abandoned docker image. There are also issues with dhcp_server_test.go and interface_wireguard_peer_test.go, I'm assuming that the newer routeros version is partially at fault. I can submit a PR with those adjustments later.

[robbisso]

@maksym-nazarenko I caught an email from you regarding the vlan_interface, it doesn't follow the naming convention for the other "interface" types, should this item be named veth_interface instead or vlan_interface -> interface_vlan?

[robbisso]

@maksym-nazarenko updated with the requested changes please let me know if there is anything else that should be changed.

[maksym-nazarenko]

@maksym-nazarenko I caught an email from you regarding the vlan_interface, it doesn't follow the naming convention for the other "interface" types, should this item be named veth_interface instead or vlan_interface -> interface_vlan?

You are right - the vlan_interface was named wrongly.
Ideally, the resource name should be the same as RouterOS command path with / replaced by _, e.f. mikrotik_interface_vlan, so your way for veth is correct one

[robbisso]

Having some issues with making non-blank fields blank on update, haven't troubleshooted too deep on it yet but wonder if it might have something to do with the generic crud operations. I don't want to mark the fields as resource replace required since they do not need replacement to update via shell or winbox. I have created a gist https://gist.github.com/robbisso/fee7e832a2b1bdbdef6cf5990a899b29 with the updated test code

[maksym-nazarenko]

Having some issues with making non-blank fields blank on update, haven't troubleshooted too deep on it yet but wonder if it might have something to do with the generic crud operations. I don't want to mark the fields as resource replace required since they do not need replacement to update via shell or winbox. I have created a gist https://gist.github.com/robbisso/fee7e832a2b1bdbdef6cf5990a899b29 with the updated test code

that's because our client does not marshal empty values, which is a broader issue (it's on my personal roadmap, but still didn't have time to look into it).

I've already faced it for pool resource

[robbisso]

Thanks, that'll help narrow it down a bit I'll try to find a patch that works for it later in the week if I have time.

[robbisso]

Having some issues with making non-blank fields blank on update, haven't troubleshooted too deep on it yet but wonder if it might have something to do with the generic crud operations. I don't want to mark the fields as resource replace required since they do not need replacement to update via shell or winbox. I have created a gist https://gist.github.com/robbisso/fee7e832a2b1bdbdef6cf5990a899b29 with the updated test code

that's because our client does not marshal empty values, which is a broader issue (it's on my personal roadmap, but still didn't have time to look into it).

I've already faced it for pool resource

Well I do know what's going wrong as far as the command it is issuing to the router goes, appears that since the value is empty it isn't sending over the "" that mikrotik is expecting to see for an unset.

I'm not super familiar with go, fixing this for string should be easy enough but depending on how go handles empty ints/datatypes there may be an issue with adjusting the marshaller for those. Perhaps you can offer some insight on that.

Due to the way the marshaller handles types in general and some fields should or should not be sent on commands such as print, I see that this is a larger issue overall. It may be best solved with adding tags to the struct field based on the command type to be run / or specifying data fields in the same manner. the check for value.IsZero() is filtering out the empty strings well as the ints == 0

[maksym-nazarenko]

Yeah, change like this requires some work.

While it might be valid for some cases for the Terraform provider itself, it breaks the client in some places.

One example is dhcp-server.

If Marshal() function skips IsZero() check, it will render all fields, including authoritative and you will get this error:

from RouterOS device: ambiguous value of authoritative, more than one possible value matches input

it's because authoritative field has several possible values:

[admin@MikroTik] /ip/dhcp-server> add authoritative=
after-2sec-delay  after-10sec-delay  no  yes

but client does not have concept of default value.
In turn, Terraform resource provides default values so acceptance test doesn't fail.

I don't have clear, easy and maintainable solution for this right now, that's why I haven't started implementation.

As an option, we could add omitempty to tags for client struct and mark individual fields to be omitted if blank, while other are passed with empty value =field= while marshaling.
However, we should be careful here, as not all fields accept empty values for unsetting (see next-pool field in pool resource)

[robbisso]

I have a working solution still tweaking it, added a field to the struct tags to handle “marshal-empty-as:value” and have started adjusting the tests and types requiring it. There appears to be a bug in routeros regarding the address pool as well, only able to get a value for the next pool via export method. I have set the tag on this to marshal none as the value in but appears a custom find will be needed for it. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Maksym ***@***.***> Sent: Tuesday, December 12, 2023 4:21:08 PM To: ddelnano/terraform-provider-mikrotik ***@***.***> Cc: Robert Bissonnette ***@***.***>; Mention ***@***.***> Subject: Re: [ddelnano/terraform-provider-mikrotik] added veth interface and tests (PR #217) Yeah, change like this requires some work. While it might be valid for some cases for the Terraform provider itself, it breaks the client in some places. One example is dhcp-server. If Marshal() function skips IsZero() check, it will render all fields, including authoritative and you will get this error: from RouterOS device: ambiguous value of authoritative, more than one possible value matches input it's because authoritative field has several possible values: ***@***.*** /ip/dhcp-server> add authoritative= after-2sec-delay after-10sec-delay no yes but client does not have concept of default value. In turn, Terraform resource provides default values<https://github.com/ddelnano/terraform-provider-mikrotik/blob/073071b590bef4a2b14174d2dc1038c9556ed7e9/mikrotik/resource_dhcp_server.go#L86> so acceptance test doesn't fail. I don't have clear, easy and maintainable solution for this right now, that's why I haven't started implementation. As an option, we could add omitempty to tags for client struct and mark individual fields to be omitted if blank, while other are passed with empty value =field= while marshaling. However, we should be careful here, as not all fields accept empty values for unsetting (see next-pool<https://github.com/ddelnano/terraform-provider-mikrotik/blob/073071b590bef4a2b14174d2dc1038c9556ed7e9/mikrotik/resource_pool.go#L103> field in pool resource) — Reply to this email directly, view it on GitHub<#217 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BDYGIPDS33UMJC3L7KOVQH3YJDDEJAVCNFSM6AAAAABANEBYOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJSHAZDSMJSHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[robbisso]

Yes it does render some errors some of the times, i'll put up a feat enhancement branch thursday or friday with whatever I have on it, essentially the other major change is looking at the command issued ending with the set operation by name. A bit hacky but it works, will switch it over to an enum to be passed or passedthrough with the existing switch case for crud operation on the backing type TO the marshaller to specify field overloads. Might even make sense to do do something along the lines of field groups based on the crud type. I only pass all fields on the set operation, but due to what looks like a mikrotik bug, getting the next-pool field value from anything other than by issuing an export command. I might make even make special caching and interpreter for handling export syntax and keep the results for duration of the single run and invalidated when changes are detected and completed for it's backing values if a specific field(s) are changed on a particular type. With a semafor or mutex to handle the parallel threads within the client itself or an app level cache for testing purposes. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Robert Bissonnette ***@***.***> Sent: Tuesday, December 12, 2023 4:30:11 PM To: ddelnano/terraform-provider-mikrotik ***@***.***>; ddelnano/terraform-provider-mikrotik ***@***.***> Cc: Mention ***@***.***> Subject: Re: [ddelnano/terraform-provider-mikrotik] added veth interface and tests (PR #217) I have a working solution still tweaking it, added a field to the struct tags to handle “marshal-empty-as:value” and have started adjusting the tests and types requiring it. There appears to be a bug in routeros regarding the address pool as well, only able to get a value for the next pool via export method. I have set the tag on this to marshal none as the value in but appears a custom find will be needed for it. Get Outlook for iOS<https://aka.ms/o0ukef>

________________________________ From: Maksym ***@***.***> Sent: Tuesday, December 12, 2023 4:21:08 PM To: ddelnano/terraform-provider-mikrotik ***@***.***> Cc: Robert Bissonnette ***@***.***>; Mention ***@***.***> Subject: Re: [ddelnano/terraform-provider-mikrotik] added veth interface and tests (PR #217) Yeah, change like this requires some work. While it might be valid for some cases for the Terraform provider itself, it breaks the client in some places. One example is dhcp-server. If Marshal() function skips IsZero() check, it will render all fields, including authoritative and you will get this error: from RouterOS device: ambiguous value of authoritative, more than one possible value matches input it's because authoritative field has several possible values: ***@***.*** /ip/dhcp-server> add authoritative= after-2sec-delay after-10sec-delay no yes but client does not have concept of default value. In turn, Terraform resource provides default values<https://github.com/ddelnano/terraform-provider-mikrotik/blob/073071b590bef4a2b14174d2dc1038c9556ed7e9/mikrotik/resource_dhcp_server.go#L86> so acceptance test doesn't fail. I don't have clear, easy and maintainable solution for this right now, that's why I haven't started implementation. As an option, we could add omitempty to tags for client struct and mark individual fields to be omitted if blank, while other are passed with empty value =field= while marshaling. However, we should be careful here, as not all fields accept empty values for unsetting (see next-pool<https://github.com/ddelnano/terraform-provider-mikrotik/blob/073071b590bef4a2b14174d2dc1038c9556ed7e9/mikrotik/resource_pool.go#L103> field in pool resource) — Reply to this email directly, view it on GitHub<#217 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BDYGIPDS33UMJC3L7KOVQH3YJDDEJAVCNFSM6AAAAABANEBYOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJSHAZDSMJSHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>