-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
add azure auth provider #225
Conversation
Thanks for the contribution, @Hanspagh.
Yeah it's not currently planned to implement updating the k8s config. After all, that file is not
You can just use K8s.Client.MintHTTPProvider.request(:get, "https:/...", nil, headers, [])
K8s.Client.MintHTTPProvider.request(:post, "https:/...", body, headers, []) |
Sorry for the delay on this, this now includes the fetching access token from a refresh token |
@@ -194,7 +194,7 @@ defmodule K8s.Client.MintHTTPProvider do | |||
@spec get_content_type(keyword()) :: binary | nil | |||
defp get_content_type(headers) do | |||
case List.keyfind(headers, "content-type", 0) do | |||
{_key, content_type} -> content_type | |||
{_key, content_type} -> content_type |> String.split(";") |> List.first() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you elaborate on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The azure end point is returning a response with content type application/json; charset=utf-8
since the charset is not needed for the parsing I just threw it away
lib/k8s/conn/auth/azure.ex
Outdated
:post, | ||
URI.new!("https://login.microsoftonline.com/#{tenant}/oauth2/v2.0/token"), | ||
payload, | ||
%{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function expects:
- uppercase string as method ("POST")
- a list of tuples, e.g. [{"Content-Type", "application/x-www-form-urlencoded"}] as headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the spec
@callback request(
method :: atom(),
uri :: URI.t(),
body :: binary,
headers :: list(),
http_opts :: keyword()
) :: response_t()
I think it is correct to use the post atom, but I will change the header to a list
test/k8s/conn/auth/azure_test.exs
Outdated
}} = Azure.create(auth, nil) | ||
end | ||
|
||
@tag :skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or remove/replace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it mostly to demonstrate that we can indeed make a HTTP request against the azure api. I will remove it, not sure we will get much from mocking this out.
Very cool, thanks. I've gone over it, just a few comments and a hint on why Dialyzer might be unhappy. |
Very nice! Thanks for the contribution, @Hanspagh! |
Added barebone azure auth provider.
Currently, it is missing one functionality which is tagged in the code as a TODO, this is writing back the refreshed token when a new one is generated with a refresh token. I am not sure this is possible with the current setup?
My previous implementation used httppoison to make the refresh token request, what is your opinion on this, should we use mint or depend on one more library? If we use mint I might need a little help getting it to work.
Based on https://github.com/kubernetes-client/python/blob/055fa706b8677207091251998dca80cab5d5afb0/kubernetes/base/config/kube_config.py#L317
closes: #162
Requirements for all pull requests
Additional requirements for new features