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

Manage SSH key via 'debops.authorized_keys' role #15

Merged
merged 7 commits into from
Jul 23, 2016

Conversation

ganto
Copy link
Member

@ganto ganto commented Jul 19, 2016

No description provided.

@drybjed
Copy link

drybjed commented Jul 19, 2016

Ohhhh, I knew that somebody will try to use this role that way, so I implemented support for role dependencies just in case. :-)

@ganto
Copy link
Member Author

ganto commented Jul 19, 2016

Wow... how did you get here so fast? 😄

So you agree with how I use it?

# Authorized key configuration for the ``debops.authorized_keys`` role.
checkmk_agent__authorized_keys__dependent_list:
- name: '{{ checkmk_agent__ssh_user }}'
ssh_keys: [ '{{ checkmk_agent__user_key }} if {{ checkmk_agent__user_key|d() }} else {{ hostvars[groups["debops_service_checkmk_server"][0]].ansible_local.checkmk_server.ssh_publickey|d("") }}' ]
Copy link
Member Author

Choose a reason for hiding this comment

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

@drybjed: what do you think of this? Is there a way how you implemented something like this more elegant?

somehow the hostvars[groups["debops_service_checkmk_server"]] expression will trigger a deprecation warning in case the group is not defined in the inventory.

Copy link

Choose a reason for hiding this comment

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

It's sshkeys. I don't think that you need to lookup keys in local facts. If you specify empty sshkeys, role should just skip management of the keys. So, defining only checkmk_agent__user_key variable in inventory should be enough.

And here, just set it like this:

- name: '{{ checkmk_agent__ssh_user }}'
  sshkeys: '{{ checkmk_agent__ssh_key }}'

That should do what you expect, although I would test it first to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is, that they key is generated during the setup of the monitoring site. If the user has to specify this manually, it would abort the playbook run, then the user has to lookup they key manually. Or he need to generate a keypair in advance and configure them before running the server setup.

I wish to find a solution where the setup just works. See also debops-contrib/ansible-checkmk_server#15

Copy link

@drybjed drybjed Jul 19, 2016

Choose a reason for hiding this comment

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

For this you will probably want to split the role into normal and "env" role (from "environment"). It would work like this:

- role: `checkmk_server/env'
- role: `debops.authorized_keys'
- role: `checkmk_server`

That way you can prepare data for other roles that need it, hand it off to the other roles and return back to managing your application. For an example, check the debops.php role and its example playbook.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this you will probably want to split the role into normal and "env" role (from "environment").

@drybjed I don't see what problem this is going to solve.

Is this bad practice to lookup a service provider in the host facts? This code is functional and behaves as intended, just that it emits a deprecation warning, in case no debops_service_checkmk_service group has been defined.

I will move it to the variable definition of checkmk_agent__user_key then the code will become a bit easier to read.

Copy link

Choose a reason for hiding this comment

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

What deprecated warning, about Undefined variable? This will be error in the future, you need to fix that.

Moving the lookup code to the variable is good, then users can modify it if they wish without the need to modify the role itself.

As for the 'role/env' solution - this comes from a pretty specific design principles of DebOps roles - they are supposed to be read-only, and you should pass configuration of other roles through default variables. In specific situations, like generating data on a host by your role, or using lookup templates, you might find that Ansible does not have the data it needs at this specific point in the playbook, or it does the lookup in the wrong role.

In the given debops.php example, the role checks what PHP package versions are available and then acts on that information, however another role, debops.logrotate is also using this information to generate the correct logrotate configuration for the php-fpm service. The information about PHP version is stored in a variable, however only debops.php sets this variable. Since debops.logrotate is supposed to run before debops.php, it would not know beforehand what PHP version it should use to generate the config file. Basically a casuality violation. :-)

To resolve this, I decided to split the debops.php role essentially into two - first one, debops.php/env contains minimum amount of tasks to "set the environment" for other roles - it checks what PHP versions are available, and even configures external APT repositories. When its tasks are done, other roles like debops.logrotate can act on variables passed through debops.php defaults since the required facts are set in the memory. After that, debops.php continues as usual to finish the configuration.

