search

BetterCloud / vault-java-driver

Zero-dependency Java client for HashiCorp's Vault
https://bettercloud.github.io/vault-java-driver/
335 stars 224 forks source link

Overall code health improvements #187

Closed jetersen closed 5 years ago

jetersen commented 5 years ago

Had to rewrite most of SSLUtils only to figure out much later it was because Travis/Github CI had issues with whether the container or java had created the ssl folder used for passing files around.

For code style, I still have some issues. The com.bettercloud.vault.json package is mostly written in 2 spacing while the rest is written in 4 spacing.

Personally, I would switch to 4 spacing and 4 continuation spacing instead of 8 for continuation spacing.

bklawans commented 5 years ago

Hmm, yes, I agree that is not really a good behavior. I can think of two changes - we can still error when a list() call is being made, or modify the list() method to return a structure (ListResults or something similar) with the API status and the results list. I actually like the second idea better, but its a pretty big api change. Let me know which approach you prefer and I’ll create a new PR with the changes.

-Barry

On Aug 25, 2019, at 7:56 AM, Joseph Petersen notifications@github.com wrote:

@casz commented on this pull request.

In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java https://github.com/BetterCloud/vault-java-driver/pull/187#discussion_r317401873:

     final Vault vault = container.getVault(NONROOT_TOKEN);

public DbContainer() {

  • container = new GenericContainer("postgres:11.3-alpine")
  • super("postgres:11.3-alpine");
  • this.withNetwork(CONTAINER_NETWORK)

  • .withNetworkAliases(hostname) This should fix up the database networking problem the two merges created. @denglertai https://github.com/denglertai @steve-perkins https://github.com/steve-perkins In src/test-integration/java/com/bettercloud/vault/util/VaultContainer.java https://github.com/BetterCloud/vault-java-driver/pull/187#discussion_r317402078:

     runCommand("vault", "login", "-ca-cert=" + CONTAINER_CERT_PEMFILE, rootToken);
  • runCommand("sh", "-c", "cat <> " + CONTAINER_CLIENT_CERT_PEMFILE + "\n" + cert + "\nEOL"); Running on CI against Travis or Github it seems the vault container's uid/gid is the only one with permissions to this file.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BetterCloud/vault-java-driver/pull/187?email_source=notifications&email_token=AANPQFCFKUDXQCIRTAVLFTLQGKMSLA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTGHOQ#pullrequestreview-279339962, or mute the thread https://github.com/notifications/unsubscribe-auth/AANPQFDK4INICBGBFWXEUQTQGKMSLANCNFSM4IPJHEKA.

bklawans commented 5 years ago

Do you have a preference between enhancing LogicalResponse to have a new method (getListData()) vs creating a new response class that holds the list?

-Barry

On Aug 25, 2019, at 7:56 AM, Joseph Petersen notifications@github.com wrote:

@casz commented on this pull request.

In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java https://github.com/BetterCloud/vault-java-driver/pull/187#discussion_r317401873:

     final Vault vault = container.getVault(NONROOT_TOKEN);

public DbContainer() {

  • container = new GenericContainer("postgres:11.3-alpine")
  • super("postgres:11.3-alpine");
  • this.withNetwork(CONTAINER_NETWORK)

  • .withNetworkAliases(hostname) This should fix up the database networking problem the two merges created. @denglertai https://github.com/denglertai @steve-perkins https://github.com/steve-perkins In src/test-integration/java/com/bettercloud/vault/util/VaultContainer.java https://github.com/BetterCloud/vault-java-driver/pull/187#discussion_r317402078:

     runCommand("vault", "login", "-ca-cert=" + CONTAINER_CERT_PEMFILE, rootToken);
  • runCommand("sh", "-c", "cat <> " + CONTAINER_CLIENT_CERT_PEMFILE + "\n" + cert + "\nEOL"); Running on CI against Travis or Github it seems the vault container's uid/gid is the only one with permissions to this file.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BetterCloud/vault-java-driver/pull/187?email_source=notifications&email_token=AANPQFCFKUDXQCIRTAVLFTLQGKMSLA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTGHOQ#pullrequestreview-279339962, or mute the thread https://github.com/notifications/unsubscribe-auth/AANPQFDK4INICBGBFWXEUQTQGKMSLANCNFSM4IPJHEKA.

bklawans commented 5 years ago

If I’m reading the code right, this turned out to be pretty easy. I moved the code that builds the list into LogicalResponse, and it only triggers when the operation is ListV1 or ListV2. I’ve updated all the unit tests and they still pass, and changed the assert in testListExceptionMessageIncludesErrorsReturnedByVault to check for a 404.

-Barry

On Aug 25, 2019, at 1:56 PM, Joseph Petersen notifications@github.com wrote:

@casz commented on this pull request.

In src/test-integration/java/com/bettercloud/vault/api/LogicalTests.java https://github.com/BetterCloud/vault-java-driver/pull/187#discussion_r317414446:

     final Vault vault = container.getVault(NONROOT_TOKEN);
  • vault.logical().list("secret/null");
  • List list = vault.logical().list("secret/null");
  • assertEquals(list.size(), 0); getListData seems responsible to me as you already have getData but that is not the same as read so perhaps we need a new response class.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/BetterCloud/vault-java-driver/pull/187?email_source=notifications&email_token=AANPQFHGDOXHECZSGMUXJLDQGLWYBA5CNFSM4IPJHEKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCTJLPI#discussion_r317414446, or mute the thread https://github.com/notifications/unsubscribe-auth/AANPQFBPPFWSNS3FZXKGWXTQGLWYBANCNFSM4IPJHEKA.

jetersen commented 5 years ago

Feel free to send a PR to my branch @bklawans 😓 That way it gets tested on CI at least 😆

bklawans commented 5 years ago

@casz Do you mean casz:chore/overallCodeHealth instead of BetterCloud/master?

jetersen commented 5 years ago

Yup casz:chore/overallCodeHealth otherwise I would have to do the whole merge dance and I already did plenty to cleanup commit history. 😆

bklawans commented 5 years ago

Will do. I'm walking out the door now, but will turn that around this evening (Pacific time zone...)

jetersen commented 5 years ago

In no hurry :)

bklawans commented 5 years ago

New PR issued to merge my changes to your branch, and I revoked the PR to the main BetterCloud repo.

jetersen commented 5 years ago

Cheers @bklawans :) I wonder though @steve-perkins if you would prefer if we keep the old method with @deprecated

bklawans commented 5 years ago

I thought about that, but since the only difference is the return type we can't have both the old and the new method, since Java doesn't consider the return type to be part of the method signature. We will have to rename the new method if we want to keep the old.

jetersen commented 5 years ago

Right the will bump to v5 and see compile issues 😊 I can live with that

jetersen commented 5 years ago

Fixing the merge conflict 😅

jetersen commented 5 years ago

If you want me to squash/rebase/rewrite history before merging please say so :)

steve-perkins commented 5 years ago

Ehh... the intermingling of getListData(...) from @bklawans work, and the other general code cleanup, is a little confusing to sort through. But a squash won't help that. I think this is good.

jetersen commented 5 years ago

@steve-perkins would you mind enabling Travis so we can have CI on pull requests 😄

steve-perkins commented 5 years ago

Post-merge, the integration test LogicalTests.testListPermissionDeniedReturnedByVault() is failing, because it's expecting a 404 response code from Vault instead of a 403.

In context, I assume this has to be a typo. I'll change the expected response to 403.

jetersen commented 5 years ago

https://github.com/BetterCloud/vault-java-driver/pull/187/commits/3231da3f3739cbbb3cfabecd3a04eeecca4bcfe3 definitely a typo honestly missed that in the merge.

steve-perkins commented 5 years ago

Looking into that now. Virtually all of my recent-enough-to-be-relevant CI experience is with Jenkins, so Travis is a bit unfamiliar.

jetersen commented 5 years ago

You just need to enable it. I already added a .travis.yml file for you in this PR. https://github.com/marketplace/travis-ci FYI free for open source.

steve-perkins commented 5 years ago

Incredibly frustrating. I believe that Travis is choking on the fact that this repository is owned by an organization. I've gone through all of the commonly-recommended troubleshooting steps (e.g. making sure that my membership in the organization is set to publicly visible). But the repo is still not available for Travis to use.

I'm going to call it a day, and start cutting a release for Maven Central. I may ping Travis support tomorrow. I've also signed up for the beta of GitHub's native CI system. That sign-up request is still pending, but anyone is welcome to write a ci.yaml for that if they like.

steve-perkins commented 5 years ago

Version 5.0.0 has been published, and should appear on Maven Central within the next few hours.

jetersen commented 5 years ago

@steve-perkins the PR already added a GitHub CI file: https://github.com/BetterCloud/vault-java-driver/blob/master/.github/workflows/gradle.yml

As for you issue enabling Travis, it could be that the project is actually still on travis-ci.org the open-source one and has yet to be migrated to travis-ci.com

So if you head here: https://travis-ci.org/BetterCloud/vault-java-driver you should be able to get it the needed permissions/

Here is the Travis announcement for travis-ci.org moving to travis-ci.com 😓 https://docs.travis-ci.com/user/migrate/open-source-on-travis-ci-com/