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

Refactor oidc-authservice by adding 4 new packages #105

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

athamark
Copy link
Contributor

@athamark athamark commented Jan 5, 2023

Goal

In this PR, we refactor our oidc-authservice codebase. There are two upstream PRs that correlate with this effort:

  1. Improvements to authenticator components #76 by @kellyma2, this PR not only reorganizes the directory/packages structure of our repo, but also applies a series of changes to isolate specific dependencies (for example coreos/go-oidc) so that they are called only within the respective packages. This is a good approach, one problem with simply proceeding with reviewing this PR is that it is outdated (over 1 year) and there are many changes which are not included. Moreover, we want a different package structure than the one proposed in the PR. We did however cherry-pick one commit that complied with our target.
  2. Introduce pkg/common for common functionality across the code #34, this is an old PR from @yanniszark and @apyrgio which proposes the introduction of pkg/common.

We examined these two efforts and concluded to a solution that hopefully combines the best aspects of both. Undoubtedly, we deviated from the above approaches wherever necessary.

Useful Material

Here is a codebase refactoring in Golang article that provides the necessary pronciples that we need to considerate when refactoring our code: https://go.dev/talks/2016/refactor.article. One important thing that we need to keep in mind is that the package structure we will implement needs to avoid any circular dependencies.

The functions, variables, structs, struct fields, interfaces and constant values names that we want to export from every one of the new packages, need to start with a capital otherwise they will not be accessible/visible to the caller. Source: https://go.dev/tour/basics/3

Exported names
In Go, a name is exported if it begins with a capital letter.
[...]
When importing a package, you can refer only to its exported names. Any "unexported" names are not accessible from outside the package.

Proposed Implementation

The goal is to introduce 4 new packages:

  1. common
  2. oidc
  3. sessions
  4. authenticators

We need to introduce these packages in the order described above.

Local package dependencies

  1. main package will import authenticators, session, oidc and common
  2. authenticators package will import sessions, oidc and common
  3. sessions package will import oidc and common
  4. oidc package will import common

To ensure that no circular dependency gets introduced we will have to transfer some functions to different files.

File structure

Here is what each directory package will include, in parenthesis I expose the old filename:

/authenticators
  |- authenticator.go
  |- idtoken.go (authenticator_idtoken.go)
  |- jwt_test.go (authenticator_jwt_test.go)
  |- jwt.go (authenticator_jwt.go)
  |- kubernetes.go (authenticator_kubernetes.go)
  |- opaque_test.go (authenticator_opaque_test.go)
  |- opaque.go (authenticator_opaque.go)
  |- session.go (authenticator_session.go)
/common
  |- errors.go
  |- settings_test.go
  |- settings.go
  |- transformer_userid_test.go
  |- transformer_userid.go
  |- util_test.go
  |- util.go
/oidc
  |- oidc_test.go
  |- oidc.go
  |- revoke.go
/sessions
  |- boltdb.go (session_store_boltdb.go)
  |- redis.go (session_store_redis.go)
  |- session.go
  |- state.go

Note to the reviewer

Since the #103 PR is not yet merged, we worked on top of it.

@athamark athamark requested a review from johnbuluba January 5, 2023 13:01
athamark pushed a commit that referenced this pull request Jan 5, 2023
Introduced a new common package which includes commonly used variables,
functions and constants across our code. This package also includes the
respective unit tests. Also factored out the definition of a Cacheable
authenticator.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Jan 5, 2023
Cleaned up direct deps on go-oidc. Established a clear interface for
for what we require from go-oidc and isolated that inside the oidc
package. This enables us to consolidate our OIDC related operations in
a single place as well as reducing irritating collisions between
go-oidc and the internal oidc package.

GitHub-PR: #105

Signed-off-by: Mike Kelly <[email protected]>
Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Jan 5, 2023
Isolated the coreos/go-oidc dependencies within our new sessions
package. Under the sessions package we have all the session related
functions and variables.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Jan 5, 2023
Add a new authenticators package which includes the implementations of
all the authentication methods that oidc-authservice currently
supports. Isolate the dependency to the
"k8s.io/apiserver/pkg/authentication/authenticator" package, inside the
new authenticators package.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
@athamark athamark force-pushed the feature-athamark-pkg-refactoring branch from 541bac2 to 0e63d97 Compare January 5, 2023 13:05
@elikatsis elikatsis requested review from elikatsis and removed request for johnbuluba February 3, 2023 13:38
athamark pushed a commit that referenced this pull request Feb 3, 2023
Introduced a new common package which includes commonly used variables,
functions and constants across our code. This package also includes the
respective unit tests. Also factored out the definition of a Cacheable
authenticator.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Feb 3, 2023
Cleaned up direct deps on go-oidc. Established a clear interface for
for what we require from go-oidc and isolated that inside the oidc
package. This enables us to consolidate our OIDC related operations in
a single place as well as reducing irritating collisions between
go-oidc and the internal oidc package.

