STS QoS Part 2 changes - Extract stats to perform QoS from haproxy to syslog server - #41
STS QoS Part 2 changes - Extract stats to perform QoS from haproxy to syslog server #41rohit2219 wants to merge 0 commit into
Conversation
jacquesh
left a comment
jacquesh
left a comment
There was a problem hiding this comment.
Can we also please write up a decent explanation of how the dynamic key extraction works and fits into the rest of the system (in docs/ I mean)?
| ntohs(addr_in->sin_port), LOG_DELIMITER, access_key, LOG_DELIMITER, | ||
| (data_direction == RL_DOWNLOAD ? "dwn" : "up"), LOG_DELIMITER, done); | ||
|
|
||
| if (sts_transaction_key) { |
There was a problem hiding this comment.
Why do we need a separate STS-specific dataxfer message? This is new to me.
There was a problem hiding this comment.
Let me think a bit more on this
There was a problem hiding this comment.
Logic changed to align with existing code. Check now
| struct htx* htx = htxbuf(&msg->chn->buf); | ||
| struct http_hdr_ctx ctx = {.blk = NULL}; | ||
|
|
||
| if (http_find_header(htx, ist("x-amz-security-token"), &ctx, 0) && (ctx.value.len > 0)) { |
There was a problem hiding this comment.
If we agree that the lua for extracting these belongs in weir-s3.lua, then this cannot live here because it is not generic across APIs. My understanding from our previous conversation was that we were going to pass this in as a parameter via the config (which can then evaluate some lua code to extract the key).
There was a problem hiding this comment.
Thats what I missed . Let me try to fix these and get back . This is precisely what i intended to catch when I raised the PR
| memcpy(token_copy, sts_token.ptr, (size_t)sts_token.len); | ||
| ha_free(&st->sts_token); | ||
| st->sts_token = token_copy; | ||
| send_log(NULL, LOG_INFO, "req_ststoken~|~%s:%d~|~%.*s~|~%s~|~%s~|~%s~|~%d~|~%s", |
There was a problem hiding this comment.
Why do we need a separate message instead of adding the sts token to the regular req log above? The existing parsing should support extra fields and if it doesn't....we should probably make it do that.
There was a problem hiding this comment.
Hmm interesting thought (Let me also think a bit more if thats possible).. My intention was to separate the STS token and the access key rate limiting logics completely as they are two different problems and need separate handling..
There was a problem hiding this comment.
For that alone, I just decided to keep it that way . Either way, performance wise there is no difference. But i believe the logic is better separated this way and looks cleaner.
rohit2219
changed the title
rohit2219
changed the title
e7e8d52 to
3e8e0e7
Compare
3 participants
Description
This will extract the STS Token + verb and bandwidth usage(no active request yet) from haproxy till syslog server. It wont push the token or usage stats to redis yet. That will be another PR. Raising small PR's for easier review.
Please provide a meaningful description of what this change will do, or is for.
Bonus points for including links to related issues, other PRs, or technical
references.
Note that by not including a description, you are asking reviewers to do extra
work to understand the context of this change, which may lead to your PR taking
much longer to review, or result in it not being reviewed at all.
Type of Change
✅ New Feature
Checklist
✅ I have read the contributing guidelines
✅ Existing issues have been referenced (where applicable)
✅ I have verified this change is not present in other open pull requests
✅ Functionality is documented
✅ New code contribution is covered by automated tests