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

Sidecar: Loads the TLS certificate during startup. #5995

Merged

Conversation

maheshbaliga
Copy link
Contributor

Closes [#5223] [#4923]

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

The sidecar did not validate the certificate and key while starting; Reading and parsing of the certificate were done only when the client initiated the TLS handshake with the sidecar. This is when the certificate related issues surfaced.

With this code change, these files are now loaded and checked by the sidecar when it starts.

Verification

Tested the fix by adding unit tests. Also, by running the sidecar and query components with TLS configurations.

GiedriusS
GiedriusS previously approved these changes Jan 2, 2023
Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

💪 thanks!

@GiedriusS GiedriusS enabled auto-merge (squash) January 2, 2023 14:09
@GiedriusS
Copy link
Member

Seems like there are some conflicts. Care to fix them? 😄

@maheshbaliga
Copy link
Contributor Author

@GiedriusS, Sure. Will fix it.

auto-merge was automatically disabled January 2, 2023 15:00

Head branch was pushed to by a user without write access

GiedriusS
GiedriusS previously approved these changes Jan 2, 2023
@GiedriusS GiedriusS enabled auto-merge (squash) January 2, 2023 15:03
Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
auto-merge was automatically disabled January 2, 2023 16:47

Head branch was pushed to by a user without write access

@maheshbaliga
Copy link
Contributor Author

@GiedriusS, the conflict has been resolved. Kindly merge the same.

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! 🌟

@saswatamcode saswatamcode merged commit 6ef005c into thanos-io:main Jan 3, 2023
Kartik-Garg pushed a commit to infracloudio/thanos that referenced this pull request Jan 16, 2023
Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
Signed-off-by: Kartik-Garg <kartik.garg@infracloud.io>
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request Apr 17, 2023
Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>

Signed-off-by: maheshbaliga <mahesh.baliga@infracloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants