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

Support kubeconfig loader #2

Closed
mbohlool opened this issue Dec 21, 2017 · 20 comments
Closed

Support kubeconfig loader #2

mbohlool opened this issue Dec 21, 2017 · 20 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@mbohlool
Copy link
Contributor

Just like python client here and here, we need to support loading kubeconfig both in cluster and outside of the cluster.

@mbohlool mbohlool added kind/feature Categorizes issue or PR as related to a new feature. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Dec 21, 2017
@guoshimin
Copy link
Contributor

I have a solution that uses FFI to call the Go code for loading the kubeconfig, including auth plugin handling. I will open a PR when it's in a good enough shape.

@jkachmar
Copy link

@guoshimin Given that #15 was merged, is there any additional work that needs to go towards supporting the kubeconfig loaders?

Out of curiosity, what was the Go FFI stuff and does it still make sense to add it here?

@guoshimin
Copy link
Contributor

Yeah we need to add support for the authentication methods in https://github.com/kubernetes/client-go/tree/master/plugin/pkg/client/auth, especially oidc, gcp, and exec. I was working on a solution that reuses the Go implementation via FFI, but was blocked by a Go bug (fixed now) and uncertainty around how to distribute that, since we would need to build some Go code. Alternatively we could also implement the logic in Haskell.

@jkachmar
Copy link

jkachmar commented Mar 1, 2019

Hmm... I admit now that I look at the Go code I realize that the exec auth would be very useful for me, so FFI-ing out to it would be swell (especially if you already have some working code.

Can you post it in a gist, branch, etc. for me/others to take a look?

With respect to packaging, I stumbled upon this blog post whose example source code demonstrates a few ways of potentially solving the problem.

@guoshimin
Copy link
Contributor

I have some code here: https://gitlab.com/guoshimin/kube-clientconfig

I haven't touched the code for a long time, it probably doesn't work out of the box.

@guoshimin
Copy link
Contributor

I updated the code and tried using the exec plugin and it seems to work. @jkachmar If you think this is the route we should go down I'll clean up the code and try to merge it.

@jkachmar
Copy link

jkachmar commented Mar 7, 2019

Awesome, thanks for bringing it up-to-date.

I've been trying to figure out how the distribution of this stuff might work and mostly been coming up blank.

If it's not too much effort to clean up the code and open a PR, maybe we could try to figure out a good way to package it up that Hackage is happy with.


So far, the only thing I can think of (and this feels like kind of an awful hack) is to have the Go code dynamically linked in, with all code using it put behind a feature flag.

Users would have to compile that code separately, ensure that the library is named correctly and on LD_LIBRARY_PATH , and enable the flag.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 6, 2019
@akshaymankar
Copy link
Member

Hello,

I tried implementing the config loader in pure Haskell here: https://github.com/akshaymankar/eirini-route-emitter/blob/46acfc7af3106977047cd99cc1d9176f11c7714a/src/KubernetesExtras/Client.hs

However, the GCP and OIDC auth code is not the best because after the the token expires, the applyAuthMethod function has no way of caching the new token for future requests. This causes unnecessary extra calls to the auth provider.
Also, it might break the OIDC implementation, because according the spec, the auth server can return a new refresh token and make the old one invalid. It doesn't happen with the server I used for testing, but it is possible.

I think this can be solved by having a global over-writable cache like the Go implementation has, but I thought I should get opinions about it before writing potentially unsafe code. As I am fairly new to Haskell, I'd be grateful if somebody could point me to useful resources or just comment here.

@akshaymankar
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 15, 2019
@guoshimin
Copy link
Contributor

@akshaymankar Thanks for making the effort. I'm also nowhere close to being an expert. Just curious, is there anything holding you back from using an MVar or similar as the mutable cache?

@akshaymankar
Copy link
Member

Nothing really. I was going to try MVar anyways, but I thought I'd first fish for opinions here. I will try it and let y'all know if it goes well. In the meanwhile, if somebody has enough experience with them, opinions are welcome :)

@akshaymankar
Copy link
Member

@guoshimin I ended up using TVar from Control.Concurrent.STM, and there is no global state kept by the library itself. The caller of kubeClient is free to keep it globally if they wish to.

With that, I think the basic kubeconfig loader I wrote is usable. Would you be open to accepting that in the client? If yes, I am willing to move that code to the client and make a PR. Perhaps you can review my code here: https://github.com/akshaymankar/eirini-route-emitter/blob/master/src/KubernetesExtras/Client.hs.

@guoshimin
Copy link
Contributor

guoshimin commented Jul 5, 2019 via email

@erthalion
Copy link

@akshaymankar could you please open a PR with the implementation? I've did a bit of review, but it looks cumbersome to actually test how does it work due to various dependencies, and I hope a real PR will make it easier. Plus it will make it more visible for everyone else who want to review.

@guoshimin
Copy link
Contributor

Yeah I was going to say that too. I took a quick look and it was more or less what I was expecting. Let's make a formal PR.

akshaymankar added a commit to akshaymankar/kubernetes-client-haskell that referenced this issue Jul 16, 2019
I needed to move a few methods to TLSUtils due to circular dependencies,
this will of course break compatibility with previous versions. I will
re-export all the previous functions again.

Apart from that, there is a fair bit of code in here without any tests,
so next thing I will do is start adding unit tests to make sure the
basics are fine.

The OIDC and GCP Auth code is difficult to test at unit level, so we
may have to figure out some way to integration test those.

I noticed that the code has mixed indentation (2 spaces and 4 spaces),
all the new code in this commit is 2 space indented. I did not reindent
4 spaces lines for easier code review. We should do that after this work
is over.

[kubernetes-client#2]
@akshaymankar
Copy link
Member

@erthalion @guoshimin and anybody else interested,
I have created a draft PR #51, please feel free to leave comments on it.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 14, 2019
@akshaymankar
Copy link
Member

Closing this as #51 is merged. Feel free to re-open if something is missing.

/close

@k8s-ci-robot
Copy link
Contributor

@akshaymankar: Closing this issue.

In response to this:

Closing this as #51 is merged. Feel free to re-open if something is missing.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

7 participants