Skip to content

String pattern added to x-correlator scheme#373

Merged
rartych merged 3 commits intocamaraproject:mainfrom
rartych:x-correlator
Jan 16, 2025
Merged

String pattern added to x-correlator scheme#373
rartych merged 3 commits intocamaraproject:mainfrom
rartych:x-correlator

Conversation

@rartych
Copy link
Copy Markdown
Contributor

@rartych rartych commented Jan 13, 2025 •
edited
Loading

What type of PR is this?

  • correction

What this PR does / why we need it:

String pattern added to x-correlator scheme to improve security and inline with current recommendation for string parameters.
The proposed pattern covers recommended UUID strings and the length of 55 allows to use strings similar to traceparent from Trace Context.

CAMARA_common.yaml was updated accordingly.

Which issue(s) this PR fixes:

Fixes #352

Does this PR introduce a breaking change?

  • Yes
  • No

Special notes for reviewers:

The change should not be breaking for current implementations using UUID in x-correlator.

Changelog input

String pattern added to x-correlator scheme

Additional documentation

PedroDiez
PedroDiez approved these changes Jan 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@PedroDiez PedroDiez left a comment

Choose a reason for hiding this comment

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

LGTM

patrice-conil
patrice-conil approved these changes Jan 13, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@patrice-conil patrice-conil left a comment

Choose a reason for hiding this comment

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

LGTM

@rartych rartych merged commit b467cbf into camaraproject:main Jan 16, 2025
@hdamker
Copy link
Copy Markdown
Collaborator

hdamker commented Jan 16, 2025

Does this PR introduce a breaking change?

x Yes
No

I'm not sure that we should introduce this as a "breaking change" ... if we do so then all 1.0.0 API versions from Fall24 will go to automatically to 2.x.x in Spring25.

I formulated my proposal along the vendor opaque value in Trace Context, to avoid that we exclude any valid vendor specific correlator by this change and therefore don't need to declare it as breaking change. But maybe too late here ...

@rartych
Copy link
Copy Markdown
Contributor Author

rartych commented Jan 16, 2025 •
edited
Loading

Formally it is a breaking change (modifying mandatory parameter).
API Design Guidelines recommend using UUID (if API Client implements "UUID-like" correlator then there is no breaking change) , however this recommendation is not included in API definitions - Possibly it can be added in x-correlator description field in CAMARA_common.yaml

@hdamker hdamker mentioned this pull request Jan 17, 2025
Closed
@Kevsy Kevsy mentioned this pull request Jan 21, 2025
Merged
2 tasks
@hdamker hdamker mentioned this pull request Jan 26, 2025
Closed
5 tasks
@hdamker hdamker mentioned this pull request Feb 28, 2025
Merged
@FabrizioMoggio FabrizioMoggio mentioned this pull request Mar 17, 2025
Closed
@rartych rartych deleted the x-correlator branch April 18, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PedroDiez PedroDiez PedroDiez approved these changes

@patrice-conil patrice-conil patrice-conil approved these changes

@eric-murray eric-murray eric-murray approved these changes

@shilpa-padgaonkar shilpa-padgaonkar Awaiting requested review from shilpa-padgaonkar

@RubenBG7 RubenBG7 Awaiting requested review from RubenBG7

Assignees

No one assigned

Labels

Spring25

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve definition of x-correlator header

5 participants

@rartych @hdamker @PedroDiez @patrice-conil @eric-murray