Pryz / terraform-provider-ldap

LDAP provider for Terraform
MIT License
36 stars 31 forks source link

Lots of enhancements and fix to issue #3 #4

Closed dihedron closed 7 years ago

dihedron commented 7 years ago

Hi @Pryz , these last two weeks I've been working on an extensive set of enhancements to your provider. I'll be using my version in a production project soon and I tested it quite extensively; I will also try to thrust it onto Hashicorp and see if they want to adopt it under their "builtins"; would it be a problem for you? this code is only partly mine and it would not be there if you didn't publish yours first...

Once I started coding, there was very little I didn't touch but you may still want to pull it anyway, or at least give me an opinion on what you think of my modifications. I'm no LDAP guru and I'm learning terraform, so any feedback is really appreciated.

Like @kasimon said in his issue #3 the use of base_dn is a problem for a bunch of reasons:

  1. the DN is actually the object's RDN, and the BaseDn is the DN of the parent object; as far as I know, the base DN should be the set of DCs one uses at the top of the tree, not the parent's DN...
  2. in the internal LDAP searches you're starting from the BaseDN and you look up the whole subtree for an object whose RDN is "foo", but there might be multiple since the visit is recursive; what then?
  3. by searching the whole subtree you don't exploit the "search by primary key" feature that you have by using a nil DN, the DN as baseDN and the base object scope:
    request := ldap.NewSearchRequest(
        dn,
        ldap.ScopeBaseObject,
        ldap.NeverDerefAliases,
        0,
        0,
        false,
        "(objectClass=*)",
        nil,
        nil,
    )
  4. you can actually omit base_dn altogether and use a complete DN, which would also allow you to create nested objects, with inner objects referencing their parent's DN like this: dn = "uid=bar,${ldap_object.foo.dn}"; by the way this creates implicit dependency so objects are created in the proper order;
  5. once you have the DN, you can use it instead of a random int as the argument to d.SetId(db) in terraform's state;
  6. update was missing, and I needed it so I implemented it (it was not easy with multivalued attributes!)
  7. destroying and recreating an object to update it is a big problem when that object contains other objects (e.g. a OU containing users); my update is surgical and in place; only changes to the DN force the creation of a new objct;
  8. the syntax for representing attributes ("name" and "value") looked a bit cumbersome, so I defined my own format; it was more difficult than expected because I discovered along the way that HCL does not support a map of lists (map[string][]string, to encode "attribute" = ["value1", "value2", ... "valueN"]) and I had to resort to a set of 1-element maps: unfortunate but still easier to write and read;
  9. I modified the docker-compose tests to have phpldapadmin available on localhost too so you can run your tests and verify the results via GUI in a second.

I know this is a load of stuff, I hope you'll appreciate or at least provide your feedback.

With my modifications, I guess issue #3 could be considered fixed: an example is in the tests.

TODO: I still need to more intimately understand and fix the acceptance tests for terraform plugins.

Pryz commented 7 years ago

Wow thanks a lot of this PR @dihedron ! I will review it during the day ;)

Pryz commented 7 years ago

@dihedron I love your PR :) Just need to fix a tiny thing in the README and I can merge it :)

dihedron commented 7 years ago

There you go! :-)

Pryz commented 7 years ago

Thanks !