-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add config for returning dummy object in the NRTMv3 response #924
base: main
Are you sure you want to change the base?
Add config for returning dummy object in the NRTMv3 response #924
Conversation
b689ff3
to
fad6fe5
Compare
Nice! This will take a bit of time to review, but my first thought is that the logic to generate dummified object text should be separated to make it easy to apply to RPSL full exports and NRTMv4 as well. Probably in the same place as remove_auth_hashes(). Makes tests a bit simpler too. (Initially I thought about putting it on RPSLObject, but we intentionally don't have that in this situation, just the text from the database.) |
Yes, the function |
@mxsasha The reason I want to keep the original value is that I want to return the original pk instead of the uppercase one in the dummy NRTM response.
|
What I meant is: to properly support dummy mirroring, dummifying needs to happen in the NRTM v3 generator, the source export (if unfiltered is not set, currently called "remove_auth_hashes" I think), and the NRTMv4 server. With as little duplication as possible. So the call from the NRTMv3 generator should be something like:
The only place where we retain the original case for the PK is in the object text, so we'd have to re-parse the whole object. PKs are case insensitive, so the indexed key is normalised. But I don't follow why you need it, your current implementation in text.py seems the most straightforward, without needing the PK separately. |
22d78e8
to
880bdc8
Compare
Ok
My implementation in text.py needs a pk to be provided. As I mentioned above, If my config for the dummy attribute is like I can't find an easy way to find the original pk from the whole object. |
c52cafe
to
3f46413
Compare
There is none. The data simply isn't extracted, as PKs are case insensitive in every other usage, and therefore normalised to upper case during parsing of the initial object. Is it such an obstacle for you? Does the PK need to be repeated in the dummy text? After all, anyone who looks at the object will see the true PK already anyways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some notes inline. We also need to extend this to the source export runner, and the NRTM4 server. In those cases, we don't have client IPs, so our only option is to always enable it if configured.
I also want to tweak the docs and settings names a bit, later.
irrd/utils/text.py
Outdated
if dummy_attributes: | ||
if get_setting(f"sources.{source}.nrtm_response_dummy_remarks"): | ||
dummy_remarks = textwrap.indent( | ||
get_setting(f"sources.{source}.nrtm_response_dummy_remarks"), "remarks:".ljust(16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does that 16 come from? From somewhere in irrd.rpsl right? Feels like we should make that a constant then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it comes from RPSL_ATTRIBUTE_TEXT_WIDTH which has been used in different places. Updated to use the constant, thanks
Nvm, thanks. |
Ok, we need this because we have some internal clients for accessing the IRRD NRTM3 server, so it needs the original data.
No problem, thanks for looking into it. |
Add new configs in IRRD for returning the dummy object in NRTMv3 response
Example config
Example response