♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault #866

adrian-wojcik · 2024-03-06T13:30:35Z

Summary

Adding additional logic and parameters to sap_rfc connector in order to retrieve credentials from Azure KeyVault.

Importance

Because, it enhances security by securely storing and managing sensitive information, eliminating the need for local storage of such data. Secondly, making Azure Key Vault the primary data source introduces flexibility, enabling seamless updates and rotation of keys and secrets without requiring modifications to the application code, thereby enhancing security levels and facilitating configuration management.

Checklist

This PR:

follows the guidelines laid out in CONTRIBUTING.md
[] links relevant issue(s)
adds/updates tests (if appropriate)
adds/updates docstrings (if appropriate)
adds an entry in CHANGELOG.md

Rafalz13

Added few comments.

viadot/tasks/sap_rfc.py

Rafalz13 · 2024-03-14T11:15:09Z

viadot/tasks/sap_rfc.py

+            saprfc_credentials_key (str, optional): Azure KV secret. Defaults to "SAP".
+            env (str, optional): SAP environment. Defaults to "PROD".
            By default, they're taken from the local viadot config.


You can modify the docstring because it is a key for the credentials - it can be taken from Azure Key Vault or local config. It depends on what is possible.

Please change for the source and flow as well.

Changed in task and flow

Rafalz13 · 2024-03-14T11:15:42Z

viadot/tasks/sap_rfc.py

@@ -97,6 +110,9 @@ def run(
            multiple options are automatically tried. Defaults to None.
            replacement (str, optional): In case of sep is on a columns, set up a new character to replace
                inside the string to avoid flow breakdowns. Defaults to "-".
+            credentials (dict, optional): The credentials to use to authenticate with SAP.


Suggested change

credentials (dict, optional): The credentials to use to authenticate with SAP.

credentials (dict, optional): The credentials to use to authenticate with SAP. Defaults to None.

Changed docstring to the old one from flow. And variable name from 'credentials' to 'sap_credentials'

Rafalz13

Please modify docstrings and add tests.

viadot/tasks/sap_rfc.py

viadot/flows/sap_rfc_to_adls.py

viadot/tasks/sap_rfc.py

…al_key'

…dentias_key' variables into source class

…the source level

gitguardian · 2024-03-18T12:53:59Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}}

Rafalz13

Added few comments.

viadot/flows/sap_rfc_to_adls.py

viadot/sources/sap_rfc.py

Rafalz13 · 2024-04-04T06:59:14Z

viadot/flows/sap_rfc_to_adls.py

@@ -67,6 +69,8 @@ def __init__(
                    ...
                    )
            sap_credentials (dict, optional): The credentials to use to authenticate with SAP. By default, they're taken from the local viadot config.
+            sap_credentials_key (str, optional): The key for sap credentials located in the local config for Azure Key Vault. Defaults to "SAP".


I made a wrong suggestion here. Please change for the rest.

Suggested change

sap_credentials_key (str, optional): The key for sap credentials located in the local config for Azure Key Vault. Defaults to "SAP".

sap_credentials_key (str, optional): The key for sap credentials located in the local config or Azure Key Vault. Defaults to "SAP".

Changed back

Rafalz13 · 2024-04-04T07:06:15Z

viadot/tasks/sap_rfc.py

