Skip to content

Issue 20457/http certificate check via proxy #20473

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Issue 20457/http certificate check via proxy #20473

wants to merge 6 commits into from

Conversation

mvanholsteijn
Copy link

What does this PR do?

support for HTTP certificate check via proxy

Motivation

I have the problem in our production environment.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@mvanholsteijn mvanholsteijn requested a review from a team as a code owner June 9, 2025 20:28
@mvanholsteijn mvanholsteijn force-pushed the issue-20457/http-certificate-check-via-proxy branch 4 times, most recently from 051f271 to b74c965 Compare June 10, 2025 15:54
@codecov Codecov
Copy link

codecov bot commented Jun 10, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.13%. Comparing base (f6efbf3) to head (989eb1f).
Report is 92 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
confluent_platform ?
hive ?
hivemq ?
http_check 94.73% <100.00%> (+0.47%) ⬆️
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
mac_audit_logs ?
presto ?
solr ?
tomcat ?
weblogic ?

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mvanholsteijn mvanholsteijn force-pushed the issue-20457/http-certificate-check-via-proxy branch from b74c965 to 2ffe409 Compare June 11, 2025 08:12
dkirov-dd
dkirov-dd requested changes Jul 8, 2025
View reviewed changes
Copy link
Contributor

@dkirov-dd dkirov-dd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @mvanholsteijn, thank you for your contribution!
The changes look good overall, I just have a few comments. 😃

http_check/tests/compose/docker-compose.yml
Comment on lines -28 to +36
image: mashape/mockbin
image: dockette/mockbin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

http_check/tests/compose/.curlrc Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this file is needed as curl isn't used anywhere.
Correct me if I'm wrong 🤔

Copy link
Author

@mvanholsteijn mvanholsteijn Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file will make it very easy for the developer to use curl to test the setup via the docker compose services with and without proxy

@mvanholsteijn @dkirov-dd
Update http_check/datadog_checks/http_check/http_check.py
32d8248 
Co-authored-by: dkirov-dd <166512750+dkirov-dd@users.noreply.github.com>
dkirov-dd
dkirov-dd requested changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@dkirov-dd dkirov-dd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!
I just have a few small requests remaining.
Also, could you resolve the conflict? I think you just need to bump the cryptography version.
Thanks again for the contribution!

http_check/tests/compose/docker-compose.yml

redis:
container_name: mockbin_redis
image: redis

mockbin:
container_name: mockbin
image: mashape/mockbin
image: dockette/mockbin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
image: dockette/mockbin
image: mashape/mockbin

If there is no reason for this change, we can revert it.

http_check/tests/compose/docker-compose.yml
ports:
- "8080:8080"
- "8080:8000"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- "8080:8000"
- "8080:8080"

If there is no reason for this change, we can revert it.

http_check/tests/compose/.curlrc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification on the use cases for this file!
I personally prefer to avoid manual testing when it can be included in the test files.
Could you please remove the file from the PR. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkirov-dd dkirov-dd

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
agent/requested-changes ecosystems/review-requested integration/http_check product/review-requested team/agent-integrations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvanholsteijn @dkirov-dd