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

dns servers cannot be removed #174

Closed
n0rad opened this issue Apr 14, 2023 · 13 comments · Fixed by #176, #177 or #179
Closed

dns servers cannot be removed #174

n0rad opened this issue Apr 14, 2023 · 13 comments · Fixed by #176, #177 or #179
Assignees
Labels
bug Something isn't working released

Comments

@n0rad
Copy link

n0rad commented Apr 14, 2023

from:

resource "routeros_dns" "dns-server" {
  allow_remote_requests = true
  servers               = "8.8.8.8,8.8.4.4,1.1.1.1,1.0.0.1"
  use_doh_server        = "https://dns.google/dns-query"
  verify_doh_cert       = false
}

to:

resource "routeros_dns" "dns-server" {
  allow_remote_requests = true
  servers               = ""
  use_doh_server        = "https://dns.google/dns-query"
  verify_doh_cert       = false
}

No changes.

@n0rad n0rad added the bug Something isn't working label Apr 14, 2023
@vaerh
Copy link
Collaborator

vaerh commented Apr 17, 2023

Hi! It's not really a bug, since this resource is a system resource. That is, we do not know from what state we changed it to what state to return it. I can do a cleanup of Servers, Dynamic Servers, Verify DoH Certificate.

@n0rad
Copy link
Author

n0rad commented Apr 17, 2023

I did not look at how the provider is working internally, so it may be more difficult to implement for a system resource.
But from a user point of view, on the mikrotik, I can empty the servers list and I think the provider should mimic the same behavior.

BTW, thx for the provider it's super useful.

@vaerh
Copy link
Collaborator

vaerh commented Apr 17, 2023

Okay, I think this kind of behavior might be necessary for more than just this resource. I'll think about how to do it better and try to make changes this week.
P.S. System resources, from the provider's current point of view, are not to be created or deleted, only read or updated. This is what is implemented internally. The deletion simply deletes the resource record without any action on the router.

@vaerh vaerh self-assigned this Apr 18, 2023
vaerh added a commit that referenced this issue Apr 18, 2023
Fixes #174
Setting the "servers"  and "use_doh_server" fields to an empty value.
Setting the "allow_remote_requests"  and "verify_doh_cert" fields to a false value.
vaerh added a commit that referenced this issue Apr 18, 2023
Fixes #174
Setting the "servers"  and "use_doh_server" fields to an empty value.
Setting the "allow_remote_requests"  and "verify_doh_cert" fields to a false value.
@gfenn-newbury
Copy link
Collaborator

🎉 This issue has been resolved in version 1.3.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@vaerh
Copy link
Collaborator

vaerh commented Apr 18, 2023

@n0rad I want to ask you to test the release on a real state file (the resource has changed its Id generation scheme, but that shouldn't be a problem).

@n0rad
Copy link
Author

n0rad commented Apr 18, 2023

Just looked at your fix and tested: deleting the resource is working, but I think you miss read my issue because I'm actually not trying to delete the resource but changing it instead.

  • I first set some dns servers = and exposing the service to my network.
  • In a second step I put in place DOH.
  • in a third step, as DOH is working as expected I want to remove the servers, but keeping DOH.

This is where it's not working, I cannot empty the server list.

@vaerh
Copy link
Collaborator

vaerh commented Apr 19, 2023

Oh yeah, I came up with an easier problem myself and solved it 🤦 I'm sorry!
Do we keep restoring properties on deletion or do I return it as it was in the new revision?

@vaerh vaerh reopened this Apr 19, 2023
vaerh added a commit that referenced this issue Apr 19, 2023
Fixes #174
Change desealization for empty string fields, lists and sets
@vaerh
Copy link
Collaborator

vaerh commented Apr 19, 2023

@n0rad I fixed the broken operating logic. Let's hope it doesn't cause problems with managing existing resources.
Thanks for the PR. A large layer of problems has opened up!

Please test the current provider behavior after the release.

vaerh added a commit that referenced this issue Apr 19, 2023
* fix: dns servers cannot be removed
Fixes #174
Change desealization for empty string fields, lists and sets

* test: Remove additional resource

* ci: Increase timeout for tests
@gfenn-newbury
Copy link
Collaborator

🎉 This issue has been resolved in version 1.3.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@n0rad
Copy link
Author

n0rad commented Apr 19, 2023

👋
Just tested adding servers, then trying to remove them.
Sadly it's still not working, it's still silently ignored.

@vaerh
Copy link
Collaborator

vaerh commented Apr 20, 2023

Yes, this problem was not present in yesterday's synthetic tests. I was able to reproduce it. A little patience and we will fix it :)

@vaerh vaerh reopened this Apr 20, 2023
vaerh added a commit that referenced this issue Apr 20, 2023
Fixes #174
Remove the "Computed" property
vaerh added a commit that referenced this issue Apr 20, 2023
Fixes #174
Remove the "Computed" property
@gfenn-newbury
Copy link
Collaborator

🎉 This issue has been resolved in version 1.3.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@n0rad
Copy link
Author

n0rad commented Apr 20, 2023

working perfectly, with empty string and removing the field.
thx a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment