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

cephadm: support pulling image with http #42521

Closed
wants to merge 1 commit into from

Conversation

guits
Copy link
Contributor

@guits guits commented Jul 28, 2021

making cephadm support pulling image from registry using http instead of
https can be a valid scenario when you are using development, ci, or very isolated
environments where you don't really want to care about TLS.

Same operation could be achieved by editing either
/etc/containers/registries.conf or /etc/docker/daemon.json but it
would be definitely more convenient if cephadm could offer this option natively.

Fixes: #51901

Signed-off-by: Guillaume Abrioux gabrioux@redhat.com

making cephadm support pulling images from registries using http instead of
https can be a valid scenario when you are using development, ci, or very
isolated environments where you don't really want to care about TLS.

Same operation could be achieved by editing either
`/etc/containers/registries.conf` or `/etc/docker/daemon.json` but it
would be definitely more convenient if cephadm could offer this option
natively.

Fixes: ceph#51901

Signed-off-by: Guillaume Abrioux <gabrioux@redhat.com>
@guits guits added the cephadm label Jul 28, 2021
@guits guits requested a review from a team as a code owner July 28, 2021 13:56
Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

Can you also add it at the top of https://docs.ceph.com/en/latest/man/8/cephadm/#synopsis with the other general (non-command-specific, e.g. --image, --docker) flags

and give a brief description of what it does here https://docs.ceph.com/en/latest/man/8/cephadm/#options

cmd = [ctx.container_engine.path, 'pull', image]
cmd = [ctx.container_engine.path, 'pull']
if ctx.skip_tls_pull:
if ctx.docker:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ctx.docker:
if isinstance(self.ctx.container_engine, Docker):

I think the docker flag being checked here only makes us prefer docker over podman. If you have docker installed and don't have podman installed I think it will use docker regardless of whether the docker flag is set, so this check isn't guaranteed to work. Our usual way of checking this is checking if the current container engine instance is a Docker one.

Copy link
Contributor

@dsavineau dsavineau left a comment

Choose a reason for hiding this comment

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

I assume this is only working for local cephadm CLI command like boostrap or adopt process right ?

Because --skip-tls-pull is only used during those steps then new nodes added to the orch won't be aware of this (I might be wrong).

@adk3798
Copy link
Contributor

adk3798 commented Jul 29, 2021

I assume this is only working for local cephadm CLI command like boostrap or adopt process right ?

Because --skip-tls-pull is only used during those steps then new nodes added to the orch won't be aware of this (I might be wrong).

@dsavineau is right about this. @guits if you want a deployment that continually skips using tls throughout the cluster life cycle (e.g. when automatically pulling the image on a newly added host you want ceph daemons on that doesn't have the image locally) you would likely have to add a module option, set the module option during bootstrap if --skip-tls-pull was passed, then modify calls to cephadm pull from the cephadm mgr module to account for this module option and set the --skip-tls-pull flag. If you just want this to work when directly running cephadm pull/adopt/bootstrap on the command line what you have here should work. Depends on your intentions.

Comment on lines +7587 to +7590
parser.add_argument(
'--skip-tls-pull',
action='store_true',
help='do not use tls when pulling image')
Copy link
Contributor

Choose a reason for hiding this comment

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

this is unfortunately a bit deceiving to users, as this only works for tiny toy environments. If you want to advertise it in the cephadm CLI, you'll need to make this actually work reliably. I.e. this needs to end up in very similar places as the container-init flag.

  1. It needs to be another MGR module option
  2. and passed though all run_cephadm calls
  3. and written to all unit.run files

As users might remove all images and rely on the systemd unit files to re-pull the images on the hosts

@guits
Copy link
Contributor Author

guits commented Aug 5, 2021

closing this in favor of #42671

@guits guits closed this Aug 5, 2021
@guits guits deleted the cephadm_tls_verify branch August 5, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants