voxpupuli / puppet-rundeck

Module for managing the installatation and configuration of the rundeck orchestration tool
https://forge.puppet.com/puppet/rundeck
MIT License
39 stars 129 forks source link

Turn serialnumber into UUID #401

Closed jescholl closed 5 years ago

jescholl commented 5 years ago

Pull Request (PR) description

This PR generates the server UUID from the serial number rather than just using the serial number directly, since serial numbers are rarely valid RFC 4122 UUIDs.

With cluster mode enabled, Rundeck requires that rundeck.server.uuid be a valid RFC 4122 UUID (Rundeck source).

This Pull Request (PR) fixes the following issues

n/a

bastelfreak commented 5 years ago

Hi @jescholl, would that change the rundeck.server.uuid for current installations? Then it would be a breaking change.

jescholl commented 5 years ago

Thanks for pointing that out @bastelfreak, I hadn't thought of the potential impact to non-clustered installations, though after digging through the Rundeck code base it looks like rundeck.server.uuid is explicitly set to null if cluster mode is not enabled. Also, most every place it is referenced, is gated with some sort of check to verify that cluster mode is enabled.

I will check a single installation today to verify that is in fact only used in cluster mode.

jescholl commented 5 years ago

I can confirm that the rundeck.server.uuid is only used in cluster mode, and changing it on a non-clustered instance has no impact.

My test included the following: 1) search the code for references to server uuid 2) search every table in the database for references to server uuid 3) search the logs for references to server uuid 4) search the API for references to server uuid 5) change rundeck.server.uuid

Searching the code

rundeck.server.uuid is only really referenced here, where it is read from disk and assigned to serverNodeUUID, but the assignment only happens if cluster mode is enabled, leaving serverNodeUUID set to it's initial value of null when cluster mode is not enabled.

Also, the vast majority of references to serverNodeUUID either check if it is null, or check if cluster mode is enabled before using it.

Searching the DB

The tables that contain the server uuid are:

They all refer to the server uuid as server_nodeuuid, and when operating as a single node the fields in all tables were null, when I switched on cluster mode it began writing the UUID, and when I turned off cluster mode again it stopped writing the UUID.

Searching the logs

None of the log files contain the server UUID, the [id].state.json log files refer to the node they ran on by fqdn with the the serverNode field, but that would not change.

Searching the API

The API endpoints that reference the server uuid are:

As you can see from their documentation all of them either don't return the server UUID outside of cluster mode, or return it as null regardless of configuration. I tested each of these to verify and can confirm that is how they behave.

Example:

$ curl -s -H 'accept: application/json' -H "X-Rundeck-Auth-Token: $RUNDECK_API_KEY" https://rundeck.example.com/api/24/system/info | jq .system.rundeck
{
  "version": "2.11.5",
  "build": "2.11.5-1",
  "node": "rundeck01.example.com",
  "base": "/var/lib/rundeck",
  "apiversion": 24,
  "serverUUID": null
}

Changing the uuid

After changing the UUID, the DB tables that contained server_nodeuuid were still assigning it to null as updates came in. All API endpoints also responded the same as before, returning null for the UUID or simply not showing it.

smasa90 commented 5 years ago

Actually if you use puppet-rundeck to configure rundeck with cluster mode enabled without specifying a correct UUID,

class { '::rundeck':
    framework_config      => {
      'rundeck.server.uuid' => fqdn_uuid($::facts['networking']['fqdn']),
    }
}

the service will never start successfully

@jescholl What do you think about not relying on serialnumber ?

jescholl commented 5 years ago

@smasa90 I'm not entirely sure I understand what you're saying. This pull request fixes the issue where rundeck fails to start in cluster mode with the invalid UUID provided by puppet-rundeck.

In my tests your code works fine in both clustered and non-clustered mode, though it doesn't enable clustered mode.

Regarding not relying on serialnumber, I don't have any strong feelings either way, I was just trying to minimize the changes.

smasa90 commented 5 years ago

@jescholl fqdn_uuid() generated UUID will be the same for a given hostname. Passing in $facts['serialnumber'] will generate potentially different UUID for the same hostname if the serialNumber ( comming from hardware or virtual hardware ) changes. To be cleaner, I think we can remove this switch case on serialnumber and pass simply the fqdn of the node to fqdn_uuid()

jescholl commented 5 years ago

That's fine by me, like I said, I don't have any strong feelings either way. I didn't want to make assumptions about design decisions, so I went with the smallest possible change that would fix the behavior.

@bastelfreak, do you (or anyone else) have an opinion on using fqdn instead of serialnumber to generate the UUID?

bastelfreak commented 5 years ago

@jescholl I've no objections, but I'm also not a rundeck expert.

smasa90 commented 5 years ago

As such, we do not have the same behavior on virtualbox on which serialNumbers are always '0' and other Hypervisors/ Hardware on which serialnumbers != '0'. Not a fatal issue, but imho, in most cases, it can be convenient to obtain same UUID for a given hostname because in most case, we give same hostname to a failing node which we have to redeploy. In both cases, it'll work, not want to block PR to be merged but it can be suitable to have other eyes on it.

jescholl commented 5 years ago

@smasa90 after thinking about it over the weekend I agree with you, that is a better solution. I have updated the PR to always set the UUID from the FQDN.

smasa90 commented 5 years ago

@jescholl Glad to hear that. TY for this PR

bastelfreak commented 5 years ago

Thanks!