feat: Add missing schema fields

[dokmic]

While working on #300, I discovered that one property was missing in older versions of RouterOS. After digging into the current architecture and checking the way the plugin SDK evaluates the difference, I discovered that the code can be written in an isomorphic way so that it won't produce any errors on older versions. Here is some explanation:

• Mikrotik follows semantic versioning and never adds required fields without default values to the existing resources so that there can only be optional fields added in later versions.
• Whenever we create a new resource, we can omit pretty much all the optional fields as they will be populated on the RouterOS side. The newly added parameters should never be in the plan for created resources, as such requests won't succeed on older versions.
• Terraform populates schema fields missing in the configuration with non-empty default values. Hence, the newly added fields with non-empty Default will make the resource incompatible with older versions.
• On update, the newly added parameters will be present in the remote resource, and Terraform will prepare a plan to set them to null if they are not present in the configuration, so it's kind of necessary to specify defaults for those.
• Before update, terraform always performs read operation and only then builds a plan -- there is already plenty of code utilizing this behavior to provide backward compatibility.
• We can make use of the DefaultFunc callback to provide a default value conditionally depending on the preceding read operation. In that case, the default value for the created resources will be empty and only be present for updating resources if it's already present remotely. The latter should produce an empty plan on update if there are no changes in the config.

I am aware of #271, and I got the idea of adding a version parameter, but it may break the declarative way of defining schemas as it requires if statements for such fields. The approach with conditional defaults should resolve pretty much all the current compatibility issues and should work as long as Mikrotik follows semantic versioning.

As the result, the PR brings the following:

