Feat/config ldap #44

sticky-note · 2020-04-05T13:38:26Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Add ldap backend support

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

myii · 2020-04-05T18:27:27Z

@sticky-note If I understand correctly, this is built upon #43, so let's resolve the issues there before coming back to this.

sticky-note · 2020-04-06T22:56:56Z

@myii as forecasted, tests are failing

myii · 2020-04-06T23:07:43Z

@sticky-note No problem, I've got the fixes for you. But I've come across a different problem: if I actually enable the ldap values in pillar.example, the formula states don't even complete:

                 ID: dhcpd.conf
           Function: file.managed
               Name: /etc/dhcp/dhcpd.conf
             Result: False
            Comment: check_cmd execution failed
              Internet Systems Consortium DHCP Server 4.4.1
              Copyright 2004-2018 Internet Systems Consortium.
              All rights reserved.
              For info, please visit https://www.isc.org/software/dhcp/
              /etc/dhcp/tmp.Gf9jcLGwGC line 18: semicolon expected.
              ldap-server "localhost"
                           ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 19: semicolon expected.
              ldap-port 389;
                         ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 20: semicolon expected.
              ldap-username "cn=dhcpadmin,dc=example,dc=com"
                             ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 21: semicolon expected.
              ldap-password "dhcppassword"
                             ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 22: semicolon expected.
              ldap-base-dn "ou=dhcp,dc=example,dc=com"
                            ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 23: semicolon expected.
              ldap-method dynamic;
                           ^
              /etc/dhcp/tmp.Gf9jcLGwGC line 24: semicolon expected.
              ldap-debug-file "/var/log/dhcp-ldap-startup.log"
                               ^
              WARNING: Host declarations are global.  They are not limited to the scope you declared them in.
              Configuration file errors encountered -- exiting
              
              If you think you have received this message due to a bug rather
              than a configuration issue please read the section on submitting
              bugs on either our web page at www.isc.org or in the README file
              before submitting a bug.  These pages explain the proper
              process and the information we find helpful for debugging.
              
              exiting.

If that's the case, then this PR needs adjusting before anything. Can you confirm that on your side? I can run Travis at the moment because I've got a number of other runs going on.

myii · 2020-04-06T23:44:31Z

@sticky-note This is the fix for the tests to get it working as-is:

myii@88a7eb1

Still need to check the failures when enabling the ldap values, though.

myii · 2020-04-06T23:54:43Z

@sticky-note So as mentioned, I've now been able to reproduce those errors in Travis:

https://travis-ci.org/github/myii/dhcpd-formula/builds/671879796

All of the instances are tripping up at the check_cmd:

dhcpd-formula/dhcpd/config.sls

Lines 14 to 22 in 69d3fdc

    
               - check_cmd: | 
        
                   sh -c ' 
        
                   export TMPDIR=$(dirname "{{ dhcpd.config }}") ; 
        
                   TMPFILE="$(mktemp)" ; 
        
                   cp "$0" "${TMPFILE}" ; 
        
                   dhcpd -t -cf "${TMPFILE}" ; 
        
                   ERROR="$?" ; 
        
                   rm -f "${TMPFILE}" ; 
        
                   exit $ERROR '

sticky-note · 2020-04-07T03:08:54Z

@myii I am not able to reproduce, with myii/feat/config-ldap on stretch vagrant box, I get :

          ID: dhcpd.conf
    Function: file.managed
        Name: /etc/dhcp/dhcpd.conf
      Result: False
     Comment: check_cmd execution failed
              Internet Systems Consortium DHCP Server 4.4.1
              Copyright 2004-2018 Internet Systems Consortium.
              All rights reserved.
              For info, please visit https://www.isc.org/software/dhcp/
              WARNING: Host declarations are global.  They are not limited to the scope you declared them in.
              Error: Cannot login into ldap server localhost:389: Can't contact LDAP server
              Configuration file errors encountered -- exiting
              
              If you think you have received this message due to a bug rather
              than a configuration issue please read the section on submitting
              bugs on either our web page at www.isc.org or in the README file
              before submitting a bug.  These pages explain the proper
              process and the information we find helpful for debugging.
              
              exiting.
     Started: 13:54:33.836694
    Duration: 1477.157 ms
     Changes:

sticky-note · 2020-04-07T04:11:29Z

@myii I wasn't able to reproduce because I hat dhcpd from isc-openldap-server-ldap which is able to parse ldap-* directives.
dhcp from isc-dhcp-server can't parse those properties.
To be able to test those properties on dhcpd from isc-openldap-server-ldap , we need to have an LDAP server running and containing the base configuration for dhcpd

myii · 2020-04-07T16:10:17Z

@sticky-note I see that error as well in Travis, we've got both examples there.

debian-10:
- https://travis-ci.org/github/myii/dhcpd-formula/jobs/671879798#L1521-L1560
- The semicolon expected errors
amazonlinux-2:
- https://travis-ci.org/github/myii/dhcpd-formula/jobs/671879800#L1392-L1413
- The Can't contact LDAP server error

The problem is that we still need to be able to confirm this is working. This can be done by preparing a second test suite. You can use the outline I've provided here:

myii@435f0f3

This results in the Travis job getting through that state and only tripping up on the service.running:

https://travis-ci.org/github/myii/dhcpd-formula/jobs/672144700#L2254-L2259

Looking at the journal, the reason is as you stated above, that the base configuration for dhcpd isn't available yet:

... systemd[1]: Starting LSB: DHCP server...
-- Subject: A start job for unit isc-dhcp-server.service has begun execution
-- Defined-By: systemd
-- Support: https://www.debian.org/support
--
-- A start job for unit isc-dhcp-server.service has begun execution.
--
-- The job identifier is 163.
... isc-dhcp-server[917]: Launching both IPv4 and IPv6 servers (please configure INTERFACES in /etc/default/isc-dhcp-server if you only want one or the other).
... dhcpd[929]: Wrote 0 leases to leases file.
... dhcpd[929]:
... dhcpd[929]: No subnet declaration for eth0 (172.17.0.2).
... dhcpd[929]: ** Ignoring requests on eth0.  If this is not what
... dhcpd[929]:    you want, please write a subnet declaration
... dhcpd[929]:    in your dhcpd.conf file for the network segment
... dhcpd[929]:    to which interface eth0 is attached. **
... dhcpd[929]:
... dhcpd[929]:
... dhcpd[929]: Not configured to listen on any interfaces!
... dhcpd[929]:
... dhcpd[929]: If you think you have received this message due to a bug rather
... dhcpd[929]: than a configuration issue please read the section on submitting
... dhcpd[929]: bugs on either our web page at www.isc.org or in the README file
... dhcpd[929]: before submitting a bug.  These pages explain the proper
... dhcpd[929]: process and the information we find helpful for debugging.
... dhcpd[929]:
... dhcpd[929]: exiting.
... dhcpd[929]: than a configuration issue please read the section on submitting
... dhcpd[929]: bugs on either our web page at www.isc.org or in the README file
... dhcpd[929]: before submitting a bug.  These pages explain the proper
... dhcpd[929]: process and the information we find helpful for debugging.
... dhcpd[929]:
... dhcpd[929]: exiting.
... isc-dhcp-server[917]: Starting ISC DHCPv4 server: dhcpdcheck syslog for diagnostics. ... failed!
... isc-dhcp-server[917]:  failed!
... systemd[1]: isc-dhcp-server.service: Control process exited, code=exited, status=1/FAILURE

So there are two ways forward here:

I've not used openldap-formula myself but since you're familiar with it, would the dhcpd configuration be possible to add to the specific test pillar test/salt/pillar/openldap.sls?
If not, we could introduce a Jinja conditional that only executes the service.running state if enabled (which it will be by default). Then, at least, we can run all of the other states to verify things are proceeding as they should.

As you try things out at your end, you test it out using:

$ bin/kitchen verify ldap-debian-10-master-py3

sticky-note · 2020-04-08T04:13:10Z

@myii You have to fill base configuration for dhcpd, it can be done via ldap.managed state which depend on python3-ldap python module. I am working on parsing dhcpd-formula pillar to fill that correctly. All docs are here https://github.com/isc-projects/dhcp/tree/master/contrib/ldap.
But I am working on a version generated by ldap-account-manager which seems to be compliant too

myii · 2020-04-08T04:22:38Z

