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

Clarify paramers_set merging of parameter values #409

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rmarx
Copy link
Contributor

@rmarx rmarx commented Mar 6, 2024

Closes #398.

PTAL @hlandau.

The suggestion of using null as a JSON value in #398 makes conceptual sense, but not really since we're working with CDDL to define the events and their fields, as it doesn't easily allow to make a type "nullable" for something like this.

As such, I changed the recommendation to explicitly setting the default value if needed; I hope that's acceptable.

For the rest, I mostly just used your text with minor editorial changes.

- The local endpoint receives parameter values from the peer and logs a
`parameters_set` event with an `owner` of `remote` reflecting the desired
value(s) expressed by the peer.
- The local endpoint logs a `parameters_set` event with an `owner` of `local`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this very confusing. Why do we need to log anything at all? There are not many parameters that are negotiated (idle timeout. anything else?).

If we really want to log this, let's create a separate owner value for that.

Copy link

Choose a reason for hiding this comment

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

No objection to a separate owner value

Copy link
Collaborator

Choose a reason for hiding this comment

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

negotiated? I don't think this is super valuable to log, since it can easily be derived from the two parameters_set events anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm still not 100% sure what we're proposing here...

Option A:

  • log parameters_set with owner=local for local value
  • log parameters_set with owner=remote for peer's value
  • log parameters_set with owner=negotiated for final value

or Option B:

  • log parameters_set with owner=negotiated for local value
  • log parameters_set with owner=negotiated for peer's value
  • log parameters_set with owner=negotiated for final value

or Option C:

  • log parameters_set with owner=negotiated final value

I'm not sure I like C (lose quite some information there... don't know which values the peers proposed originally?).
I'm also not sure how either A or B improves upon the proposed text though... you're still logging 3 events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marten-seemann : as discussed in the editor's meeting, PTAL and let me know which changes, if any, you'd like here.

Consequently, each parameter value in a `parameters_set` event supersedes the
previous value of that parameter if present. If a parameter does not appear in a
given `parameters_set` event, its value is unchanged from the previous event
which set that parameter. If a parameter never appears in a `parameters_set`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't work for 0-RTT, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0-RTT is special cased in the paragraph below this one and should use parameters_restored. (That's not to say that that is 100% watertight, I might have missed something in that flow, but it's indeed not the intent to use only parameters_set for 0-RTT and this new text is not intended for 0-RTT indeed).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this event should be logged more than once per connection. Transport parameters are only exchanged once, after all. If we have a separate owner value as suggested above, this property would also hold in qlog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event will be logged at least twice per connection by definition, since there are two sets of TPs: one for the local entity and one for the peer. This event also tracks more state than just transport parameters (e.g., TLS level params like resumption or early data) that are probably set in a different codepath/at a different time and benefit from having multiple parameters_set.

I agree with the subset of your statement that the event probably shouldn't be logged more than once per connection for specific fields with the same owner though.

Still, for me personally the most important part here is If a parameter does not appear in a given parameters_set event, its value is unchanged from the previous event which set that parameter. to help make clear that subsequent parameters_set do not invalidate earlier values, which I agree with @hlandau is not 100% clear right now.

Copy link

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

LGTM, though one comment

event, it retains its default value. This default value thus cannot be restored
implicitly (for example by omitting a parameter in a subsequent
`parameters_set`) and, if needed, should instead be restored explicitly by
re-assigning the default value in an new `parameters_set` event.
Copy link

Choose a reason for hiding this comment

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

If we don't support null I guess that means some default values (i.e., absence of a TP) might be unrepresentable in qlog.

Copy link
Contributor Author

@rmarx rmarx Mar 7, 2024

Choose a reason for hiding this comment

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

I'm not sure I understand? Absence of a TP is simply that: absence of a TP in the parameters_set.

Whether this means the TP was actually absent on the wire or whether the implementation didn't/couldn't log it is indeed unclear, but the qlogger can do what they want (e.g., also simply couldn't log null), so that's never fixable imo.

If you mean to say that the default value of a TP is null, then I'd say that derives from the RFC that defines the TP, and absence implies that value?

Copy link

Choose a reason for hiding this comment

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

I guess this is not so much a problem for TPs as they are only exchanged once. But it seems like this event is intended to be more general.

For other kinds of parameters which might be renegotiable I could see a problem if it is possible for a feature to transition from negotiated to the default/non-negotiated state.

Maybe it is not a problem in practice? I'm just envisaging this event type being expanded to other things besides TPs in future.

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.

Difference between negotiated vs actually accepted/enabled TPs
3 participants