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

Onyx 16433 #2482

Merged
merged 2 commits into from
Feb 10, 2022
Merged

Onyx 16433 #2482

merged 2 commits into from
Feb 10, 2022

Conversation

foxefj-cyber
Copy link
Contributor

Desired Outcome

Please describe the desired outcome for this PR. Said another way, what was
the original request that resulted in these code changes? Feel free to copy
this information from the connected issue.

Implemented Changes

Adding ability for the kubernetes client to use the environment defined http(s)_proxy environment variable when connecting to kubernetes.

Connected Issue/Story

CyberArk internal issue link: ONYX-16433

Definition of Done

At least 1 todo must be completed in the sections below for the PR to be
merged.

Changelog

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a
    CHANGELOG update

Test coverage

  • This PR includes new unit and integration tests to go with the code
    changes, or
  • The changes in this PR do not require tests

Documentation

  • Docs (e.g. READMEs) were updated in this PR
  • A follow-up issue to update official docs has been filed here: insert issue ID
  • This PR does not require updating any documentation

Behavior

  • This PR changes product behavior and has been reviewed by a PO, or
  • These changes are part of a larger initiative that will be reviewed later, or
  • No behavior was changed with this PR

Security

  • Security architect has reviewed the changes in this PR,
  • These changes are part of a larger initiative with a separate security review, or
  • There are no security aspects to these changes

@foxefj-cyber foxefj-cyber force-pushed the ONYX-16433 branch 2 times, most recently from de1f890 to a0b075f Compare February 9, 2022 19:00
@foxefj-cyber foxefj-cyber requested review from adamouamani and a team February 9, 2022 19:00
@foxefj-cyber foxefj-cyber marked this pull request as ready for review February 9, 2022 19:00
@@ -0,0 +1,3 @@
FROM travix/tinyproxy:latest

COPY proxy/tinyproxy.conf /etc/tinyproxy.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding the final newline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@@ -59,7 +59,8 @@ def options
ssl_options: {
cert_store: @cert_store,
verify_ssl: OpenSSL::SSL::VERIFY_PEER
}
},
http_proxy_uri: ENV['https_proxy'] ? ENV['https_proxy'] : ENV['http_proxy']
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're treating ENV['https_proxy'] as a truthy value anyway, this can be simplified to:

          http_proxy_uri: ENV['https_proxy'] || ENV['http_proxy']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 25 to 26
#oc adm policy remove-scc-from-user anyuid -z default
#oc --ignore-not-found=true delete project $CONJUR_AUTHN_K8S_TEST_NAMESPACE
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really not needed anymore? (I'm most curious about not deleting the project/namespace)

If not, can we delete the lines rather than commenting them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uncommented them


# Rails.logger writes the logs to the environment log file
oc exec $pod_name -- bash -c "cat /opt/conjur-server/log/test.log" >> "output/$PLATFORM-authn-k8s-logs.txt"

echo "Printing Logs from Conjur to the console"
echo "==========================="
cat "output/$PLATFORM-authn-k8s-logs.txt"
#cat "output/$PLATFORM-authn-k8s-logs.txt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be uncommented again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 184 to 186
# set test password value
# password=$(openssl rand -hex 12)
# conjurcmd conjur variable values add inventory-db/password $password
#password=$(openssl rand -hex 12)
#conjurcmd conjur variable values add inventory-db/password $password
Copy link
Contributor

Choose a reason for hiding this comment

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

I realize that you just cleaned up the formatting of these lines, but if they were commented out before you started working on it, we should just remove these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

that is defined in the id and the kubernetes host can be reached through a
http_proxy

@http_proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing there must be some sort of behind-the-scenes magic connecting this tag to the ci/authn-k8s/dev/dev_conjur_http_proxy.template.yaml?

It would probably be worth adding a small comment above this test explaining what it does, and perhaps link to the template file for reference in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment added :)

ci/authn-k8s/test_oc_entrypoint.sh Outdated Show resolved Hide resolved
app/domain/authentication/authn_k8s/k8s_object_lookup.rb Outdated Show resolved Hide resolved
app/domain/authentication/authn_k8s/k8s_object_lookup.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
codihuston
codihuston previously approved these changes Feb 10, 2022
Copy link
Contributor

@codihuston codihuston left a comment

Choose a reason for hiding this comment

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

Looks like you took care of Micah's remarks. I'm going to LGTM this!

Copy link
Contributor

@codihuston codihuston left a comment

Choose a reason for hiding this comment

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

LGTM!

@codeclimate
Copy link

codeclimate bot commented Feb 10, 2022

Code Climate has analyzed commit b3b8d6f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.0% (-1.4% change).

View more on Code Climate.

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

Successfully merging this pull request may close these issues.

None yet

3 participants