Skip to content
This repository was archived by the owner on Mar 13, 2022. It is now read-only.

Refresh GCP tokens on retrieval by overriding client config method. #92

Merged
k8s-ci-robot merged 1 commit into from
Nov 2, 2018
Merged

Refresh GCP tokens on retrieval by overriding client config method. #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions config/kube_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,24 @@ def _load_cluster_info(self):
if 'insecure-skip-tls-verify' in self._cluster:
self.verify_ssl = not self._cluster['insecure-skip-tls-verify']

def _using_gcp_auth_provider(self):
return self._user and \
'auth-provider' in self._user and \
'name' in self._user['auth-provider'] and \
self._user['auth-provider']['name'] == 'gcp'

def _set_config(self, client_configuration):
if self._using_gcp_auth_provider():
# GCP auth tokens must be refreshed regularly, but swagger expects
# a constant token. Replace the swagger-generated client config's
# get_api_key_with_prefix method with our own to allow automatic
# token refresh.
def _gcp_get_api_key(*args):
return self._load_gcp_token(self._user['auth-provider'])
client_configuration.get_api_key_with_prefix = _gcp_get_api_key

@mbohlool mbohlool Oct 31, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am ok with this hack. just to make sure we are protected when things change in configuration class of generated client, can we add proper tests to make sure get_api_key_with_prefix exists and being called the way we expected it.
I can't tell from tests that we are protected against changes in configuration class. please add them if they are missing. e.g. if get_api_key_with_prefix got rename, or if it is being called but it returns something else. I see a whitebox test for get_api_key_with_prefix, but it won't protect us against get_api_key_with_prefix being removed or renamed in configuration class).

@TrevorEdwards TrevorEdwards Nov 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. I added three new test cases which I believe cover all of the concerns that you list.

if 'token' in self.__dict__:
# Note: this line runs for GCP auth tokens as well, but this entry
# will not be updated upon GCP token refresh.
client_configuration.api_key['authorization'] = self.token

@roycaihw roycaihw Oct 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

api_key['authorization'] will still be set for GCP on config loading, but it won't be updated/persisted on refresh. Also manually setting api_key['authorization'] won't work (although I suppose currently the workarounds aren't based on doing this manually). We should document this behavior.

@TrevorEdwards TrevorEdwards Oct 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added comments to make that behavior more clear, unless you mean that you want this documented somewhere else.

The only existing workaround I know of will catch exceptions when an API call fails (via a wrapper) and then reload the config. I believe with this change, they will never hit that exception.

# copy these keys directly from self to configuration object
keys = ['host', 'ssl_ca_cert', 'cert_file', 'key_file', 'verify_ssl']
Expand Down
121 changes: 106 additions & 15 deletions config/kube_config_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import yaml
from six import PY3, next

from kubernetes.client import Configuration

from .config_exception import ConfigException
from .kube_config import (ConfigNode, FileOrData, KubeConfigLoader,
_cleanup_temp_files, _create_temp_file_with_content,
Expand All @@ -34,7 +36,9 @@

EXPIRY_DATETIME_FORMAT = "%Y-%m-%dT%H:%M:%SZ"
# should be less than kube_config.EXPIRY_SKEW_PREVENTION_DELAY
EXPIRY_TIMEDELTA = 2
PAST_EXPIRY_TIMEDELTA = 2
# should be more than kube_config.EXPIRY_SKEW_PREVENTION_DELAY
FUTURE_EXPIRY_TIMEDELTA = 60

NON_EXISTING_FILE = "zz_non_existing_file_472398324"

Expand All @@ -47,9 +51,9 @@ def _format_expiry_datetime(dt):
return dt.strftime(EXPIRY_DATETIME_FORMAT)


def _get_expiry(loader):
def _get_expiry(loader, active_context):
expired_gcp_conf = (item for item in loader._config.value.get("users")
if item.get("name") == "expired_gcp")
if item.get("name") == active_context)
return next(expired_gcp_conf).get("user").get("auth-provider") \
.get("config").get("expiry")

Expand All @@ -73,8 +77,11 @@ def _raise_exception(st):
TEST_PASSWORD = "pass"
# token for me:pass
TEST_BASIC_TOKEN = "Basic bWU6cGFzcw=="
TEST_TOKEN_EXPIRY = _format_expiry_datetime(
datetime.datetime.utcnow() - datetime.timedelta(minutes=EXPIRY_TIMEDELTA))
DATETIME_EXPIRY_PAST = datetime.datetime.utcnow(
) - datetime.timedelta(minutes=PAST_EXPIRY_TIMEDELTA)
DATETIME_EXPIRY_FUTURE = datetime.datetime.utcnow(
) + datetime.timedelta(minutes=FUTURE_EXPIRY_TIMEDELTA)
TEST_TOKEN_EXPIRY_PAST = _format_expiry_datetime(DATETIME_EXPIRY_PAST)

TEST_SSL_HOST = "https://test-host"
TEST_CERTIFICATE_AUTH = "cert-auth"
Expand Down Expand Up @@ -371,6 +378,13 @@ class TestKubeConfigLoader(BaseTestCase):
"user": "expired_gcp"
}
},
{
"name": "expired_gcp_refresh",
"context": {
"cluster": "default",
"user": "expired_gcp_refresh"
}
},
{
"name": "oidc",
"context": {
Expand Down Expand Up @@ -509,7 +523,24 @@ class TestKubeConfigLoader(BaseTestCase):
"name": "gcp",
"config": {
"access-token": TEST_DATA_BASE64,
"expiry": TEST_TOKEN_EXPIRY, # always in past
"expiry": TEST_TOKEN_EXPIRY_PAST, # always in past
}
},
"token": TEST_DATA_BASE64, # should be ignored
"username": TEST_USERNAME, # should be ignored
"password": TEST_PASSWORD, # should be ignored
}
},
# Duplicated from "expired_gcp" so test_load_gcp_token_with_refresh
# is isolated from test_gcp_get_api_key_with_prefix.
{
"name": "expired_gcp_refresh",

@roycaihw roycaihw Oct 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

expired_gcp_refresh is same as expired_gcp. Is the reason for having expired_gcp_refresh that expired_gcp will be mutated during tests?

@TrevorEdwards TrevorEdwards Oct 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Added a comment to note that.

"user": {
"auth-provider": {
"name": "gcp",
"config": {
"access-token": TEST_DATA_BASE64,
"expiry": TEST_TOKEN_EXPIRY_PAST, # always in past
}
},
"token": TEST_DATA_BASE64, # should be ignored
Expand Down Expand Up @@ -630,16 +661,20 @@ def test_load_user_token(self):
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_DATA_BASE64, loader.token)

def test_gcp_no_refresh(self):
expected = FakeConfig(
host=TEST_HOST,
token=BEARER_TOKEN_FORMAT % TEST_DATA_BASE64)
actual = FakeConfig()
fake_config = FakeConfig()
# swagger-generated config has this, but FakeConfig does not.
self.assertFalse(hasattr(fake_config, 'get_api_key_with_prefix'))

@mbohlool mbohlool Oct 31, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we add a test to make sure swagger-generated config has this?

@TrevorEdwards TrevorEdwards Nov 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="gcp",
get_google_credentials=lambda: _raise_exception(
"SHOULD NOT BE CALLED")).load_and_set(actual)
self.assertEqual(expected, actual)
"SHOULD NOT BE CALLED")).load_and_set(fake_config)
# Should now be populated with a gcp token fetcher.
self.assertIsNotNone(fake_config.get_api_key_with_prefix)
self.assertEqual(TEST_HOST, fake_config.host)
# For backwards compatibility, authorization field should still be set.
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_DATA_BASE64,
fake_config.api_key['authorization'])

def test_load_gcp_token_no_refresh(self):
loader = KubeConfigLoader(
Expand All @@ -654,20 +689,48 @@ def test_load_gcp_token_no_refresh(self):
def test_load_gcp_token_with_refresh(self):
def cred(): return None
cred.token = TEST_ANOTHER_DATA_BASE64
cred.expiry = datetime.datetime.now()
cred.expiry = datetime.datetime.utcnow()

loader = KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="expired_gcp",
get_google_credentials=lambda: cred)
original_expiry = _get_expiry(loader)
original_expiry = _get_expiry(loader, "expired_gcp")
self.assertTrue(loader._load_auth_provider_token())
new_expiry = _get_expiry(loader)
new_expiry = _get_expiry(loader, "expired_gcp")
# assert that the configs expiry actually updates
self.assertTrue(new_expiry > original_expiry)
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
loader.token)

def test_gcp_get_api_key_with_prefix(self):
class cred_old:
token = TEST_DATA_BASE64
expiry = DATETIME_EXPIRY_PAST

class cred_new:
token = TEST_ANOTHER_DATA_BASE64
expiry = DATETIME_EXPIRY_FUTURE
fake_config = FakeConfig()
_get_google_credentials = mock.Mock()
_get_google_credentials.side_effect = [cred_old, cred_new]

loader = KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
active_context="expired_gcp_refresh",
get_google_credentials=_get_google_credentials)
loader.load_and_set(fake_config)
original_expiry = _get_expiry(loader, "expired_gcp_refresh")
# Call GCP token fetcher.
token = fake_config.get_api_key_with_prefix()
new_expiry = _get_expiry(loader, "expired_gcp_refresh")

self.assertTrue(new_expiry > original_expiry)
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
loader.token)
self.assertEqual(BEARER_TOKEN_FORMAT % TEST_ANOTHER_DATA_BASE64,
token)

def test_oidc_no_refresh(self):
loader = KubeConfigLoader(
config_dict=self.TEST_KUBE_CONFIG,
Expand Down Expand Up @@ -893,5 +956,33 @@ def test_user_exec_auth(self, mock):
self.assertEqual(expected, actual)


class TestKubernetesClientConfiguration(BaseTestCase):
# Verifies properties of kubernetes.client.Configuration.
# These tests guard against changes to the upstream configuration class,
# since GCP authorization overrides get_api_key_with_prefix to refresh its
# token regularly.

def test_get_api_key_with_prefix_exists(self):
self.assertTrue(hasattr(Configuration, 'get_api_key_with_prefix'))

def test_get_api_key_with_prefix_returns_token(self):
expected_token = 'expected_token'
config = Configuration()
config.api_key['authorization'] = expected_token
self.assertEqual(expected_token,
config.get_api_key_with_prefix('authorization'))

def test_auth_settings_calls_get_api_key_with_prefix(self):
expected_token = 'expected_token'

def fake_get_api_key_with_prefix(identifier):
self.assertEqual('authorization', identifier)
return expected_token
config = Configuration()
config.get_api_key_with_prefix = fake_get_api_key_with_prefix
self.assertEqual(expected_token,
config.auth_settings()['BearerToken']['value'])


if __name__ == '__main__':
unittest.main()