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

alpha versions fail to raise MDSconnection issue #2750

Open
smithsp opened this issue Apr 26, 2024 · 8 comments
Open

alpha versions fail to raise MDSconnection issue #2750

smithsp opened this issue Apr 26, 2024 · 8 comments
Assignees
Labels
api/python Relates to the Python API branch/alpha This is present on or relates to the alpha branch bug An unexpected problem or unintended behavior tool/mdsip Relates to one of the MDSplus over IP tools (mdsipd, mdsipsd, mdsip_server) US Priority

Comments

@smithsp
Copy link

smithsp commented Apr 26, 2024

Affiliation
GA

Version(s) Affected
Server on alpha-7-140-72 with clients at 7.7.15 or 7-140-71

Platform
The server is on RHEL8. The clients are on linux and osx systems.

Describe the bug
Previously (like with the previous server version on atlas) when a connection was denied, then MDSplus would raise a Connection error.

To Reproduce
In python:

import MDSplus
c=MDSplus.Connection('atlas.gat.com:8000')  # <------ This doesn't raise an error any more when denied connection.

Expected behavior
Expected:

>>> import MDSplus
>>> c=MDSplus.Connection('mdstest.gat.com:8000')
Error in connect: Access denied
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/fusion/projects/codes/atom/omfit_omega_v3.x/atom_git/mambaforge_stable/lib/python3.7/site-packages/MDSplus/connection.py", line 142, in __init__
    raise MdsIpException("Error connecting to %s" % (hostspec,))
MDSplus.connection.MdsIpException: %MDSPLUS-E-Unknown, Error connecting to mdstest.gat.com:8000

This error isn't being thrown with the latest server versions.

Impact
OMFIT relies on the exception being thrown to know to tunnel through our bastion host. We have codes in OMFIT that run off-site that need to access MDSplus and are not white listed, so they tunnel through the bastion host.

@smithsp smithsp added the bug An unexpected problem or unintended behavior label Apr 26, 2024
@smithsp smithsp changed the title alpha and ga_atlas versions fail to raise MDSconnection issue alpha versions fail to raise MDSconnection issue Apr 26, 2024
@mwinkel-dev
Copy link
Contributor

Hi @smithsp -- The "previous server version on Atlas" was stable_7.96.9, correct? If so, that is from 7-Feb-2020.

And yes, there have been changes to connection behavior in the following years. For example, PR #2288 altered some aspects of connections.

Regardless, alpha_7.140.72 should raise an error. I will now investigate what has changed in the past ~4 years.

@mwinkel-dev mwinkel-dev added US Priority api/python Relates to the Python API tool/mdsip Relates to one of the MDSplus over IP tools (mdsipd, mdsipsd, mdsip_server) branch/alpha This is present on or relates to the alpha branch labels Apr 26, 2024
@mwinkel-dev mwinkel-dev self-assigned this Apr 26, 2024
@smithsp
Copy link
Author

smithsp commented Apr 26, 2024

@mwinkel-dev I don't mean stable_7.96.9. I mean alpha-7.139-59.

@mwinkel-dev
Copy link
Contributor

Hi @smithsp -- Thanks for the clarification! That news greatly simplifies the troubleshooting.

@smithsp
Copy link
Author

smithsp commented Apr 26, 2024

Also the bug is not on the ga_atlas build.

@mwinkel-dev
Copy link
Contributor

Hi @smithsp -- Both facts are consistent, which is also good news. In a few minutes, I should be able to reproduce the problem.

@mwinkel-dev
Copy link
Contributor

I have just reproduced the problem with current alpha.

@mwinkel-dev
Copy link
Contributor

mwinkel-dev commented Apr 26, 2024

Root cause was an incomplete fix for Issue #2652. (Note that the issue was fixed after alpha_7.139.59.)

That bug involved a status variable that was overloaded and thus failed (i.e., allowed unauthorized clients to connect).

The fix for that bug was to eliminate the overloading by using two variables: status and auth_status. Unfortunately, the auth_status wasn't used to set the flag in the message header that was returned to the client.

Submitting a two line PR to complete the fix.

@mwinkel-dev
Copy link
Contributor

Note that the PR #2752 fix handles the scenario when a client successfully establishes a network connection but fails the user authorization check. It returns a failure status to the client so that the client can raise an exception.

The existing code already raises exceptions on these related scenarios:

  • attempt to establish a network connection to an unreachable (or non-existent) server, and
  • attempt to establish a network connection to a non-existent port on a reachable server.

WhoBrokeTheBuild added a commit that referenced this issue May 21, 2024
* Gm apd java (#2729)

* Improve APD support for Java interface

* Improve APD support for Java - forgotten files

* Commit packages

* When activate debug trace, now compiles without error. (#2735)

This fixes Issue 2734.

* Fix: reduce open files due to dispatcher (#2740)

* Fix: reuse action_server connection id in ServerConnect; avoid duplicates in list

* Fix: set dispatched early; unset if dispatching failed; prevent race on fast actions

* Fix: lock Clients in ServerQAction; cleanup and check before use

* Fix: reconnect dropped connections

* Fix: use correct windows SOCKET print format

* Fix: satisfy rhel7 c standard

* Gm apd thin cpp (#2742)

* Added ADP support in C++ thin client

* Added tdi fun

* Added TDI FUn

* Fix commands

* Gm new marte (#2743)

* more parameters for marte2_simulink_generic

* Proceed with the new implementation

* Proceed

* Proceed

* Proceed

* Proceed

* Proceed

* proceed

* Proceed

* Proceed

* Partially tested version

* Added execution times recording

* Proceed

* Procced with debugging

* Proceed

* Proceed

* Proceed

* Fixes for multisampled acquisition

* Remove quotes from string parameters

* Minor fixes

* Procced debugging

* Debugging

* More channels

* Debug Distributed configuration

* Fix sognal recording for synchronized inputs

* Further debug

* Further debug

* Small fixes

* Close ti final version

* Forgotten fix

* Make port visible, fix parameter name

* unaligned nids

* Increase DiscontinuityFactor

* Discontinuityfactor

* More channels

* Proceed with the new implementation

* Proceed

* Proceed

* Proceed

* Proceed

* Proceed

* proceed

* Proceed

* Proceed

* Partially tested version

* Added execution times recording

* Proceed

* Procced with debugging

* Proceed

* Proceed

* Proceed

* Fixes for multisampled acquisition

* Remove quotes from string parameters

* Minor fixes

* Procced debugging

* Debugging

* More channels

* Debug Distributed configuration

* Fix sognal recording for synchronized inputs

* Further debug

* Further debug

* Small fixes

* Close ti final version

* Forgotten fix

* Make port visible, fix parameter name

* unaligned nids

* Increase DiscontinuityFactor

* Discontinuityfactor

* More channels

* Packages updated

* Remove print

* Remove error messages

---------

Co-authored-by: mdsplus <mdsplus@roactive2.rfx.local>

* Docs: Improve documentation for getSegment* python wrappers (#2732)

Add explanation and rename parameters for:
* getSegmentLimits
* getSegmentList

* Fix: Update JAVASOURCE to 8 to support JDK 17 (#2747)

* Fix: improve mdstcl's error handling and add comments (#2746)

* add comments regarding action service

* send_reply() now does cleanup_client() on bad socket

* explain mdstcl's receiver thread cannot access main thread's connection list

* Improve handling of non-MDSplus error codes

* add comments regarding action dispatch

* add comment explaining receiver thread select loop

* Fix: multiple string escape warnings thrown by python 12 (#2748)

```
mdsplus/pydevices/RfxDevices/FAKECAMERA.py:40: SyntaxWarning: invalid escape sequence '\C'
  {'path': ':EXP_NODE', 'type': 'text', 'value': '\CAMERATEST::FLIR:FRAMES'},

mdsplus/pydevices/RfxDevices/PLFE.py:220: SyntaxWarning: invalid escape sequence '\#'
  '^(\#[0-5][01]([01][0-9][0-9]|2[0-4][0-9]|25[0-5])){6}$', msg)

mdsplus/pydevices/RfxDevices/CYGNET4K.py:361: SyntaxWarning: invalid escape sequence '\E'
  self.serialIO(b'\x55\x99\x66\x11\x50\EB', None)

mdsplus/pydevices/RfxDevices/CYGNET4K.py:461: SyntaxWarning: invalid escape sequence '\8'
  return self.setValue(b'\81\x82', min(0xFFF, value), True)

mdsplus/pydevices/MitDevices/dt100.py:161: SyntaxWarning: invalid escape sequence '\.'
  regstr = '([0-9\.]*) [0-9] ST_(.*)\r\n'
```

The \CAMERATEST became \\CAMERATEST
The regex strings should be python r-strings `r""`, but to maintain backwards compatibility, we're using \\
The broken hex-codes now have x in them

* Build: Resolve linker error after updating the windows builder to Fedora 39 (#2749)

* Build: Resolve linker error after updating the windows builder to Fedora 39

This appeared after updating the mdsplus/builder:windows docker image to Fedora 39, and Wine to 9.0
The newer libxml2 tried to link dynamically unless we explicitly set LIBXML_STATIC

* Hopefully fix the MdsTreeNodeTest

It turns out that this was failing previously, but we weren't properly catching the error

* Fix errors in windows build from newer gcc

* Docs: Update sites.csv (#2615)

add Startorus Fusion in Xi'an, China

* Fix: mdsip now sends proper auth status back to the client (#2752)

Fixes issues #2750 and #2652

* Fix: mdstcl's `show current` no longer segfaults when no tree paths defined (#2754)

* Fix: "show current" no longer segfaults when no tree paths defined

* Fix: corrected typo in error message

* Use original error message so tests pass

* Fix: Add Debian 12 and Ubuntu 24.04 and support GCC 12+ (#2753)

* Build: Add Debian 12 and Ubuntu 24.04

* Add extra flags for GCC 12+ and stub imp for Python 3.12

GCC 12+ triggers a bunch of false positive warnings (which we treat as errors)
This adds AX_C_FLAGS to configure those `-Wno-*` flags for GCC 12+
`cmdExecute.c` now uses snprintf to avoid buffer overflow warnings, also generated by GCC 12+
`compound.py.in` now supports Python 3.12+

* compound.py now supports Python 2.7.. again

---------

Co-authored-by: Stephen Lane-Walsh <slwalsh@psfc.mit.edu>

* Fix: Improve error messaging when calling Setup Device in jTraverser (#2744)

* Improve error messaging when calling Setup Device in jTraverser

e.getMessage() sometimes returned null, but just e will always print something
Add a printStackTrace() for InvocationTargetException exceptions to show the encapsulated error

* Add import for InvocationTargetException

* Build: Fix off-by-one versions produced by Jenkins (#2756)

This fixes the bug where `--os=bootstrap` wasn't receiving the version from `--version=x.y.z`
However, confusingly, this also changes the Jenkinsfile to not use that feature, and instead use `git tag` in order to embed the proper git information as well as the proper version information
The `--os=bootstrap` and `--version` fix is still included just so that it doesn't break if someone else tries to use it

* Build: Increase default test timeout to 1h (#2757)

When the build server(s) are at capacity, it's not unreasonable for a test to take more than 10 seconds, which was the old default timeout
This sets the default to 1h, and removes the overrides in various tests

* Gm fix filter (#2755)

* Allow filtering data from MinMax resampling; remove useless thread in jServer

* Fix compile error

* Remove debug message

* Make Windows Compiler happy

* Build: Fix 'HEAD' in `show version` and tag error (#2758)

Jenkins builds in a detached HEAD state, which caused bootstrap to use HEAD as the branch name
We pass --branch= to the bootstrap call in Jenkins, but $BRANCH wasn't being passed into the bootstrap docker container
Also, attempts to build alpha versions with tags that already existed failed

* Fix: mdstcl show version tag and links (#2760)

Fixes Issue #2759

* Feature: CompileTree will exit with non-zero status code for error messages. (#2446)

And error message should go to stderr.

* Build: Add package override for ubuntu and debian (#2761)

Override sections for Ubuntu 24 and Debian Bookworm were added.

* Fix: Python release version tag (#2764)

* Feature: Add "Date:" to show version output (#2767)

Implements #2766

Example:
```
$ mdstcl sho ver

MDSplus version: 7.140.75
----------------------
  Release:  alpha_release-7-140-75
  Date:     Thu May 16 17:43:14 UTC 2024
  Browse:   https://github.com/MDSplus/mdsplus/tree/alpha_release-7-140-75
  Download: https://github.com/MDSplus/mdsplus/releases/tag/alpha_release-7-140-75
```

* Fix: remove abort flag from RfxDevices DIO2 initialization (#2769)

Fixes issue #2768

* Fix: Missing repo metadata signing (#2770)

This will hopefully fix the lack of signed metadata files that are preventing us from automatically publishing releases

---------

Co-authored-by: GabrieleManduchi <gabriele.manduchi@igi.cnr.it>
Co-authored-by: mwinkel-dev <122583770+mwinkel-dev@users.noreply.github.com>
Co-authored-by: Timo Schroeder <zack-vii@users.noreply.github.com>
Co-authored-by: mdsplus <mdsplus@roactive2.rfx.local>
Co-authored-by: Josh Stillerman <jas@psfc.mit.edu>
Co-authored-by: Fernando Santoro <44955673+santorofer@users.noreply.github.com>
Co-authored-by: Louwrensth <Louwrensth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/python Relates to the Python API branch/alpha This is present on or relates to the alpha branch bug An unexpected problem or unintended behavior tool/mdsip Relates to one of the MDSplus over IP tools (mdsipd, mdsipsd, mdsip_server) US Priority
Projects
None yet
Development

No branches or pull requests

2 participants