Skip to content

[Merged by Bors] - Support LDAP using TLS#408

Closed
sbernauer wants to merge 16 commits intomainfrom
feat/ldap-tls
Closed

[Merged by Bors] - Support LDAP using TLS#408
sbernauer wants to merge 16 commits intomainfrom
feat/ldap-tls

Conversation

@sbernauer
Copy link
Copy Markdown
Member

@sbernauer sbernauer commented Feb 28, 2023

Description

Fixes #382

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

@sbernauer sbernauer marked this pull request as draft February 28, 2023 14:58
@sbernauer
Copy link
Copy Markdown
Member Author

@sbernauer
Copy link
Copy Markdown
Member Author

@sbernauer sbernauer marked this pull request as ready for review March 1, 2023 13:03
@sbernauer
Copy link
Copy Markdown
Member Author

Test succeeded, re-running after merging main

@sbernauer sbernauer self-assigned this Mar 1, 2023
@vsupalov vsupalov self-requested a review March 1, 2023 13:59
Copy link
Copy Markdown
Contributor

@vsupalov vsupalov left a comment

Choose a reason for hiding this comment

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

Looks great! I have nothing apart from small observations. I think the tradeoff "if we have ldap, we expect tls to be enabled" is reasonable, although the idealist in me complains. We can roll with it, it will probably not be an issue for any real usecase. (And for those who might complain, not having non-bind LDAP would be an issue as well probably.)

One part which is missing, is adjusting the LDAP docs over in the usage.adoc file. Once that is adjusted, we can merge this.

@sbernauer sbernauer requested a review from vsupalov March 2, 2023 13:25
Copy link
Copy Markdown
Contributor

@vsupalov vsupalov left a comment

Choose a reason for hiding this comment

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

:shipit:

@sbernauer
Copy link
Copy Markdown
Member Author

bors r+
Thx for the review!

bors bot pushed a commit that referenced this pull request Mar 2, 2023
@bors
Copy link
Copy Markdown
Contributor

bors bot commented Mar 2, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Support LDAP using TLS [Merged by Bors] - Support LDAP using TLS Mar 2, 2023
@bors bors bot closed this Mar 2, 2023
@bors bors bot deleted the feat/ldap-tls branch March 2, 2023 15:11
bors bot pushed a commit that referenced this pull request Mar 6, 2023
# Description

During resolving conflicts between #407 and #408 this line was not removed, thus creating (and overwriting!) the truststore *after* the ldap tls cert was added. Result was the ldap tls cert missing from the truststore
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.

Implement authentication with LDAP secured via TLS

2 participants