feat: Adds support to new Federated Auth parameters for OIDC #1874

oarbusi · 2024-01-25T13:53:51Z

Description

Adds support for new federated auth attributes to support OIDC IdP.

Resource federated_settings_identity_provider now supports both types of IdP: SAML and OIDC (workforce only)
Data source federated_settings_identity_provider now supports both types of IdP: SAML and OIDC (workforce only)
Data source federated_settings_identity_providers now returns both types of IdP: SAML and OIDC (workforce only)

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-220798

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

oarbusi · 2024-01-29T14:15:09Z

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

@@ -360,95 +462,12 @@ func resourceMongoDBAtlasFederatedSettingsIdentityProviderImportState(ctx contex
 		return oldSDKImport(federationSettingsID, idpID, d, meta)
 	}

-	federatedSettingsIdentityProvider, _, err := connV2.FederatedAuthenticationApi.GetIdentityProvider(context.Background(), *federationSettingsID, *idpID).Execute()


For consistency with other resources, I am removing the non necessary code in import. Only necessary attribute to set in import is the Id

lantoli · 2024-01-29T16:07:24Z

...e/federatedsettingsidentityprovider/data_source_federated_settings_identity_provider_test.go

@@ -36,6 +36,35 @@ func TestAccFedDSFederatedSettingsIdentityProvider_basic(t *testing.T) {
 	})
 }

