-
Notifications
You must be signed in to change notification settings - Fork 16
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
I3S - Simplify login to i3s through oneview client #166
Comments
@marikrg , I think the reason this was done is because I3S doesn't have an endpoint to login itself; it just uses the token provided from OneView. So you'd have to create a OneView client, then copy that into an I3S client. It seems like it could make sense to roll that all into 1, but then the client would hold 2 separate URLs (1 for OV & 1 for I3S). That seems strange/wrong to implement to me, so although it's a little strange, I think the current solution is the best way (as long as I3S doesn't have a login endpoint itself). @aalexmonteiro , do you have anything to add/correct about that? |
@jsmartt @aalexmonteiro So, if I'm running many operations, like one of our devops tools could, our expected design is to always assume my token has expired and invoke the |
@fgbulsoni , It's strange the token is expiring so fast. @jsmartt , has implemented a |
I've never had an issue with it, even after pretty long runs using a single token. It looks like by default, tokens expire 24-hrs after creation. It looks like a session can be extended as well, but I kept getting 401 unauthorized responses after trying to extend my own session. I'm sure we can figure something out if need be, but I think until OneView allows persistent tokens, most users will use a username/password to log in at the beginning of the execution and never run into issues. One possible solution I thought of would be to keep track of when the session was created & how long it would be valid, so that any time a rest call was made after it is expected to expire, we can automatically refresh the token. But again, I don't see it as a problem that needs to be solved at this point |
@marikrg Would you mind sharing how this was implemented on the Python SDK, for comparison purposes? |
The Python SDK authentication is based on the OneView / Image Streamer UI:
In the Python SDK, first of all, the user needs to create the OneViewClient: oneview_client = OneViewClient.from_json_file(EXAMPLE_CONFIG_FILE) You can check an example of json config file at: https://github.com/HewlettPackard/python-hpOneView/blob/master/examples/config-rename.json Then, to create the ImageStreamerClient, you must call the create_image_streamer_client method from the previously created OneViewClient instance. image_streamer_client = oneview_client.create_image_streamer_client() |
In addition, in the Python SDK the user can provide either an username/password or a token. |
I like the |
I like it as well, and another plus side of this approach is that since it acts similarly to the way the GUI handles the authentication, it might be the way a customer would expect it to be done. |
The way it was done in python is interesting. But I have some thoughts on this: 1-In Ruby we will have to remove the inheritance between oneview client and i3s client and use composition, this will imply code duplication. In python it has a bit of replication (I do not see problems), 2-Although interesting, somehow we have two instance variables for each type of client. Current code. client = client = OneviewSDK::Client.new(params)
client_i3s = OneviewSDK::ImageStreamer::Client.new(client.token) Proposed code client = OneviewSDK::Client.new
client_i3s = client.create_i3s_client I do not see any great advantage to change, since we will lose code reuse. That is my opinion, but that does not mean that I am right. |
The idea is to make this token not visible to the end user, like in the proposed code. I cannot see why it will be necessary to remove the inheritance, and if removing would cause code duplication. Also, it would be easier for the tools to have a creation method passing both data in the params: def initialize(options = {})
@token = if options[:oneview_client]
@oneview_client = OneviewSDK::Client.new(options[:oneview_client])
@oneview_client.token
else
options[:token] || ENV['I3S_TOKEN']
[...]
end Also, I noticed the initialize in the ImageStreamer::Client is almost completely duplicated, we have a lot of room for improvements there. |
@tmiotto , I got it. Now that seems clear for me. |
I agree with @tmiotto about to pass data for |
I think it's still good for the I3S client initialize method to have all the options that it has now, but it would be nice for the OV client to have the def new_i3s_client(opts = {})
OneviewSDK::ImageStreamer::Client.new(opts.merge(token: @token))
end And usage: i3s_client = ov_client.new_i3s_client
# or
i3s_client = ov_client.new_i3s_client(api_ver: 300) Simple, yet flexible |
Yeah... this is a resonable solution, in the code it would look like: @ov_client = OneviewSDK::Client.new(
url: 'https://xxx.xxx.xxx.xxx',
user: 'Admin',
password: 'admin',
ssl_enabled: true,
api_version: 300
)
@i3s_client = @ov_client.new_i3s_client(
url: 'https://yyy.yyy.yyy.yyy',
ssl_enabled: true,
api_version: 300
) |
Changing tittle since it seems we've all agreed it would be good/important to have this feature implemented. |
Scenario/Intent
As far as I can see, currently I'm able to authenticate on Image Streamer only using a token
I have tried to access with an username/password and I got the error:
Must set token option
From the tools point of view, may be better for some of them authenticate with username/password. The token expires frequently and the tools doesn't provide (and I believe it shouldn't) a way to generate it.
Then, it shouldn't be supported by ImageStreamer client?
Environment Details
Steps to Reproduce
Must set token option
is raised.Expected Result
Possible solution:
Actual Result
[What actually happens after the steps above? Include error output or a link to a gist.]
The text was updated successfully, but these errors were encountered: