'letsencrypt.conf' was included unconditionally #26
Conversation
|
Could you give a bit more details on the issue this is supposed to fix ? (like, how did you trigger the error, etc). The commit and PR is quite too terse. |
|
If you provide the certificate manually and don't use letsencrypt, you can still use force_tls. In this case this inclusion is not possible as the file does not exist. |
|
I see so this would also fail if I use use_freeipa and force_tls. And if we use use_letsencrypt without setting website_domain. I am however not sure how we should fix the 2nd corner case, and if that matter in practice. I kinda dislike having all the conditional bubbling up to the vhost.conf file, so what about creating the file inconditionnaly, outside of the letsencrypt.yml file ? |
|
Using force_tls is a practice we should be encouraging, so the first case should really work. As for the second, we should decide how to handle parameters which become non-optional when activating a feature. I saw you used the 'fail' action in other roles IIRC, so this should be addressed too (conditional in the included rules, so no complexity in the template). Having fake configuration files makes things difficult to debug: is the content missing? If I see such a file I would really think letsencypt is being used. Also later we would like to create tests, and this would really make things difficult too. |
|
This case of minimal inclusion could be handled using a special conf.d directory, and the feature.yml would drop a template or symlink into it. This would make the condition generic in the vhost template. |
|
We did switch to having the file being always there rather than dropping/removing it around the 18th of April (see 4b36e21 , or 02a1eb2 ). While I see your point on being faster to debug and clearer, and this is valid, I fear this might later also result into weird errors if 2 roles start to write the same file silently. With the current "file is always written" policy, we would detect the failure much faster (since it would fail right away), and not fail under some specific settings (thus being harder to see). However, since file are always written anyway, maybe we do not need to have 20 of them, and we can consolidate into 1 single file with a ton of template, if that's easier to reason with. |
|
I tend to be ok with the single file solution, but the question would then be: do we allow external roles to add their own rules; if so we could have the same need and a single file would not work. It seems the path in this role is to stack features we need, so in this case it can handle all this internally. Nevertheless you seem concerned about complexity inflation, so we need to find ways to handle it, either internally or by delegating (and leaving the core smaller). Once it is decided I will implement it by updating this PR. |
|
So, I do not propose to ditch the approach of using a include directory. I propose to reduce the number of file we drop here in the httpd role, since we drop then anyway. I wouldn't like getting a huge vhost.conf template instead (I have done in the past, and I am not sure that's), but if the fact there is by default 5 or 6 files in the directory (most being empty) is a issue, then we should maybe strive to have less. I did that because it was easier to understand for me (and allowed cleaner separation on the playbook side), but the solution would be to just push all in 1 template we place there. With the added bonus of removing placeholder.conf, who is just here to be empty for apache 2.4. So instead of having on the playbook side 1 task file to generate 1 single file to be dropped in the directory, just have that in 1 template, consolidated with others So that seems to strike the balance between "we have lots of useless file" and "we have lots of logic hardcoded in a big template". It keep the related code in one file (ie, mod_security stuff would be in mod_security.yml, not in a separate file and in the big template), without littering too much for the case we need separation. |
No description provided.