GitHub-PR: #105

Signed-off-by: Mike Kelly <[email protected]>
Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Feb 3, 2023
Isolated the coreos/go-oidc dependencies within our new sessions
package. Under the sessions package we have all the session related
functions and variables.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
athamark pushed a commit that referenced this pull request Feb 3, 2023
Add a new authenticators package which includes the implementations of
all the authentication methods that oidc-authservice currently
supports. Isolate the dependency to the
"k8s.io/apiserver/pkg/authentication/authenticator" package, inside the
new authenticators package.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
@athamark athamark force-pushed the feature-athamark-pkg-refactoring branch from 0e63d97 to 3f80ac1 Compare February 3, 2023 13:53
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Introduce a new 'common' package which includes commonly used variables,
functions and constants across our code. This package also includes the
respective unit tests. Also factor out the definition of a Cacheable
authenticator.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Clean up direct imports of 'go-oidc'. Establish a clear interface for
for what we require from 'go-oidc' and isolate it inside the 'oidc'
package. This enables to consolidate the OIDC related operations in a
single place as well as to reduce irritating collisions between
'go-oidc' and the internal 'oidc' package.

GitHub-PR: #105

Signed-off-by: Mike Kelly <[email protected]>
Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Isolate the 'gorilla/sessions' dependencies within our new 'sessions'
package. Under the 'sessions' package we have all the session related
functions and variables.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Add a new authenticators package which includes the implementations of
all the authentication methods that oidc-authservice currently supports.
Isolate the dependency to the
"k8s.io/apiserver/pkg/authentication/authenticator" package, inside the
new authenticators package.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
Introduce a new 'common' package which includes commonly used variables,
functions and constants across our code. This package also includes the
respective unit tests. Also factor out the definition of a Cacheable
authenticator.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Clean up direct imports of 'go-oidc'. Establish a clear interface for
for what we require from 'go-oidc' and isolate it inside the 'oidc'
package. This enables to consolidate the OIDC related operations in a
single place as well as to reduce irritating collisions between
'go-oidc' and the internal 'oidc' package.

GitHub-PR: #105

Signed-off-by: Mike Kelly <[email protected]>
Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Isolate the 'gorilla/sessions' dependencies within our new 'sessions'
package. Under the 'sessions' package we have all the session related
functions and variables.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
elikatsis pushed a commit that referenced this pull request Feb 7, 2023
Add a new authenticators package which includes the implementations of
all the authentication methods that oidc-authservice currently supports.
Isolate the dependency to the
"k8s.io/apiserver/pkg/authentication/authenticator" package, inside the
new authenticators package.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
Athanasios Markou added 3 commits February 7, 2023 16:23
Clean up direct imports of 'go-oidc'. Establish a clear interface for
for what we require from 'go-oidc' and isolate it inside the 'oidc'
package. This enables to consolidate the OIDC related operations in a
single place as well as to reduce irritating collisions between
'go-oidc' and the internal 'oidc' package.

GitHub-PR: #105

Signed-off-by: Mike Kelly <[email protected]>
Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
Isolate the 'gorilla/sessions' dependencies within our new 'sessions'
package. Under the 'sessions' package we have all the session related
functions and variables.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
Add a new authenticators package which includes the implementations of
all the authentication methods that oidc-authservice currently supports.
Isolate the dependency to the
"k8s.io/apiserver/pkg/authentication/authenticator" package, inside the
new authenticators package.

GitHub-PR: #105

Signed-off-by: Athanasios Markou <[email protected]>
Reviewed-by: Ilias Katsakioris <[email protected]>
@elikatsis elikatsis force-pushed the feature-athamark-pkg-refactoring branch from 3f80ac1 to ce07332 Compare February 7, 2023 14:24
@elikatsis elikatsis merged commit ce07332 into master Feb 7, 2023
@elikatsis elikatsis deleted the feature-athamark-pkg-refactoring branch February 7, 2023 14:26
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.

2 participants