Revert cert default, add require_certificate_validation Behavior Flag

[damian3031]

1. Revert cert default to False.
2. Add a behavior flag require_certificate_validation, which defaults to False for now.
3. Display following warning in dbt logs if cert isn't explicitly specified and require_certificate_validation flag is set to False (or left at the default):

[WARNING]: SSL certificate validation is disabled by default. It is legacy behavior which will be changed in future releases. It is strongly advised to enable `require_certificate_validation` flag or explicitly set `cert` configuration to `True` for security reasons. You may receive an error after that if your SSL setup is incorrect.

4. Setting require_certificate_validation to True is equivalent to setting cert to True (if cert isn't specified explicitly).

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[mikealfare]

I don't know that this captures the logic that I was picturing. In my mind, it looks like this:

• if the user has set cert, take that setting
• if the behavior flag evaluates to true (in this case because the user set it to True since the default is False), then take whatever cert is (not the flag itself)
• if the behavior flag evaluates to false (could be the user did not set, or specifically set it to false) then set cert=False (the legacy default)

I think this also means the default setting above for cert should remain True.

[damian3031]

Currently, logic is as follows:

1. If cert is explicitly set to True, False, or a string containing a certificate path, that value is used, and the behavior flag is not taken into consideration.

   As I understand it, behavior flag is intended to control default behavior when cert is not set.

2. If cert is not set and the behavior flag is not set, the flag defaults to False. In this case, False is passed to Trino Python Client verify parameter, meaning SSL verification is disabled. This aligns with the default behavior before the 1.8.3 release.

3. If cert is not set but the behavior flag is set to True, True is passed to the TPC, enabling SSL verification. This is the behavior I would expect when using a flag named require_certificate_validation.
   Falling back to the cert value when the flag is explicitly set to True feels counterintuitive to me.

Does this logic make sense? Or do you think it needs to be adjusted?

[mikealfare]

As I understand it, behavior flag is intended to control default behavior when cert is not set.

Ah, I think this is where we differ. And it's more confusing since we're effectively using a boolean to set another boolean. The behavior flag indicates whether we are using the new behavior. The new behavior just so happens to line up with the behavior flag itself. So technically we should have (pseudocode):

if behavior_flag is False:
    default(cert) = None
elif behavior_flag is True:
    default(cert) = True

But since None and False are both "false-y" and we only check the bool of the value after checking whether the user supplied it, you're right that this could be represented as:

bool(default(cert)) == bool(behavior_flag)

Personally, I would still jump through the hoop to make it clear that the behavior flag and the cert default setting are not the same thing; perhaps I'm being pedantic. Either way, the current logic should work. Since this is yours to maintain, I'm good with it if you are.

[damian3031]

I can see it now, thanks! I’ve updated it so that the behavior flag now controls the default cert value. I also added a new test test_cert_default_value which checks the value that cert is set to for a specific behavior flag.

[Tonayya]

Hi @damian3031 hope you're well. With regards to scenarios where the behavior flag is not set and default is False, we're throwing a lot of warnings:

/venv/dbt-sha-fc7c96277b7e0eb5dd31174e88f42c79e0e26d5c/lib/python3.11/site-packages/urllib3/connectionpool.py:1099: InsecureRequestWarning: Unverified HTTPS request is being made to host 'starburst.somethingsomething'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings

Is there any way to implement a fix to silence these warnings? There are hundreds of them in each dbt Cloud job run and are rendering our console logs practically useless for the users.

[nguyenann13]

We have the same request as @Tonayya , these warnings are filling up our logs and make it impossible to read through the run metrics.

[Tonayya]

Hi @nguyenann13 this is being addressed in this PR. 😸

[damian3031]

@Tonayya @nguyenann13 We added the ability to silence InsecureRequestWarning in dbt-trino 1.8.5. It can be enabled by setting suppress_cert_warning: true in profiles.yml