-
Notifications
You must be signed in to change notification settings - Fork 16
Create a Saas Connector from a Template [#814] #1076
Conversation
…mplates # Conflicts: # data/saas/dataset/mailchimp_dataset.yml # src/fidesops/main.py # src/fidesops/ops/api/v1/endpoints/saas_config_endpoints.py
…key already exists.
…te in the template endpoint. Don't save ConnectionConfig until secrets are validated.
…s and datasets with instance_fides_key. - Fix datadog yaml so it can be included in the saas connector registry. There was an error in how the saas config was formatted.
…to have brackets around the "instance_fides_key" to indicate these will be replaced. Update the fides_key definition to allow "<instance_fides_key>" with brackets specifically to pass validation.
…crets that should be supplied for a given connector. Use the saas config type instead of the fides key for the model title. Add test verifying that fides key /instance key validation works as expected.
- Add new endpoint to postman collection - Add drafts doc. - Update old response body in docs for connection types.
@@ -1,59 +1,59 @@ | |||
saas_config: | |||
- fides_key: datadog_connector_example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an error in the datadog yamls which showed up when we validate the saas connector templates that are defined in the registry on startup.
if value == "<instance_fides_key>": | ||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added so we can specify the fides_keys in the saas connector templates as <instance_fides_key>
, which will be replaced with the name the user actually wants when a resource is created from a template. Without this, the files fail validation since angle brackets can't be in a FidesKey.
@galvana wanted to follow existing conventions in the saas config files that have angle brackets around fields that are meant to be replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a small comment above line 14, like "this is needed to support SaaS connector templates"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call
connection_config: ConnectionConfig = ( | ||
create_connection_config_from_template_no_save( | ||
db, connector_template, template_values | ||
) | ||
) | ||
except KeyOrNameAlreadyExists as exc: | ||
raise HTTPException( | ||
status_code=HTTP_400_BAD_REQUEST, | ||
detail=exc.args[0], | ||
) | ||
|
||
connection_config.secrets = validate_secrets( | ||
template_values.secrets, connection_config | ||
).dict() | ||
connection_config.save(db=db) # Not persisted to db until secrets are validated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We create the ConnectionConfig with the ConnectionConfig.saas_config attached first without saving. Creating the ConnectionConfig lets us use existing logic in validate_secrets
to dynamically figure out which secret types are expected for this particular types of saas config. Not saving this means we don't have to delete the connectionconfig if validation fails.
If this passes, we add the connection config secrets and then save the resource.
dataset_config: DatasetConfig = create_dataset_config_from_template( | ||
db, connection_config, connector_template, template_values | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ConnectionConfig needs to be saved to the database before we create a DatasetConfig. I should be catching all of the known instances that would cause DatasetConfig creation to fail, but it's always possible something happens here. Should I add something that removes the created ConnectionConfig if DatasetConfig creation fails?
Probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I've added this functionality -
model: Type[SaaSSchema] = create_model( | ||
f"{self.saas_config.fides_key}_schema", | ||
f"{self.saas_config.type}_schema", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added because we now have the saas_config templates fides_keys as <instance_fides_key> which would cause validation to fail when we're trying to build schemas.
db, connection_config, connector_template, template_values | ||
) | ||
|
||
return dataset_config.dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently just returning the finished dataset as confirmation the process is complete, but is there something more useful here? What is the ideal response body? Main thing, we need to make sure we don't return ConnectionConfig secrets here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure, I think it depends on how front-end wishes to use / display info? This should be fine for now though.
forgot to run unsafe checks, I assume these will fail, I'll need to update the |
…in the saas fixtures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good, with small comments, doing some manual testing now.
saas_connector_registry.toml
Outdated
@@ -0,0 +1,64 @@ | |||
[adobe_campaign] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this file might be better located in our data/saas
dir? what do you think?
if not connector_template: | ||
raise HTTPException( | ||
status_code=HTTP_404_NOT_FOUND, | ||
detail=f"SaaS connector type '{saas_connector_type}' is not registered.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I clearer message would be something like "SaaS connector type '{saas_connector_type}' is not yet available in Fidesops. For a list of available SaaS connectors, refer to xyz endpoint"
).dict() | ||
connection_config.save(db=db) # Not persisted to db until secrets are validated | ||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we throw a friendly or at least unique message if not all required fields are provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a specific error to catch if it makes it to this point.
I added this as a final catch-all in case the database was down or we had some other unexpected failure, so I can remove the connection_config we just created.
If it makes it here, the connection_config exists, the instance_key is the correct format and has been verified to not exist already, and the dataset has been verified to be of the proper form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh I see, I'm thinking more about what if secrets aren't valid, or the needed secrets aren't provided? I'll test this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the secrets are validated just above with connection_config.secrets = validate_secrets( template_values.secrets, connection_config ).dict()
.
I'm reusing the same code we wrote for the connection config secrets endpoint! Note this endpoint doesn't attempt to run a test of the secrets, it just saves them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my unit tests on this endpoint show a lot of the error cases that are handled!
db, connection_config, connector_template, template_values | ||
) | ||
|
||
return dataset_config.dataset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure, I think it depends on how front-end wishes to use / display info? This should be fine for now though.
if value == "<instance_fides_key>": | ||
return value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a small comment above line 14, like "this is needed to support SaaS connector templates"
…ser-* data categories after the fideslang update, so they would show up if the user picked a "user" data category.
Remaining failing hubspot test is unrelated to this work. It was noticed in the fideslang work https://github.com/ethyca/fidesops/runs/7792050618?check_suite_focus=true, trying a fix now. |
Also heads up that #1059 might be merged first, which includes a postman collection change. |
@pattisdr Manually tested and it looks good. I was able to confirm the dataset returned for the new endpoint. Also, I think the response error is specific enough when one of the required secrets fields isn't present. e.g.
|
ok great, getting the branch up to date, thanks for the review @eastandwestwind |
…mplates # Conflicts: # CHANGELOG.md # docs/fidesops/docs/postman/Fidesops.postman_collection.json
Hubspot failing test is a pre-existing issue, and has been discussed here- #890 (comment) Going ahead and merging this ticket |
It also seems like some of the failure is related to fideslang, in that some of the data categories changed on the hubspot dataset, which causes more fields to be returned under various policies. |
* Starting point for SaaS connector templates * Fix imports from restructuring. * Get happy path working for instantiate connector from template endpoint. * Remove updating connector instances for now - out of scope. * Test nonexistent templates, secrets validation, instance key / fides key already exists. * Create DatasetConfigs and ConnectionConfigs instead of create_or_update in the template endpoint. Don't save ConnectionConfig until secrets are validated. * Add the other saas connectors to the registry and update their configs and datasets with instance_fides_key. - Fix datadog yaml so it can be included in the saas connector registry. There was an error in how the saas config was formatted. * Update the fides_keys in the existing saas configs and dataset yamls to have brackets around the "instance_fides_key" to indicate these will be replaced. Update the fides_key definition to allow "<instance_fides_key>" with brackets specifically to pass validation. * Fix a side effect on a separate endpoint that returns the types of secrets that should be supplied for a given connector. Use the saas config type instead of the fides key for the model title. Add test verifying that fides key /instance key validation works as expected. * - Update CHANGELOG - Add new endpoint to postman collection - Add drafts doc. - Update old response body in docs for connection types. * Replace the <instance_fides_key> with a properly formatted fides_key in the saas fixtures. * If DatasetConfig creation fails, delete the recently created ConnectionConfig. * Address some of the saas integration tests where I've changed the fides_key. * Fix typos. * Fix typo. * Fix unrelated bug where hubspot dataset has new datacategories with user-* data categories after the fideslang update, so they would show up if the user picked a "user" data category. * Respond to CR. Co-authored-by: Dawn Pattison <[email protected]>
Purpose
Rather than making five or so separate API requests, it would be useful to be able to create all the resources for a SaasConnector in a single request.
Adds an endpoint to be able to create all the resources for a SaaS Connector in one place.
This also starts the creation of a registry for SaaS Connectors that other parts of the application can eventually use as a source of truth. This registry defines templates that we use to create the necessary resources.
Changes
saas
ConnectionConfig with a saas config attached without saving.instance_fides_key
with theinstance_key
specified by the user.instance_fides_key
is also replaced in this json.Out of scope
Checklist
CHANGELOG.md
fileCHANGELOG.md
file is being appended toUnreleased
section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #814