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

Added in cluster configuration support #271

Merged

Conversation

Bobbins228
Copy link
Contributor

@Bobbins228 Bobbins228 commented Aug 2, 2023

Issue link

Closes #255

What changes have been made

Changed the config check method to load the config file first and if it doesn't work it will try load the in cluster configuration.

Verification steps

  • From the terminal run in the codeflare-sdk directory poetry build
  • In a cluster go to the Open Datahub dashboard
  • Create a codeflare Jupytr notebook
  • uninstall the current codeflare-sdk with pip uninstall codeflare-sdk -y
  • Drag and drop the build into the Jupyter notebook
  • Install the build with pip install codeflare_sdk-0.0.0.dev0-py3-none-any.whl
  • Restart the Notebook kernel
  • Run through a demo notebook without logging in with token + server (This is important because it affects how config files are loaded)
  • cluster.up() should work without a kubernetes config file and default to using the incluster_config

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

Comment on lines 165 to 170
try:
config.load_kube_config()
except config.config_exception.ConfigException:
try:
config.load_incluster_config()
except:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway to avoid this nested try/except statement? Can we check that we are in the cluster before trying to load the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check to see if the config file exists and if not a check to see if we are in the cluster before using load_incluster_config

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks I think that's much better. I'll test it this morning.

Copy link
Collaborator

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

Looks good, just one change needed for error handling

elif "KUBERNETES_PORT" in os.environ:
config.load_incluster_config()
else:
print(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to raise a PermissionsError instead of just printing. Either that, or we need to catch the ConnectionRefusedError/MaxRetryError caused by this later down the road with the Kubernetes API. Since we are no longer blocking here, we are currently getting:

MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /apis/mcad.ibm.com/v1beta1/namespaces/openshift-operators/appwrappers (Caused by NewConnectionError(': Failed to establish a new connection: [Errno 61] Connection refused'))

instead of:

PermissionError: Action not permitted, have you put in correct/up-to-date auth credentials?

on various API calls

@kpouget
Copy link

kpouget commented Aug 14, 2023

@Bobbins228 I rebased the PR 😅
I didn't expected Github to allow me to do it, but it worked 🙃
anyway, I'll launch a test with this up to date PR, I needed the branch to be up to date to be able to test it

@kpouget
Copy link

kpouget commented Aug 16, 2023

@Bobbins228 FYI, this PR worked with my in-cluster script using codeflare SDK :)

Copy link
Collaborator

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

Looks good, just one more error handling suggestion

src/codeflare_sdk/cluster/auth.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Maxusmusti Maxusmusti left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Maxusmusti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2023
@openshift-merge-robot openshift-merge-robot merged commit 77a82e2 into project-codeflare:main Aug 21, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes authentication not working from inside the cluster
5 participants