-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Exclude some profile fields from Jinja rendering when they are not valid Jinja. #7630
Conversation
|
||
try: | ||
rendered = super().render_value(value, keypath) | ||
except Exception as ex: |
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.
Catch a more specific exception 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.
I would except I do not know what will be in the exception message here, and I'm concerned that the risk of it containing sensitive information that would be logged could outweigh the value of a narrower catch.
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.
Is the issue that we don't have any idea of the Exception? Or is it that we expect a few specific ones (let's say ExceptionA and ExceptionB for simplicity) and also want to make sure that we catch anything? If it's the latter, then would this be a compromise:
try:
rendered = super().render_value(value, keypath)
except ExceptionA:
# do some ExceptionA stuff
except ExceptionB:
# do some ExceptionB stuff
except Exception as ex:
# this shouldn't happen so throw a dbt exception, such as DbtCompilerException
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 like @mikealfare's suggestion here
def test_no_jinja_for_password(self, project, profiles_root): | ||
with open(pathlib.Path(profiles_root, "profiles.yml"), "w") as profiles_yml: | ||
profiles_yml.write( | ||
"""test: |
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 you store this string as a constant at the top to make it easier to read?
""" | ||
) | ||
result = dbtRunner().invoke(["parse"]) | ||
assert result.success # for now, at least, just ensure success |
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.
Is there a failure case we can test 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.
I'll add an additional check that scans the log messages for the password string.
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.
Looks okay, though I think more tests might be good. One with an env_var that has Jinja characters?
@gshank I added a test along the lines you suggested. |
@@ -0,0 +1,6 @@ | |||
kind: Fixes | |||
body: Exclude password fields from Jinja rendering. |
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.
One idea for updating the changelog entry to reflect the latest approach:
body: Exclude password fields from Jinja rendering. | |
body: Fall back if rendering the password field fails. |
…lid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage
* Exclude some profile fields from Jinja rendering when they are not valid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage * CT-2583: Update changelog entry to reflect new approach
…lid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage
…lid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage
…lid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage
* Exclude some profile fields from Jinja rendering when they are not valid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage * Remove unneeded unit test. --------- Co-authored-by: Peter Webb <[email protected]>
* Exclude some profile fields from Jinja rendering when they are not valid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage * Exclude some profile fields from Jinja rendering when they are not valid Jinja. (#7630) * CT-2583: Exclude some profile fields from Jinja rendering. * CT-2583: Add functional test. * CT-2583: Change approach to password jinja detection * CT-2583: Extract string constant and add additional checks * CT-2583: Improve unit test coverage * Remove unneeded unit test. --------- Co-authored-by: Peter Webb <[email protected]>
resolves #7629
resolves #8345
Main branch PR for #7629, which will also require backports.
Description
Excludes some profile fields from Jinja rendering.
Checklist
changie new
to create a changelog entry