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

Don't retry from retry #616

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Don't retry from retry #616

merged 5 commits into from
Aug 5, 2024

Conversation

ekr
Copy link
Collaborator

@ekr ekr commented May 27, 2024

This was split off from PR#609. Hopefully this is uncontroversial.

Comment on lines 888 to 889
Clients SHOULD NOT accept "retry_config" in response to
a connection initiated in response to a "retry_config".
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a little more color to add here?

This is mostly about the case where a retry config is supplied by one server instance, but other server instances don't work. That is a server configuration error, unless the client decided to keep the retry config for too long, in which case it is a signal that the client shouldn't hold on to old state.

Adding that sort of context might make the SHOULD a little more useful.

@ekr
Copy link
Collaborator Author

ekr commented May 27, 2024

I added some text.

in this situation is a signal that the server is misconfigured, e.g.,
the server might have multiple inconsistent configurations so that the
client reached a node with configuration A in the first connection and
a node with configuration B in the second. If a client does not retry,

Choose a reason for hiding this comment

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

The previous paragraph also talks about a "retry". But that is an ECH disabled retry.
In this paragraph, based on the context, it is referring to an ECH enabled retry.
Should the client be allowed to perform an ECH disabled retry here? And should anything about that, allowed or not allowed, be noted in this paragraph also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is a reasonable question. Updated the text.

Note that I also broke out the requirement to report the error separately as I think it applies to both cases.

@ekr
Copy link
Collaborator Author

ekr commented Aug 3, 2024

@martinthomson?

Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

What is the merge conflict?

@ekr
Copy link
Collaborator Author

ekr commented Aug 5, 2024

I had two retry change PRs off the same place and the first one merged.

@ekr ekr merged commit ba609f6 into tlswg:master Aug 5, 2024
1 check passed
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.

3 participants