Skip to content

TS-4624: use the server UUID in the Via header #804

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
zwoop merged 1 commit into from
Jul 18, 2016
Merged

TS-4624: use the server UUID in the Via header #804

zwoop merged 1 commit into from
Jul 18, 2016

Conversation

@yatsukhnenko
Copy link
Contributor

@yatsukhnenko yatsukhnenko commented Jul 17, 2016

If I correctly understood the task 😃

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

Almost, but not quite. :) All APIs that are prefixed TS are the public APIs, and we don't use those in the internal code. Look at the log tags in logging that uses the internal APIs for the UUID. E.g.

const char *uuid = (char *)Machine::instance()->uuid.getString();

Makes sense?

@zwoop zwoop self-assigned this Jul 17, 2016
@zwoop zwoop added this to the 7.0.0 milestone Jul 17, 2016
@zwoop zwoop added the HTTP label Jul 17, 2016
@yatsukhnenko
Copy link
Contributor Author

yatsukhnenko commented Jul 17, 2016

Don't quite understand why public API can't be used in internal code, but ok, changed code to Machine::instance()->uuid.getString()

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

It's an inefficiency for starters (you go through a C-> C++ wrapper layer, for no good reason (remember, the public APIs are all C, whereas in the core, you can use all the C++ objects and methods directly).

In this case, it's marginal, but there are other places where it would be inefficient. And, it would add dependencies to the C-APIs in the C++ core which are undesirable in some cases.

zwoop
zwoop reviewed Jul 17, 2016
View reviewed changes
proxy/http/HttpTransactHeaders.cc Outdated
*/
memcpy(via_string, Machine::instance()->ip_hex_string, Machine::instance()->ip_hex_string_len);
via_string += Machine::instance()->ip_hex_string_len;
if (uuid != NULL) {
Copy link
Contributor

@zwoop zwoop Jul 17, 2016

Choose a reason for hiding this comment

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

I don't think the machine uuid can ever be NULL, but harmless to check I guess :).

Copy link
Contributor Author

@yatsukhnenko yatsukhnenko Jul 17, 2016

Choose a reason for hiding this comment

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

Reinsurance :) Shoul I delete this check?

Copy link
Contributor

@zwoop zwoop Jul 17, 2016

Choose a reason for hiding this comment

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

Yeah nuke it IMO. The Machine UUID is guaranteed to be initiated on startup.

Copy link
Contributor Author

@yatsukhnenko yatsukhnenko Jul 17, 2016

Choose a reason for hiding this comment

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

done :)

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

[approve ci]

@atsci
Copy link

atsci commented Jul 17, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/439/ for details.

@atsci
Copy link

atsci commented Jul 17, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/333/ for details.

@yatsukhnenko
Copy link
Contributor Author

yatsukhnenko commented Jul 17, 2016

Thanks for explaining about API usage

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

[approve ci]

@atsci
Copy link

atsci commented Jul 17, 2016

FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/441/ for details.

@atsci
Copy link

atsci commented Jul 17, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/335/ for details.

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

I suspect this failed for other reasons, trying again [approve ci].

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

I checked the core on this FreeBSD build, and it seems completely unrelated to pretty much anything :).

(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00000000006ec34a in EThread::process_event (this=0x804107000, e=0x803f2c880, calling_code=<value optimized out>) at I_Continuation.h:153
#2  0x00000000006ec8ec in EThread::execute (this=0x804107000) at ../../../iocore/eventsystem/UnixEThread.cc:245
#3  0x00000000006ebb82 in spawn_thread_internal (a=0x803ef5700) at ../../../iocore/eventsystem/Thread.cc:84
#4  0x00000008027514f5 in pthread_create () from /lib/libthr.so.3
#5  0x0000000000000000 in ?? ()

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016

@yatsukhnenko What do you think of a future feature (config) to disable this Via checking completely? If I know there's not chance of a loop, why bother checking the Via header? I guess one could just nuke the Via header on the incoming request, but there might be other reasons to keep it?

This is completely unrelated to this PR though, just thinking out loud.

@atsci
Copy link

atsci commented Jul 17, 2016

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/443/ for details.

@atsci
Copy link

atsci commented Jul 17, 2016

Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/337/ for details.

@zwoop
Copy link
Contributor

zwoop commented Jul 17, 2016 •
edited
Loading

Did a quick test of this, and it looks good:

Via: http/1.1 hostname[f6f5162d-5a02-4d10-ac6e-35aede508fe5] (ApacheTrafficServer/7.0.0)

@yatsukhnenko
Copy link
Contributor Author

yatsukhnenko commented Jul 18, 2016

What do you think of a future feature (config) to disable this Via checking completely? If I know there's not chance of a loop, why bother checking the Via header?

@zwoop, makes sense. Should I create new jira for this?

@zwoop
Copy link
Contributor

zwoop commented Jul 18, 2016

Yeah, file a Jira probably. I could have sworn we've had issues before where loop detection kicked in when it shouldn't (e.g. you legitimately proxy to yourself). But I can find an issue on it, maybe you can take a look too.

@zwoop zwoop merged commit 66ab52e into apache:master Jul 18, 2016
@yatsukhnenko
Copy link
Contributor Author

yatsukhnenko commented Jul 18, 2016

This one?

@yatsukhnenko yatsukhnenko deleted the TS-4624 branch July 18, 2016 20:13
JosiahWI pushed a commit to JosiahWI/trafficserver that referenced this pull request Jul 19, 2023
Add support for tunnel route config that has both match group and por…
3657e5a 
…t variable (apache#9594) (apache#804)

Conflicts:
	doc/admin-guide/files/sni.yaml.en.rst
	iocore/net/P_SNIActionPerformer.h
	tests/gold_tests/tls/tls_tunnel.test.py

Co-authored-by: Zhengxi Li <zli11@yahooinc.com>
masaori335 pushed a commit to masaori335/trafficserver that referenced this pull request May 29, 2025
[10.0.x] Upgrade el9 docker image (apache#804)
13c5b33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@zwoop zwoop

Labels

HTTP

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@yatsukhnenko @zwoop @atsci