@@ -97,6 +110,9 @@ def run(
            multiple options are automatically tried. Defaults to None.
            replacement (str, optional): In case of sep is on a columns, set up a new character to replace
                inside the string to avoid flow breakdowns. Defaults to "-".
+            sap_credentials (dict, optional): The credentials to use to authenticate with SAP. By default, they're taken from the local viadot config.


In this case by default it is taken from KeyVault (lines 136-141). Default is None and it is taken from KeyVault here in task or local config on the 'source' level (SAPRFC class). Please take a look and adjust docstring.

♻️ Changed credentials logic to get credentials from KeyVault

6e19edd

adrian-wojcik changed the base branch from main to dev March 6, 2024 13:30

adrian-wojcik requested a review from Rafalz13 March 6, 2024 13:31

🚀 Added changes to changelog

58ec8fc

adrian-wojcik changed the title ~~♻️ Changed credentials logic to get credentials from KeyVault~~ ♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault Mar 6, 2024

Rafalz13 reviewed Mar 14, 2024

View reviewed changes

adrian-wojcik added 2 commits March 14, 2024 13:55

⚡️ Change 'query' parameter as required on the __init__ level

db63d91

🎨 Changed docstring for parameter 'saprfc_credentials_key'

bec218b

Rafalz13 requested changes Mar 14, 2024

View reviewed changes

viadot/tasks/sap_rfc.py Outdated Show resolved Hide resolved

viadot/flows/sap_rfc_to_adls.py Outdated Show resolved Hide resolved

viadot/flows/sap_rfc_to_adls.py Outdated Show resolved Hide resolved

viadot/tasks/sap_rfc.py Outdated Show resolved Hide resolved

adrian-wojcik added 8 commits March 14, 2024 14:45

🎨 Change 'sap_credentials' varable name and docstring

bdec339

🎨 changed variable name from 'saprfc_credential_key' to 'sap_credenti…

505196a

…al_key'

🎨 Changed env default value 'PROD' to 'DEV'

2895eb8

♻️ Change the logic of passing 'env' , 'sap_credentials' and 'sap_cre…

f456ed2

…dentias_key' variables into source class

♻️ Change logic of checking credentials_keys dictionary from task to …

e6a9810

…the source level

🐛 fix parameter name bug

92c2b70

♻️ Refactor saprfc task in order to meet new logic in source

2d3ee8d

♻️ refactor logic in source regarding credentials exceptions

e5ee5b7

adrian-wojcik requested a review from Rafalz13 March 18, 2024 13:04

🚀 Add tests for new logic for source and task level

371ddbd

Rafalz13 reviewed Mar 29, 2024

View reviewed changes

viadot/flows/sap_rfc_to_adls.py Outdated Show resolved Hide resolved

viadot/flows/sap_rfc_to_adls.py Outdated Show resolved Hide resolved

viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved

viadot/sources/sap_rfc.py Outdated Show resolved Hide resolved

adrian-wojcik added 2 commits April 2, 2024 08:07

🎨 Changed docstrings in source, task nad flow

ad17a3f

🎨 Changed logger.warning string in source

f0d218a

adrian-wojcik requested a review from Rafalz13 April 2, 2024 06:11

Merge branch 'dev' into sap_rfc_local_config_to_key_voult_change

922ae5a

Rafalz13 reviewed Apr 4, 2024

View reviewed changes

🎨 Correct docstrings

5108fec

adrian-wojcik requested a review from Rafalz13 April 4, 2024 08:05

Rafalz13 approved these changes Apr 5, 2024

View reviewed changes

Rafalz13 merged commit 624f5b8 into dyvenia:dev Apr 5, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault #866

♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault #866

adrian-wojcik commented Mar 6, 2024 •

edited by Rafalz13

Loading

Rafalz13 left a comment

Rafalz13 Mar 14, 2024

adrian-wojcik Mar 14, 2024

Rafalz13 Mar 14, 2024

adrian-wojcik Mar 14, 2024

Rafalz13 left a comment

gitguardian bot commented Mar 18, 2024 •

edited

Loading

Rafalz13 left a comment

Rafalz13 Apr 4, 2024 •

edited

Loading

adrian-wojcik Apr 4, 2024

Rafalz13 Apr 4, 2024

adrian-wojcik Apr 4, 2024

	credentials (dict, optional): The credentials to use to authenticate with SAP.
	credentials (dict, optional): The credentials to use to authenticate with SAP. Defaults to None.

	sap_credentials_key (str, optional): The key for sap credentials located in the local config for Azure Key Vault. Defaults to "SAP".
	sap_credentials_key (str, optional): The key for sap credentials located in the local config or Azure Key Vault. Defaults to "SAP".

♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault #866

♻️ Changed credentials logic in sap_rfc to get credentials from KeyVault #866

Conversation

adrian-wojcik commented Mar 6, 2024 • edited by Rafalz13 Loading

Summary

Importance

Checklist

Rafalz13 left a comment

Choose a reason for hiding this comment

Rafalz13 Mar 14, 2024

Choose a reason for hiding this comment

adrian-wojcik Mar 14, 2024

Choose a reason for hiding this comment

Rafalz13 Mar 14, 2024

Choose a reason for hiding this comment

adrian-wojcik Mar 14, 2024

Choose a reason for hiding this comment

Rafalz13 left a comment

Choose a reason for hiding this comment

gitguardian bot commented Mar 18, 2024 • edited Loading

️✅ There are no secrets present in this pull request anymore.

Rafalz13 left a comment

Choose a reason for hiding this comment

Rafalz13 Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

adrian-wojcik Apr 4, 2024

Choose a reason for hiding this comment

Rafalz13 Apr 4, 2024

Choose a reason for hiding this comment

adrian-wojcik Apr 4, 2024

Choose a reason for hiding this comment

adrian-wojcik commented Mar 6, 2024 •

edited by Rafalz13

Loading

gitguardian bot commented Mar 18, 2024 •

edited

Loading

Rafalz13 Apr 4, 2024 •

edited

Loading