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

Revalidation for ECN #4037

Closed
wants to merge 1 commit into from
Closed

Revalidation for ECN #4037

wants to merge 1 commit into from

Conversation

martinthomson
Copy link
Member

It's OK to try to validate ECN again if it fails.

This removes duplicate text and replaces it with a forward reference.

Closes #4028.

It's OK to try to validate ECN again if it fails.

This removes duplicate text and replaces it with a forward reference.

Closes #4028.
@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -transport labels Aug 20, 2020
@@ -3892,6 +3889,7 @@ errors are detected.
Endpoints validate ECN for packets sent on each network path independently. An
endpoint thus validates ECN on new connection establishment, when switching to a
server's preferred address, and on active connection migration to a new path.
If validation fails, an endpoint could also periodically attempt validation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a could. Ignoring ECN is inappropriate and dangerous to the health of other flows. (If a flow received CE-marks it could reasonably assume ECN is working, if it see nothing it needs at some point to recheck this works.)

I'd expect this to be needed: Why therefore is this not a SHOULD?

Copy link
Contributor

Choose a reason for hiding this comment

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

An endpoint receiving CE marks should always feed them back to the sender. Validation is about actively marking packets as ECT by the sender. However, I think it would be nice to be more explicit and add a reference to the respective section at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a could and not a SHOULD because making a recommendation would require us to make that recommendation actionable. That means describing the circumstances under which revalidation is desirable.

I realize that not all routes are static, but you would have to establish a case for revalidation in order to make any sort of statement with normative force.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear: I suggest we keep the text that says: "An endpoint validates the use of ECN on the path, both during connection establishment and when migrating to a new path ({{migration}})."

... Although the simplest way to do this is to monitor an increase in the ECN counters reported.

I suspect there have been too many threads/issues related to ECN text (some have diverged and some concluded with no action) and those people (me included) involved should now read a clean version of the ECN text. I do not expect this text to be hard to finalise.

Copy link
Member Author

Choose a reason for hiding this comment

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

This appears immediately above:

It is possible for faulty network devices to corrupt or erroneously drop packets with ECN markings. To provide robust connectivity in the presence of such devices, each endpoint independently validates ECN counts and disables ECN if errors are detected.

This would seem to suffice.

during connection establishment and when migrating to a new path
({{migration}}).
and the peer is able to access the ECN codepoint in the IP header; see
{{ecn-validation}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the new text is wrong. Support by the endpoint isn't sufficient. The path validation is required to use ECN, and this spec needs to say this for each new path.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also in favour or keep this text here. It's a bit of redundant but I think it's useful to make this clear from the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how this text contradicts. Given that it is old, all I'm reading from this feedback is a preference to keep text that better belongs in the referenced section. And that section already concentrates on the path aspects (dropping, corruption).

@martinthomson martinthomson linked an issue Aug 25, 2020 that may be closed by this pull request
@martinthomson
Copy link
Member Author

Plan is to merge #4059 instead.

@martinthomson martinthomson deleted the ecn-any-time branch September 24, 2020 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Try again to use ECN? Allowing QUIC to suspend ECN usage
4 participants