Feature/ldap kerberos configuration

[Useurmind]

Hi,

I have added fields for kerberos authentication to the LDAP federation resource.
They were missing and we are using them in our scenario.

Please tell me if there are any problems with the code.
I also added docs for the new properties.

Kind Regards
Jochen

[tomrutsaert]

Looks good, 👍 Also great that you added docs!

But before I approve, could you add/do following things:

• Run 'make fmt' or gofmt on the files you have changed. (The pipelines are failing because of this)
• Add an example of your fields in /example/main.tf or a new tf file in /example
• Add a test in 'resource_keycloak_ldap_user_federation_test.go' to prove your changes are working. There are already a couple of tests present that you can extend or use as an example for a new test.

[tomrutsaert]

It seems some of your tests are failing.
Are they also failing locally?

[Useurmind]

They run locally with go test ./provider.
But I did not start a Keycloak to test the provider on.

The actual reason was a copy paste error.

[Useurmind]

Now it fails with some error that I do not know what it means:

testing.go:640: Step 0 error: Missing newline after argument: On /tmp/tf-test872870681/main.tf line 35: An argument definition must end with a newline.

[tomrutsaert]

Well these are tests that are ran against local keycloak. You should at least run the tests that test the logic you have changed.
A local environment can easily be set up with following command in the root of the project:
make local
This will spin up db, ldap and keycloak instance
Once everything is up and running, you can execute following command to only run the ldap user Federation tests
KEYCLOAK_CLIENT_ID=terraform KEYCLOAK_CLIENT_SECRET=884e0f95-0f42-4a63-9b1f-94274655669e KEYCLOAK_REALM=master KEYCLOAK_URL="http://localhost:8080" TF_ACC=1 go test github.com/mrparkers/terraform-provider-keycloak/provider -v --run TestAccKeycloakLdapUserFed
On current master branch all these tests still work.

Looking at the error you posted above, it might have to do with test input format...
I do not have the time to look into it myself,.....

[mrparkers]

I think the issue with the failing tests is that all of the string parameters aren't surrounded in quotes. Take a look at line 706 for an example.

[Useurmind]

You are obviously right, sir! :)

[mrparkers]

LGTM code-wise.

From a usability perspective, I'm wondering if it would make more sense to group the kerberos related attributes into its own block. This can help separate the kerberos config from the rest of the ldap federation config, and it can allow you to correctly set required attributes if you want to enable kerberos support.

When you look in the GUI, there's a switch for enabling kerberos, and when it's enabled, the realm, server principal, and keytab are all required. So we want to make sure that when the user wants to enable kerberos, that those fields are required. However, we can't make them required in the schema because not everyone wants to use kerberos.

So I think the solution should be to add kerberos as a set in the schema. This can be optional, but when it's set, the subschema will have required attributes.

So in HCL it might look like this:

resource "keycloak_ldap_user_federation" "ldap_user_federation" {
  // other config
  kerberos { // by specifying this block, `allow_kerberos_authentication` is assumed to be true
    server_principal = "HTTP/keycloak.local@FOO.LOCAL" // required
    use_kerberos_for_password_authentication = false // optional
    key_tab = "/etc/keycloak.keytab" // required
    kerberos_realm = "FOO.LOCAL" // required
  }
}

What do you think about this?

If you'd like to take a stab at refactoring this, here's an example of another resource that has a subschema like the one I was talking about: https://github.com/mrparkers/terraform-provider-keycloak/blob/master/provider/resource_keycloak_openid_client.go#L128-L151

[Useurmind]

That sounds reasonable.

I will have a look at it this weekend.

[mrparkers]

looks good, nice work!