Add and use auth factory by Isan-Rivkin · Pull Request #8821 · treeverse/lakeFS

Isan-Rivkin · 2025-03-20T07:24:25Z

Closes #8822

Additional Manual testing performed:

Upgrade OSS version (No auth)
Upgrade OSS no RBAC to RBAC (on-prem solution)

github-actions · 2025-03-20T07:30:17Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-03-20T07:30:27Z

E2E Test Results - Quickstart

nopcoder

We should avoid using IsAdvancedAuth the factory should provide implementation - basic or advanced.

nopcoder · 2025-03-25T10:08:20Z

pkg/auth/cache.go

+type (
+	CredentialSetFn     func() (*model.Credential, error)
+	UserSetFn           func() (*model.User, error)
+	UserPoliciesSetFn   func() ([]*model.Policy, error)
+	ExternalPrincipalFn func() (*model.ExternalPrincipal, error)
+)


from a quick look it seems like we need to start using generics here

Elaborate?
I explored several options here but the small changes don't seem very valuable and others i had required larger design that i prefer to be out of scope.

not a blocker. but it looks like a copy paste for each type we like to cache

It's indeed the case.
I'm comfortable with this since it happens once in a blue moon.
But open to suggestions

nopcoder · 2025-03-25T10:08:55Z

pkg/auth/factory/build.go

+	"github.com/treeverse/lakefs/pkg/config"
+	"github.com/treeverse/lakefs/pkg/kv"
+	"github.com/treeverse/lakefs/pkg/logging"
+)
+
+var errSimplifiedOrExternalAuth = errors.New(`cannot set auth.ui_config.rbac to non-simplified without setting an external auth service`)


nit;

Suggested change

"github.com/treeverse/lakefs/pkg/config"

"github.com/treeverse/lakefs/pkg/kv"

"github.com/treeverse/lakefs/pkg/logging"

)

var errSimplifiedOrExternalAuth = errors.New(`cannot set auth.ui_config.rbac to non-simplified without setting an external auth service`)

"github.com/treeverse/lakefs/pkg/config"

"github.com/treeverse/lakefs/pkg/kv"

"github.com/treeverse/lakefs/pkg/logging"

)

var errSimplifiedOrExternalAuth = errors.New("cannot set auth.ui_config.rbac to non-simplified without setting an external auth service")

Isan-Rivkin · 2025-03-25T11:47:12Z

pkg/auth/cache.go

 	return v.([]*model.Policy), nil
 }

+func (c *LRUCache) GetExternalPrincipal(key string, setFn ExternalPrincipalFn) (*model.ExternalPrincipal, error) {


everything in cache.go is copied from enterprise

Isan-Rivkin · 2025-03-25T11:49:20Z

We should avoid using IsAdvancedAuth the factory should provide implementation - basic or advanced.

I agree but we have some other considerations - The IsAdvancedAuth is used to determine flow during setup, and indicator for our logging middlewares.
The AuthAPI doesn't know if it's advanced or not if we don't tell it, since it also uses ACL.

nopcoder

talked f2f about the use of is advance auth. thanks.

Isan-Rivkin · 2025-03-25T15:05:47Z

@nopcoder FYI, I copied a BUG from fluffy, using policyCache cache.Cache for external principals, added small commit after approve to use dedicated class.

Isan-Rivkin · 2025-03-25T15:21:13Z

Fixed unit-test missing new param

yonipeleg33

LGTM, very nice work!
Had some minor, non-blocking comments.
I only block because I think this PR calls for more testing - aside from the unit tests, I'd like to see at least a description of manual tests performed on this branch, including backwards compatibility - spinning up a new binary after an old one should proceed from the same state in the auth db seamlessly.

yonipeleg33 · 2025-03-25T15:48:44Z

cmd/lakefs/cmd/run.go

 		defer func() { _ = c.Close() }()

-		// usage report setup - default usage reporter is a no-op
+		// usage report setup - default usage reporter ids a no-op


revert, looks like a typo you added

yonipeleg33 · 2025-03-25T15:53:24Z

pkg/auth/factory/build.go

this file is a bit odd to me. why not inline everything into modules/auth/factory/build.go?
a good sign that it's serving a questionable purpose is that you call it authfactoryhelper when importing - "helper" is a vague term.

it's not blocking, just not sure what's the purpose

The initiation logic needs to be re-used in enterprise as well, for context, in enterprise the build new function of the factory looks like this:

import ossauthfactory "github.com/treeverse/lakefs/pkg/auth/factory" func NewAuthService(ctx context.Context, cfg ossconfig.Config, logger logging.Logger, kvStore kv.Store, metadataManager *ossauth.KVMetadataManager) ossauth.Service { if !cfg.(*config.Config).Features.RBAC { logging.ContextUnavailable().Info("Creating non-local rbac service") return ossauthfactory.NewAuthService(ctx, cfg, logger, kvStore, metadataManager) } // ... // return RBAC local service return auth.NewAuthService(...) }

yonipeleg33 · 2025-03-25T15:58:32Z

pkg/auth/service.go

 	logger                    logging.Logger
 	cache                     Cache
 	externalPrincipalsEnabled bool
+	isAdvancedAuth            bool // Using RBAC, not ACL


then why not call it isRbacEnabled?
i can get why you want to use more general terms like "advanced auth" at the interface level, but this is the concrete value here - i don't see a reason not to give it the proper name it deserves.

this code makes more sense to me as a reader:

func (a *APIAuthService) IsAdvancedAuth() bool { return a.isRbacEnabled }

Not done, I am maintaining the terminology we have.
Let's decouple between how we determine ( == RBAC External or External) VS the meaning (advanced auth).
We have an existing terminology which is NoAuth, Simplified Auth (ACL) and Advanced Auth (RBAC Server with more).
The current best way to determine were using "rbac + other features" is based on the RBAC value but that's not the meaning it's just the best we have.
The auth service doesn't need to know RBAC or not. That's encapsulated at the config level, like it was before.

sure, i get the decoupling idea. but your comment says otherwise :-)
if the comment is correct, there should be no issue with fusing the comment into the variable name. same meaning, stronger semantics.

(still not blocking)

Isan-Rivkin · 2025-03-25T17:09:09Z

LGTM, very nice work! Had some minor, non-blocking comments. I only block because I think this PR calls for more testing - aside from the unit tests, I'd like to see at least a description of manual tests performed on this branch, including backwards compatibility - spinning up a new binary after an old one should proceed from the same state in the auth db seamlessly.

You're asking a good question - how was this tested, updated in the description PR description now!

yonipeleg33

If you could specify a bit more about the tests that'd be great, I'm not sure what does "Upgrade OSS version" mean exactly.
A short explanation like "built lakefs from master, ran with such and such config, then ran X..." would be very helpful for me.

But not blocking any longer, as there's at least minimal documentation of the tests performed. TYVM, and again - awesome work 💪

add and use auth factory

0ba3f9a

Isan-Rivkin self-assigned this Mar 20, 2025

Isan-Rivkin requested a review from yonipeleg33 March 20, 2025 07:24

Isan-Rivkin added the exclude-changelog PR description should not be included in next release changelog label Mar 20, 2025

Isan-Rivkin removed the request for review from yonipeleg33 March 20, 2025 08:00

Isan-Rivkin added 8 commits March 23, 2025 15:37

update principals local cache

2eb7d66

auth builder

5ed0d67

play with go sum

33d39db

update auth factory

4cc164f

Merge branch 'master' into auth-modules

4058bb6

added auth service checks

51dc12f

fix type

0fb9f4b

fix linter

2149ef7

Isan-Rivkin requested review from nopcoder and yonipeleg33 March 25, 2025 09:41

nopcoder requested changes Mar 25, 2025

View reviewed changes

Isan-Rivkin commented Mar 25, 2025

View reviewed changes

fix quotes

ffce504

Isan-Rivkin requested a review from nopcoder March 25, 2025 11:48

nopcoder approved these changes Mar 25, 2025

View reviewed changes

Isan-Rivkin added 2 commits March 25, 2025 17:04

BUG: fix cache instance for external principal

427105a

init lru cache

5b8ab93

Isan-Rivkin added 2 commits March 25, 2025 17:19

fix unittest oopsie

407bc48

fix unit-test missing param

836f6ea

update unit test to work with additional client new

bdc99a6

yonipeleg33 suggested changes Mar 25, 2025

View reviewed changes

Isan-Rivkin requested a review from yonipeleg33 March 25, 2025 17:09

yonipeleg33 approved these changes Mar 25, 2025

View reviewed changes

Isan-Rivkin merged commit 236e1a7 into master Mar 26, 2025
43 checks passed

Isan-Rivkin deleted the auth-modules branch March 26, 2025 08:14

Conversation

Isan-Rivkin commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results - DynamoDB Local - Local Block Adapter

Uh oh!

github-actions bot commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results - Quickstart

Uh oh!

nopcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isan-Rivkin commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nopcoder left a comment

Choose a reason for hiding this comment

Uh oh!

Isan-Rivkin commented Mar 25, 2025

Uh oh!

Isan-Rivkin commented Mar 25, 2025

Uh oh!

yonipeleg33 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isan-Rivkin Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonipeleg33 Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isan-Rivkin commented Mar 25, 2025

Uh oh!

yonipeleg33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Isan-Rivkin commented Mar 20, 2025 •

edited

Loading

github-actions bot commented Mar 20, 2025 •

edited

Loading

github-actions bot commented Mar 20, 2025 •

edited

Loading

Isan-Rivkin commented Mar 25, 2025 •

edited

Loading

Isan-Rivkin Mar 25, 2025 •

edited

Loading

yonipeleg33 Mar 25, 2025 •

edited

Loading