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

flyte-cli setup-config throws JSONDecodeError #653

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

pingsutw
Copy link
Member

Signed-off-by: Kevin Su [email protected]

TL;DR

flyte-cli setup-config throws JSONDecodeError because we tried to get admin credential in insecure mode.

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

Get credentials only in a secure mode.

Tracking Issue

flyteorg/flyte#163

Follow-up issue

NA

image

Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #653 (fa580a7) into master (d6a8c5d) will increase coverage by 0.11%.
The diff coverage is 90.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
+ Coverage   85.45%   85.57%   +0.11%     
==========================================
  Files         355      355              
  Lines       29246    29292      +46     
  Branches     2368     2374       +6     
==========================================
+ Hits        24993    25067      +74     
+ Misses       3624     3589      -35     
- Partials      629      636       +7     
Impacted Files Coverage Δ
flytekit/clis/flyte_cli/main.py 47.53% <85.71%> (+2.73%) ⬆️
tests/flytekit/unit/cli/test_flyte_cli.py 98.85% <100.00%> (+0.25%) ⬆️
flytekit/models/task.py 92.13% <0.00%> (-1.01%) ⬇️
flytekit/core/condition.py 90.00% <0.00%> (ø)
flytekit/core/resources.py 100.00% <0.00%> (ø)
flytekit/common/translator.py 90.19% <0.00%> (ø)
tests/flytekit/unit/core/test_conditions.py 98.70% <0.00%> (ø)
tests/flytekit/unit/core/test_imperative.py 99.08% <0.00%> (ø)
tests/flytekit/unit/core/test_type_hints.py 96.13% <0.00%> (ø)
tests/flytekit/unit/core/test_composition.py 99.20% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6a8c5d...fa580a7. Read the comment docs.

# This is where we handle the value read from the flyte-cli config file, if any, for the insecure flag.
# Previously we tried putting it into the default into the declaration of the option itself, but in click, it
# appears that flags operate like toggles. If both the default is true and the flag is passed in the command,
# they negate each other and it's as if it's not passed. Here we rectify that.
if _INSECURE_FLAG and _INSECURE_FLAGS[0] not in prefix_args:
prefix_args.append(_INSECURE_FLAGS[0])

# Use host url in config file if users don't specify the host url
if _HOST_FLAGS[0] not in prefix_args:
prefix_args.extend([_HOST_FLAGS[0], _six.text_type(_HOST_URL)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prefix_args.extend([_HOST_FLAGS[0], _six.text_type(_HOST_URL)])
prefix_args.extend([_HOST_FLAGS[0], str(_HOST_URL)])

@wild-endeavor
Copy link
Contributor

I installed the sha from this PR into a new virtualenv and then ran the following.

host location

$ flyte-cli -h localhost:30081 -i setup-config
Config file not found at default location, relying on environment variables instead.
                        To setup your config file run 'flyte-cli setup-config'
Usage: flyte-cli setup-config [OPTIONS]
Try 'flyte-cli setup-config --help' for help.

Error: Missing option '-h' / '--host'.

Is this intended? Are you supposed to have to specify the host after setup-config?

client_id missing

$ flyte-cli setup-config -h localhost:30081 -i
Config file not found at default location, relying on environment variables instead.
                        To setup your config file run 'flyte-cli setup-config'
Welcome to Flyte CLI! Version: 0.0.0+develop
Traceback (most recent call last):
  File "/Users/ytong/envs/tstsetup/bin/flyte-cli", line 8, in <module>
    sys.exit(_flyte_cli())
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/flytekit/clis/flyte_cli/main.py", line 2372, in setup_config
    f.write("client_id={}".format(data["client_id"]))
KeyError: 'client_id'

Looking at the response, it looks like Admin is returning:
{'error': 'unknown service flyteidl.service.AuthMetadataService', 'code': 12, 'message': 'unknown service flyteidl.service.AuthMetadataService'} when auth isn't on.

The file is still being written correctly:

$ cat ~/.flyte/config
[platform]
url = localhost:30081
insecure = True

but the stack trace is ugly and made me think the entire command failed. Can we hide it somehow?


f.write("[credentials]")
f.write("\n")
f.write("client_id={}".format(data["client_id"]))
Copy link
Member Author

@pingsutw pingsutw Sep 13, 2021

Choose a reason for hiding this comment

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

cc @wild-endeavor.

File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/ytong/envs/tstsetup/lib/python3.8/site-packages/flytekit/clis/flyte_cli/main.py", line 2372, in setup_config
    f.write("client_id={}".format(data["client_id"]))
KeyError: 'client_id'

It seems like you used the old version of flytekit.
I already removed f.write

@pingsutw
Copy link
Member Author

pingsutw commented Sep 13, 2021

Is this intended?

No, I didn't test it before.

I just ran the command flyte-cli -h localhost:30081 -i setup-config in my local laptop, and it throws an error. Will fix it ASAP.
Error messages:

    response = _requests.get(config_url)
  File "/Users/kevin/opt/anaconda3/envs/flyte/lib/python3.8/site-packages/requests/api.py", line 75, in get
    return request('get', url, params=params, **kwargs)
  File "/Users/kevin/opt/anaconda3/envs/flyte/lib/python3.8/site-packages/requests/api.py", line 61, in request
    return session.request(method=method, url=url, **kwargs)
  File "/Users/kevin/opt/anaconda3/envs/flyte/lib/python3.8/site-packages/requests/sessions.py", line 542, in request
    resp = self.send(prep, **send_kwargs)
  File "/Users/kevin/opt/anaconda3/envs/flyte/lib/python3.8/site-packages/requests/sessions.py", line 655, in send
    r = adapter.send(request, **kwargs)
  File "/Users/kevin/opt/anaconda3/envs/flyte/lib/python3.8/site-packages/requests/adapters.py", line 516, in send
    raise ConnectionError(e, request=request)
requests.exceptions.ConnectionError: HTTPSConnectionPool(host='none', port=443): Max retries exceeded with url: /config/v1/flyte_client (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f82b0d91220>: Failed to establish a new connection: [Errno 8] nodename nor servname provided, or not known'))

Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Copy link
Member Author

pingsutw commented Sep 14, 2021

Fixed it.
image

@wild-endeavor wild-endeavor merged commit 72f46e6 into flyteorg:master Sep 14, 2021
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.

2 participants