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

create entraid credentials provider #3250

Open
wants to merge 5 commits into
base: support-aad
Choose a base branch
from

Conversation

ofekshenawa
Copy link
Collaborator

@ofekshenawa ofekshenawa commented Jan 28, 2025

  • Create Credential Provider for EntraID
  • Create Token-Manager

andy-stark-redis and others added 4 commits January 17, 2025 10:29
* DOC-4560 basic transaction example

* DOC-4560 added pipe/transaction examples
* DOC-4450 added hgetall and hvals doc examples

* DOC-4449 added hgetall and hvals doc examples

* DOC-4449 rewrote to avoid Collect and Keys functions (not available in test version of Go)

* DOC-4449 replaced slices.Sort function with older alternative

* DOC-4449 removed another instance of slices.Sort

* DOC-4449 fixed bugs in tests

* DOC-4449 try sort.Strings() for sorting key lists

---------

Co-authored-by: Vladyslav Vildanov <[email protected]>
…ests will be. (#3241)

* Sort the slices of strings in doctest to make the output deterministic

* fix wording
@ndyakov
Copy link
Collaborator

ndyakov commented Jan 30, 2025

As far as I understood the idea, we would like to have this outside of the go-redis repo (maybe a new repo in http://github.com/redis-developer/ ? ). @vladvildanov can correct me if i'm wrong.

I will leave some comments related to naming things, other than that let's have a full review when we move it.

@@ -0,0 +1,129 @@
package entra_id
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the official go styleguide it is suggested to not use _ in package names. Let's rename the package to entraid or even if we end up with a different name once we move the repo, we can follow the styleguide.

)

// EntraIdIdentityProvider defines the interface for an identity provider
type EntraIdIdentityProvider interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we think about how this interface will look when called outside of this package it will be entra_id.EntraIdIdentityProvidor . As you can see there is a repetition - EntraId which we drop. Here is a brief section related to repetition in the go styleguide

}

// EntraIdCredentialsProvider manages credentials and token lifecycle
type EntraIdCredentialsProvider struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This type can also be renamed to CredentialsProvider


go 1.18.0

toolchain go1.23.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are not required to specify a toolchain. If we specify that this module needs go 1.18 any available toolchain that is with a higher version will be used. Let's remove this line

@vladvildanov
Copy link
Collaborator

@ofekshenawa @ndyakov is right. All EntraID specific code should be outside of go-redis and should be optional for end user. If user needs EntraID authentication he should add an additional dependency to it's project. However, all the common interfaces (e.g TokenManager, IdentityProvider, CredentialProvider) should be a part of go-redis, so it will be possilble to plug any CredentialProvider from outside. Check "Packaging" section in the design document

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

Successfully merging this pull request may close these issues.

4 participants