Refresh exec-based API credentials when they expire #250
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Welcome @emenendez! |
This is a fix for kubernetes-client/python#741. As described in kubernetes-client/python#741, some of the authentication schemes supported by Kubernetes require updating the client's credentials from time to time. The Kubernetes Python client currently does not support this, except for when using the `gcp` auth scheme. This is because the OpenAPI-generated client code does not generally expect credentials to change after the client is configured. However, in OpenAPITools/openapi-generator#3594, the OpenAPI generator added a (undocumented) hook on the `Configuration` object which provides a method for the client credentials to be refreshed as needed. Now that this hook exists, the `load_kube_config()` function, used by the Kubernetes API to set up the `Configuration` object from the client's local k8s config, just needs to be updated to take advantage of this hook. This patch does this for `exec`-based authentication, which should resolve kubernetes-client/python#741. Also, as noted above, `load_kube_config()` already has a special-case monkeypatch to refresh GCP tokens. I presume this functionality was added before the OpenAPI generator added support for the refresh hook. This patch also refactors the GCP token refreshing code to use the new hook instead of the monkeypatch. Tests are also updated.
f6f9fb9 to
70b78cd
Compare
|
/assign @roycaihw |
| self.assertEqual(TEST_HOST, fake_config.host) | ||
| # For backwards compatibility, authorization field should still be set. |
There was a problem hiding this comment.
nit: why do we remove this comment?
There was a problem hiding this comment.
This test tests GCP token refresh functionality -- before this patch, Configuration.get_api_key_with_prefix() was completely replaced with a GCP-specific monkeypatch. api_key['authorization'] was never used, however, it was still set to a valid token for backwards compatibility, as this comment describes.
However, this patch refactors the GCP token refresh logic to use the new refresh_api_key_hook functionality, which requires less custom code for GCP. With this patch, api_key['authorization'] is used to hold the actual current GCP access token, as it is with other authentication types -- so we still assert that this value is set, but it's no longer only for backwards compatibility -- it's used to hold the actual token. This comment is removed to avoid confusion as it refers to logic which this patch factors out.
| def test_get_api_key_with_prefix_exists(self): | ||
| self.assertTrue(hasattr(Configuration, 'get_api_key_with_prefix')) | ||
| def test_refresh_api_key_hook_exists(self): | ||
| self.assertTrue(hasattr(Configuration(), 'refresh_api_key_hook')) |
There was a problem hiding this comment.
Why do we use Configuration() instead of Configuration?
There was a problem hiding this comment.
This change is made because this patch changes the way this code interacts with the upstream Configuration class to refresh the API tokens. Before, we overrode the get_api_key_with_prefix() method, but now we override Configuration.refresh_api_key_hook, which is created in Configuration.__init__() here: https://github.com/kubernetes-client/python/blob/master/kubernetes/client/configuration.py#L99.
Because that property is only created when Configuration is initialized, we have to switch to Configuration() here.
| if ('expiry' in self.__dict__ and _is_expired(self.expiry)): | ||
| self._load_authentication() | ||
| self._set_config(client_configuration) | ||
| client_configuration.refresh_api_key_hook = _refresh_api_key |
There was a problem hiding this comment.
Is the indentation correct? Do we want to install the hook only when token is set?
There was a problem hiding this comment.
My reasoning here was that only token-based authentication types would require refreshing -- other types (for example, certificate-based authentication) are expected to use stable credentials and therefore wouldn't need this hook.
However, if that's not a valid assumption, I don't think it would cause any harm to install this hook in all cases, other than perhaps running the refresh hook too often. Happy to update in that case!
| def test_get_api_key_with_prefix_exists(self): | ||
| self.assertTrue(hasattr(Configuration, 'get_api_key_with_prefix')) | ||
| def test_refresh_api_key_hook_exists(self): | ||
| self.assertTrue(hasattr(Configuration(), 'refresh_api_key_hook')) |
There was a problem hiding this comment.
This change is made because this patch changes the way this code interacts with the upstream Configuration class to refresh the API tokens. Before, we overrode the get_api_key_with_prefix() method, but now we override Configuration.refresh_api_key_hook, which is created in Configuration.__init__() here: https://github.com/kubernetes-client/python/blob/master/kubernetes/client/configuration.py#L99.
Because that property is only created when Configuration is initialized, we have to switch to Configuration() here.
| self.assertEqual(TEST_HOST, fake_config.host) | ||
| # For backwards compatibility, authorization field should still be set. |
There was a problem hiding this comment.
This test tests GCP token refresh functionality -- before this patch, Configuration.get_api_key_with_prefix() was completely replaced with a GCP-specific monkeypatch. api_key['authorization'] was never used, however, it was still set to a valid token for backwards compatibility, as this comment describes.
However, this patch refactors the GCP token refresh logic to use the new refresh_api_key_hook functionality, which requires less custom code for GCP. With this patch, api_key['authorization'] is used to hold the actual current GCP access token, as it is with other authentication types -- so we still assert that this value is set, but it's no longer only for backwards compatibility -- it's used to hold the actual token. This comment is removed to avoid confusion as it refers to logic which this patch factors out.
| if ('expiry' in self.__dict__ and _is_expired(self.expiry)): | ||
| self._load_authentication() | ||
| self._set_config(client_configuration) | ||
| client_configuration.refresh_api_key_hook = _refresh_api_key |
There was a problem hiding this comment.
My reasoning here was that only token-based authentication types would require refreshing -- other types (for example, certificate-based authentication) are expected to use stable credentials and therefore wouldn't need this hook.
However, if that's not a valid assumption, I don't think it would cause any harm to install this hook in all cases, other than perhaps running the refresh hook too often. Happy to update in that case!
|
Thanks for the fix. Is it likely to be approved and merged? |
|
/lgtm Sorry for the late reply. Thank you for the fix! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: emenendez, roycaihw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
As described in kubernetes-client/python#741, some of the authentication schemes supported by Kubernetes require updating the client's credentials from time to time. The Kubernetes Python client currently does not support this, except for when using the
gcpauth scheme. This is because the OpenAPI-generated client code does not generally expect credentials to change after the client is configured.However, in OpenAPITools/openapi-generator#3594, the OpenAPI generator added a (undocumented) hook on the
Configurationobject which provides a method for the client credentials to be refreshed as needed. Now that this hook exists, theload_kube_config()function, used by the Kubernetes API to set up theConfigurationobject from the client's local k8s config, just needs to be updated to take advantage of this hook.This patch does this for
exec-based authentication, which should resolve kubernetes-client/python#741.Also, as noted above,
load_kube_config()already has a special-case monkeypatch to refresh GCP tokens. I presume this functionality was added before the OpenAPI generator added support for the refresh hook. This patch also refactors the GCP token refreshing code to use the new hook instead of the monkeypatch.Tests are also updated.
Which issue(s) this PR fixes:
This is a fix for kubernetes-client/python#741, but we shouldn't resolve that issue until this change is merged to
masterand the submodule ref is updated in kubernetes-client/python.Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
N/A