Finish HTTP proxy support in Kubernetes authenticator #2766
Conversation
micahlee
force-pushed
the
cnjr-230-websocket-proxy
branch
2 times, most recently
from
15373bf
to
d8dfaa9
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
This ensures we detect the issue in which a http proxy is not correctly used with kube exec to inject the Conjur client certificate.
micahlee
force-pushed
the
cnjr-230-websocket-proxy
branch
from
d8dfaa9
to
4906133
Compare
micahlee
force-pushed
the
cnjr-230-websocket-proxy
branch
2 times, most recently
from
964d06e
to
452a32e
Compare
micahlee
changed the title
micahlee
force-pushed
the
cnjr-230-websocket-proxy
branch
7 times, most recently
from
a7dfc17
to
3b48304
Compare
micahlee
force-pushed
the
cnjr-230-websocket-proxy
branch
from
3b48304
to
9d54d17
Compare
|
Code Climate has analyzed commit 9d54d17 and detected 5 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 99.2% (50% is the threshold). This pull request will bring the total coverage in the repository to 89.9% (-1.8% change). View more on Code Climate. |
| emit(:__close) | ||
|
|
||
| Thread.kill(@thread) if @thread |
There was a problem hiding this comment.
Is this to ensure we can safely spawn a new thread in begin_event_loop below?
It feels a bit disjointed to have the lifecycle of a variable spread across three different classes, but without changing the interface, there's not much we can do.
There was a problem hiding this comment.
I don't think it so much for spawning a new thread, but just to make sure we clean up the thread already created.
I started down a path of refactoring more of the event loop, but I became concerned about verifying that it hadn't regressed in the timeframe we have for code freeze, so I backed out of that. There is definitely room to decouple some of these lifecycle and thread management routines, though. It's really difficult to try to add unit tests for this class the way it's written right now. The _spec.rb file for the websocket client is really more of an integration test right now. It gets the job done, but is slower than it needs to be and doesn't really test all of the lifecycle edge cases.
Considering we've fixed 3+ bugs in this code over the past year, this wouldn't be a bad candidate for focusing on in a quality sprint at some point, to make this code easier to test and make changes to. I tried to do some of that with the OpenSSL setup and the proxy support, but the thread/event loop is still just kind of spaghetti code. CC: @adamouamani
There was a problem hiding this comment.
Nice work on this @micahlee. I always like seeing what these protocols look like under the hood.
Desired Outcome
The outcome of this PR is to support connecting through an HTTP proxy for all phases of the Kubernetes authenticator, including certificate injection.
Implemented Changes
Previously, the Kubernetes authenticator supported an HTTP proxy for the REST-ful API calls. For example, when checking the existence of a k8s resource. However, it failed to use the proxy server for the execute call to inject the authentication certificate. This was due to this phase using a separate websocket client that lacked proxy support.
This PR implements tests to detect the proxy implementation gap and adds support for proxies to the websocket client.
Connected Issue/Story
CyberArk internal issue ID: CNJR-230
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security