-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat(python): add kubernetes authentication method #280
base: main
Are you sure you want to change the base?
Conversation
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.
Hey @Dents6679.
Thank you for your contributions! Would mind signing the DCO so we could continue with this?
There are few places that I would like you to review. Is it possible to write the tests for KubernetesAuthentication
? It would be nice to have them.
Hey @erka 👋 I introduced Tom to this missing feature and offered to put some tests in place for him on this (just for clarity). I will get round to it before the week is out 🤞 Im hoping to bring over some of the stuff we have in Flipt dagger pipelines to fabricate a service account token. We will see haha, or I might bail on that and just mock stuff out. |
cf8afd3
to
5cf5c32
Compare
…n SDK Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
…n error. Signed-off-by: Tom Dent <[email protected]>
…ic datetime parser. Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
Signed-off-by: Tom Dent <[email protected]>
…n, again). Signed-off-by: Tom Dent <[email protected]>
…n, again, again). Signed-off-by: Tom Dent <[email protected]>
…n, again, again, again). Signed-off-by: Tom Dent <[email protected]>
…n, again, again, again, again). Signed-off-by: Tom Dent <[email protected]>
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.
LGTM
payload = {"service_account_token": service_account_token} | ||
try: | ||
response = requests.post( | ||
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount", |
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.
is it possible that people may have some other hostnames to access the Flipt in their cluster? @GeorgeMac
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.
Good spot! This has been hardcoded to pass the test haha
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.
See my suggestion above to pass in the URL as an argument to the Kuberentes client @Dents6679. Then we can thread the URL in here before we make the request.
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount", | |
f"http://{self.url}/auth/v1/method/kubernetes/serviceaccount", |
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.
Hey @Dents6679 we just got a good bit of feedback from @erka
Here are some suggestions to get this code working with the correct URL for flipt.
class KubernetesAuthentication(AuthenticationStrategy): | ||
default_service_token_path = "/var/run/secrets/kubernetes.io/serviceaccount/token" # noqa: S105 | ||
|
||
def __init__(self, token: str, service_account_token_path: str = default_service_token_path) -> None: | ||
self.token = token | ||
self.service_account_token_path = service_account_token_path | ||
self.token_expiry = None |
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.
@erka has correctly pointed out that we have hard-coded our URL to be one that works in our test suite, but needs to be configurable in the real world (and the test suite will set it appropriately).
I also noticed that we're passing in a token, that we won't need or know when we create this client. So instead lets change this parameter to be a string for the url.
class KubernetesAuthentication(AuthenticationStrategy): | |
default_service_token_path = "/var/run/secrets/kubernetes.io/serviceaccount/token" # noqa: S105 | |
def __init__(self, token: str, service_account_token_path: str = default_service_token_path) -> None: | |
self.token = token | |
self.service_account_token_path = service_account_token_path | |
self.token_expiry = None | |
class KubernetesAuthentication(AuthenticationStrategy): | |
default_service_token_path = "/var/run/secrets/kubernetes.io/serviceaccount/token" # noqa: S105 | |
def __init__(self, url: str, service_account_token_path: str = default_service_token_path) -> None: | |
self.service_account_token_path = service_account_token_path | |
self.url = url | |
self.token = None | |
self.token_expiry = None |
payload = {"service_account_token": service_account_token} | ||
try: | ||
response = requests.post( | ||
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount", |
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.
See my suggestion above to pass in the URL as an argument to the Kuberentes client @Dents6679. Then we can thread the URL in here before we make the request.
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount", | |
f"http://{self.url}/auth/v1/method/kubernetes/serviceaccount", |
|
||
@pytest.fixture(scope="session") | ||
def sync_k8s_flipt_client(flipt_url, flipt_auth_token): | ||
return FliptClient(url=flipt_url, authentication=KubernetesAuthentication(flipt_auth_token)) |
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.
As per above comments, we're changing this argument to be the url that Flipt is on:
return FliptClient(url=flipt_url, authentication=KubernetesAuthentication(flipt_auth_token)) | |
return FliptClient(url=flipt_url, authentication=KubernetesAuthentication(url=flipt_url)) |
|
||
@pytest.fixture | ||
def async_k8s_flipt_client(flipt_url, flipt_auth_token): | ||
return AsyncFliptClient(url=flipt_url, authentication=KubernetesAuthentication(flipt_auth_token)) |
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.
Same as above:
return AsyncFliptClient(url=flipt_url, authentication=KubernetesAuthentication(flipt_auth_token)) | |
return AsyncFliptClient(url=flipt_url, authentication=KubernetesAuthentication(url=flipt_url)) |
Hey @Dents6679. Do you need any help to finish this up? |
This change adds support for the kubernetes authentication method to the python server side SDK.