So there are two ways forward here:

I've not used openldap-formula myself but since you're familiar with it, would the dhcpd configuration be possible to add to the specific test pillar test/salt/pillar/openldap.sls?

If not, we could introduce a Jinja conditional that only executes the service.running state if enabled (which it will be by default). Then, at least, we can run all of the other states to verify things are proceeding as they should.

@sticky-note So shall we just do number 2 for now?

myii · 2020-04-09T00:44:55Z

Thanks, @sticky-note. Testing against ldap is going to take more effort, so let's get this merged for now.

saltstack-formulas-travis · 2020-04-09T00:52:22Z

🎉 This PR is included in version 0.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

sticky-note · 2020-04-09T12:52:53Z

@myii I have juste realized that dhcpd.conf and dhcpd configuration stored in LDAP are mixed up when used together so we can test LDAP connection and dhcpd.conf without filling dhcp configuration in LDAP

myii · 2020-04-09T12:55:00Z

@sticky-note Sure, please send through another PR with the changes. Don't worry too much if the verification stage fails again, I've tightened up the tests in this formula a lot, as a way of checking how useful that approach is.

sticky-note requested a review from myii April 5, 2020 13:38

sticky-note force-pushed the feat/config-ldap branch 3 times, most recently from 0ae90c6 to cf7d330 Compare April 6, 2020 00:07

sticky-note mentioned this pull request Apr 6, 2020

Refactor templates to not use pillar.get #46

Merged

19 tasks

sticky-note force-pushed the feat/config-ldap branch 3 times, most recently from b9c45ab to 8c66ef5 Compare April 6, 2020 22:26

sticky-note force-pushed the feat/config-ldap branch 5 times, most recently from 8fff3d3 to 492d040 Compare April 7, 2020 04:02

feat(ldap): add ldap backend support

241f267

sticky-note force-pushed the feat/config-ldap branch from 492d040 to 241f267 Compare April 7, 2020 04:07

test: fix tests for PR 44

5128b1f

myii approved these changes Apr 9, 2020

View reviewed changes

myii merged commit 1d44ef8 into saltstack-formulas:master Apr 9, 2020

saltstack-formulas-travis added the released label Apr 9, 2020

myii mentioned this pull request Apr 9, 2020

fix(fedora): install dhcp-server and resolve rubocop violations #49

Merged

19 tasks

sticky-note deleted the feat/config-ldap branch April 9, 2020 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/config ldap #44

Feat/config ldap #44

sticky-note commented Apr 5, 2020 •

edited

Loading

myii commented Apr 5, 2020

sticky-note commented Apr 6, 2020

myii commented Apr 6, 2020

myii commented Apr 6, 2020

myii commented Apr 6, 2020

sticky-note commented Apr 7, 2020 •

edited

Loading

sticky-note commented Apr 7, 2020 •

edited

Loading

myii commented Apr 7, 2020

sticky-note commented Apr 8, 2020 •

edited

Loading

myii commented Apr 8, 2020

myii commented Apr 9, 2020

saltstack-formulas-travis commented Apr 9, 2020

sticky-note commented Apr 9, 2020

myii commented Apr 9, 2020

Feat/config ldap #44

Feat/config ldap #44

Conversation

sticky-note commented Apr 5, 2020 • edited Loading

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

myii commented Apr 5, 2020

sticky-note commented Apr 6, 2020

myii commented Apr 6, 2020

myii commented Apr 6, 2020

myii commented Apr 6, 2020

sticky-note commented Apr 7, 2020 • edited Loading

sticky-note commented Apr 7, 2020 • edited Loading

myii commented Apr 7, 2020

sticky-note commented Apr 8, 2020 • edited Loading

myii commented Apr 8, 2020

myii commented Apr 9, 2020

saltstack-formulas-travis commented Apr 9, 2020

sticky-note commented Apr 9, 2020

myii commented Apr 9, 2020

sticky-note commented Apr 5, 2020 •

edited

Loading

Does this PR introduce a `BREAKING CHANGE`?

sticky-note commented Apr 7, 2020 •

edited

Loading

sticky-note commented Apr 7, 2020 •

edited

Loading

sticky-note commented Apr 8, 2020 •

edited

Loading