search

obmarg / kazan

Kubernetes API client for Elixir
https://hex.pm/packages/kazan
MIT License
140 stars 35 forks source link

Allow token use in server auth config #39

Closed lemonsrc closed 6 years ago

lemonsrc commented 6 years ago

When using Kazan in our projects, we tend to use an auth token as opposed to passing the kubeconfig path like it is mentioned in the Kazan README. The issue we have had is that the auth header is not added to requests when using the following config because it does not match when building the list of headers.

config :kazan, :server, %{
  url: <url>,
  auth: %{
    token: <token>
  }
}

We have been using these changes [in the PR] to use an auth token from the config. Is there a better/preferred way to accomplish this or should we consider merging this in to allow for this use case?

obmarg commented 6 years ago

Nice, thanks for the contribution @lemonsrc - looks like a use case I missed when trying to improve the mix config support lately.

I do wonder if it would be better to convert your map to the appropriate Auth struct at the point it's read from the mix config. Would that work for your use case, or do you sometimes pass a map in as the server parameter to the run functions as well?

jeremytregunna commented 6 years ago

@obmarg Talking with @lemonsrc, for our use case, we only use it this way. So whatever implementation that you think needs to come in or take place, it won't affect anything we're doing so long as we can continue to use this token config in the mix config still. If you have some direction on how you'd like us to solve that for the purposes of this PR, we can do that in here too.

obmarg commented 6 years ago

@jeremytregunna ok, cool.

So I put together a WIP implementation of a fix last night in this commit. I just need to get some tests of the Server.from_map function written. Happy to do this myself, but it may take a day or two. Though if either of you guys have free time to do it before then, that'd also be good. Just let me know I guess...

obmarg commented 6 years ago

Thanks for the contribution here @lemonsrc & @jeremytregunna but I've ended up merging my implementation from #41 for this. Pretty sure it should cover your use case, but let me know if I've got that wrong.

jeremytregunna commented 6 years ago

@obmarg Cool, thank you. We've made that change, and we'll file a new issue if there are problems.