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

auth: define auth handlers as private hive cell #24074

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Feb 28, 2023

The "null" auth handler is defined as private hive cell within the auth manager cell. The auth manager collects all provided auth handlers in the value group "authHandlers" - which is a feature provided by hive / uber/dig.

This way, multiple auth handlers can register themselves for a different auth type.

Following auth handlers can depend on other components too, without having the manager as core component to know everything. e.g. ~mtlsAuthHandler -> ~keyProvider (which can be implemented by spiffe/spire or something else.

The authentication interface (auth request & response) needs to be defined and implemented once it's clarified.

The "null" auth handler is defined as private hive cell within the auth
manager cell. The auth manager collects all provided auth handlers in
the value group "authHandlers" - which is a feature provided by hive / uber/dig.

This way, multiple auth handlers can register themselves for a different
auth type.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter requested a review from a team as a code owner February 28, 2023 16:35
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 28, 2023
@mhofstetter mhofstetter added the release-note/misc This PR makes changes that have no direct user impact. label Feb 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 28, 2023
@mhofstetter mhofstetter added the area/servicemesh GH issues or PRs regarding servicemesh label Feb 28, 2023
@mhofstetter
Copy link
Member Author

/test

Copy link
Member

@meyskens meyskens left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Change itself LGTM, but one question for later: It seems like the drop message will include the type of auth requested, which implies that the type of auth is selectable on a per-CiliumNetworkPolicy basis.

Up until now, we've only really talked about having a single auth method configured per Cilium install. Do we anticipate having multiple auth methods available, and then the Policy chooses one, or is this mainly for testing and using the Null auth provider?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 2, 2023
@mhofstetter
Copy link
Member Author

Change itself LGTM, but one question for later: It seems like the drop message will include the type of auth requested, which implies that the type of auth is selectable on a per-CiliumNetworkPolicy basis.

Up until now, we've only really talked about having a single auth method configured per Cilium install. Do we anticipate having multiple auth methods available, and then the Policy chooses one, or is this mainly for testing and using the Null auth provider?

auth type is currently configurable on a per policy basis in the CRD - which provides the possibility to switch between multiple auth types at runtime. therefore also gets passed through datapath and finally letting the manager chose the responsible handler.

but i think we need to discuss this in detail, as this also implies, that all the necessary infrastructure needs to be in place.

@YutaroHayakawa YutaroHayakawa merged commit 1eac9cf into cilium:master Mar 3, 2023
@mhofstetter mhofstetter deleted the pr/mhofstetter/auth-handler-registration branch March 3, 2023 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants