NMS-16237: add support for RFC2348 "TFTP blksize" to the tftp implementation#8184
NMS-16237: add support for RFC2348 "TFTP blksize" to the tftp implementation#8184dino2gnt wants to merge 8 commits intorelease-35.xfrom
Conversation
dino2gnt
requested review from
cgorantla,
indigo423,
marshallmassengill and
synqotik
|
I've tested this in my lab and things seem to be working as expected. I think I would target this at 2025 or maybe even 2024? It's a pretty minimal change but it's a good quality of life improvement for the feature. I would suggest we tweak the docs for this a bit too explaining that suffix variable a bit if nothing else and mentioning the support for RFC2348. |
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
Code changes look good. See my comments for other notes.
I changed the behavior, now no matter how the user interprets the |
| @Override | ||
| public void onFileReceived(InetAddress address, String fileName, byte[] content) { | ||
| if (fileName.endsWith(fileNameSuffix)) { | ||
| if (fileName.endsWith(fileNameSuffix) || fileName.contains(fileNameSuffix)) { |
There was a problem hiding this comment.
Anecdotally, multiple users in multiple instances have been confused by "filenameSuffix" and what "suffix" actually means when relating to "filename" (this tripped up Marshall while were testing this, in fact). Enforcing where the matching string lives in the string is completely arbitrary and confusing to the end user. It could absolutely be simplified to:
if (fileName.contains(fileNameSuffix)) {
if you prefer.
There was a problem hiding this comment.
Then it is incomplete. Current code only removes the fileNameSuffix at the end of the fileName
future.complete(Either.right(new Success(content, fileName.substring(0, fileName.length() - fileNameSuffix.length()), scriptOutput)));
There was a problem hiding this comment.
Wasn't aware we removed it at all :D I'll take a closer look.
There was a problem hiding this comment.
private static String uniqueFilenameSuffix() {
// the term "monitor" is required to not consume it from the sink module, see class DeviceConfigDispatcher
return BASE64_URL_ENCODER.encodeToString(uuidToBytes(UUID.randomUUID())) + "-monitor";
}
This is what fileNameSuffix actually adds for each retrieval , this is added to keep track of each retrieval. It doesn't want to receive any random TFTP upload. It wants to retrieve exactly what is sent from the script.
If there is anything not clear from this, we can update docs.
…/deviceconfig/tftp/impl/TftpServerImpl.java no fun :/ Co-authored-by: Chandra Gorantla <chandra@opennms.com>
marshallmassengill
left a comment
marshallmassengill
left a comment
There was a problem hiding this comment.
LGTM. I would like to target this at M2025 though.
I don't think we should introduce support for a new protocol feature in the middle of a Meridian lifecycle. I'd prefer this goes into whatever Horizon and later M2026. |
|
Fair point... ship it. We're not that far off from 2026 anyway. |
cgorantla
left a comment
cgorantla
left a comment
There was a problem hiding this comment.
Don't think we need any change in Retrieverimpl. fileNameSuffix is designed to be at the end after the suffix like send: put juniper.conf.gz juniper.conf.gz${filenameSuffix}.
fileNameSuffix is used to keep track of what was requested and doesn't want to just retrieve any random upload.
| @Override | ||
| public void onFileReceived(InetAddress address, String fileName, byte[] content) { | ||
| if (fileName.endsWith(fileNameSuffix)) { | ||
| if (fileName.endsWith(fileNameSuffix) || fileName.contains(fileNameSuffix)) { |
There was a problem hiding this comment.
private static String uniqueFilenameSuffix() {
// the term "monitor" is required to not consume it from the sink module, see class DeviceConfigDispatcher
return BASE64_URL_ENCODER.encodeToString(uuidToBytes(UUID.randomUUID())) + "-monitor";
}
This is what fileNameSuffix actually adds for each retrieval , this is added to keep track of each retrieval. It doesn't want to receive any random TFTP upload. It wants to retrieve exactly what is sent from the script.
If there is anything not clear from this, we can update docs.
cgorantla
left a comment
cgorantla
left a comment
There was a problem hiding this comment.
Also we might want to re-target this to release-36.x or to foundation-2025
3 participants
This also:
${filenameSuffix}is checked (endwith()/contains())Happy to re-target to a different branch if y'all think this is simple enough to go back into other Meridians, otherwise i think this will get it into Horizon 35 and Meridian 2026 ?
Pretty limited blast radius either way.
1>
Accessing the socket via reflection and the blksize test were written with Claude's assistance.
External References