• Adds DefaultIfSupported helper function.
• Refactors the vrf parameter in the RADIUS resource.
• Refactors a bunch of existing parameters in the ethernet resource: poe_out, poe_priority, cable_setting, disable_running_check, sfp_rate_select, sfp_shutdown_temperature.
• Adds missing PoE parameters to the ethernet resource: poe_voltage, poe_lldp_enabled, power_cycle_interval, power_cycle_ping_enabled, power_cycle_ping_address, power_cycle_ping_timeout.
• Adds missing address_list_extra_time to the DNS resource without breaking compatibility with any older versions (resolves Resource routeros_ip_dns missing address_list_extra_time fields #270 and closes New field address_list_extra_time in routeros_dns (in 7.10) #230).

The changes are compatible with both 6.x and 7.x versions of RouterOS.

UPD: #303 (comment)

[vaerh]

Hi Michael,
I haven't had a chance to look at your code yet, but from the description I have a few questions/comments:

• The more I dig into the MT ecosystem, the less sure I am that there won't be some unexpected changes to the default fields. But this can be disregarded as this is just my view of the problem.
• The main point because of which the fields were filled with default values is to avoid the need to make them "Computed". Unfortunately I don't remember in which configuration I encountered a problem where changing the "Optional + Computed" fields didn't cause TF to detect these changes. Is there any assurance that this problem will not recur?

[dokmic]

@vaerh

Unfortunately I don't remember in which configuration I encountered a problem where changing the "Optional + Computed" fields didn't cause TF to detect these changes.

It might be if the Optional parameter was set to false as the field is considered read-only.

Is there any assurance that this problem will not recur?

I don't expect anything like that. It's mainly for the update operation when the value is present remotely and not in the configuration. Terraform will skip that if it matches the returned default value. The beauty of this approach is that it doesn't change the serialization function, so I don't expect any side effects.

[vaerh]

@dokmic
I looked at your code. It's an interesting implementation.
I apologize, but I'm boring you with my questions.
We have a great AlwaysPresentNotUserProvided function, why can't we just do away with it? In case there are values coming in from ROS that we have not specified by default, they will just quietly get written to the state file.

resource "routeros_interface_ethernet" "test" {
  factory_name = "ether3"
  name         = "swtich-eth0"
  mtu          = 9000
}

request body:  
{
    "arp": "enabled",
    "arp-timeout": "auto",
    "auto-negotiation": "yes",
    "disable-running-check": "yes",
    "disabled": "no",
    "full-duplex": "yes",
    "loop-protect": "default",
    "loop-protect-disable-time": "5m",
    "loop-protect-send-interval": "5s",
    "mdix-enable": "yes",
    "mtu": "9000",
    "name": "swtich-eth0",
    "rx-flow-control": "off",
    "sfp-rate-select": "high",
    "tx-flow-control": "off"
}
response body:
{
    ".id": "*3",
    "advertise": "10M-half,10M-full,100M-half,100M-full,1000M-full", AlwaysPresentNotUserProvided
    "arp": "enabled",
    "arp-timeout": "auto",
    "auto-negotiation": "true",
    "cable-settings": "default", AlwaysPresentNotUserProvided
    "default-name": "ether3",
    "disable-running-check": "true",
    "disabled": "false",
    "loop-protect": "default",
    "loop-protect-disable-time": "5m",
    "loop-protect-send-interval": "5s",
    "loop-protect-status": "off",
    "mac-address": "54:05:AB:FE:F4:93", AlwaysPresentNotUserProvided
    "mtu": "9000",
    "name": "swtich-eth0",
    "orig-mac-address": "54:05:AB:FE:F4:93",
    "running": "true",
    "rx-broadcast": "0",
    "rx-bytes": "0",
    "rx-flow-control": "off",
    "rx-multicast": "0",
    "rx-packet": "0",
    "tx-broadcast": "5",
    "tx-bytes": "3751",
    "tx-flow-control": "off",
    "tx-multicast": "21",
    "tx-packet": "26"
}
state file:
        {
          "schema_version": 0,
          "attributes": {
            "___id___": null,
            "___path___": null,
            "___skip___": null,
            "advertise": "10M-half,10M-full,100M-half,100M-full,1000M-full", <<<
            "arp": "enabled",
            "arp_timeout": "auto",
            "auto_negotiation": true,
            "bandwidth": null,
            "cable_settings": "default", <<<
            "combo_mode": null,
            "comment": null,
            "default_name": "ether3",
            "disable_running_check": true,
            "disabled": false,
            "factory_name": "ether3",
            "full_duplex": true,
            "id": "*3",
            "l2mtu": null,
            "loop_protect": "default",
            "loop_protect_disable_time": "5m",
            "loop_protect_send_interval": "5s",
            "loop_protect_status": "off",
            "mac_address": "54:05:AB:FE:F4:93", <<<
            "mdix_enable": true,
            "mtu": 9000,
            "name": "swtich-eth0",
            "orig_mac_address": "54:05:AB:FE:F4:93",
            "poe_out": "off",
            "poe_priority": null,
            "running": true,
            "rx_flow_control": "off",
            "sfp_rate_select": "high",
            "sfp_shutdown_temperature": null,
            "slave": null,
            "speed": null,
            "switch": null,
            "tx_flow_control": "off"
          },

I guess I can't understand your idea. Would you mind explaining it to me?

[dokmic]

@vaerh, Thank you so much for the explanation. You are absolutely right. In my PR, I was basically reinventing the wheel (AlwaysPresentNotUserProvided). I saw that helper function, but I got confused as it doesn't properly handle boolean and numeric fields.

I fixed AlwaysPresentNotUserProvided to check the value presence in the raw config using a similar approach as in the isEmpty helper. With that fix, there is no longer a need to toggle default values on the read operation.

I've updated my PR to reuse this helper in the missing fields -- this should be compatible with all the supported RouterOS versions.

[dokmic]

Okay, here is the summary of the updated PR:

• Fix AlwaysPresentNotUserProvided to handle schema.TypeBool and schema.TypeInt correctly.
• Rename cable_settings to cable_setting in routeros_interface_ethernet as it was incorrect.
• Remove logic skipping disable_running_check, cable_setting, poe_out, and poe_priority in favor of AlwaysPresentNotUserProvided as it is redundant when the default value is not provided.
• Suppress diff for sfp_rate_select and sfp_shutdown_temperature as those fields aren't always present.
• Add missing PoE fields: poe_lldp_enabled, poe_voltage, power_cycle_interval, power_cycle_ping_enabled, power_cycle_ping_address, power_cycle_ping_timeout.
• Refactor routeros_radius_incoming to use AlwaysPresentNotUserProvided in the vrf field.
• Add missing address_list_extra_time in routeros_ip_dns, which won't break older RouterOS versions with fixed AlwaysPresentNotUserProvided.

[vaerh]

What version of ROS are you testing the provider?
In v7.10 this field looks exactly like "cable-settings":

Warning: Field 'cable_settings' not found in the schema

[vaerh]

I have to put aside some part of things and start versioning schemas. In fact, I don't update the docker container either, so as not to lose any changes in schemas....

[dokmic]

I don't have this particular one in my fleet. While adding the PoE fields, I noticed the difference in their docs. After double-checking and a quick search, I thought that was a typo on the provider's side. I reverted the rename. Sorry about that 😞

I have to put aside some part of things and start versioning schemas.

It shouldn't be necessary at this point. We can cover almost all of the cases with AlwaysPresentNotUserProvided rather than introducing an option (#271) requiring users to update that on every upgrade.

[vaerh]

@dokmic
Please join the discussion #305

[vaerh]

🎉 This PR is included in version 1.25.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