added identity provider resource #92

AndrewChubatiuk · 2019-02-13T15:49:03Z

Restored #61 PR

…form-provider-keycloak into identity-providers

mrparkers · 2019-02-14T14:38:47Z

@AndrewChubatiuk what are your thoughts into splitting each identity provider into multiple resources? I'm concerned that trying to handle every identity provider in one resource would make it too complex and difficult to properly test.

AndrewChubatiuk · 2019-02-14T15:08:37Z

Okay, I can split them

mrparkers · 2019-02-14T15:17:09Z

Cool. Let's just start with a couple to see if that route makes sense. I understand that, for the most part, the APIs are very similar, so I expect a decent amount of code duplication.

AndrewChubatiuk · 2019-02-14T16:20:06Z

Well, yes, code duplications is a problem and suppose it'll be painful to support this. I have one suggestion:
I could leave only generic attributes in keycloak_identity_provider and add config attribute of type map that accepts any key-value pairs and create datasources which configure and make validation of each identity provider:

resource keycloak_identity_provider oidc1 {
  alias   = "oidc"
  realm   = "master"
  enabled = true
  config {
      "token_url" = "value"
      "lol" = ""
  }
}

resource keycloak_identity_provider oidc2 {
  realm   = "master"
  enabled = true
  config = "${data.keycloak_google_identity_provider.example.config}" {
}

data keycloak_oidc_identity_provider_config example {
  token_url         = "https://example.com"
  authorization_url = "https://example.com"
  client_id         = "<your_client_id>"
  client_secret     = "<your_client_secret>"
}

AndrewChubatiuk · 2019-02-14T16:27:51Z

This approach is similar to aws_iam_policy_document datasource in terraform-provider-aws but it's less complex

mrparkers · 2019-02-14T16:56:06Z

That's an interesting way to approach this, but I don't think data sources are intended to be used that way. Typically, a data source is used when you want Terraform to fetch some configuration from a remote server so you can use it to help configure other resources.

I agree that the code duplication this would produce is unfortunate, but a lot of the Keycloak APIs for different resources are very similar so I don't think there is much we can do to avoid that. There is already a fair amount of this in the keycloak_*_protocol_mapper resources, and the keycloak package tries to consolidate similar API fields into a single file to help with this.

In general, I think I still prefer code duplication over a single resource that attempts to handle many use cases. I want to avoid a scenario where a change to this resource to fix an issue with the Google identity provider (for example) may inadvertently break the support for the Twitter identity provider.

AndrewChubatiuk · 2019-02-14T17:07:21Z

aws_iam_policy_document datasource in aws provider doesn't fetch data, it just makes a policy validation.
On a top of this page page there's a definition of data source. It allows data not only to be fetched but also computed.
Also all ignition datasources do not fetch data from internet, they just prepare a valid configuration in the output

mrparkers · 2019-02-14T21:21:36Z

Thanks for sharing. I see how having a data source that provides a consistent interface for configuration could be useful, especially for validation purposes.

Despite this, I am still of the opinion that these identity providers should be split to multiple resources. If the code duplication issue becomes a problem, we can take advantage of other solutions, such as code generation or factory functions/methods.

…form-provider-keycloak into identity-providers

AndrewChubatiuk · 2019-03-28T06:21:29Z

@mrparkers added some tests

mrparkers · 2019-03-28T21:16:57Z

Thanks, I'll take some time this weekend to review it.

mrparkers · 2019-04-15T14:12:44Z

provider/keycloak_attribute_importer_identity_provider_mapper.go

+		} else if attr, ok := data.GetOk("attribute_name"); ok {
+			rec.Config.Attribute = attr.(string)
+		} else {
+			return nil, fmt.Errorf(`provider.keycloak: keycloak_attribute_importer_identity_provider_mapper: %s: neither "attribute_name" nor "attribute_friendly_name" are set`, data.Get("name").(string))


I am not sure if this was present in my last review or if I just missed it, but this feels like we are re-implementing the Required schema attribute that comes with the Terraform SDK out of the box.

In my opinion, this resource could probably be split into oidc and saml versions since there are some attributes that are required or not allowed depending on the value of providerId.

Thoughts?

There are about 15 identity providers and mappers for these providers are the same or almost same as for OIDC or SAML provider. If we're going to have a separate mapper for each IDP type there'll be about 60 different mappers which confuses more than several conditions in the same provider.

What do you think about it?

That's a fair point. I didn't consider that there were so many different mappers for different identity providers.

With this in mind, what would you think about updating the error message to explain that a particular field is only required because this is a saml or oidc mapper as opposed to re-using the Terraform core error string for required fields? This will make it more clear to the user that, while an attribute is not marked as Required, it is still required because of the provider id.

If you're on board with this, then I am fine with you just leaving it as-is as long as you're okay with me updating this in the future.

mrparkers · 2019-04-15T14:13:12Z

provider/keycloak_attribute_to_role_identity_provider_mapper.go

+		} else if attr, ok := data.GetOk("attribute_name"); ok {
+			rec.Config.Attribute = attr.(string)
+		} else {
+			return nil, fmt.Errorf(`provider.keycloak: keycloak_attribute_to_role_identity_provider_mapper: %s: neither "attribute_name" nor "attribute_friendly_name" are set`, data.Get("name").(string))


Same comment as above

mrparkers · 2019-04-15T14:30:20Z

provider/keycloak_saml_identity_provider.go

+			Optional:     true,
+			Default:      "",
+			ValidateFunc: validation.StringInSlice(keys(nameIdPolicyFormats), false),
+			StateFunc: func(value interface{}) string {


What is this needed for?

What exactly?

The StateFunc. I don't necessarily have a problem with it, it's just the first time it has been used in this provider and I am curious what the need was.

Allowed values for name_id_policy_format attribute are same as in Keycloak UI (Windows Domain Qualified Name, Persistent, etc) and StateFunc converts this value to an actual value which is used in API requests

Okay, so users would be expected to do something like this?

name_id_policy_format = "Windows Domain Qualified Name"

Yes. It's same as in Keycloak UI

mrparkers · 2019-04-15T15:19:00Z

provider/keycloak_oidc_identity_provider.go

+	rec.ProviderId = "oidc"
+	rec.Config = &keycloak.IdentityProviderConfig{
+		BackchannelSupported: keycloak.KeycloakBoolQuoted(data.Get("backchannel_supported").(bool)),
+		UseJwksUrl:           keycloak.KeycloakBoolQuoted(true),


I left this comment originally and I don't think it was addressed - I don't think this attribute is useful without the ability to customize the jwks url (correct me if I'm wrong here). If this is the case, I think this attribute should either be removed or updated to allow users to customize this url

mrparkers · 2019-04-15T16:27:57Z

A couple of general comments unrelated to the review -

Apologies for taking so long with getting this PR across the finish line, I spent all of last week at a Conference so I wasn't able to dedicate time into reviewing this.
After my comments from the last review are addressed and this is merged, I'll spend some time within the next few weeks writing docs for this so I can better learn how to use this feature myself. I'll cut a 1.7.0 release so you can use this right away, but I may make slight changes after I understand this feature better and release them in 1.7.1. Is that alright with you?

AndrewChubatiuk · 2019-04-15T16:29:32Z

That's okay.
Yes, no problem

mrparkers

One last review (I swear I will merge this after this is addressed).

I attempted to run the example/main.tf configuration and ran into a few issues.

mrparkers · 2019-04-17T16:35:07Z

example/main.tf

+	realm = "${keycloak_realm.test.id}"
+	name = "Attribute: email"
+	claim_name = "upn"
+	identity_provider_alias = "${keycloak_oidc_identity_provider.adfs.alias}"


This reference is invalid - it doesn't point to another resource within this tf file

mrparkers · 2019-04-17T16:35:43Z

example/main.tf

+	user_attribute = "email"
+}
+
+resource keycloak_oidc_user_attribute_idp_mapper email {


It doesn't look like this resource is defined within provider.go - was this renamed to something else after this was committed?

mrparkers · 2019-04-17T16:35:50Z

example/main.tf

+	single_sign_on_service_url = "https://example.com/auth"
+}
+
+resource keycloak_saml_user_attribute_idp_mapper email {


It doesn't look like this resource is defined within provider.go - was this renamed to something else after this was committed?

Forgot to update example.tf after I updated naming of idp mapper resources. Just pushed my changes

mrparkers · 2019-04-18T16:02:40Z

provider/keycloak_hardcoded_role_identity_provider_mapper.go

+
+func getHardcodedRoleIdentityProviderMapperFromData(data *schema.ResourceData, _ interface{}) (*keycloak.IdentityProviderMapper, error) {
+	rec, _ := getIdentityProviderMapperFromData(data)
+	rec.IdentityProviderMapper = "oidc-hardcoded-role-idp-mapper"


Can this resource not be used for SAML?

This resource is also used for SAML, but looks like it's an issue in Keycloak API

You can try to create this mapper manually

mrparkers

Thanks for the contribution and for your patience with my reviews!

Andrii Chubatiuk added 15 commits February 13, 2019 17:45

added identity provider

3a2786f

minor fix

870432c

fixed resource names

39103d9

minor fix

65ea14d

fixed format

234c108

updated provider

121eaf9

Merge branch 'identity-providers' of github.com:AndrewChubatiuk/terra…

288cff3

…form-provider-keycloak into identity-providers

updated mapper tests

362795c

Merge branch 'identity-providers' of github.com:AndrewChubatiuk/terra…

4651127

…form-provider-keycloak into identity-providers

fixed import

2ae86b7

minor fix

eec8137

minor fix

61b5125

minor fix

8d2f0a8

removed unused import

035ee87

fixed mapper import

2f6c1c8

AndrewChubatiuk and others added 7 commits March 1, 2019 17:32

Merge branch 'master' into identity-providers

e09b968

added generic identity providers and mappers

0b9050f

removed tests

014c559

added resources to provider

a24ff11

fixed mappers

f8617c0

minor fix

f1a1749

using closure to reduce amount of code duplications

ad4bd67

Andrii Chubatiuk added 10 commits March 27, 2019 12:29

minor fix

a1cc21d

minor fix

5f701f3

Merge branch 'identity-providers' of github.com:AndrewChubatiuk/terra…

05a0bce

…form-provider-keycloak into identity-providers

added tests for hardcoded attribute idp mapper

bf5dfe1

minor fix

4b555c1

added attribute importer mapper test

472c6f6

added user template mapper test

9a23f4c

added one more attribute mapper

f5f26fb

minor fix

ea79c9d

Trigger build

a940353

fixed error handling

e605a61

mrparkers reviewed Apr 15, 2019

View reviewed changes

AndrewChubatiuk and others added 2 commits April 15, 2019 18:20

Merge branch 'master' into identity-providers

286ff63

removed jwks

cffe5dd

updated error messages for idp mappers

a5fd008

mrparkers suggested changes Apr 17, 2019

View reviewed changes

Andrii Chubatiuk added 6 commits April 17, 2019 23:22

updated examples

102cc9e

fixed api

d20ce2e

minor fix

80da50f

keycloak issue workaround

8b8bc1d

keycloak issue workaround

0c16d33

keycloak issue workaround

f83da20

mrparkers reviewed Apr 18, 2019

View reviewed changes

mrparkers approved these changes Apr 18, 2019

View reviewed changes

mrparkers merged commit e899c32 into keycloak:master Apr 18, 2019

added identity provider resource #92

added identity provider resource #92

Conversation

AndrewChubatiuk commented Feb 13, 2019

mrparkers commented Feb 14, 2019

AndrewChubatiuk commented Feb 14, 2019

mrparkers commented Feb 14, 2019

AndrewChubatiuk commented Feb 14, 2019 • edited Loading

AndrewChubatiuk commented Feb 14, 2019

mrparkers commented Feb 14, 2019

AndrewChubatiuk commented Feb 14, 2019 • edited Loading

mrparkers commented Feb 14, 2019

AndrewChubatiuk commented Mar 28, 2019

mrparkers commented Mar 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewChubatiuk Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers commented Apr 15, 2019

AndrewChubatiuk commented Apr 15, 2019

mrparkers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrparkers left a comment

Choose a reason for hiding this comment

AndrewChubatiuk commented Feb 14, 2019 •

edited

Loading

AndrewChubatiuk commented Feb 14, 2019 •

edited

Loading

AndrewChubatiuk Apr 15, 2019 •

edited

Loading