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

WebHook should provide old data together with new data #3451

Closed
robertpenz opened this issue Aug 26, 2019 · 13 comments
Closed

WebHook should provide old data together with new data #3451

robertpenz opened this issue Aug 26, 2019 · 13 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Milestone

Comments

@robertpenz
Copy link

Environment

  • Python version: 3.6.8
  • NetBox version: 2.6.2

Proposed Functionality

We've implemented a Webhook which gets informed for IP address and service changes, the problem now is that we only have the new values and not the old values. e.g. the dns_name field is changed from bla1.example.com to bla2.example.com we are only seeing the second name in the webhook call. This makes it impossible to delete the old DNS record and add the new one. We also took a look at the change log and even there only the new name is shown.

This makes is quit impossible to trigger stuff directly from the netbox which needs to know the old value to clean up. Please provide the old values along with the new ones in the webhook call.

Use Case

User creates Device/VM in Netbox and assigns IP address with DNS Name --> DNS Name gets written into the DNS Server. User changes the DNS Name, old DNS Name gets deleted in the DNS and the new one created.

Database Changes

No, the old value should be read from database before writing new and than send the webhook with both.

External Dependencies

none

@tb-killa
Copy link
Contributor

We are also currently testing whether the webhooks will meet our future requirements.

Since I don't know if I should ask a new FR for this, I'm happy to give my topic under this FR otherwise.

For our tests we use the change for the IP addresses.
If I go to an address, but it doesn't make any changes but only clicks on Update, a webhook is generated directly. In fact, in my opinion, a webhook should only take place when a change is made.
Since an internal check between the cache and the current FORM value has to take place first, this requires a little more work.

@candlerb
Copy link
Contributor

candlerb commented Sep 7, 2019

I would very much like this too.

This could be done in a backwards-compatible way by adding a "data-before" member to the webhook message.

Another use case is for ignoring unimportant changes: you may wish to take action if field A changes, but not field B.

@candlerb
Copy link
Contributor

candlerb commented Sep 8, 2019

A little research into how it could be implemented: this SO post has some pointers.

  • FieldTracker from django-model-utils is a plugin which can do this - but that adds another dependency
  • Use the pre-init signal to take a snapshot of fields at the point the model is instantiated
  • Re-read the object from the database just before saving it (ugh)

I think it would also be useful to have previous state in the ObjectChange history - making it easier to revert an object to its previous state.

@jeremystretch
Copy link
Member

User creates Device/VM in Netbox and assigns IP address with DNS Name --> DNS Name gets written into the DNS Server. User changes the DNS Name, old DNS Name gets deleted in the DNS and the new one created.

You should be able to simply delete all DNS records for the IP address in question that don't match the current name (if any) without needing to consider what the previous name was. This approach is simpler, and additionally ensures the eventual cleanup of old records even if a webhook is missed occasionally.

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Oct 17, 2019
@candlerb
Copy link
Contributor

You should be able to simply delete all DNS records for the IP address in question that don't match the current name (if any) without needing to consider what the previous name was.

That's harder to achieve than you might think: consider that either the name might change, or the IP address, or both. In the case where both change, it's impossible.

However, even in the case where only one changes, it's still tricky. Suppose you have:

Now you change the address to 192.0.2.2. But in the webhook you have no record that it was originally 192.0.2.1.

In order to cleanup the PTR record you have to:

  • query the DNS to find any existing A record for www.example.com
  • see if there is an existing PTR record from that address back to www.example.com
  • if there is, delete it

And similar logic in the opposite direction (if you change the name, without changing the IP address)

@jeremystretch
Copy link
Member

Even easier would be to repopulate the entire DNS database from NetBox at set intervals, instead of trying to track ad hoc changes in realtime.

I don't want to get into a discussion about this particular use case; I'm just presenting alternative solutions. Implementing the proposed change is likely to be very challenging and require a lot of time that I personally don't have to spend on it. So, do we have any volunteers?

@robertpenz
Copy link
Author

  1. We're sending the user that did the action an email with a report to shows what worked and what not (e.g. the name be in use already, the IP address is in a PTR zone the DNS server does not control, the dnsname is in a zone that is not on the DNS server, ...) ... Even better than an Email would be a possible that the web hook reports the problem back to Netbox and Netbox shows the error and does not commit the change to the config, but I believe that's a too big change. Doing that via a cron job would not report the info to the admin which did the change at the time he/she needs it.
  2. The problem with checking at intervals is also that this works only if everything is in Netbox, otherwise that won't work as the script that runs at intervals don't know what to delete as it got deleted in Netbox and what was in the first place not in the Netbox.
  3. Searching for all A Records that point to an IP address is not performant task, as you need to get also zones and look for the IP, as a DNS Server is not build for this kind of request, we need via the Powerdns API over 10sec in our case. Relying on the PTR records has the risk of deleting only the A record that respond to the PTR, and you're out of luck if there is no PTR. We do that currently for interface deletion but for services that not a solution as its CNames and not A/PTR.

Your idea has only a chance to work on a green field deployment, not if you're migrating to Netbox as your source of truths.

@jeremystretch jeremystretch added status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation type: feature Introduction of new functionality to the application and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Oct 21, 2019
@candlerb
Copy link
Contributor

Searching for all A Records that point to an IP address is not performant task, as you need to get also zones and look for the IP, as a DNS Server is not build for this kind of request, we need via the Powerdns API over 10sec in our case

As long as existing forward and reverse match, it's not actually necessary to do the full scan over the zones - you can query the existing DNS both forward and reverse. I have published some code which works for me at https://github.com/candlerb/netbox-webhook-dnsupdate/ - note that it uses dynamic DNS updates, but you can probably change it to use pdns API.

However, that's by-the-by for this ticket, which is a more general feature request (not specifically for DNS)

@stale
Copy link

stale bot commented Dec 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Please see our contributing guide.

@stale stale bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Dec 6, 2019
@stale
Copy link

stale bot commented Dec 13, 2019

This issue has been automatically closed due to lack of activity. In an effort to reduce noise, please do not comment any further. Note that the core maintainers may elect to reopen this issue at a later date if deemed necessary.

@stale stale bot closed this as completed Dec 13, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 12, 2020
@jeremystretch jeremystretch reopened this Mar 4, 2021
@stale stale bot removed the pending closure Requires immediate attention to avoid being closed for inactivity label Mar 4, 2021
@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: needs owner This issue is tentatively accepted pending a volunteer committed to its implementation labels Mar 4, 2021
@jeremystretch
Copy link
Member

jeremystretch commented Mar 4, 2021

I'm resurrecting this issue because it should be doable with #5913 in place. That FR extends change logging to record a pre-change serialized snapshot of an object, which will be present on the instance at the time an outgoing webhooks gets enqueued.

@jeremystretch jeremystretch self-assigned this Mar 4, 2021
@netbox-community netbox-community unlocked this conversation Mar 4, 2021
@jeremystretch jeremystretch removed the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Mar 4, 2021
@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label Mar 4, 2021
@jeremystretch jeremystretch added this to the v2.11 milestone Mar 4, 2021
@jeremystretch
Copy link
Member

NetBox's change logging uses a minimal form of serialization, whereas outgoing webhooks utilize the REST API serializers. As a result, object serializations in webhooks have a fair bit more detail and are not directly comparable to the changelog forms. For example, a site's status would appear as a single string in a changelog representation:

"status": "active"

This is in contrast to the more detailed representation in the REST API:

"status": {
    "value": "active",
    "label": "Active"
}

I see two options. We either attach the minimalistic pre-change data in its current form to the webhook for reference by the receiver, or we update the change logging serialization to leverage the same REST API serializers. The later option probably isn't ideal because it may introduce a substantial amount of overhead, particularly concerning the representation of related objects.

A third option would be to change webhooks to the minimalized representation, however much of that data is of limited use on its own (e.g. "site": 123 doesn't tell us much), and such a change would be very disruptive to existing consumers.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed status: accepted This issue has been accepted for implementation labels Mar 4, 2021
@markkuleinio
Copy link
Contributor

Would it be feasible to add version selector in the webhook configuration? Current webhooks would be "v1" when upgrading NetBox, and then user could select "v2" format that would include the pre-change data in the webhook call but data would be in minimal format.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Mar 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

No branches or pull requests

5 participants