Skip to content

cephadm: Allow users to use a custom dashboard ssl port#36716

Merged
sebastian-philipp merged 1 commit intoceph:masterfrom
jmolmo:dashboard_ports_2
Aug 21, 2020
Merged

cephadm: Allow users to use a custom dashboard ssl port#36716
sebastian-philipp merged 1 commit intoceph:masterfrom
jmolmo:dashboard_ports_2

Conversation

@jmolmo
Copy link
Member

@jmolmo jmolmo commented Aug 19, 2020

This modification allows the user to create a new bootstrap cluster using a predefined SSl port for the dashboard.
If firewall is enabled, any new manager daemon deployed in new hosts will take care of open the required ports for all the services enabled in the manager.

Two new parameters for cephadm tool (aka binary or standalone):
Command bootstrap:
--ssl-dashboard-port SSL_DASHBOARD_PORT
Port number used to connect with dashboard using SSL
Command deploy:
--tcp-ports TCP_PORTS
List of tcp ports to open in the host firewall

Note:
Opening ports in new hosts when deploying new manager modules only will work when this modification will be part of the used Ceph container image.

Fixes: https://tracker.ceph.com/issues/44628

Signed-off-by: Juan Miguel Olmo Martínez jolmomar@redhat.com

Copy link
Contributor

@sebastian-philipp sebastian-philipp left a comment

Choose a reason for hiding this comment

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

Yes, this look good! (minus some minor nits)

Comment on lines +343 to +351
if ports:
daemon_spec.ports = ports
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a look at

DEFAULT_SERVICE_PORT = 3000

If you want, you can extract this into a new method of CephadmService, with a default implementation, like so and hook this into make_daemon_spec :

class CephadmService:
    def get_ports(self, daemon_id: str, spec: ServiceSpec) -> List[int]:
        default_port = getattr(self, 'DEFAULT_SERVICE_PORT')
        return [default_port] or []

And suddenly you also support the monitoring stack.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see too much advantages of this approach... (maybe I miss something).
Taking into account that now, the "ports" is now an attribute of the "CephadmDaemonSpec" class. I would prefer
to add the "ports" attribute to the "ServiceSpec" class, and then we can start to manage properly ports in an all the services.
In any case I think that this must be managed as a separate PR coming from a separate tracker issue.
wdyt?

This modification allows the user to create a new bootstrap cluster using a predefined SSl port for the dashboard.
If firewall is enabled, any new manager daemon deployed in new hosts will take care of open the required ports for all the services enabled in the manager.

Two new parameters for cephadm tool (aka binary or standalone):
Command bootstrap:
--ssl-dashboard-port SSL_DASHBOARD_PORT
                      Port number used to connect with dashboard using SSL
Command deploy:
--tcp-ports TCP_PORTS
                      List of tcp ports to open in the host firewall

Signed-off-by: Juan Miguel Olmo Martínez <jolmomar@redhat.com>
@jmolmo jmolmo force-pushed the dashboard_ports_2 branch from f4a9a6e to 08c4a53 Compare August 20, 2020 12:16
@sebastian-philipp sebastian-philipp added the wip-swagner-testing My Teuthology tests label Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants