Skip to content

Propagate ca_cert_file_path and verify settings to auth client#1509

Closed
goyalankit wants to merge 2 commits into
flyteorg:masterfrom
goyalankit:master
Closed

Propagate ca_cert_file_path and verify settings to auth client#1509
goyalankit wants to merge 2 commits into
flyteorg:masterfrom
goyalankit:master

Conversation

@goyalankit

Copy link
Copy Markdown
Member

TL;DR

Propagate ca_cert_file_path and verify settings to auth client

Flytekit throws CERTIFICATE_VERIFY_FAILED on auth even if user has specified the certificate path or has set insecureSkipVerify to False. This change adds the verify to requests library according to user configured settings.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

@welcome

welcome Bot commented Feb 13, 2023

Copy link
Copy Markdown

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

Signed-off-by: Ankit Goyal <angoyal@linkedin.com>
@codecov

codecov Bot commented Feb 13, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1509 (e10cc95) into master (ecded3e) will decrease coverage by 0.02%.
The diff coverage is 11.11%.

❗ Current head e10cc95 differs from pull request most recent head d2b5860. Consider uploading reports for the commit d2b5860 to get more accurate results

@@            Coverage Diff             @@
##           master    #1509      +/-   ##
==========================================
- Coverage   69.45%   69.43%   -0.02%     
==========================================
  Files         305      305              
  Lines       28581    28590       +9     
  Branches     2699     2701       +2     
==========================================
+ Hits        19850    19851       +1     
- Misses       8213     8221       +8     
  Partials      518      518              
Impacted Files Coverage Δ
flytekit/clients/raw.py 25.45% <0.00%> (-0.40%) ⬇️
flytekit/clis/auth/auth.py 9.93% <0.00%> (-0.07%) ⬇️
flytekit/clis/auth/credentials.py 0.00% <ø> (ø)
flytekit/configuration/internal.py 16.21% <0.00%> (-0.23%) ⬇️
flytekit/configuration/__init__.py 36.72% <50.00%> (+0.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ByronHsu
ByronHsu previously approved these changes Feb 14, 2023
Comment thread flytekit/clis/auth/credentials.py Outdated
@kumare3

kumare3 commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

@goyalankit I have this PR in progress, that completely overhauls the auth subsystem - #1458

@goyalankit

Copy link
Copy Markdown
Member Author

@goyalankit I have this PR in progress, that completely overhauls the auth subsystem - #1458

Got it.. but this is still valid? I can close my PR if you want to take these changes into account in your PR? The main issue is that we are not passing the verify switch to request commands: here and here

Co-authored-by: Kevin Su <pingsutw@gmail.com>
Signed-off-by: Ankit Goyal <angoyal@linkedin.com>
@goyalankit

Copy link
Copy Markdown
Member Author

Closing as the changes have been incorporated in #1458

@goyalankit goyalankit closed this Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants