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.

Sign up for GitHub

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

Jump to bottom

Support login with role_id and secret_id #303

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Support login with role_id and secret_id #303

wants to merge 6 commits into from

Conversation

mhodovaniuk
Copy link

@mhodovaniuk mhodovaniuk commented Jun 6, 2022 •
edited
Loading

Add role secret_id support to login.

Fixes #302

@mhodovaniuk mhodovaniuk changed the title Support login with role_id and secret_id #302 Support login with role_id and secret_id Jun 6, 2022
@rossabaker
Copy link
Collaborator

Thanks! I just approved the new build, but I think we're still going to get binary compatibility exceptions. This library has a stable API, so we need to be careful about breaking changes.

There are a couple things we can do here:

  • add overloads that take the new parameter, so the original signatures are still there
  • create a "builder" pattern, so we can do something like login.withRoleSecretId(...).execute. (The uses of MeterBuilder in otel4s, and its implementation, are an example of what I'm talking about.) It's a bit more code, but it's more futureproof as new parameters get added.

@rossabaker
Copy link
Collaborator

Oh, the build passed because of the MiMa exceptions. Yeah, we're going to need to find a way to make this pass without those. Does that otel4s example make sense how the pattern might be applied here? I can give more detail if it doesn't.

@mhodovaniuk
Copy link
Author

mhodovaniuk commented Jun 9, 2022 •
edited
Loading

Thanks for the feedback!
Implemented loginAndKeep and loginAndKeepSecretLeased with builder pattern. Also, implemented login with builder pattern to make API consistent.
I moved implicits from loginAndKeep and loginAndKeepSecretLeased to Operation apply method because I could not move them to the Operation constructor - it would break usage and .apply(...) will be needed instead of (...).

@rossabaker rossabaker self-requested a review June 17, 2022 04:17
@rossabaker
Copy link
Collaborator

Sorry, got distracted, but tagging myself so I can look at again tomorrow when I'm fresh.

@mhodovaniuk
Copy link
Author

@rossabaker a gentle reminder on this.

rossabaker
rossabaker requested changes Sep 28, 2022
View reviewed changes
Copy link
Collaborator

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Sorry it took me so long to get back to this.

We still have those MiMa exceptions that we need to get rid of. I wonder if we could make the old signatures private, and delegate to your new implementations.

This looks mostly correct, but a couple comments on the builder pattern.

core/src/main/scala/com/banno/vault/Vault.scala
def apply(roleId: String)(implicit F: Concurrent[F]): F[VaultToken]
}

final case class VaultLoginOperationImpl[F[_]](client: Client[F], vaultUri: Uri, roleSecretId: Option[String] = None) extends VaultLoginOperation[F] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When this is a case class, we can't add new parameters later in a bincompat fashion, so this isn't quite the builder pattern. Making it an abstract class with a private constructor should work. You'll need to implement your own copy then. I also don't think this needs to be public.

Copy link
Author

Choose a reason for hiding this comment

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

@rossabaker thanks you for getting back to this
May I kindly ask you to provide some examples of similar implementations with "abstract class with a private constructor"?
I tried to get back to the examples that you provided before to get some insights, but it looks like the project was refactored and now it uses case class.

core/src/main/scala/com/banno/vault/Vault.scala
def apply(roleId: String, tokenLeaseExtension: FiniteDuration)(implicit A: Async[F]): Stream[F, String]
}

final case class VaultLoginAndKeepOperationImpl[F[_]](client: Client[F], vaultUri: Uri, roleSecretId: Option[String] = None) extends VaultLoginAndKeepOperation[F] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as VaultLoginOperationImpl.

@Koroeskohr
Copy link

Hello! I'm looking into this feature, I'd be willing to finish this MR if we can agree this is the implementation that should be targeted :)

@mhodovaniuk
Copy link
Author

Good luck @Koroeskohr !
As an alternative, I can recommend https://github.com/jopenlibs/vault-java-driver. It is a zero-dependency Java client. For example, my code is on the ZIO stack, so this was a huge plus. Additionally, the maintainer is very supportive; I was missing a small feature, and they merged my PR very quickly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rossabaker rossabaker rossabaker requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add secret_id support to login to Vault
3 participants
@mhodovaniuk @rossabaker @Koroeskohr