+func TestAccFedDSFederatedSettingsIdentityProvider_oidcBasic(t *testing.T) {
+	acc.SkipTestExtCred(t)


would it be possible easy to have a migration test even with acc.SkipTestExtCred(t) or it's hard to do?

I would say that it's not straight forward even with acc.SkipTestExtCred(t) but I am open to discussing it. Maybe as part of enabling acceptance tests in the CI we could create migration tests?

This could be good test to add to ensure no breaking changes are encountered. It can import a resource with previous version and then update to the latest and expect an empty plan.

+1 with the test. Perhaps no need to block the release (and this PR) but it's a good one to have, we should actually aim at always have a test like this.

I tried to add the migration test with the help of @lantoli and it seems that there is an issue when running a migration test using ExternalProvider and ImportState in the acceptance test step at the same time. We will look again after release.

lantoli · 2024-01-29T16:08:06Z

...e/federatedsettingsidentityprovider/data_source_federated_settings_identity_provider_test.go

+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckMongoDBAtlasFederatedSettingsIdentityProvidersExists(resourceName),
+
+					resource.TestCheckResourceAttrSet(resourceName, "federation_settings_id"),


can we also check for the expected value or you think it's not so important?

I think that testing that the attribute itself is set it's enough. In the unit tests in model_federated_settings_identity_provider_test.go we are already testing the values.

I agree with you we're using unit tests and we shouldn't really "test the other components", but think about an API change that changes a computed value (like it's been happening recently): how would we detect it if not through these tests?

Good point! I included some value checks in both OIDC and SAML acceptance test to check most important attributes values

lantoli · 2024-01-29T16:09:21Z

...rvice/federatedsettingsidentityprovider/data_source_federated_settings_identity_providers.go

+	samlParams := &admin.ListIdentityProvidersApiParams{
+		FederationSettingsId: federationSettingsID.(string),
+		Protocol:             conversion.StringPtr("SAML"),
+	}


can both protocols be enabled at the same time?

One federation can have multiple IdPs, so you can have one SAML IdP and one OIDC IdP in the same federation. But one IdP can be either SAML or OIDC, not both

.../federatedsettingsidentityprovider/data_source_federated_settings_identity_providers_test.go

lantoli · 2024-01-29T16:11:02Z

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

+			return diag.FromErr(fmt.Errorf("error setting sso debug enabled (%s): %s", d.Id(), err))
+		}
+
+		if err := d.Set("sso_url", federatedSettingsIdentityProvider.SsoUrl); err != nil {


not specific to this PR, there are many error checks like this that seem a bit verbose and we might be able to extract to some helper functions

lantoli · 2024-01-29T16:11:51Z

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

@@ -230,6 +295,11 @@ func resourceMongoDBAtlasFederatedSettingsIdentityProviderUpdate(ctx context.Con
 		return diag.FromErr(fmt.Errorf("error retreiving federation settings identity provider (%s): %s", federationSettingsID, err))
 	}

+	if d.HasChange("protocol") {


related to a question above, can be only 1 or 2 protocols at the same time?

if only one, have you tried to change the protocol and see how it's handled the attributes that can only be in one type of protocol? e.g. if just protocol changes, it would probably fail because you would need to delete the fields only used in the old protocol and add the ones used in the new one

Yes, if you only change the protocol it fails

how does it fail? is it a readable error?

Unexpected error from the server. From looking at the code, updating protocol or trying to update from SAML to OIDC or the other way around is not possible

AgustinBettati

LGTM, small comment in import operation

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go

...ervice/federatedsettingsidentityprovider/data_source_federated_settings_identity_provider.go

...rvice/federatedsettingsidentityprovider/data_source_federated_settings_identity_providers.go

oarbusi added 2 commits January 24, 2024 15:13

add OIDC to read of federated_settings_identity_provider resource

3b28df1

fix import to not set unnecessary fields

7a2d2c5

github-actions bot added the enhancement label Jan 25, 2024

oarbusi added 11 commits January 25, 2024 14:02

lint

b751c29

add new fields to update

533b0fa

data source read

749e4fb

return OIDC and SAML in plural data source

a685e8f

fix

4e60825

Merge branch 'master' into CLOUDP-220798

ae17b51

update sdk in plural data source

ab962b2

add attributes to schema of data source

b50e3af

fix schemas

34b9f74

change flattener and add tests

443202d

fix lint

a11c77e

oarbusi commented Jan 29, 2024

View reviewed changes

oarbusi marked this pull request as ready for review January 29, 2024 14:15

oarbusi requested a review from a team as a code owner January 29, 2024 14:15

lantoli reviewed Jan 29, 2024

View reviewed changes

.../federatedsettingsidentityprovider/data_source_federated_settings_identity_providers_test.go Show resolved Hide resolved

lantoli reviewed Jan 29, 2024

View reviewed changes

lantoli approved these changes Jan 29, 2024

View reviewed changes

AgustinBettati reviewed Jan 29, 2024

View reviewed changes

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go Show resolved Hide resolved

...l/service/federatedsettingsidentityprovider/resource_federated_settings_identity_provider.go Outdated Show resolved Hide resolved

marcosuma reviewed Jan 30, 2024

View reviewed changes

...ervice/federatedsettingsidentityprovider/data_source_federated_settings_identity_provider.go Outdated Show resolved Hide resolved

marcosuma reviewed Jan 30, 2024

View reviewed changes

...rvice/federatedsettingsidentityprovider/data_source_federated_settings_identity_providers.go Outdated Show resolved Hide resolved

marcosuma approved these changes Jan 30, 2024

View reviewed changes

oarbusi added 2 commits January 30, 2024 09:20

PR comments

772bb15

check values in acceptance tests

54f6bc8

oarbusi merged commit cf26ffe into master Jan 30, 2024
35 checks passed

oarbusi deleted the CLOUDP-220798 branch January 30, 2024 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds support to new Federated Auth parameters for OIDC #1874

feat: Adds support to new Federated Auth parameters for OIDC #1874

oarbusi commented Jan 25, 2024 •

edited

Loading

oarbusi Jan 29, 2024

lantoli Jan 29, 2024

oarbusi Jan 29, 2024

AgustinBettati Jan 29, 2024

marcosuma Jan 30, 2024

oarbusi Jan 30, 2024

lantoli Jan 29, 2024

oarbusi Jan 29, 2024

marcosuma Jan 30, 2024

oarbusi Jan 30, 2024

lantoli Jan 29, 2024

oarbusi Jan 29, 2024

lantoli Jan 29, 2024

lantoli Jan 29, 2024

lantoli Jan 29, 2024

oarbusi Jan 29, 2024

marcosuma Jan 30, 2024

oarbusi Jan 30, 2024

AgustinBettati left a comment

feat: Adds support to new Federated Auth parameters for OIDC #1874

feat: Adds support to new Federated Auth parameters for OIDC #1874

Conversation

oarbusi commented Jan 25, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

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

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

AgustinBettati left a comment

Choose a reason for hiding this comment

oarbusi commented Jan 25, 2024 •

edited

Loading