As I said, this is a very situational solution and you won't need this with the most roles. However, I suggested it here since you mentioned that the SSH keys on the specific hosts are generated by the role. To avioid the need to run the playbook twice to complete the configuration, you can do it like this:

  • checkmk_server/env generates the SSH keys and slurps them into memory
  • debops.authorized_keys configures the SSH keys using data from checkmk_server defaults + SSH keys
  • checkmk_server finishes the configuration

This should keep things clean and consistent.

@drybjed
Copy link

drybjed commented Jul 19, 2016

@ganto I see everything. :-) And sure, if a role has support for use as a dependency, I'm fine with it. Otherwise I wouldn't add variables like *_dependent_*.

@ganto
Copy link
Member Author

ganto commented Jul 19, 2016

btw. yes, it fits the purpose perfectly. Thanks a lot for thinking ahead. 👍

@ganto
Copy link
Member Author

ganto commented Jul 21, 2016

What deprecated warning, about Undefined variable? This will be error in the future, you need to fix that.

Sorry, I meant deprecation warnings. Yes, I'm aware of that and that's why I raised the issue.

Moving the lookup code to the variable is good, then users can modify it if they wish without the need to modify the role itself.

Done that. Now I think, I just need some jinja foo to make the deprecation warnings go away then I'm good.

As for the 'role/env' solution - this comes from a pretty specific design principles of DebOps roles - they are supposed to be read-only, and you should pass configuration of other roles through default variables.

I don't think I fully understand this or how I violate this. You maybe talk about an issue that I haven't realized yet.

So far, I thought about the following procedure:

  • There is a global playbook, where there are the server and the agent roles. The playbook will define the order in a way, that the server role is run first, generates the SSH key pair for the monitoring sites it creates, set the public key as Ansible local fact so that all subsequent runs of the agent playbook will already find the SSH public key in the host facts.
  • If initially the roles are run individually, the user has to make sure that the server role is run first, otherwise agent registration (not yet commited here) will fail anyway.
  • If the server is not managed by DebOps, the user has to add the SSH key manually (current situation)
  • The debops.authorization_keys role is run as "immediate" playbook dependency of the agent role, so it has basically the same view, hasn't it? If the debops.authorization_keys is run during the normal DebOps playbook run, it doesn't know about the authorized_keys__dependent_list variable injection of the agent role yet, does it? So there is no need to generate the key pair before that.

Now I'm trying to understand how such a 'server/env' would change the situation:

  • I don't think I can force it to run before the global authorized_keys role run and I don't think it needs to (see above)
  • The 'server/env' needs to run prior to the agent role, so it could be added as a playbook dependency to the agent role. This is not really feasible with the entire server role otherwise the complete server role would run twice. That's an advantage of the 'role/env' solution.
  • It doesn't really affect the authorized_keys run as we just have to make sure that either the server role or the 'server/env' role are run prior to the agent role.
  • The 'server/env' role still has to lookup the monitoring host in the groups fact the same way as the proposed implementation looks for the host fact now.
  • The key pair generation would need take place in a temporary location as the monitoring site and user are not yet created. So it would require some logic to move the key around after the site was created.
  • There would be a new conditional dependency on the 'server/env' role which only make sense if the server is managed by DebOps and probably confuses users which are only using the agent role. The proposed solution is mostly transparent in this regard.
  • Overall the procedure seems overly complicated to me if the reason is only to make sure, that there is a key pair in every case where the agent role is run.

Probably I missed some points of your solution or failures of mine. Can you hint me?

@drybjed
Copy link

drybjed commented Jul 21, 2016

@ganto With your explanation that the server role is supposed to run before the agent role and that the server role generates the SSH keys which then are used by agent role, my proposal for server/env is not needed anymore. I think that I got confused a bit, but with your explanation of how eveyrthing is ordered it makes more sense. Sorry for confusing you back. :-)

@ganto
Copy link
Member Author

ganto commented Jul 21, 2016

@drybjed Ok, I see. Now it makes sense. Sorry, that I didn't make this clear from the beginning.

ganto added 3 commits July 22, 2016 07:58
This should shut up a deprecation warning that the content of
an inexistent dictionary element is undefined.
@ganto
Copy link
Member Author

ganto commented Jul 23, 2016

After some more testing I'm confident that I found most rough spots. If there are no more comments, I'm going to merge that soon.

@ganto ganto merged commit 665e6b1 into debops-contrib:master Jul 23, 2016
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.

2 participants