DuckCorp » DuckCorp Infrastructure

Pierre-Louis Bonicoli wrote:

• commit 32a7617e10d7fe955eae735dee6203909949b8e8: add wrapper around zabbix_get to set TLS options automagically

• ansible/roles/dc-monitoring/templates/adm_zbx_get

• /etc/zabbix/certs/ should not be hardcoded.

True, fixed.

• use long name for options (s/-s/--host/ and s/-k/--key/)

Better indeed. Done.

• commit b9e770bcbccdb3398ae7335fecd9a0776546735e: configure Zabbix agents to use TLS

• TLSServerCertSubject and TLSServerCertIssuer could be set

You're write but unfortunately these features are currently unusable because « Wildcards and regexp's are not supported in matching. ».

• commit 9de23a53378245c659fca291d3f300c5a25d50f0: install Zabbix TLS certificates

• ansible/group_vars/all/vars.yml and ansible/group_vars/sup-servers/vars.yml

• Use inventory_hostname instead of ansible_nodename: ansible_nodename is the hostname, playbook should not assume that the hostname is equal to the inventory_hostname.

True, fixed. I made the mistake outside the PR, so I'll try to make a wider review.

• ansible/group_vars/all/vars.yml

• why deploy.pki_path instead of pki_path ?

I don't remember :-)
I chose pki.path so that we can have other pki parameters later.

• just in case, pki_path should end with /

It's safe but I think we should strengthen our code to not count on this (as doubling the slash has no impact). Done anyway.

• ansible/roles/dc-monitoring/tasks/agent.yml and ansible/roles/dc-monitoring/tasks/poller.yml

• perm dictionnary: owner key missing
• file task Create TLS certificates directory: ower, group and mode missing
• notify: Restart Zabbix [Agent|Poller] should be used when certificates are updated. It seems that all tasks could be moved inside the block and the notify could be put at the end of this block.
• certificates part of ansible/roles/dc-monitoring/tasks/agent.yml and ansible/roles/dc-monitoring/tasks/poller.yml could be factorized.

• ansible/roles/dc-monitoring/vars/main.yml

• tls_install_dir: the name is too generic and could be used elsewhere
• Use a role default instead of a role variable.

• commit 1aff4e2147d475cac8fa329fdffc36a59cfedfb1: created CA and certificates for Zabbix TLS

• equal signs aren't aligned

Old problem, to be fixed outside this PR.

• pki/conf/server/duckcorp-monitoring_zabbix_agent_elwing.cnf, extendedKeyUsage

• pki/conf/templates/server.cnf contains keyUsage instead of extendedKeyUsage, is the template obsolete ?
• *KeyUsage should be critical

for these too I think we need to revise the template again.

• both serverAuth and clientAuth ?

Yep, this is the proxy, so it act as server for local clients and client for central server.
As for clients, they can be polled (s2s) or connect to the server (active mode, c2s).
Am I wrong?

• why msSGC and nsSGC are included in extendedKeyUsage ?

I guess I worked on this before you revamped the template.
I see almost all certs are not following the template. Is this a problem to keep them? (I don't remember the meaning, not just obsolete?)
We could update all certs config and either regen/deploy depending on your advice.
I would handle this in another commit (it is not a new problem introduced here and fixing the other certs would be outside the PR topic).

• commit b0759e8ce89ddc42da9ee0dcd271bf16f3e646c1: use a different MySQL client config for supervision

• /etc/mysql/supervision.cnf isn't handled by ansible ?

No, it was a quick fix but you're right.
I need to see how to setup the required account/perms.
It was done outside this PR, so later in another commit.

• The zabbix test/probe could be renamed s/mysql.ping/mysql.alive/.

Why not.
Same, outside this PR.

• zabbix_agentd.conf.j2: PidFile=/var/run/zabbix/zabbix_agentd.pid: /run should be used instead of /var/run

Will do in another commit.

merged