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

Interim IX-F JSON importer #585

Open
arnoldnipper opened this issue Oct 9, 2019 · 26 comments
Open

Interim IX-F JSON importer #585

arnoldnipper opened this issue Oct 9, 2019 · 26 comments

Comments

@arnoldnipper
Copy link
Contributor

@arnoldnipper arnoldnipper commented Oct 9, 2019

This interim IX-F JSON importer should be in place until the data ownership task-force comes up with a policy. This issue also relates to #518 and #540

There is a tick box "Allow IXP Update" which governs the behaviour of the import. By default, this is unticked (set to "no"). If "Allow IXP Update" is ticked, an entry may be created, changed and removed. The entry then is completely governed by the settings in the IX-F JSON import.

allow_ixp_update: no

  • If a network has an IXP entry with differing (asn, ipaddr4, ipaddr6), an email is sent to the ix operator, the net owner and the Admin committee. This also covers the case that the network does not have an entry in the IXP member list at all
  • If a network has an IXP entry with any other differing information (speed, route server peer), this information is not changed
  • If a network does not have an entry for the IXP, nothing is done

allow_ixp_update: yes

  • If a network has an IXP entry with any differing information, the entry is updated (IPv4, IPv6, speed, route server peer)
  • If a network does not have an entry for the IXP, one is added if there is not a conflicting record already. If there is a conflicting record an email is sent to the ix operator, the net owners and the Admin committee
  • If a network does not have an entry in the IXP member list, the network IXP entry is removed
@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Oct 10, 2019

@nbakker, @fkorsback, @durkovic, @peeringdb/pc and @peeringdb/ac does that sound like a good compromise?

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Oct 10, 2019

Sure, that will re-enable the importer for allow_ixp_update:yes users and avoid automatic/unwanted deletions for allow_ixp_update:no users, so hopefully both camps will be happy for now.

Later, in the data ownership TF, I'd suggest to get some more info about data quality, namely:

  • how many records are present in IX-F exports but missing in PDB for various reasons
    • network not listing IXP presence
    • network not registered in PDB #391
  • how many records have other differences (speed, route server peer)
@job

This comment has been minimized.

Copy link
Contributor

@job job commented Oct 10, 2019

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Oct 10, 2019

@job The purpose of this ticket is to find an emergency interim fix in order to re-enable IX-F importer until the final solution is agreed, designed & implemented.

If you look at https://github.com/peeringdb/peeringdb/blob/master/peeringdb_server/ixf.py
the offending code is in process_deletions() which removes entries not matching (asn,ipaddr4,ipaddr6)

If the above problem is patched by an emergency fix, we'll have enough time to discuss everything in detailm but currently we're lacking vital functionality.

@job

This comment has been minimized.

Copy link
Contributor

@job job commented Oct 10, 2019

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Oct 10, 2019

Well, IX-F importer is not working since Aug 30-th and all networks relying on it didn't get updates since then. Automatic updates were publicly announced and are not working for 1.5 months now, so
I do believe we need an emergency fix asap.

The situation with multiple connections you raised works properly in present code, since match is performed on each (asn,ipaddr4,ipaddr6) separately.

Documenting the importer in detail is for sure reasonable, but given a new task force was just created, it might be good to discuss it first and only based on agreement draft the new spec.

@job

This comment has been minimized.

Copy link
Contributor

@job job commented Oct 10, 2019

@nbakker

This comment has been minimized.

Copy link

@nbakker nbakker commented Oct 10, 2019

That order of work doesn’t work for me.

Same

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Oct 10, 2019

The (fairly complete!) description of the things that happen when allow_ixp_update is set to true or false is not yet complete and should be 100% complete before we can proceed with an implementation.

Imho my algorithm covers all cases, even with multiple connections to an ix as we are always talking about the triple (asn, ipv4,ipv6).

Fully agree that we do not need an emergency fix. But we also did not need an emergency fix to disable the importer ;-)

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Oct 10, 2019

Fully agree that we do not need an emergency fix. But we also did not need an emergency fix to disable the importer ;-)

Fair enough :-)
But since disabling of the importer created problems for others, let's try to fix those as well.

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Oct 10, 2019

@peeringdb/pc could I please have supporters. Tnx!

@fkorsback

This comment has been minimized.

Copy link

@fkorsback fkorsback commented Oct 11, 2019

The only thing i care about is that when a network set "allow_ixp_update: no" on their profile, they will not get any data automatically deleted in any field anywhere on their profile.

The only one that should be able to delete a record from an opted-out network is the AC if the automatic script finds out that there is discrepancies between the user-submitted data and what is coming in from the IXF importer. I do understand that other network relies on the IXF importer so i think it's important that we get the function activated. To me it seems like Arnold suggestion is a fair middle ground that should keep both camps happy.

Then we let the DTF group figure out exactly how these functions will work in the future and how we treat data ownership in the general sense.

@job

This comment has been minimized.

Copy link
Contributor

@job job commented Oct 11, 2019

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Oct 11, 2019

@job the original spec for IX-F importer is here:
https://docs.peeringdb.com/ix-f-json-import-rules/

This ticket just replaces "entry is removed" with "an email is sent"

A that's the whole point of this ticket - avoid removal of entries and generate notification emails instead.

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Oct 12, 2019

@job the original spec for IX-F importer is here:
https://docs.peeringdb.com/ix-f-json-import-rules/

This ticket just replaces "entry is removed" with "an email is sent"

A that's the whole point of this ticket - avoid removal of entries and generate notification emails instead.

Exactly ... the tool is in place since 11/2017 iirc. And as far as I can see it was working bugless so far (seen as a tool).

Two modifications are needed.

  • Notify involved parties in case an entry is marked as "to be deleted" AND "Allow IXP Update " is set to "No"

  • Notify involved parties in case an entry needs to be created AND "Allow IXP Update is set to "Yes" AND IPv4/IPv6 address already is in use

@eloos

This comment has been minimized.

Copy link

@eloos eloos commented Oct 30, 2019

+1

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Nov 5, 2019

To summarize

allow_ixp_update: no

  • If a network has an IXP entry with differing (asn, ipaddr4, ipaddr6), create a DeskPRO ticket, emailing the network and the ix. (1)

  • If a network has an IXP entry with any other differing information (speed, route server peer), this information is not changed

  • If a network does not have an entry for the IXP, nothing is done

allow_ixp_update: yes

  • If a network has an IXP entry with any differing information, the entry is updated (IPv4, IPv6, speed, route server peer) (1)

  • If a network does not have an entry for the IXP,

    • if there is not a conflicting record, a new entry is added.
    • If there is a conflicting record, create a DeskPRO ticket, emailing the networks and the ix.

(1) This also covers the case that the network does not have an entry in the IXP member list at all. In the case of "allow_ixp_update: yes" the update then actually means removal. This prevents involved parties from being flooded by tickets

A DeskPRO ticket will be opened once only and thereby creates a lock. Only if the lock is removed a new ticket can be created.

@peeringdb/pc please make up your mind

@funkestefan

This comment has been minimized.

Copy link
Contributor

@funkestefan funkestefan commented Nov 5, 2019

I like the proposal.
I would like to see allow_ixp_update as an option for each IXP my network is present at.

@grizz

This comment has been minimized.

Copy link
Member

@grizz grizz commented Nov 5, 2019

I like the proposal.

I do as well, I agree with @job that it might be best to do the specs in a flow chart type style instead of allow_update being yes or no. I believe I was the one who wrote the original specs, and there are still ambiguities for things like what to do with multiple IX connections. @arnoldnipper I have some time this month, happy to help go through that with you if you'd like and hammer out better specs.

I would like to see allow_ixp_update as an option for each IXP my network is present at.

@funkestefan I don't think I agree with that, but either way, it should be a separate ticket with its own discussion, happy to argue further there. :)

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Nov 5, 2019

I don't think there's any problem with multiple IX connections. Each IX connection must have different IP address, so it's handled separately, one by one.

@grizz

This comment has been minimized.

Copy link
Member

@grizz grizz commented Nov 5, 2019

There may not be, point being, specs could use a little more work before implementation or we stand the chance of running into issues down the road.

@mcmanuss8

This comment has been minimized.

Copy link
Contributor

@mcmanuss8 mcmanuss8 commented Nov 5, 2019

To summarize
...
@peeringdb/pc please make up your mind

+1

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Nov 6, 2019

I would like to see allow_ixp_update as an option for each IXP my network is present at.

@funkestefan, agree with @grizz to single this out. While I like the idea I fail to see how to implement this in practice. Some networks are connected to more than 200 IXes. In theory, you could be connected to 709 IXes as of today.

@arnoldnipper

This comment has been minimized.

Copy link
Contributor Author

@arnoldnipper arnoldnipper commented Nov 6, 2019

@koalafil , this issue has support from @grizz @eloos @mcmanuss8 @arnoldnipper Please promote to the next stage. Happy to work with @grizz to proper spec for our vendor.

@job

This comment has been minimized.

Copy link
Contributor

@job job commented Nov 6, 2019

I don't think there's any problem with multiple IX connections. Each IX connection must have different IP address, so it's handled separately, one by one.

I foresee issues with multiple connections if we don't specify the workflow:

Imagine if you have connection A & B, both have an IPv4 and IPv6 address - however the IPv4 addresses have been swapped (compared to IX-F), the IPv6 aren't.

@durkovic

This comment has been minimized.

Copy link

@durkovic durkovic commented Nov 6, 2019

Imagine if you have connection A & B, both have an IPv4 and IPv6 address - however the IPv4 addresses have been swapped (compared to IX-F), the IPv6 aren't.

If the importer checks for exact match on 3-tuples (asn, ipaddr4, ipaddr6), then both records will yield a mismatch and 2 tickets will be created.

@arnoldnipper arnoldnipper added this to the Consensus milestone Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.