Implement serial console proxy #174
Conversation
tzumainn
commented
Aug 14, 2024
tzumainn
commented
- add a serial console proxy (based on Nova)
- create REST API that users can use to create/delete console auth tokens
esi_leap/db/sqlalchemy/models.py
Outdated
| __table_args__ = (Index("console_auth_tokens_node_uuid_idx", "node_uuid"),) | ||
|
|
||
| id = Column(Integer, primary_key=True, nullable=False, autoincrement=True) | ||
| node_uuid = Column(String(255), nullable=True) |
There was a problem hiding this comment.
Should node_uuid and token_hash nullable=False?
There was a problem hiding this comment.
Good catch! Fixed.
tzumainn
force-pushed
the
serial-console-proxy
branch
from
28baf87 to
af71be5
Compare
| context = pecan.request.context | ||
| node_uuid_or_name = new_console_auth_token["node_uuid_or_name"] | ||
|
|
||
| # enable Ironic console |
There was a problem hiding this comment.
Should enabling the console be a separate step? I think the process should be:
openstack baremetal node console enable <node>
openstack esi mumble <node>
(where mumble is whatever command is necessary to generate the token)
In other words, we shouldn't be tying the console enable state to whether or not there currently exists a valid token.
There was a problem hiding this comment.
I don't disagree with this, except I do think it's convenient to the user to simply be able to run a single command and get the token and the enabled console in one swoop. I think you mentioned in the demo meeting that the CLI command should perhaps be openstack esi console enable instead of openstack esi console token create, and I kind of like that.
I could put the console enablement in the CLI instead - well, more likely the SDK? Or I could rename this controller to console.py instead?
There was a problem hiding this comment.
Part of my concern is the console behavior when there are multiple users: with the current model, it looks like deleting a token will also disable the console. If there are multiple tokens in play, isn't that the wrong behavior? That's why I think token management should be separate from console enable/disable.
There was a problem hiding this comment.
Ah, that's definitely a fair point. I'll disentangle the two then.
esi_leap/common/utils.py
Outdated
| @@ -18,3 +20,8 @@ | |||
|
|
|||
| def get_resource_lock_name(resource_type, resource_uuid): | |||
| return resource_type + "-" + resource_uuid | |||
|
|
|||
|
|
|||
| def get_sha256_str(base_str): | |||
There was a problem hiding this comment.
This is only ever called from esi_leap/objects/console_auth_token.py. Can it go there instead of utils.py? Trying to avoid utils.py becoming a dumping ground.
There was a problem hiding this comment.
Yep! It was in utils.py in nova because it was called from multiple places, but I guess it's unlikely we'll need to generate another hashed token.
| console_info = ironic.get_ironic_client().node.get_console( | ||
| connect_info.node_uuid | ||
| ) | ||
| url = urlparse.urlparse(console_info["console_info"]["url"]) |
There was a problem hiding this comment.
What does the console url look like in this case, and should we be validating the value somehow (e.g., if the user enabled a console type that we don't support)?
There was a problem hiding this comment.
The console URL is actually controlled by Ironic; users have no control over it whatsoever.
There was a problem hiding this comment.
I think you have missed the nature of my question; I understand that the console url is controlled by ironic. However, will it differ depending on the enabled console type?
There was a problem hiding this comment.
Ah - it will if it's web console enabled, in which case the URL will simply be an http URL. So for future-proofing purposes, perhaps we should check if the URL is prefixed with tcp; if not, return an error (since the serial console proxy won't work for a web console).
esi_leap/console/websocketproxy.py
Outdated
| def send_head(self): | ||
| # This code is copied from this example patch: | ||
| # https://bugs.python.org/issue32084#msg306545 | ||
| path = self.translate_path(self.path) | ||
| if os.path.isdir(path): | ||
| parts = urlparse.urlsplit(self.path) | ||
| if not parts.path.endswith("/"): | ||
| # Browsers interpret "Location: //uri" as an absolute URI | ||
| # like "http://URI" | ||
| if self.path.startswith("//"): | ||
| self.send_error( | ||
| HTTPStatus.BAD_REQUEST, "URI must not start with //" | ||
| ) | ||
| return None | ||
|
|
||
| return super(ProxyRequestHandler, self).send_head() |
There was a problem hiding this comment.
The problem this is meant to solve apparently was fixed in Python 3.11.
There was a problem hiding this comment.
Ah - I'll take this out and test again!
tzumainn
force-pushed
the
serial-console-proxy
branch
from
af71be5 to
e3a7874
Compare
|
@larsks I believe these changes address your concerns - let me know if I missed anything! |
esi_leap/console/__init__.py
Outdated
| # under the License. | ||
|
|
||
| """ | ||
| :mod:`nova.console` -- Wrappers around console proxies |
There was a problem hiding this comment.
We should probably replace nova.console with esi_leap.console throughout.
esi_leap/console/__init__.py
Outdated
| .. automodule:: nova.console | ||
| :platform: Unix | ||
| :synopsis: Wrapper around console proxies such as noVNC to set up | ||
| multi-tenant VM console access. |
There was a problem hiding this comment.
...and probably fix this description.
There was a problem hiding this comment.
Fixed - thanks for pointing that out!
* add a serial console proxy (based on Nova) * create REST API that users can use to create/delete console auth tokens
tzumainn
force-pushed
the
serial-console-proxy
branch
from
e3a7874 to
11dadab
Compare
