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.

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

Add config option for skipping hostname resolution in Gelf logging #25674

Merged
merged 1 commit into from
May 19, 2022
Merged

Add config option for skipping hostname resolution in Gelf logging #25674

merged 1 commit into from
May 19, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented May 19, 2022

Relates to: #25643

@geoand geoand mentioned this pull request May 19, 2022
Closed
yazinnnn
yazinnnn reviewed May 19, 2022
View reviewed changes
...sions/logging-gelf/runtime/src/main/java/io/quarkus/logging/gelf/GelfLogHandlerRecorder.java
final JBoss7GelfLogHandler handler = new JBoss7GelfLogHandler();
if (config.skipHostnameResolution) {
if (previousSkipHostnameResolution == null) {
System.clearProperty(PROPERTY_LOGSTASH_GELF_SKIP_HOSTNAME_RESOLUTION);
Copy link

Choose a reason for hiding this comment

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

when previousSkipHostnameResolution is null, it shouldn't clear the property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not true

Copy link

Choose a reason for hiding this comment

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

ok, it works anyway.

@geoand geoand requested a review from loicmathieu May 19, 2022 08:46
@geoand geoand marked this pull request as ready for review May 19, 2022 08:46
@loicmathieu
Copy link
Contributor

Two questions:

  • Why do you set back the system property ? If it's only used by the logging-gelf library it seems pointless
  • I don't really understand the impact of setting originHost and what it does ? Can this be a secrity risk ?

@geoand
Copy link
Contributor Author

geoand commented May 19, 2022

Why do you set back the system property ? If it's only used by the logging-gelf library it seems pointless

The idea is that we should never taint the environment unless it's necessary.

I don't really understand the impact of setting originHost and what it does ? Can this be a secrity risk ?

Not sure, but why do you think it's a security issue? FWIW, none of this new stuff is enabled by default.

loicmathieu
loicmathieu approved these changes May 19, 2022
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

OK for me, your answers make sense

@geoand geoand merged commit aa8ba94 into quarkusio:main May 19, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 19, 2022
@geoand geoand deleted the #25643 branch May 19, 2022 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yazinnnn yazinnnn yazinnnn left review comments

@loicmathieu loicmathieu loicmathieu approved these changes

Assignees
No one assigned
Labels
area/logging
Projects
None yet
Milestone
2.10.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@geoand @loicmathieu @yazinnnn