Skip to content
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

Add Unix Domain Socket support to RequestsWrapper #7585

Merged
merged 8 commits into from
Sep 16, 2020

Conversation

florimondmanca
Copy link
Contributor

@florimondmanca florimondmanca commented Sep 15, 2020

What does this PR do?

Add support for performing HTTP requests to unix://... URLs to RequestsWrapper.

Example:

url = "unix:///var/run/docker.sock"
response = self.http.get(url)

Note: Contrary to what I thought initially, in the end we don't need the user to take any action re: persist_connections. We automatically use the shared session in this case, since we need to mount the adapter on a session and this is not supported by just using the requests top-level API as we do in the default non-persisted case. There's work planned to pay off this bit of tech debt so we build a session in the same way in both cases (more context in #7579 and #7582).

Motivation

Required for the upcoming IoT Edge integration (#7465), and currently marked as TODO in our developer docs.

Additional Notes

QA notes

Note: please QA this on Windows too. If you don't have a Windows machine, either use a Cloud VM (Azure) or kindly pass the card along to someone who has when you're done. 🙏

There's a bug in Docker that prevents sharing Unix sockets between the host and a container (see docker/for-mac#483). This means doing integration/e2e tests on macOS is not possible (only possible on Linux or Windows). If you want to save yourself from spinning up a Linux/Windows VM, here's on way to test this feature using the macOS host Agent…

First, install the macOS RC build on your machine.

Then, run this script to start a local UDS server (Python 3 only):

# Install dependencies: $ pip install uvicorn starlette
import uvicorn
from starlette.responses import PlainTextResponse

app = PlainTextResponse("Hello, World!")

if __name__ == "__main__":
    uvicorn.run(app, uds="/tmp/test.sock")

Verify the server works:

$ curl --unix-socket /tmp/test.sock http://localhost
Hello, World!

Add a custom check that performs an HTTP request to the UDS endpoint:

# ~/.datadog-agent/checks.d/hello.py
from datadog_checks.base import AgentCheck

__version__ = "1.0.0"


class HelloCheck(AgentCheck):
    def check(self, instance):
        r = self.http.get('unix:///tmp/test.sock')
        assert r.text == "Hello, World!"
        self.service_check('hello.can_connect', self.OK, message=r.text)
# ~/.datadog-agent/conf.d/hello.yaml
instances: [{}]

Run a check and make sure it succeeds:

$ datadog-agent check hello
=== Service Checks ===
[
  {
    "check": "my_http.can_connect",
    "host_name": "UPPERlower.com",
    "timestamp": 1600275835,
    "status": 0,
    "message": "",
    "tags": []
  }
]
...

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

mgarabed
mgarabed previously approved these changes Sep 16, 2020
Copy link
Member

@mgarabed mgarabed left a comment

Choose a reason for hiding this comment

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

LGTM!

@florimondmanca florimondmanca merged commit cf4c040 into master Sep 16, 2020
@florimondmanca florimondmanca deleted the fm/http-unixsocket branch September 16, 2020 16:22
github-actions bot pushed a commit that referenced this pull request Sep 16, 2020
* Add Unix Domain Socket support to RequestsWrapper

* Sync config files

* Sync dep files

* Fix env var

* Add test for already-quoted URLs

* Auto-use persisted session if unix:// URL was given

* Lint

* Skip integration test on Windows cf4c040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants