Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass-through un-configurable options for enterprise connections #802

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Aug 31, 2023

🔧 Changes

A notable behavior of the management API's PATCH /api/v2/connections/{id} endpoint is that the options property does not abide by normal PATCH conventions. Instead, the entire object will be replaced by the one provided in the PATCH payload.
This behavior is problematic because there are several unconfigurable connection options for various connection types that need to be preserved for the connection to operate properly. This PR attempts to preserve several of these by reading the current connection options and "passing-through" several of them to the PATCH payload.
Granted, this is a short-term solution. Long-term, we'll need to devise a more holistic solution to avoid frequent, persistent updates to these pass-through properties.

This is a refactor from the fix introduced on v0.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@sergiught sergiught marked this pull request as ready for review August 31, 2023 09:42
@sergiught sergiught requested a review from a team as a code owner August 31, 2023 09:42
@codecov-commenter
Copy link

Codecov Report

Merging #802 (4ea953b) into patch/enterprise-connection-options (bfb9f2b) will decrease coverage by 0.07%.
The diff coverage is 85.93%.

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                           @@
##           patch/enterprise-connection-options     #802      +/-   ##
=======================================================================
- Coverage                                90.51%   90.44%   -0.07%     
=======================================================================
  Files                                       99       99              
  Lines                                    13226    13350     +124     
=======================================================================
+ Hits                                     11972    12075     +103     
- Misses                                     889      903      +14     
- Partials                                   365      372       +7     
Files Changed Coverage Δ
internal/auth0/connection/expand.go 94.74% <85.93%> (-1.92%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely more organized than before 👍 . Only a couple small suggestions.

return err
}

existingOptions := existingConnection.Options.(*management.ConnectionOptionsAD)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized after the initial implementation is that we should perform a nil check against existingConnection.Options before the type assertion.

Copy link
Contributor Author

@sergiught sergiught Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: Can an already created enterprise connection, not have any options defined? 🤔

I added a nil check in which case we'll skip over the func call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure and to your point, even if it were possible it should be incredibly rare. This would only be precautionary since we've seen occasional panics with type assertions. If you are certain that it is not applicable here, we can remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't mind keeping the check for now.

existingOptions := existingConnection.Options.(*management.ConnectionOptionsAD)

expandedOptions := connection.Options.(*management.ConnectionOptionsAD)
expandedOptions.Thumbprints = existingOptions.Thumbprints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -773,3 +780,173 @@ func expandConnectionOptionsScopes(data *schema.ResourceData, options scoper) {
options.SetScopes(true, scope.(string))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# passThroughUnconfigurableConnectionOptions maintains crucial connection options by forwarding them from the current options to the PATCH payload
# This is necessary because the /api/v2/connections/{id}, endpoint does not follow usual
# PATCH behavior, the 'options' property is entirely replaced by the payload object.

Or something similar.

Base automatically changed from patch/enterprise-connection-options to v1 August 31, 2023 12:10
@sergiught sergiught force-pushed the patch/pass-through-enterprise branch from 4ea953b to 6a3234c Compare August 31, 2023 13:55
@sergiught sergiught requested a review from willvedd August 31, 2023 13:56
@sergiught sergiught merged commit c772a8f into v1 Aug 31, 2023
@sergiught sergiught deleted the patch/pass-through-enterprise branch August 31, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants