saltstack-formulas / apache-formula

Set up and configure the Apache HTTP server
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 285 forks source link

Feature (rhel7/httpd 2.4) : hardening apache and code refactoring #251

Closed k-hamza closed 5 years ago

k-hamza commented 5 years ago

Context The main goal of the changes is to be able to enforce values of directives for hardening purpose, based on CIS Benchmarks apache document The structure of pillars is modified into lists / dictionnaries so the content can be "easily" parsed and modified

The result is a complete rewrite of config and vhosts states

All changes are backward compatible, they are all new files added (with -ng suffix)

Summary of Changes

Testing

k-hamza commented 5 years ago

yes, you are right. Thanks !

alxwr commented 5 years ago

@k-hamza I greatly appreciate your effort! Your code even solves some problems I came across. Thank you! (The following is not a criticism of your work, but of the "ng" pattern in general.)

@aboe76 @whiteinge @javierbertoli @landergate Is there a way not to use the "ng" pattern? In my opinion this leads into an maintenance mess. To be honest I regard the "ng" pattern as some sort of redundant addition to Git. It's really a formula within a formula. Hard to understand, hard to maintain. In this case I can't easily see the changes, because everything is in a new file.

We already discussed this in https://github.com/saltstack-formulas/postfix-formula/issues/83. (@ixs)

I suggest we start versioning this formula. (https://semver.org) Let's add a tag (i.e. 0.1.0) to mark the state before this PR and after that (depending on possible breaking of Pillar data) set 0.2.0 or 1.0.0. People could reference the tags. Patches to old versions (if anyone wants to keep them) would go into branches like v0.1.0-master. From there they could be ported to other branches way easier than from one sub directory to another.

The only real disadvantage I can think of is having to write a migration guide from one version to the other.

If we really have to use the "ng" pattern, we should put everything into its very own namespace (read: directory) as the php-formula does.

In the hopes of providing constructive criticism, alxwr


@aboe76 @k-hamza I'll have to test the "ng" version of the formula on one of my machines. (Sadly I've got to finish other stuff first.)

aboe76 commented 5 years ago

@alxwr I know about the issue, with "ng" formula's and I would like to split those too.

I don't think that semver and git branches are the solution, those are to "developer"-minded solutions and are hard for simple sysadmins to grasp or even create PR against.

I would like to suggest to create separate formula's:

php-formula\
  php\
  LICENSE
  README.rst
php-ng-formula
  php\
  LICENSE
  README.rst
apache-formula
  apache\
  LICENSE
  README.rst
apache-ng-formula
  apache\
  LICENSE
  README.rst

Only downside to this is if somebody wants to use both formula's on one saltmaster...

javierbertoli commented 5 years ago

@alxwr, I completely dislike the ng solution mostly because, as you and @aboe76 mention, it follows a 'formula-in-formula' concept. But it has already crept into many formulas (from the top of my head, I rememeber it's present in apache, fail2ban, nginx, php...). It seems most of the times as the only solution to 'here's this big refactoring/update to the formula, it's a pain/impossible to make it backward compatible and there's no other proposed method in the formulas ecosystem to introduce these changes nicely' :disappointed:

At this point, separating the formulas can be a solution, but I think that, sooner or later, somebody will again submit a 'formula-in-formula' 'ng-ng' PR, like 'apache-ng/ng' :smile:

In the past, when writing Puppet & Chef, the Semver + Release Tags/Branches worked nice for me, but I again agree with @aboe76, it's not the easiest thing to learn how to use and interact for simple sysadmins (it took me a while to do it right :yum:)

If we decide to do SemVer, we'll need to add proper documentation (perhaps in the template mentioning that:

etc, and try to enforce/maintain things that way whenever possible.

When I moved to Salt, I tried to read about the proposed method to maintain formulas, and found that Salt has a Salt Package Manager, but sincerely, never used it nor know if it is widely used.

My 2c.

alxwr commented 5 years ago

@aboe76 @javierbertoli I hear your concerns regarding sysadmins. Those are my concerns too. :-) In my opinion it's not too much to ask/force somebody to learn how Git branches work. (But I may be wrong.) This is state-of-the-art technology after all. Furthermore, ng/ is confusing too¹.

I'm not in favor of creating a new repository for every refactoring, because it's not easier to maintain for anyone.

I also was thinking about doing version control in sub directories like so: v1/apache/..., v2/apache/... Current development would happen in the latest version directory. It's still a maintenance mess, but it seems better than ng/ng/ng/.../ng/.

The Formula File docs suggest the version format YYYYMM.release. This contains no assurance of compatibility anyways. "We strongly recommend forking a formula repository into your own GitHub account to avoid unexpected changes to your infrastructure." Since formulas never made a promise to be backwards compatible, nobody should use a potentially unstable master anyways.

Maybe we should give up on „be backwards compatible at any cost“.¹ In my view there are at least two effective options to kill an Open Source project.

a) Make it too hard to use. b) Make it too hard to maintain.

I get that keeping backwards compatibility tries to avoid a), but it drives the formulas to b). (And furthermore: ng/ng/ng/ goes against a) again.)

Therefore, another proposal:


Don't take me wrong. I'm not here to enforce my point of view. I'm just committed to keep usability high and maintenance cost low, because I want the formulas to succeed. In the end, I'll go with the developers' consensus. :-)


¹ Why do I have a strong opinion on this? I held a one-day Salt workshop for sysadmins of several departments of a German corporation a few months ago. They really liked Salt and especially the formulas. They had two concerns regarding the formulas:

ixs commented 5 years ago

My 2cents as I got pinged. ;-)

It's good to see everybody agree that ng sucks and should not be used. I really dislike the idea of forking off an ng change into a separate repository. This is mostly for the following two reasons:

But even in-repo -ng is terrible. I stumbled over the ng version of fail2ban the other day and while that version resolves a few bugs of the non-ng version, it was also not functional. Needed one or two patches first. Does this mean noone was using -ng at all other than me?

At the end of the day, I think semver and controlled breakage is the sensible option. And for the poor busy and overwhelmed sysadmin we can add some easy to understand documentation that explains how to update your formula and again suggest to use your own fork in order to control the rate of change.

aboe76 commented 5 years ago

Ok, let's ask saltstack Organisation to make it clear what they have in mind with the formula's, because there is something like spm https://docs.saltstack.com/en/latest/topics/spm/

And I too want the saltstack-formulas be as successful as it can be, because, one, I don't want a mess of multiple roles like ansible-galaxy and I don't want it to become another forge where you need to learn some pdk to submit...

@saltstack-formulas/core and @saltstack-formulas/moderators can we have some input on this so we can align all our efforts?

javierbertoli commented 5 years ago

A valid question that relates to this discussion

aboe76 commented 5 years ago

@javierbertoli yes it is, I was hoping to get the saltstack core team involved to maybe write something in saltstack docs on formulas and git tags...and how to use them, that way we can start teaching people on how to use them by letting them read the official saltstack docs.

k-hamza commented 5 years ago

I must say that I agree with you on "ng" pattern ... it is an "anti-pattern" of git usage

In the saltstack documentation I found this about formula versionning.

It seems that it is the "official answer" to the question, and it is mostly the same as the proposals above. May be it would be usefull to report it on the template formula

What do you think about it ?

noelmcloughlin commented 5 years ago

I'm think if there is any way to keep one-on-one relation between formula and implementation then we should aim for that. When rewriting the mongdb-formula recently I had this exact dilemma - how to improve without losing backwards compatibility - I eventually found a way to support old and new in same implementation by having a twin-track map.jinja

See section two for backwards-compat data structures:

In this pattern the first section is the current-preferred-implementation and the second section is a reconstruction of the depreciated dicts to keep backwards compatibility. In this way there is no need for two separate formulas for one piece of software. I will admit writing the SECOND section in map.jinja is challenging but advantage is that its transparent to end users. I'd advocate for some kind of similar approach - let map.jinja do the heavy lifting. Cheers.

aboe76 commented 5 years ago

@k-hamza nice finds, in the documentation, let's make it so, I think the majority is for semver and tagging, can you create a PR to document and link to the docs in the template formula.

@alxwr, @noelmcloughlin and @javierbertoli let's educate people and refer to the template-formula.

aboe76 commented 5 years ago

@k-hamza maybe we can try this after we set some tags: https://github.com/vaab/gitchangelog

k-hamza commented 5 years ago

can you create a PR to document and link to the docs in the template formula.

done : https://github.com/saltstack-formulas/template-formula/pull/16

k-hamza commented 5 years ago

Hello team, hope you are doing well @alxwr, did you have time to test or to look into the code ? Thanks !

alxwr commented 5 years ago

@k-hamza I'm trying hard to get some GitHub issues and PRs done. :-) I'll give your code a try as soon as I can.

alxwr commented 5 years ago

@k-hamza I (rather hastily) tested this on Ubuntu 18.04 by including all ng-states and running them against pillar.example. Other than minor distribution-related bugs this worked fine. I would merge it if it wasn't for the ng-stuff. :-)

