EDI SFTP doesn't work

Bug #2040514 reported by Chris Sharp
36
This bug affects 6 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
High
Unassigned
3.11
Fix Released
High
Unassigned
3.12
Fix Released
High
Unassigned

Bug Description

We found when testing SFTP EDI transfer for the upcoming Ingram change that the current SFTP mechanism doesn't actually work for our use cases. I propose reimplementing this with Net::SFTP::Foreign instead of the very clunky Net::SSH2. Working branch on the way...

Revision history for this message
Chris Sharp (chrissharp123) wrote :
Changed in evergreen:
status: New → Confirmed
Revision history for this message
Mike Rylander (mrylander) wrote :

Thanks, Chris!

A note from an initial scan of the code: we should either leave all the ssh2 stuff in place (in particular, the module installation and the disconnect code that's trimmed by the patch) or remove all of it (the 'use' line to pull Net::SSH2 in, and at least the conditional branches that that take us down the ssh2 road). Right now, it looks like a fresh installation on a new server will be unhappy because Net::SSH2 isn't being installed (directly, maybe it's a dep that something else pulls in), but it's still used in the code. Using this on an existing, working EG instance looks like it will be fine, though, so testing of the main logical changes can proceed, IMO.

Revision history for this message
Jason Boyer (jboyer) wrote :

I've fixed some syntax errors and added a couple followups here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/jboyer/lp2040514_reimplement_acq_edi_sftp / working/collab/jboyer/lp2040514_reimplement_acq_edi_sftp

I didn't add my signoff since Chris hasn't either, but the branch is much more testable now.

Revision history for this message
Terran McCanna (tmccanna) wrote :

Adding pullrequest to get more attention

Changed in evergreen:
milestone: 3.12-beta → 3.next
tags: added: pullrequest
Revision history for this message
Chris Sharp (chrissharp123) wrote :

Branch with Jason's additions and signoffs everywhere is here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp2040514_reimplement_acq_edi_sftp_signoff

We need a second signoff on the initial commit, for whoever comes behind.

Revision history for this message
Galen Charlton (gmc) wrote :

I put it through its paces, and there are problems:

[1] I got it to upload files via SFTP, but (a) adding the "StrictHostKeychecking=no" option should be considered and (b) it would be good to give it a mode where it can use SSH keys instead of a password. (That may be as simple as not passing along a null/empty password and relying on the Evergreen admin to have set up SSH keys instead on the server.)

[2] ls_sftp() doesn't work and appears to be primarily a copy-and-paste of ls_ftp, including trying to use $server->_ftp and methods such as ->dir() that do not exist in Net::SFTP::Foreign. Consequently, edi_fetcher.pl doesn't work at all for SFTP accounts.

Changed in evergreen:
assignee: Chris Sharp (chrissharp123) → nobody
tags: added: needswork
removed: pullrequest
tags: added: acq
Revision history for this message
Joan Kranich (jkranich) wrote :

Ingram sent a Service Alert today stating that as of April 15, 2024 they would no longer support non-secured FTP.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

I'll take this one to review the code and address Galen's suggestions in comment #6.

We're keen to have this working before the Ingram deadline of April 15.

Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
status: Confirmed → In Progress
Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → Chris Sharp (chrissharp123)
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Following up on an IRC conversation with Chris Sharp yesterday, I have assigned this bug to him at his request: http://irc.evergreen-ils.org/evergreen/2024-02-27#i_546305

Revision history for this message
Jason Stephenson (jstephenson) wrote :

And, after another IRC conversation, Chris Sharp has asked me to take it over.

Changed in evergreen:
assignee: Chris Sharp (chrissharp123) → Jason Stephenson (jstephenson)
Changed in evergreen:
status: In Progress → Confirmed
milestone: 3.next → 3.13-beta
Revision history for this message
Jason Stephenson (jstephenson) wrote :

I pushed a branch to working/collab/dyrcona/lp2040514_reimplement_acq_edi_sftp_signoff (https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dyrcona/lp2040514_reimplement_acq_edi_sftp_signoff).

The branch has my signoff added to Chris Sharp's and Jason Boyer's commits. It also add 3 commits. The top commit has the release notes entry. The next fixes issues with the ls_sftp and get_sftp functions. The third sets the "StrictHostKeychecking=no" option as Galen suggests in comment #6. Feel free to squash these into 1.

I did not attempt to add the ability to specify an identity key because I think that should be a different bug.

I successfully tested the branch with production data on a couple of local virtual machines. With one set up as the SFTP server and the other running Evergreen, I was able to create purchase orders, activate them, run the edi_order_pusher.pl and get them sent to the my test SFTP server in the appropriate location.

I also tested retrieval of ORDRSP and INVOIC messages by copying some from production to the SFTP VM and running edi_fetcher.pl.

I used the attached SQL to grab data from production for purchase orders that did not exist in the test database. (It helps if the test database is a few weeks/days behind production.)

I set all but one or two test providers to active = false in the test database. I edited the EDI account information of the test accounts to use SFTP with an account on my SFTP VM. (It helps to allow login via password in the SSH configuration, or this won't work.)

I don't know how you would test this with Concerto data.

Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
tags: added: pullrequest
removed: needswork
Revision history for this message
Jason Stephenson (jstephenson) wrote :

Ugh! I noticed a typo in one of my commit messages, so I fixed that and force pushed over the above branch.

If you've started looking at this, please grab the branch again.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Blake and I (Jason S.) reviewed this in IRC today. Blake signed off and pushed the changes to main and rel_3_12.

I cherry-picked the changes to rel_3_11 from main in order to resolve a conflict with a missing prerequisite file.

Thanks, Chris, Mike, Jason B., Terran, Galen, and Joan for your contributions to this bug!

Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.