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

[CLI] Add flag to disable certificate verification #23780

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented Oct 7, 2024

Description

Adds --insecure flag to the CLI to skip SSL certificate verification.

This is a cherry-pick of trinodb/trino@1c5b9215a2d1b04b5f84e2b3e86 with modifications to allow it to compile within PrestoDB

Motivation and Context

When deploying presto behind an SSL-enabled proxy or configuring it directly using systems like kubernetes, the system will issue self-signed certificates which can make debugging more difficult when clients have not been properly configured with the CA. When debugging clusters which are deployed this way it would be useful to be able to connect to them directly by just bypassing certificate verification.

Impact

Additional flag for the CLI

Test Plan

  • New unit test

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== RELEASE NOTES ==

General Changes
* Add a flag to the Presto CLI which allows skipping SSL certificate verification `:pr:23780`

@ZacBlanco ZacBlanco force-pushed the upstream-cli-insecure-mode branch from fa9fff4 to 0aa72e5 Compare October 7, 2024 18:10
@steveburnett
Copy link
Contributor

Nit for release note entry for consistency with the Order of changes in the Release Notes Guidelines.

== RELEASE NOTES ==

General Changes
* Add a flag to the Presto CLI which allows skipping SSL certificate verification `:pr:23780`

Copy link
Contributor

@steveburnett steveburnett 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 doc, especially updating to capture other missing information that was not part of this PR! A minor nit of formatting, and otherwise looks great.

steveburnett
steveburnett previously approved these changes Oct 7, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good.

@ZacBlanco ZacBlanco force-pushed the upstream-cli-insecure-mode branch 3 times, most recently from 20336b6 to c30c44e Compare October 7, 2024 19:38
Cherry-pick of trinodb/trino@1c5b9215a2d1b04b5f84e2b3e86

with minor modifications for Presto

Co-authored-by: Lewuathe <lewuathe@me.com>
@ZacBlanco ZacBlanco force-pushed the upstream-cli-insecure-mode branch from c30c44e to e56eda6 Compare October 7, 2024 19:38
@ZacBlanco ZacBlanco marked this pull request as ready for review October 7, 2024 21:56
@ZacBlanco ZacBlanco requested review from elharo and a team as code owners October 7, 2024 21:56
@ZacBlanco ZacBlanco requested a review from presto-oss October 7, 2024 21:56
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

Thank you, this is really useful for development and local debugging.

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

@tdcmeehan tdcmeehan merged commit 96c58c9 into prestodb:master Oct 8, 2024
58 checks passed
@jaystarshot jaystarshot mentioned this pull request Nov 1, 2024
25 tasks
@tdcmeehan tdcmeehan added the from:IBM PR from IBM label Dec 13, 2024
@prestodb-ci prestodb-ci requested review from a team, bibith4 and namya28 and removed request for a team December 13, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:IBM PR from IBM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants