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

Static TLS Configuration Store Library Implementation #18

Merged
merged 1 commit into from
May 31, 2022

Conversation

rmehta19
Copy link
Collaborator

This PR is a static implementation of the TLS Configuration Store Library. No calls to S2A are made.

A note on tls_config_store_test.go

  • The 'Equal' function cannot be used to validate the contents of rootCertPool because it is not yet part of a go release(https://cs.opensource.google/go/go/+/4aacb7ff0f103d95a724a91736823f44aa599634). A solution could be to temporarily copy the implementation of 'Equal' in the master branch of the golang. However the implementation accesses fields from the x509.CertPool struct, which are lowercase, hence private. Therefore, in our package we cannot implement Equal by defining an alias of CertPool, because CertPool fields are private.

@rmehta19 rmehta19 self-assigned this May 26, 2022
@rmehta19 rmehta19 force-pushed the tls_config_lib_impl branch 2 times, most recently from e0e9002 to 483ecf4 Compare May 26, 2022 22:25
Copy link
Collaborator

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Riya! It's looking great! Left a handful of mostly-minor comments. Should be good to go after that.

internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store_test.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store_test.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store_test.go Outdated Show resolved Hide resolved
@rmehta19
Copy link
Collaborator Author

Thanks for the PR Riya! It's looking great! Left a handful of mostly-minor comments. Should be good to go after that.

Thanks for the review Matt!

Copy link
Collaborator

@tavishvaidya tavishvaidya left a comment

Choose a reason for hiding this comment

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

Did a first pass, will do another one after all changes are in, thanks!

@rmehta19
Copy link
Collaborator Author

Did a first pass, will do another one after all changes are in, thanks!
Thanks Tavish!

@rmehta19 rmehta19 force-pushed the tls_config_lib_impl branch from 81bab25 to ad41d67 Compare May 27, 2022 20:50
@rmehta19
Copy link
Collaborator Author

Thanks for the comments! addressed them all in the latest push to this branch, let me know what you think!

Copy link
Collaborator

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

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

LGTM with a few minor nits.

Thanks for the PR Riya!

internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/tls_config_store.go Outdated Show resolved Hide resolved
internal/v2/tls_config_store/BUILD Outdated Show resolved Hide resolved
@rmehta19 rmehta19 force-pushed the tls_config_lib_impl branch from ad41d67 to 5877cec Compare May 27, 2022 22:00
@rmehta19 rmehta19 force-pushed the tls_config_lib_impl branch from 5877cec to 727a2d7 Compare May 27, 2022 23:04
@rmehta19 rmehta19 merged commit fd87324 into main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants