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

TLS Verification Bugfixes #11910

Merged
merged 8 commits into from
Jun 24, 2021
Merged

TLS Verification Bugfixes #11910

merged 8 commits into from
Jun 24, 2021

Conversation

HridoyRoy
Copy link
Contributor

@HridoyRoy HridoyRoy commented Jun 21, 2021

The crux of the matter is:

  1. diagnose.Error does NOT set the error status of the span or fail the span. diagnose.Fail does that.
  2. sanitizedListeners simply doesn't include listeners that failed the initListeners check. We are using instead the unsanitized listeners for the TLS verification checks.
  3. diagnose.Fail combined with diagnose.Error in a particular check caused the output to only print out warnings, but NOT set status. This is because in the output code, if there were warnings the check name and status of the span were not printed (they were skipped).

In addition to these 3 things, I also removed some diagnose calls in production code (that code was meant to be cleaned up in a separate PR), and moved the listener TLS checks that were in operator_diagnose.go into the diagnose package.

@HridoyRoy HridoyRoy requested review from hghaf099 and swayne275 June 21, 2021 20:52
@vercel vercel bot temporarily deployed to Preview – vault June 21, 2021 20:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 21, 2021 20:53 Inactive
@HridoyRoy HridoyRoy marked this pull request as draft June 21, 2021 21:29
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 22, 2021 16:04 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 22, 2021 16:04 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 22, 2021 16:14 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 22, 2021 16:14 Inactive
@HridoyRoy HridoyRoy marked this pull request as ready for review June 22, 2021 16:20
Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

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

I think it looks good! A few small comments then re-request and I'll approve

vault/diagnose/helpers_test.go Outdated Show resolved Hide resolved
vault/diagnose/helpers_test.go Show resolved Hide resolved
vault/diagnose/tls_verification.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault June 22, 2021 19:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 22, 2021 19:21 Inactive
@HridoyRoy HridoyRoy merged commit bbef373 into main Jun 24, 2021
@HridoyRoy HridoyRoy deleted the diagnose-tls-bugfixes branch June 24, 2021 17:43
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