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

feat(python): add kubernetes authentication method #280

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Dents6679
Copy link

This change adds support for the kubernetes authentication method to the python server side SDK.

@Dents6679 Dents6679 requested a review from a team as a code owner August 21, 2024 15:45
Copy link
Collaborator

@erka erka left a 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.

flipt-python/flipt/authentication/__init__.py Show resolved Hide resolved
flipt-python/flipt/authentication/__init__.py Outdated Show resolved Hide resolved
@GeorgeMac
Copy link
Member

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.

Copy link
Member

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

LGTM

@GeorgeMac GeorgeMac requested a review from erka August 23, 2024 12:32
payload = {"service_account_token": service_account_token}
try:
response = requests.post(
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount",
Copy link
Collaborator

@erka erka Aug 23, 2024

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

Copy link
Member

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

Copy link
Member

@GeorgeMac GeorgeMac Aug 23, 2024

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.

Suggested change
"http://flipt:8080/auth/v1/method/kubernetes/serviceaccount",
f"http://{self.url}/auth/v1/method/kubernetes/serviceaccount",

Copy link
Member

@GeorgeMac GeorgeMac left a 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.

Comment on lines +28 to +34
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
Copy link
Member

@GeorgeMac GeorgeMac Aug 23, 2024

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.

Suggested change
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",
Copy link
Member

@GeorgeMac GeorgeMac Aug 23, 2024

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.

Suggested change
"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))
Copy link
Member

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:

Suggested change
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))
Copy link
Member

Choose a reason for hiding this comment

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

Same as above:

Suggested change
return AsyncFliptClient(url=flipt_url, authentication=KubernetesAuthentication(flipt_auth_token))
return AsyncFliptClient(url=flipt_url, authentication=KubernetesAuthentication(url=flipt_url))

@erka
Copy link
Collaborator

erka commented Sep 4, 2024

Hey @Dents6679.

Do you need any help to finish this up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants