[CDRIVER-4454] Azure auto-KMS testing#1104
Conversation
- A timeout during a partial read is still an error. - Prevent a slow server from trickling data and causing an eternal wait (Keep track of time while reading) - Reject very large HTTP responses.
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
Nice, LGTM with an update to the timeout handling and documentation for the error constants.
I suggest moving mock server to drivers-evergreen-tools to make it accessible to other drivers when adding specification tests.
| template engines - all in a single file and with no dependencies other than the | ||
| Python Standard Library. | ||
|
|
||
| Homepage and documentation: http://bottlepy.org/ |
There was a problem hiding this comment.
Neat, I did not know about bottle.
When moving this to drivers-evergreen-tools, consider using Flask. The mock OSCP server depends on Flask.
There was a problem hiding this comment.
I chose Bottle here because it is a single-file library that does not required additional dependencies. Flask etc will require a form of external dependency management. I'm fine doing external dev/CI Python dependencies, but I would like for there to be a more reliable development/CI workflow then is currently present.
build/fake_azure.py
Outdated
| if TYPE_CHECKING: | ||
| from typing import Protocol | ||
|
|
||
| class _RequstParams(Protocol): |
There was a problem hiding this comment.
| class _RequstParams(Protocol): | |
| class _RequestParams(Protocol): |
build/fake_azure.py
Outdated
| try: | ||
| return fn() | ||
| except AssertionError as e: | ||
| traceback.format_exc() |
There was a problem hiding this comment.
| traceback.format_exc() | |
| traceback.print_exc() |
| MONGOC_ERROR_CLIENT_SIDE_ENCRYPTION, /* An error coming from libmongocrypt */ | ||
| MONGOC_ERROR_POOL | ||
| MONGOC_ERROR_POOL, | ||
| MONGOC_ERROR_AZURE, |
There was a problem hiding this comment.
Please update the errors.rst with the new domain and value.
| // +1 to prevent passing zero as a timeout | ||
| mcd_get_milliseconds (mcd_timer_remaining (timer)) + 1, | ||
| &host_list, | ||
| error); |
There was a problem hiding this comment.
Good catch. If _mongoc_http_send requires a non-zero timeout, I suggest checking the timeout before each blocking call:
#define CHECK_TIMEOUT(msg) \
if (mcd_get_milliseconds (mcd_timer_remaining (timer)) == 0) { \
bson_set_error (error, \
MONGOC_ERROR_STREAM, \
MONGOC_ERROR_STREAM_SOCKET, \
"Timeout before %s: %s", \
msg, \
req->host); \
goto fail; \
}
CHECK_TIMEOUT ("connect")mongoc_stream_tls_handshake_block interprets 0 as "no timeout".
There was a problem hiding this comment.
Looking into this, there's a several goofs in timing that may be a problem, e.g. an unguarded multiply of timeout_ms * 1000 that can overflow. Requiring the timeout to not be zero is strange behavior.
I also realize now that asserting on overflow in the duration code is probably bad (for example, one might want mcd_seconds(INT64_MAX) to become a maximum duration, rather than terminate the program, and using assertions implies that the caller is supposed to know what the time encoding is).
| return mcd_azure_access_token_from_imds (out, | ||
| NULL, // Use the default host | ||
| 0, // Default port as well | ||
| NULL, // No extra headres |
There was a problem hiding this comment.
| NULL, // No extra headres | |
| NULL, // No extra headers |
|
@kevinAlbs I did some changes to the time arithmetic to be saturating rather than asserting (See this commit). It shouldn't have any effect on the behavior of the in most cases, but I feel better about it. Could you take a quick look at that change? |
src/libmongoc/src/mongoc/mcd-time.h
Outdated
|
|
||
| /// Convert an abstract duration to a number of milliseconds | ||
| /** | ||
| * @brief Obtain the cound of full milliseconds encoded in the given duration |
2 participants
Refer: CDRIVER-4454
This changeset adds a simple IMDS-mimicking HTTP server based on Bottle. Further test cases and behaviors can be added to this server in the future. This also includes addition tweaks to the HTTP API and a timer API. (This changeset builds upon #1103)
Individual commits can be viewed in order.