Skip to content

http(): templated url("") #4663

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
Merged
merged 15 commits into from
Nov 6, 2023
Merged

http(): templated url("") #4663

merged 15 commits into from
Nov 6, 2023

Conversation

alltilla
Copy link
Collaborator

@alltilla alltilla commented Oct 3, 2023

This PR does not implement generating metrics dynamically based on the resolution of the url, but we should do so in the future.

Signed-off-by: Attila Szakacs attila.szakacs@axoflow.com

alltilla added a commit to alltilla/syslog-ng that referenced this pull request Oct 3, 2023
@alltilla
news: add entry for syslog-ng#4663
65e3713 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla force-pushed the http-templated-url branch from e961adc to 65e3713 Compare October 3, 2023 13:22
@MrAnno
Copy link
Collaborator

MrAnno commented Oct 5, 2023 •
edited
Loading

We've discussed the security implications of this PR privately, I think a few points need to be addressed before merging it.

@bazsi bazsi mentioned this pull request Oct 5, 2023
Closed
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Oct 24, 2023
@alltilla
news: add entry for syslog-ng#4663
333f798 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla force-pushed the http-templated-url branch 2 times, most recently from c668be7 to e74033b Compare October 24, 2023 08:30
@kira-syslogng
Copy link
Contributor

Build FAILURE

@alltilla alltilla requested a review from MrAnno October 30, 2023 07:44
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Oct 30, 2023
@alltilla
news: add entry for syslog-ng#4663
f17764b 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Oct 30, 2023
@alltilla
news: add entry for syslog-ng#4663
059030c 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@MrAnno
Copy link
Collaborator

MrAnno commented Oct 30, 2023

I'll review this tomorrow! Thanks

@MrAnno
Copy link
Collaborator

MrAnno commented Nov 1, 2023

I think this is still missing one step that we discussed with Bazsi: we should parse the beginning of the URL and we should not allow macros coming from message in the schema or the domain part of the URI (not even in escaped form). Note that the SCL use-case of the parameterized URL is different than the runtime unsafe templating feature. We agreed that we don't want to support templated schemas or domains at first, and even if we eventually wanted to do such things, we would mark it as unsafe with a special syntax.

Can you explain why "flushing on worker key change" is forced in this PR, please? (I originally thought this functionality depends solely on the needs of the actual destination.)

LGTM otherwise

alltilla added a commit to alltilla/syslog-ng that referenced this pull request Nov 2, 2023
@alltilla
news: add entry for syslog-ng#4663
ea3d7c5 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
Copy link
Collaborator Author

alltilla commented Nov 2, 2023 •
edited
Loading

Can you explain why "flushing on worker key change" is forced in this PR, please? (I originally thought this functionality depends solely on the needs of the actual destination.)

Regarding the http driver, I think the option is only useful for templated urls, and have no effect otherwise. I wanted to make it easier to set templates in urls, so I set it to yes by default for the http driver.

we should parse the beginning of the URL and we should not allow macros coming from message in the schema or the domain part of the URI (not even in escaped form).

Done, see http: disallow setting templates to various parts of URL

@kira-syslogng
Copy link
Contributor

Build FAILURE

alltilla added a commit to alltilla/syslog-ng that referenced this pull request Nov 2, 2023
@alltilla
news: add entry for syslog-ng#4663
7625175 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@kira-syslogng
Copy link
Contributor

Build FAILURE

MrAnno
MrAnno previously approved these changes Nov 2, 2023
View reviewed changes
Copy link
Collaborator

@MrAnno MrAnno left a comment

Choose a reason for hiding this comment

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

Thank you very much!

alltilla added a commit to alltilla/syslog-ng that referenced this pull request Nov 2, 2023
@alltilla
news: add entry for syslog-ng#4663
eddb937 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
alltilla added a commit to alltilla/syslog-ng that referenced this pull request Nov 2, 2023
@alltilla
news: add entry for syslog-ng#4663
85ef024 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@kira-syslogng
Copy link
Contributor

Build FAILURE

1 similar comment
@kira-syslogng
Copy link
Contributor

Build FAILURE

alltilla and others added 12 commits November 3, 2023 15:04
@alltilla
http: pass url instead of target in internal functions
26347aa 
This prepares easier transition to templated urls.

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: add template support for load-balancer
653ee8a 
Currently the http-worker is only using the literal functionality.
Implementing support there will be done in the following commits.

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: add helper functions to load balancer
9073a1b 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: support templated url("")
993828f 
If we are batching, the first message of the batch will be used
for the template evaluation.

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: turn on flush-on-worker-key-change() by default
1cc9bc3 
I believe the only use case of using worker-partition-key() in the
http() driver is when we are configuring a templated url("").

In that scenario we should be flushing on worker key change.

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@bazsi @alltilla
http: add support for escaped url("") templates
1b847a6 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@bazsi @alltilla
http: add support for adding the worker ID as $_ in url templates
4247832 
Signed-off-by: Balazs Scheidler <balazs.scheidler@axoflow.com>
@alltilla
http: support space in templated urls
b507103 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
templates: classify escaped literal string templates as literal
a5099ee 
Escaping does not have an effect on literal strings, so we can still
consider them literals.

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: disallow setting templates to various parts of URL
6406a5d 
For safety reasons we do not let the user setting templates to these
parts:
  * scheme
  * host
  * port
  * user
  * password

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
news: add entry for syslog-ng#4663
9ebd836 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
compat: add curl.h
d49a677 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: do not check templated url safety where API is not available
746a186 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
http: do not check url template safety if no templates are used
67f0cd8 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla
Copy link
Collaborator Author

alltilla commented Nov 3, 2023

Thanks for the review! I have updated the PR and I hope it works now :/

@alltilla alltilla force-pushed the http-templated-url branch 2 times, most recently from 036c105 to 344c541 Compare November 6, 2023 09:17
@alltilla
Copy link
Collaborator Author

alltilla commented Nov 6, 2023 •
edited
Loading

Finally, it seems like it works :) 9e5a957 was needed for cmake.

@kira-syslogng
Copy link
Contributor

Build FAILURE

@alltilla
cmake: inherit dependencies' target compile defs in add_unit_test()
9e5a957 
Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>
@alltilla alltilla requested a review from MrAnno November 6, 2023 11:05
@MrAnno
Copy link
Collaborator

MrAnno commented Nov 6, 2023

Thank you. I'll merge it in 10 minutes, I'll just try it once again :)

@MrAnno
Copy link
Collaborator

MrAnno commented Nov 6, 2023

($ escaping doesn't work in the validation method, but I don't see a real use-case there)

@MrAnno MrAnno merged commit 2a59528 into syslog-ng:master Nov 6, 2023
@czanik czanik mentioned this pull request Nov 8, 2023
Closed
@eldarnash eldarnash mentioned this pull request Nov 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@MrAnno MrAnno MrAnno approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alltilla @MrAnno @kira-syslogng @bazsi