Update Redis Enterprise to set tls_verify to false by default in check code (ECOINT-109)

[mitcherthewitcher]

What does this PR do?

A brief description of the change being made with this pull request.

This PR explicitly sets the default for tls_verify to False in check code.

Motivation

What inspired you to submit this pull request?

I encountered a scenario where I overwrote the default conf.yaml that is generated during integration installation and provided a minimal check configuration to redis_enterprise.

I provided the following instance configuration:

instances: 
  - openmetrics_endpoint: https://localhost:8071/metrics
    skip_proxy: true

Per the default conf.yaml, I expected tls_verify to be equal to False when running the check, but encounter SSL errors in agent status.

These errors are resolved by updating our config

instances: 
  - openmetrics_endpoint: https://localhost:8071/metrics
    skip_proxy: true
    tls_verify: false

I am proposing we update the check code to explicitly use self.instance.get("tls_verify", False) to make the integration behavior consistent and align with documentation.

I added a unit test to illustrate this. Please run ddev test redis_enterprise and observe the passing tests. Then comment out this change and rerun ddev test redis_enterprise and observe test failure.

Review checklist

• PR has a meaningful title or PR has the no-changelog label attached
• Feature or bugfix has tests
• Git history is clean
• If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo
• If this PR includes a log pipeline, please add a description describing the remappers and processors.

Additional Notes

Anything else we should know when reviewing?

I don't exactly like the idea of having insecure SSL be the default behavior for the check.

This in mind, a more security conscious alternative to the above is still setting a default in code with, self.instance.get("tls_verify", True), only this time we set default to True, and then make updates to spec.yaml and documentation to reflect this. Happy to proceed with either option. 😄

[iliakur]

I left some minor comments.

Since this is a breaking change we should ask the maintainers what they think.
Please reach out to [email protected]

[mitcherthewitcher]

Error contacting maintainers

I attempted emailing the maintainers [email protected] as requested and my email bounced back.

Hopefully mentioning @redis-field-engineering will get some eyes on the PR from their end.

Original email request

Hello Redis Engineering (@redis-field-engineering),

I hope you're doing well.

I'm reaching out regarding PR #2607, which introduces a breaking change to the Redis Enterprise Datadog integration. Given the potential impact, I wanted to get your input on the approach and any concerns you might have before moving forward.

Could you take a look and share your thoughts? If there's anything we should adjust to ensure a smooth transition for users, I'd really appreciate your feedback.

Looking forward to your insights.

[dd-dominic]

Installation command in readme also needs to be updated with correct version

[jeremyplichta]

Thank you @mitcherthewitcher and @dd-dominic for reviewing and coordinating these changes. If a user goes through the trouble of setting up a tls connection to Redis they would likely expect the authenticity of that connection to be verified unless they explicitly specify tls_verify:false in the config.

Having this value default to false when not specified (even though its stated in documentation) seems like it would be similar to if curl defaulted to specifying -k by default, which would not be desirable.

@mitcherthewitcher - would it be possible to rework this PR and the tests so that the documentation is changed to correctly specify that the default value for tls_verify is true? This seems like the right thing to do security-wise. I think it's unlikely to break users as this is how it works in the code and the default config.yaml shows using tls_verify: false

Last comment here, it seems like mentioning @redis-field-engineering did not actually notify us. We are also working on updating the permissions on that email list you got a bouncer back on. In the future mentioning @jeremyplichta and @j8-redis will be the best way to go. We will also work with DataDog to see if we can get these users added to a CODEOWNERS file for this repo and subpath so we are automatically included in these PRs going forward.

[mitcherthewitcher]

@mitcherthewitcher - would it be possible to rework this PR and the tests so that the documentation is changed to correctly specify that the default value for tls_verify is true? This seems like the right thing to do security-wise. I think it's unlikely to break users as this is how it works in the code and the default config.yaml shows using tls_verify: false

@jeremyplichta Happy to make these changes. Is it still necessary to do a 2.0.0 breaking change release for this or would 1.1.2 be sufficient?

[jeremyplichta]

@mitcherthewitcher - would it be possible to rework this PR and the tests so that the documentation is changed to correctly specify that the default value for tls_verify is true? This seems like the right thing to do security-wise. I think it's unlikely to break users as this is how it works in the code and the default config.yaml shows using tls_verify: false

@jeremyplichta Happy to make these changes. Is it still necessary to do a 2.0.0 breaking change release for this or would 1.1.2 be sufficient?

Yes, I think 1.1.2 sounds sufficient. @j8-redis - thoughts?