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

feat: use test host name for marker if localhost alias #370

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fzipi
Copy link
Member

@fzipi fzipi commented Oct 12, 2024

what

  • use test's host destination if an alias for localhost

why

  • allow to test using different logfiles

caveats

Fixes #361

Comment on lines +238 to +240
if err != nil {
return nil, fmt.Errorf("ftw/run: can't resolve destination %+v: %w", dest, err)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know if this even make sense. Maybe just ignoring the error and moving on?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it does. Running the test makes no sense because resolution will fail again.

@fzipi fzipi requested a review from theseion October 12, 2024 14:32
@theseion
Copy link
Collaborator

What's the use case?

@theseion
Copy link
Collaborator

theseion commented Nov 7, 2024

Ok, I understand the use case.

@fzipi fzipi marked this pull request as ready for review November 7, 2024 22:59
Comment on lines +238 to +240
if err != nil {
return nil, fmt.Errorf("ftw/run: can't resolve destination %+v: %w", dest, err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it does. Running the test makes no sense because resolution will fail again.

}
// we only take the first IP address
if net.ParseIP(addrs[0]).IsLoopback() {
host = dest.DestAddr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apart from the comment, I think this should be set to the host entry of the current stage. The Host header will be the deciding factor for choosing the location / virtual host, so if we change the host here to mylocalhostalias but the test request has a host name of anotheralias, the two requests will end up in different locations / virtual hosts.

Suggested change
host = dest.DestAddr
// With multiple locations / virtual hosts, logs can be written to different files,
// in which case the internal requests for marking and flushing must end up in the
// same log file (host name match).
host = dest.DestAddr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marker requests' Host header is always "localhost"
2 participants