Skip to content

Hash namespace+proxy ID when creating socket path#17204

Merged
freddygv merged 1 commit into
mainfrom
cc-4929-cap-socket-path
May 9, 2023
Merged

Hash namespace+proxy ID when creating socket path#17204
freddygv merged 1 commit into
mainfrom
cc-4929-cap-socket-path

Conversation

@freddygv
Copy link
Copy Markdown
Contributor

@freddygv freddygv commented May 1, 2023

Description

UNIX domain socket paths are limited to 104-108 characters depending on the OS. This limit was quite easy to exceed when testing the feature on Kubernetes, due to how proxy IDs encode the Pod ID eg:
metrics-collector-59467bcb9b-fkkzl-hcp-metrics-collector-sidecar-proxy

To ensure we stay under the lower 104 character limit this commit makes a couple changes:

  • Use a b64 encoded SHA1 hash of the namespace + proxy ID to create a short and deterministic socket file name.
  • Add validation to proxy registrations and proxy-defaults to enforce a limit on the socket directory length.

Testing & Reproduction steps

  • Added unit tests
  • Manual testing with long socket path at and above limit

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@github-actions github-actions Bot added the theme/cli Flags and documentation for the CLI interface label May 1, 2023
@freddygv freddygv requested a review from Achooo May 1, 2023 16:25
@freddygv freddygv force-pushed the cc-4929-cap-socket-path branch 2 times, most recently from f00ccd3 to 4616e69 Compare May 1, 2023 19:41
@freddygv freddygv added the pr/no-changelog PR does not need a corresponding .changelog entry label May 1, 2023
@freddygv freddygv force-pushed the cc-4929-cap-socket-path branch from 4616e69 to 6849c65 Compare May 1, 2023 23:52
@freddygv freddygv marked this pull request as ready for review May 2, 2023 01:16
Comment thread agent/proxycfg/connect_proxy.go
Copy link
Copy Markdown
Contributor

@Achooo Achooo May 8, 2023

Choose a reason for hiding this comment

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

this makes sense, based on my napkin math:

  • sha1 output => 160 bits => 20 bytes
  • base64 => 4*20/3 = 26.6 so 27 bytes but round up to multiple of 4 => 28

So 104 - (5+28) = 71 , where:

  • 28 is file name length
  • 5 is extension length

So we have 71 chars remaining for the dir length

@freddygv freddygv requested review from a team, kyhavlov and ndhanushkodi and removed request for a team May 8, 2023 21:23
Comment thread agent/structs/structs_test.go Outdated
UNIX domain socket paths are limited to 104-108 characters, depending on
the OS. This limit was quite easy to exceed when testing the feature on
Kubernetes, due to how proxy IDs encode the Pod ID eg:
metrics-collector-59467bcb9b-fkkzl-hcp-metrics-collector-sidecar-proxy

To ensure we stay under that character limit this commit makes a
couple changes:
- Use a b64 encoded SHA1 hash of the namespace + proxy ID to create a
  short and deterministic socket file name.
- Add validation to proxy registrations and proxy-defaults to enforce a
  limit on the socket directory length.
@freddygv freddygv force-pushed the cc-4929-cap-socket-path branch from 158be9c to 7bb0dcf Compare May 9, 2023 17:21
@freddygv freddygv merged commit 7c3e9cd into main May 9, 2023
@freddygv freddygv deleted the cc-4929-cap-socket-path branch May 9, 2023 18:20
freddygv added a commit that referenced this pull request May 9, 2023
UNIX domain socket paths are limited to 104-108 characters, depending on
the OS. This limit was quite easy to exceed when testing the feature on
Kubernetes, due to how proxy IDs encode the Pod ID eg:
metrics-collector-59467bcb9b-fkkzl-hcp-metrics-collector-sidecar-proxy

To ensure we stay under that character limit this commit makes a
couple changes:
- Use a b64 encoded SHA1 hash of the namespace + proxy ID to create a
  short and deterministic socket file name.
- Add validation to proxy registrations and proxy-defaults to enforce a
  limit on the socket directory length.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/no-changelog PR does not need a corresponding .changelog entry theme/cli Flags and documentation for the CLI interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants