Opened 2 years ago

Closed 12 months ago

#20549 closed enhancement (implemented)

Make bridge descriptor sanitization more maintainable and more modular

Reported by: iwakeh Owned by: metrics-team
Priority: High Milestone:
Component: Metrics/CollecTor Version:
Severity: Normal Keywords: metrics-2018
Cc: Actual Points:
Parent ID: #20542 Points:
Reviewer: iwakeh Sponsor:

Description

SanitizedBridgeServerDescriptor should extend BridgeServerDescriptor.
It receives a BridgeServerDescriptor at construction time and sanitizes the descriptor.
All getters should only supply sanitized data; this explicitely includes method from the Descriptor interface like getRawDescriptorBytes.

In addition, a SanitizedBridgeServerDescriptorTest should be created that verifies the sanitation and data integrity.

Child Tickets

Change History (13)

comment:1 Changed 17 months ago by karsten

Summary: implement SanitizedBridgeServerDescriptorImplement SanitizedBridgeServerDescriptor class that encapsulates the sanitizing logic for bridge server descriptors

Try to make the summary slightly more useful.

comment:2 Changed 17 months ago by karsten

Keywords: metrics-2018 added

comment:3 Changed 17 months ago by karsten

Owner: set to metrics-team
Status: newassigned

comment:4 Changed 16 months ago by iwakeh

Also pay attention to the issues mentioned in #23981's comments.

Shouldn't this be moved to metrics-2017?

comment:5 in reply to:  4 ; Changed 16 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

Replying to iwakeh:

Also pay attention to the issues mentioned in #23981's comments.

Right. In particular, I'd like to try out the following change that I suggested on that ticket:

"While working on this patch I considered taking a different approach where we'd include placeholders for parts that we cannot sanitize yet and fill them in after finishing to read the original descriptor. This could be a List<StringBuilder> to retain order of original lines and where we'd keep references to some StringBuilders that we need to sanitize after reading. Still something to think about. Let's do that after fixing this ugly bug."

Shouldn't this be moved to metrics-2017?

Sure. Worth trying.

comment:6 in reply to:  5 Changed 16 months ago by karsten

Status: assignedneeds_review

Replying to karsten:

"While working on this patch I considered taking a different approach where we'd include placeholders for parts that we cannot sanitize yet and fill them in after finishing to read the original descriptor. This could be a List<StringBuilder> to retain order of original lines and where we'd keep references to some StringBuilders that we need to sanitize after reading. Still something to think about. Let's do that after fixing this ugly bug."

I started an implementation in my task-20549 branch that doesn't get us all the way to implementing this ticket yet, but which may be a start. Feedback welcome!

comment:7 Changed 15 months ago by iwakeh

Reviewer: iwakeh

Adding these to my reviewing list.

comment:8 Changed 14 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

comment:9 Changed 12 months ago by karsten

Priority: MediumHigh

Raising priority, as this may soon become more relevant when reprocessing bridge descriptors.

comment:10 Changed 12 months ago by iwakeh

This s a different approach from the tickets description. But, I really like this idea and added seven more commits. The commit comments should describe the details, if necessary. The general changes are some name changes, like introducing DescriptorBuilder as builder, which we might reuse elsewhere.
The new DescriptorBuilder also accepts appending DescriptorBuilder now.

Please review seven commits on the task-20549 branch.

Maybe, we should change the title and summary to a more general "Make bridge descriptor sanitization more maintainable and more modular"?

comment:11 Changed 12 months ago by iwakeh

Status: needs_reviewneeds_information

Changing to needs_information also meaning needs_review.

comment:12 Changed 12 months ago by karsten

Status: needs_informationmerge_ready
Summary: Implement SanitizedBridgeServerDescriptor class that encapsulates the sanitizing logic for bridge server descriptorsMake bridge descriptor sanitization more maintainable and more modular

Great! Your additional changes look good to me. I made two tiny changes in my task-20549 branch (this. and a whitespace change), squashed, rebased, and pushed to master. Setting status to merge_ready.

Do we want to make further changes as part of this ticket? If so, can you start a list of next steps here? If not, can you close this ticket? Thanks!

comment:13 Changed 12 months ago by iwakeh

Resolution: implemented
Status: merge_readyclosed

Changes look ok, thanks!

Well, there'll always be more steps, but let's call this ticket finished.

I created #25307 as I'm now aware that this should be quite easy (just a question to clarify on that ticket).

Closing.
Thanks!

Note: See TracTickets for help on using tickets.