@k-hamza @aboe76 @noelmcloughlin @javierbertoli we should now discuss a transition to apache-formula 2.0. I'm against merging the PR in the ng-style. I propose we rather make this a good example of versioning a salt formula by using the means already established in https://github.com/saltstack-formulas/template-formula.

@myii Is https://github.com/saltstack-formulas/template-formula/issues/21 sufficient to serve as a checklist for this transition?

myii commented 5 years ago

@alxwr https://github.com/saltstack-formulas/template-formula/issues/21 is still a WIP and does need more discussion (no-one has commented there yet for adjustments!). But if it helps with this process, then please feel free to use it by all means. As a member, you are able to edit the comment, in order to copy the Markdown directly. That will ease pasting and modifying it for any other formula.

myii commented 5 years ago

Sorry, I was checking for the other labels and I ended up removing the label by mistake! Reinstated, so nothing to see here!

alxwr commented 5 years ago

minor note: I'm looking forward to help with this PR, but I've already burnt through my GitHub time budget for this week. :smile:

myii commented 5 years ago

@alxwr Your efforts are appreciated. See you next week!

noelmcloughlin commented 5 years ago

Nice PR @k-hamza I'm up to my neck in other formulae so cannot review.

@alxwr Regarding the ng pattern I have good idea how to smoothly transition away.

  1. Identify all ng-formulae.
  2. Assume ng is the primary content.
  3. Obsolete non-ng content but empower the non-ng API
    • i.e. update non-ng SLS files so they invoke apache.ng.xxxx states
    • i.e. delete apache/map.jinja and yaml files in non-ng formulae.
    • i.e. apache.init points to apache.ng.init
    • i.e. update README and stop documenting ng API
    • i.e. update pillar.example and stop documenting ng pillar data

Note: we need to update the map.jinja so it can handle both ng and non-ng pillar data - this is possible I have experimented with this idea in mongoDB.

  1. In say 6 months time get rid of ng structure; the ng API has not be documented for 6 months so assume this is long enough for people to have re-adopted the non-ng API.

WDYT?

I hope focus can be on template-formulae before apache 2.0. There are some new concepts & ideas being discussed which I need to get up to speed with anyway. thanks.

k-hamza commented 5 years ago

I agree not to merge it in master branch. I suggest that you create a "develop-v2.0.0" branch (name for example) where to merge this PR. In the meantime, i will start cleaning the "ng-" stuff, so the merge will contain "clean" content that will need some little work in order to be a good example of versioning a salt formula.

myii commented 5 years ago

@k-hamza Thanks for all of your hard work.

There have been quite a few developments over the last week or two and we're trying to revamp the way SaltStack-Formulas does things. One of the discussion points has been the ng concept. At the moment, it is still up for discussion how exactly this should be dealt with. For example:

  1. Using release tags, just bump the major version and merge into master.
  2. Use separate branches for different versions (as you have suggested).
  3. Use a separate repo for ng formulas.

A further issue is that we're working on how to integrate these formulas with SPM. That will also have an effect on the final decision.

So the takeaway is to keep track of developments before investing a portion of time into a specific option. Best ways to do that:

alxwr commented 5 years ago

@k-hamza @aboe76 @myii I'm all for the "develop-v2.0.0" branch, as long as it will get merged into master at some point. :-) This gives us time to stabilize 2.0 while still having a stable master branch. Do you all agree?

myii commented 5 years ago

@alxwr @k-hamza I prefer method 2 as well but I don't know what the repercussions are going to be.

One further point: we're close to providing a solution for automatically updating the changelog, FORMULA file, tags and releases for each PR merge. During this process, we've compared across all existing SaltStack Formulas to work out comparitive "starting" version numbers:

screenshot69

So if that is accepted, this apache-formula is currently v0.37.0. The new branch will then be develop-v1.0.0 accordingly. Please feel free to comment on this issue as well.

k-hamza commented 5 years ago

@alxwr @myii ok then, let's create develop-v1.0.0 branch so we can begin stabilize the code

alxwr commented 5 years ago

@k-hamza @myii I just created the develop-v1.0.0 branch.

aboe76 commented 5 years ago

@alxwr nice thanks for that,

@k-hamza shall I change the base branch of the PR to target develop-v1.0.0

this is possible with: https://github.blog/2016-08-15-change-the-base-branch-of-a-pull-request/

k-hamza commented 5 years ago

@aboe76 Thanks for this trick ! just changed the base branch of the PR now @alxwr let me do some cleaning : remove all the "-ng" stuff

aboe76 commented 5 years ago

@k-hamza merged it,

k-hamza commented 5 years ago

Thanks for the merge