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

[Merged by Bors] - LDAP authentication #374

Closed
wants to merge 59 commits into from

Conversation

vsupalov
Copy link
Contributor

@vsupalov vsupalov commented Jan 10, 2023

Description

This will resolve part of #144
The ticket can be merged once the stretch goals are reached as well.

A new iteration on the changes prototyped in #341

This iteration will include:

  • A closer resemblance to the ticket requirements - using a list of authenticators
  • Non-usage of LDAP for inter-node authentication (basic authentication instead)
  • Erroring out if both TLS auth and LDAP auth are configured

Follow-up Work

  • Interconnection with an OPA authorization config, if provided (former stretch goal)
  • Adding ldaps:// support (former stretch goal)
  • Druid does not like anonymous LDAP access (without bind credentials). I have not found a way to configure it. This however, seems to be a usecase we want to support generally.

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Please don't be afraid of the number of comments, lots of them are duplicates. Happy to have a chat :)

rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
rust/operator-binary/src/druid_controller.rs Outdated Show resolved Hide resolved
rust/operator-binary/src/druid_controller.rs Outdated Show resolved Hide resolved
rust/operator-binary/src/druid_controller.rs Outdated Show resolved Hide resolved
rust/operator-binary/src/druid_controller.rs Outdated Show resolved Hide resolved
@vsupalov vsupalov force-pushed the feature/144-ldap-authentication-v2 branch from 0961a12 to c161c00 Compare January 19, 2023 12:23
fhennig
fhennig previously approved these changes Jan 25, 2023
@vsupalov
Copy link
Contributor Author

Alright, thanks for the review folks! I will rebase and merge this branch once the release is through.

I will create a comment on the main issue with the findings from this stage, so we can keep track of follow-up tasks there.

@vsupalov
Copy link
Contributor Author

vsupalov commented Jan 26, 2023

Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

LGTM overall. Just some docs.

rust/crd/src/ldap.rs Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/usage.adoc Outdated Show resolved Hide resolved
@vsupalov
Copy link
Contributor Author

vsupalov commented Jan 30, 2023

Copy link
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

LGTM!

@vsupalov vsupalov dismissed sbernauer’s stale review January 30, 2023 15:13

Thanks! All changes have been implemented, and the PR has one approval. Merging time!

@stackabletech stackabletech deleted a comment from bors bot Jan 30, 2023
@vsupalov
Copy link
Contributor Author

bors r+

bors bot pushed a commit that referenced this pull request Jan 30, 2023
# Description

This will resolve part of #144
The ticket can be merged once the stretch goals are reached as well.

A new iteration on the changes prototyped in #341

This iteration will include:

* A closer resemblance to the ticket requirements - using a list of authenticators
* Non-usage of LDAP for inter-node authentication (basic authentication instead)
* Erroring out if both TLS auth and LDAP auth are configured

## Follow-up Work

* Interconnection with an OPA authorization config, if provided (former stretch goal)
* Adding ldaps:// support (former stretch goal)
* Druid does not like anonymous LDAP access (without bind credentials). I have not found a way to configure it. This however, seems to be a usecase we want to support generally.
@vsupalov
Copy link
Contributor Author

Here we goooo :shipit:

@bors
Copy link
Contributor

bors bot commented Jan 30, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title LDAP authentication [Merged by Bors] - LDAP authentication Jan 30, 2023
@bors bors bot closed this Jan 30, 2023
@bors bors bot deleted the feature/144-ldap-authentication-v2 branch January 30, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants