cloud-init: SSH key injection improvements #15015
Conversation
github-actions
bot
added
Documentation
hamistao
force-pushed
the
ssh_key_injection_improvements
branch
from
db761c8 to
b79a4ac
Compare
hamistao
force-pushed
the
ssh_key_injection_improvements
branch
from
779dcc9 to
10f5b89
Compare
hamistao
force-pushed
the
ssh_key_injection_improvements
branch
from
10f5b89 to
02b646a
Compare
hamistao
force-pushed
the
ssh_key_injection_improvements
branch
3 times, most recently
from
b02c5c3 to
4d6efe7
Compare
|
@hamistao please ping me when ready for another review |
Signed-off-by: hamistao <[email protected]>
Signed-off-by: hamistao <[email protected]>
Signed-off-by: hamistao <[email protected]>
This refactoring accomplishes a few things: 1- Takes advantage of the `cloud-init` package to simplify the names of entities within this package; 2-Refactors imports; 3-Remodels its API to only expose the `GetResultingCloudConfig` and `GetEffectiveConfigKey` methods, containing all the necessary logic for the other packages to use; 4-Uses a cloudConfig type for operations on cloud-init configuration, that helps us to better encapsulate the process, with each function having a clear responsibility. Signed-off-by: hamistao <[email protected]>
This is done by creating `mergeSSHKeysIntoCloudConfig` and using the proper API from the `cloudinit` package. Signed-off-by: hamistao <[email protected]>
Signed-off-by: hamistao <[email protected]>
The whole point of refactoring the `cloudinit` package. Now we can't handle user-data and vendor-data as if they are completely independent, so having the function `GetResultingCloudConfig` that handles both *-data allows us to easily add this additional check. And in a way that is transparent to the other packages. Signed-off-by: hamistao <[email protected]>
Now that the important logic is centralized in `GetResultingCloudConfig` instead of scattered amongst packages, we can easily test the entire process of handling cloud-init configuration with unit tests. In other words this considerably increases the coverage of these unit tests. Signed-off-by: hamistao <[email protected]>
This helps to avoid parsing an invalid conifiguration, as the `#cloud-config` comment is mandatory, and also helps us check if the configuration is in a supported format. Signed-off-by: hamistao <[email protected]>
hamistao
force-pushed
the
ssh_key_injection_improvements
branch
from
4d6efe7 to
22d8c5e
Compare
|
@tomponline This is now ready |
| func MergeSSHKeyCloudConfig(instanceConfig map[string]string, cloudConfig string) (string, error) { | ||
| // Check if cloudConfig is in a supported format. | ||
| // A YAML cloud config without #cloud-config is invalid. | ||
| // The "#cloud-config" tag can be either on the first or second lines. |
There was a problem hiding this comment.
why not the third, fourth or n lines? This seems an odd statement.
There was a problem hiding this comment.
this already seems to be in an earlier commit, so looks like a bad rebase somewhere
| } | ||
| // For values containing cloud-init seed data, try to merge into them additional SSH keys present on the instance config. | ||
| // If parsing the config is not possible, abstain from merging the additional keys. | ||
| if strings.HasSuffix(key, ".user-data") { |
There was a problem hiding this comment.
Please can we do exact matching on the relevant keys rather than suffix matching otherwise customer keys like user.foo.user-data would be in scope incorrectly.
| // Get raw data from instance config. | ||
| vendorDataKey := cloudinit.GetEffectiveConfigKey(instanceConfig, "vendor-data") | ||
| userDataKey := cloudinit.GetEffectiveConfigKey(instanceConfig, "user-data") | ||
| vendorData, userData := cloudinit.GetResultingCloudConfig(instanceConfig, vendorDataKey, userDataKey, d.inst.Name(), d.inst.Project().Name) |
There was a problem hiding this comment.
In my view it would be clearer to return a cloudinit.Config (or similarly named) struct that contained fields for userdata, vendordata etc rather than multiple return values as it allows you to pass around the overall cloud config.
| // Get raw data from instance config. | ||
| vendorDataKey := cloudinit.GetEffectiveConfigKey(instanceConfig, "vendor-data") | ||
| userDataKey := cloudinit.GetEffectiveConfigKey(instanceConfig, "user-data") | ||
| vendorData, userData := cloudinit.GetResultingCloudConfig(instanceConfig, vendorDataKey, userDataKey, d.inst.Name(), d.inst.Project().Name) |
There was a problem hiding this comment.
Could we have the logic for GetEffectiveConfigKey moved inside GetResultingCloudConfig and then avoid the caller needing to figure this stuff out by just having a requestedKey argument that the the cloudinit package figures out what needs to be returned?
There was a problem hiding this comment.
Rather than "Resulting" lets use "Effective" here as that aligns more with other concepts in LXD (effective project for example).
|
I'd left some comments about trying to reduce the size of the API to keep as much of the cloud-init key-specific logic away from the caller's concern and inside the package itself. Also lets try and avoid using prefix matching as that is prone to unexpected failure. |
user-data's fields overwrite vendor-data's fields, so merging SSH keys can result in adding a "users" fields that did not exist before, having the side effect of overwriting vendor-data's "users" field.
In order to fix we had to extend the logic for merging SSH keys into cloud-config, and that prompted us to refactor the merging logic, moving it to a newly created
cloudinitpackage.