-
Notifications
You must be signed in to change notification settings - Fork 0
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
New module for generating remmina file #2
base: feature/refactor-the-world
Are you sure you want to change the base?
New module for generating remmina file #2
Conversation
Still lacks a lot of stuff I have elsewhere. Slowly migrating it over. Currently features: - ssh templates (missing proxy support) - host_vars plugin exposing hostname variables in branching A lot of missing, most importantly roles/playbook layouts; will be coming shortly.
Assorted random stuff I've been pondering doing for a while.
Hopefully we can remove this stuff as part of talking directly to jenkins. Hopefully.
almost there. need to generalise how different ssh users might have to use "become".
For some reason I don't inherit the variable properly. It looks like the group_vars file isn't included since other commands I put in there isn't read either
- Introduce support for jump hosts - Create a simple filter plugin to replace dicts - Update readme/todo
..only used while testing filter plugins
avoiding pythons non-deterministic dictionaries
For some reason, libuv failed with a missing m4 dependency.
Build fixes for libuv.
- add hosts that were missing - use consistent versioning for ubuntu1204 - add missing gaps in systemd setup
also, force sudo over su at requireio
..this is needed for debian7 as well. The shipped monit is old enough to not support process watching, so stick with pidfile.
work in progress of installing tap2junit which is needed to convert tap output to junit xml, suitable for reading in jenkins.
using only hardware (or the "smart" way) won't always give us the ansible vcpu stuff, which we use for `JOBS`.
did some mistakes by not committing often enough. These will likely be refactored once the branch will merge. Pretty readable changes though.
- avoid installing tap2junit for release hosts - add release hosts to worker host match - add new smartos15 worker - few notes to readme; todo is growing!
That's awesome! @joaocgreis can you also help review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@piccoloaiutante thanks for your effort in this, it's great to see this taking shape! And sorry for my delay reviewing, I wanted to give a careful review and actually try to run it. I hope you find this helpful.
Even if this is a moderate file, there is a lot to grasp. I might have missed your reason for some of the options you've taken. If so, please feel free to discuss! I'm trying to make this as simple as possible for someone new to just run.
It looks like you're testing on OSX. I'm on Linux so some things may differ, let me know if I've taken wrong assumptions. Would be great to have this working on both!
I like the file format you've defined for the secrets. @jbergstroem this bypasses the inventory file completely, and it makes sense to me. Ports should be randomized and secret as much as possible, and I see no reason to make the IPs public either. I understand that for Unix the inventory file is useful to run ansible, but Windows relies heavily on host_vars
. When the actual windows playbook gets ported here, it would be good to translate the secrets into host_vars
, or find a way to use them directly.
module = AnsibleModule( | ||
argument_spec=dict( | ||
path=dict(required=True, type='str'), | ||
serverfile=dict(required=True, type='str'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path
could be not required, default to $HOME/.remmina/
(you seem to be testing this on OSX, I'm not familiar with remmina there but the default could be per os?). path
could be called remminapath
, to make it obvious (it makes sense to me that the .remmina
files are written directly to there, to make them usable right away).
You're reading serverfile
from inside path
below, but it should be an absolute location, so it can point to the file in secrets, wherever the secrets repo is cloned. serverfile
could be the only required argument.
argument_spec=dict( | ||
path=dict(required=True, type='str'), | ||
serverfile=dict(required=True, type='str'), | ||
secret=dict(required=True, type='str'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secret
is not needed, it can be read from path/remmina.pref
and look for a line that starts with secret=
. If the file does not exist, just exit with error that remmina should be run once before this.
secret=dict(required=True, type='str'), | ||
passphrase=dict(required=False, type='str'), | ||
gpgdir=dict(required=False, type='str'), | ||
gpgbinary=dict(required=False, type='str') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to write my password anywhere but to dotgpg/gpg directly, it can very easily be left in bash history or log files. I tried to run this without specifying a passphrase, but got a TypeError: 'NoneType' object is not iterable
. Perhaps that was just ansible getting in the way of the prompt, if that was the case then serverfile
should just point to an already unencrypted file, and require the user to run dotgpg/gpg directly before this.
Can you perhaps use dotgpg instead, would it work? Requiring dotgpg to be in the path seems reasonable to me, you can just check if dotgpg -h
can be run and error out if not. Plus, it would require no arguments. Instructions at: nodejs#577 (comment) . Note: to install gem
on Ubuntu:
sudo apt-get install ruby-full ruby git-core &&
sudo gem install rubygems-update &&
sudo update_rubygems
@@ -0,0 +1,115 @@ | |||
#!/usr/bin/env python |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file should have the exec bit set: chmod +x remmina_config.py
|
||
import base64 | ||
from Crypto.Cipher import DES3 | ||
import gnupg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note that pip install pycrypto gnupg
is needed, perhaps in the README.md and surround this in a try block that prints a helpful error?
outfile.write("server={0}:{1}\n".format(ip.strip(), port.strip())) | ||
outfile.write("username={0}\n".format(username.strip())) | ||
outfile.write("password={0}\n".format(generate_password(password, secret))) | ||
outfile.write("colordepth=15") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also end the file with \n
(username, ip, password, port) = server_list[host] | ||
try: | ||
filename = "{0}.remmina".format(host.strip()) | ||
outfile = open(os.path.join(path,filename), "w") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't overwrite the file if it already exists because users can add other options using the remmina interface (shared folders, etc). Can you just print a warning if it exists?
server_list = yaml.load(decrypted_server_list) | ||
remmina_files = [] | ||
for host in server_list: | ||
(username, ip, password, port) = server_list[host] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not working as intended, server_list[host]
is good, but the variables don't get assigned. I get lines like server=ip:port
in the resulting files. (Python 2.7.12)
gpgdir='/Users/michele/Github/remmina/.gpg' \ | ||
passphrase='1234567890' \ | ||
secret='MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0MTIzNDU2Nzg=' \ | ||
gpgbinary='/usr/local/bin/gpg'" |
There was a problem hiding this 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 a simple playbook to run this, like https://github.com/jbergstroem/build/blob/feature/refactor-the-world/ansible/playbooks/write-ssh-config.yml ? The location of the script file could be passed using --extra-vars
.
Also, can you add a line in https://github.com/jbergstroem/build/blob/feature/refactor-the-world/ansible/README.md#getting-things-done ?
Thanks for the feedback Joao.
I also use I will review work within a day or two! Exciting. |
@jbergstroem yes, the @piccoloaiutante can you rename |
@joaocgreis ok, thanks for clearing that out. I'll have a look and see if I can provide any suggestions. |
@joaocgreis thanks for your in deep review. I'll go through it in the next days and I'll apply your feedbacks. |
8fa2a89
to
8bc3a5f
Compare
This is a a new ansible module for generating
.remmina
files starting from a yml file. The yml file should have a structure like this:This ansible module is expecting 5 params:
yml
file is storedyml
filenameThe execution of this module will create one
.remmina
file per server entry in server list file: each one of this file will be named as the server name (liketest-example-win10-x64-1.remmina
).