Skip to content

Hash ir/infra resources#559

Merged
danehans merged 8 commits intoenvoyproxy:mainfrom
Alice-Lilith:alicewasko/dev/ir-hashing
Oct 14, 2022
Merged

Hash ir/infra resources#559
danehans merged 8 commits intoenvoyproxy:mainfrom
Alice-Lilith:alicewasko/dev/ir-hashing

Conversation

@Alice-Lilith
Copy link
Copy Markdown
Member

@Alice-Lilith Alice-Lilith commented Oct 13, 2022

Changes some infra names so that everything is hashed now (service/deployment/configmap/serviceaccount). It preserves up to 48 characters of the original resource's name and then adds an 8 character hash (48 chosen because when appending the 8 character hash and the envoy prefix("envoy") it adds up to 63 characters) .

The labels for the owning gateway name and namespace have not been altered.

Should resolve #441

AliceProxy added 2 commits October 13, 2022 10:47
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
@Alice-Lilith Alice-Lilith requested a review from a team as a code owner October 13, 2022 17:49
Signed-off-by: AliceProxy <alicewasko@datawire.io>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #559 (5cc2daf) into main (01a638c) will increase coverage by 0.52%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #559      +/-   ##
==========================================
+ Coverage   60.19%   60.71%   +0.52%     
==========================================
  Files          45       45              
  Lines        5564     5567       +3     
==========================================
+ Hits         3349     3380      +31     
+ Misses       2008     1984      -24     
+ Partials      207      203       -4     
Impacted Files Coverage Δ
internal/envoygateway/config/config.go 0.00% <ø> (ø)
internal/status/status.go 0.00% <ø> (ø)
internal/ir/infra.go 67.41% <50.00%> (ø)
internal/gatewayapi/translator.go 85.52% <100.00%> (ø)
internal/infrastructure/kubernetes/configmap.go 75.43% <100.00%> (-7.02%) ⬇️
internal/infrastructure/kubernetes/deployment.go 87.74% <100.00%> (ø)
internal/infrastructure/kubernetes/service.go 75.00% <100.00%> (ø)
...ternal/infrastructure/kubernetes/serviceaccount.go 82.75% <100.00%> (ø)
internal/provider/kubernetes/gateway.go 51.08% <100.00%> (+0.32%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

AliceProxy added 3 commits October 13, 2022 15:33
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
@Alice-Lilith Alice-Lilith requested a review from arkodg October 13, 2022 22:48
@Alice-Lilith Alice-Lilith changed the title Hash ir/infra resources with long names Hash ir/infra resources Oct 13, 2022
Comment on lines 27 to 29
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be more efficient if the func first checks the provided name is less or equal to 32 chars and then return early with the provided name?

Copy link
Copy Markdown
Member Author

@Alice-Lilith Alice-Lilith Oct 13, 2022

Choose a reason for hiding this comment

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

Initially it was only going to hash names that were longer than 60 characters. Per this comment I changed it to now hash every name regardless of length. Reverting the last couple commits would return it to only hashing if the provided name is longer than 60 characters and returning early with the provided name if not.
#559 (comment)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This bit has been updated to use up to 48 characters so that with the "envoy- and the 8 character hash we can go all the way up to 63 characters max. Hashing still occurs on all resources regardless of initial name length at @arkodg's suggestion but I'm good with doing it either way.

AliceProxy added 2 commits October 13, 2022 18:06
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Signed-off-by: AliceProxy <alicewasko@datawire.io>
Copy link
Copy Markdown
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@danehans danehans merged commit 39456ca into envoyproxy:main Oct 14, 2022
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.

Hash the Names of Managed Infra Resources

4 participants