Fix issues with certificate based auth #42
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.
DetailsInstructions 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. |
k8s-ci-robot
added
cncf-cla: no
|
CLA updated. |
k8s-ci-robot
added
cncf-cla: yes
|
This change is also included in #39. |
k8s-ci-robot
added
size/S
DavidHuie
changed the title
DavidHuie
force-pushed
the
add-missing-require
branch
from
0b96619 to
4ea5dad
Compare
|
@drubin also cc: @brendandburns |
drubin
left a comment
drubin
left a comment
There was a problem hiding this comment.
Just had a few questions around tests (so we don't break these fixes in future).
But the changes make sense.
| end | ||
| end | ||
|
|
||
| context 'when it is already called' do |
There was a problem hiding this comment.
@DavidHuie We should maybe adjust this test so that it works with the given refactoring.
If we remove it entirely we don't have a test that checks the caching functionality isn't removed.
There was a problem hiding this comment.
Good call. I re-wrote the test to use the newest bit of code.
| require 'kubernetes/api_client' | ||
| require 'kubernetes/configuration' | ||
| require 'kubernetes/config/error' | ||
| require 'kubernetes/utils' |
There was a problem hiding this comment.
Does this mean we were missing a test that would have highlighted this?
There was a problem hiding this comment.
Yeah, although I think getting the import situation completely fixed is a bit out of the scope of this PR. IMO, all require calls should just be made within kubernetes/lib/kubernetes.rb so things like this aren't missed.
Without this import, `Kubernetes` module functions like `Kubernetes.create_temp_file_and_set` aren't actually useable from within the `KubeConfig` class, breaking certificate-based auth.
We currently save the string *path* to tempfiles within the `@temp_files` cache in the `Kubernetes` module. This is incorrect because tempfiles are deleted when Ruby garbage collects the parent `Tempfile` instance. This change replaces the string path with the `Tempfile` instance so that the files exist indefinitely. I also drop the function `cache_temp_file` since it's unused and its behavior is incorrect with regards to tempfiles.
DavidHuie
force-pushed
the
add-missing-require
branch
from
4ea5dad to
7205308
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, DavidHuie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
4 participants
This PR fixes two different bugs: a missing import and a tempfile issue. The existence of both of these bugs actually prevents using kubeconfig files containing certificates. More precise descriptions of the fixes are in the commit messages.