Fix incorrect syntax in "gitlab_rails['ldap_servers']" field

[jnicholas1]

In reference to issue #92.

It appears that no matter how a hash is presented to GitLab, it will not accept it. The field must be presented as YAML, at least in the version I tested.

• The "gitlab_rails['ldap_servers']" field is presented as YAML instead of a hash. In my testing the "to_yaml" call used in the template always produced valid YAML that GitLab accepted.
• Updated the Rspec Test.
• Tested on GitLab 8.13.3, rev 8d79ab3 on CentOS 7

[eric4tech]

Using GitLab 8.13.5 on RHEL6, the "gitlab_rails['ldap_servers']" field works fine for me as a hash. However, I am interested in getting the YAML format working as well. I need to be able to get the password from a parameter, and I think the YAML format will help with this.

Here is an example from my gitlab.rb:
gitlab_rails['ldap_servers'] = {"MyAD"=>{"active_directory"=>true, "allow_username_or_email_login"=>false, "base"=>"ou=OU,dc=ad,dc=example,dc=com", "bind_dn"=>"ldap@ad.example.com", "block_auto_created_users"=>false, "group_base"=>"ou=OU,dc=ad,dc=example,dc=com", "host"=>"ldap.ad.example.com", "label"=>"MyAD", "method"=>"ssl", "password"=>"myldapadminpwd", "port"=>636, "uid"=>"sAMAccountName", "user_filter"=>""}}

The input for this parameter in Puppet (using Foreman) is yaml and is formatted exactly how the example in the README appears. https://forge.puppet.com/vshn/gitlab#ldap-configuration-example

[jnicholas1]

That's interesting. Maybe the hash format cannot contain certain characters? The puppet run output looked identical to what you've pasted, but GitLab would not accept it. Also, as mentioned in the issue, I did not forget to add the additional top-level hash such as "Main" (appears as "MyAD" in your example).

I may be wrong about this, but since the documentation suggests using a YAML block, this module should do so. The fact that a hash works is either incidental or there for backwards compatibility.

Why would the YAML format allow you to pass the password as a parameter? If you're talking about a Puppet variable, you can already reference those from either Hiera or within the manifest depending on how you're using the module.

[eric4tech]

I agree that the module should try to accommodate the YAML block. I imagine hash works for backwards compatibility. I was trying to restructure the gitlab_rails section to work with this, but I haven't found a good way to do so while maintaining the "future-proof" setup that vshn has in place. I ended up just making ldap_enabled and ldap_servers their own parameter.

As for the password, I got that figured out now. So my module works as I want, but is not consistent with the master, so I'm not sure I want to submit a pull request.

[LongLiveCHIEF]

The more and more I think about this, the more and more I think that perhaps using the augeas resource might be a better approach than a custom decorator function and template file altogether.

[jnicholas1]

You may want to considering reverting this change or looking for an alternate solution.

I discovered that simply calling "to_yaml" doesn't mean that the YAML block will always be formatted in the way GitLab expects. Our team has a long and complex user_filter string that we use to select teams that should have access, but ruby renders it inappropriately which causes authentication failures.

Calling "to_yaml" may cause unpredictable string formatting (varies with ruby version) and may break user's LDAP configurations. Unfortunately, I didn't discover this until I changed the configuration.

Maybe it would be easier and more straight-forward to avoid the issue altogether and simply require that the user specify a multi-line string for this value? I can't think of any way to render this YAML safely and consistently otherwise.

[LongLiveCHIEF]

I've taken to altering the gitlab_rails iterator in the template to exclude the ldap hashes, and then just make each ldap key a parameter to the main class.

I want to figure out how to use augeas to modify this file in the long run, to avoid template additions/deletions due to version updates from the omnibus, but haven't had time to work on that yet.

[earsdown]

I agree with @jnicholas1 - this change should probably be reverted while we explore alternative solutions. As it stands, this is a breaking change because we are dependent on the module's previous behaviour.

How we solved this (via our profile module) was to write out our ldap_servers hash as YAML to a separate file, and use the YAML.load_file function in /etc/gitlab/gitlab.rb instead. Although not stated in Gitlab's documentation, we're fairly confident this is another valid and future-proof way of specifying gitlab["ldap_servers"]. And it means we won't have to hard-code the specific settings that take hash values in the gitlab.rb.erb template.

Our /etc/gitlab/gitlab.rb looks like this:

gitlab_rails['ldap_servers'] = YAML.load_file("/etc/gitlab/gitlab_rails-ldap_servers.yaml")

And our /etc/gitlab/gitlab_rails-ldap_servers.yaml file looks like this:

---
main:
  label: LDAP
  host: ldap.example.com
  port: '636'
  uid: uid
  method: ssl
  bind_dn: user
  password: password
  active_directory: true
  allow_username_or_email_login: true
  base: ou=users,dc=example,dc=com
  user_filter: "(objectClass=*)"

To render the separate YAML file, we're doing something like this:

define profile::gitlab::confighash (
  $section,
  $value,
) {
  file { "/etc/gitlab/${section}-${name}.yaml":
    ensure  => 'file',
    owner   => 'root',
    group   => 'root',
    mode    => '0600',
    content => inline_template('<%=@value.to_yaml%>'),
    notify  => Class['::gitlab'],
  }
}

profile::gitlab::confighash { 'ldap_servers':
  section => 'gitlab_rails',
  value   => { "main" => { "label" => "LDAP", "host" => "ldap.example.com", ... } },
}

This means we can specify our ldap_servers setting as an actual hash (e.g. via hiera), which is much nicer than trying to construct our hash as a valid YAML string. It also means we can use eyaml to encrypt the LDAP password.

There are several other settings that also take hash values (like backup_upload_connection). We'd really like a solution that doesn't require the module's gitlab.rb.erb template to know about every possible setting that takes a hash.

@jnicholas1 maybe we can help you out with your user_filter to_yaml problem?