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

ipv6_neighbor_discovery dns server configuration fix attempt #509

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

remil1000
Copy link

Greetings,

first thanks for the great work, this provider is quite a wonder

Second, a small PR, that, I think, fix a probable typo

In the routeros_ipv6_neighbor_discovery resource, the schema is implemented to use dns_servers but it seems this keyword is not present in the RouterOS API (tested on 7.14 and 7.15)

Example below from the /rest/ipv6/nd API endpoint

[
  {
    ".id": "*1",
    "advertise-dns": "true",
    "advertise-mac-address": "true",
    "default": "true",
    "disabled": "true",
    "dns": "",
    "hop-limit": "unspecified",
    "interface": "all",
    "invalid": "false",
    "managed-address-configuration": "false",
    "mtu": "unspecified",
    "other-configuration": "false",
    "pref64": "",
    "ra-delay": "3s",
    "ra-interval": "3m20s-10m",
    "ra-lifetime": "30m",
    "ra-preference": "medium",
    "reachable-time": "unspecified",
    "retransmit-interval": "unspecified"
  }
]

When I attempt to push a Neighbord Discovery configuration I get errors sampled below:

│ Error: PUT 'http://host/rest/ipv6/nd' returned response code: 400, message: 'Bad Request', details: 'unknown parameter dns-servers'
│ 
│   with routeros_ipv6_neighbor_discovery.envs["remi"],
│   on envs.tf line 89, in resource "routeros_ipv6_neighbor_discovery" "envs":
│   89: resource routeros_ipv6_neighbor_discovery envs {

╷
│ Warning: Field 'invalid' not found in the schema
│ 
│   with routeros_ipv6_neighbor_discovery.envs["remi"],
│   on envs.tf line 89, in resource "routeros_ipv6_neighbor_discovery" "envs":
│   89: resource routeros_ipv6_neighbor_discovery envs {
│ 
│ [MikrotikResourceDataToTerraform] The field was lost during the Schema development: ▷ 'invalid': 'false' ◁
╵
╷
│ Warning: Field 'default' not found in the schema
│ 
│   with routeros_ipv6_neighbor_discovery.envs["remi"],
│   on envs.tf line 89, in resource "routeros_ipv6_neighbor_discovery" "envs":
│   89: resource routeros_ipv6_neighbor_discovery envs {
│ 
│ [MikrotikResourceDataToTerraform] The field was lost during the Schema development: ▷ 'default': 'false' ◁
╵
╷
│ Warning: Field 'dns' not found in the schema
│ 
│   with routeros_ipv6_neighbor_discovery.envs["remi"],
│   on envs.tf line 89, in resource "routeros_ipv6_neighbor_discovery" "envs":
│   89: resource routeros_ipv6_neighbor_discovery envs {
│ 
│ [MikrotikResourceDataToTerraform] The field was lost during the Schema development: ▷ 'dns': '' ◁

This PR changes the DNS configuration keyword to dns (and remove the CIDR validation check as RouterOS awaits a comma separated list of host address(es)) and also add two computed fields for default and invalid

Let me know if this looks correct for you and I'll add relevant documentation items - unfortunately I'm not familiar enough with terraform provider to add proper testing, on that one I may need guidance

@remil1000 remil1000 requested a review from a team as a code owner July 23, 2024 14:02
@vaerh
Copy link
Collaborator

vaerh commented Jul 23, 2024

Thank you very much!
The patch looks good, but I'd like to keep the resource backwards compatibility. So I just added a new missing attribute.

Please check it after the release.

I need to add a mention somewhere about the documentation: it is generated at release automatically :)

@vaerh vaerh merged commit 160bfc2 into terraform-routeros:main Jul 23, 2024
3 checks passed
@vaerh
Copy link
Collaborator

vaerh commented Jul 23, 2024

🎉 This PR is included in version 1.57.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vaerh vaerh added the released label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants