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

Support for Brocade FOS-based SAN devices #254

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Support for Brocade FOS-based SAN devices #254

merged 1 commit into from
Feb 26, 2018

Conversation

marcelhuth
Copy link
Contributor

Hi,

I made a few changes that makes train able to detect FOS (Fabric OS) of Brocade Fibre-Channel switches.

The whole thing has currently a problem (stated in the code as well): The OS of these devices is linux based and does respond to uname command correctly when logging in as root user. If you log in with root, train will detect Unix/Linux/Generic Linux what is not what we want here. If you log in with a different (normal) user, that user will get rbash shell as CLI and uname command is not available what makes it possible to "fall through" the unix detection and detect a brocade_fos platform.

Normally you won't log in using root user to those devices (vendor says it's not supported), nevertheless you could do this and it would break the detection.

One workaround would be to handle brocade detection before unix, but that would cause version command run on all systems (except windows). I guess this is not really an option. Unfortunately I have no idea how to get around this problem currently.

Thanks for your feedback!

@marcelhuth marcelhuth requested a review from a team February 8, 2018 11:33
@chris-rock
Copy link
Contributor

@marcelhuth This is awesome! If it is detected as Linux here https://github.com/chef/train/blob/master/lib/train/platforms/detect/specifications/os.rb#L329, you could add a check for brocade just before, therefore it will test brocade before jumping into generic linux.

@marcelhuth
Copy link
Contributor Author

marcelhuth commented Feb 8, 2018

@chris-rock Thanks for your feedback.
Actually it would not work this way unfortunately. Normally you won't work with root user on that devices. If you are not root (the standard use case) the CLI won't get you a stdout from uname and so the device is not a unix-family for train. At the end an exception will be thrown here https://github.com/chef/train/blob/master/lib/train/platforms/detect/scanner.rb#L41

So the code currently work for the standard use case (normal user to login) at the moment. It gets complicated when using root (and there are people working with root on the devices, even though it's not supported/recommended).

@jquick
Copy link
Contributor

jquick commented Feb 13, 2018

Looks good to me. Thanks for your work on this @marcelhuth!

@marcelhuth
Copy link
Contributor Author

Hi @jquick ,
thanks for approving this. Please do not merge anything at the moment... I'm still thinking about how to fix the problem with different logins that lead to different behavior.

As @chris-rock stated the check could be moved just before "Generic Linux", but we only reach that code when using root user. So I'm curently thinking about having two checks for brocade_fos. The first check has to be done just after unix family (if logging in with any user different from root we reach this code and anything is fine) the second check has to be done before "Generic Linux" (this will be reached when logging in with root). When the second check find a brocade_fos we have to change the previously detected os.family from "unix" to "brocade", and I don't know if this is good practice or if this could even work...

A second possibility is to throw an exception for "unsupported user in OS" (or something similar) when having the second check true.

I would like to avoid the situation where somebody depends on the os.platform is "unix" and does anything that is supported there that breaks the brocade device when using root user.

Do you have any ideas on that?

Thanks,
Marcel

@marcelhuth
Copy link
Contributor Author

Hi @jquick and @chris-rock
I tried to fix the issue with the root login. It should not break the default "Generic Linux" case just skip the detection of Linux if the devices responds to Brocade's version command and then go on with the next platform. What do you think?
Thanks,
Marcel

@jquick
Copy link
Contributor

jquick commented Feb 18, 2018

This works but gets us into a position of having to support the downstream platforms by ignoring it. Instead I think you had the right idea of putting the bash version right above linux in its own family then redefine the family again at the bottom. I am submitting my own refactor for arista which has the same issue. Ill post here the section, when I post it.

@jquick
Copy link
Contributor

jquick commented Feb 22, 2018

@marcelhuth
Copy link
Contributor Author

Hi @jquick - saw your PR already ;) As this is now in master, I'll get my stuff changed to fit to the new base os family layout. I'll refactor the bash vs. "normal" shell login in the way you did with Arista as well. I'll try to get that ready the next days. Thanks for helping.

Signed-off-by: Marcel Huth <marcel.huth111@gmail.com>
@jquick
Copy link
Contributor

jquick commented Feb 26, 2018

Looks good to me! Thanks for your work on this @marcelhuth !

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @marcelhuth Great addition!

@jquick jquick merged commit 63e301c into inspec:master Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants