-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Require TLS for server RPC when enabled #2526
Conversation
Fixes #2525 We used to be checking a RequireTLS field that was never set. Instead we can just check the TLSConfig.EnableRPC field and require TLS if it's enabled. Added a few unfortunately slow integration tests to assert the intended behavior of misconfigured RPC TLS. Also disable a lot of noisy test logging when -v isn't specified.
29ac18c
to
11206c7
Compare
client/client_test.go
Outdated
QueryOptions: structs.QueryOptions{Region: "global"}, | ||
} | ||
var out structs.SingleNodeResponse | ||
deadline := time.Now().Add(1234 * time.Millisecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is 1234 coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thin air. I would love a better way of testing "ensure X doesn't happen" (where "X" in this case is successfully making the RPC call).
I guess I should put a sleep in this loop as well so it's not spinning as fast as possible.
I'll poke around for a minute and try to come up with something a little better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you may just want to add a method to testutil that takes a total duration and a wait between duration and runs a function to assert. Then run for 2 seconds hitting the api every 250ms or something ensuring it doesn't succeed.
1234ms was far longer than needed and not sleeping between iterations was just mean.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #2525
We used to be checking a RequireTLS field that was never set. Instead we
can just check the TLSConfig.EnableRPC field and require TLS if it's
enabled.
Added a few unfortunately slow integration tests to assert the intended
behavior of misconfigured RPC TLS.
Also disable a lot of noisy test logging when -v isn't specified.