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

Add ldap testing image with active directory schema #166

Merged

Conversation

eformat
Copy link
Member

@eformat eformat commented May 19, 2023

add a testing image for open ldap server with active directory schema from openldap. see:

https://git.openldap.org/openldap/openldap/-/blob/master/servers/slapd/schema/msuser.ldif

this is linked to tests coming in this PR - "Ldap Group Provider" - trinodb/trino#17518

there are some slight tweaks to the msuser schema to make it load correctly e.g whitespace (tab for spaces) and rename a couple of clashing attributes.

for testing, the memberOf attribute is intentionally made a String. for RDN based memberOf, we can use the existing openldap image OK.

# 1.3.6.1.4.1.1466.115.121.1.12 - Distinguished Name syntax
# 1.3.6.1.4.1.1466.115.121.1.15 - Directory String syntax
# NO-USER-MODIFICATION
olcAttributeTypes: ( MSADat2:102
  NAME 'memberOf'
  SYNTAX '1.3.6.1.4.1.1466.115.121.1.15' )
#

@cla-bot
Copy link

cla-bot bot commented May 19, 2023

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@eformat eformat changed the title add ldap testing image with active directory schema Add ldap testing image with active directory schema May 19, 2023
@eformat
Copy link
Member Author

eformat commented May 23, 2023

i have signed the cla and returned on 16/5/23. cheers

@ebyhr ebyhr requested a review from Praveen2112 May 25, 2023 23:00
@eformat
Copy link
Member Author

eformat commented Jun 12, 2023

cc: @Praveen2112 .. this will need to merge as well .. for tests.

@eformat
Copy link
Member Author

eformat commented Jun 12, 2023

cc: trinodb/trino#17518

# limitations under the License.

ARG ARCH
FROM testing/centos7-oj17:unlabelled$ARCH
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to have an unlabelled image which could be shared both by OpenLDAP and AD configuration - Most of the statements are similar to OpenLDAP and it would be nice if we could extract them.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, its possible. would involve refactoring both images of course. the code + tests for LDAP authentication are not impacted at the moment - but would be if we do this

Copy link
Member Author

Choose a reason for hiding this comment

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

OK @Praveen2112 - refactored into a common base now. PTAL.

@cla-bot cla-bot bot added the cla-signed label Jun 20, 2023
@eformat
Copy link
Member Author

eformat commented Jun 21, 2023

@Praveen2112 - i may need some help around order of these builds to make this pass testing ? can you advise best way to achieve it (base will need to be built + pushed i think)

testing/centos7-oj17-openldap-base
               \
                 \     --  testing/centos7-oj17-openldap-active-directory
                   \
                     \   --  testing/centos7-oj17-openldap
                                       |
                                testing/centos7-oj17-openldap-referrals

@eformat
Copy link
Member Author

eformat commented Jun 27, 2023

@Praveen2112 i have fixed up the ldap test now - looks like a random other image test build failed which is unrelated.

What else is holding this one up from being merged ?

@Praveen2112
Copy link
Member

Thanks for fixing it. Can we squash and rearrange the commit - first commit would be the refactor one and AD can added in the next commit.

@eformat eformat force-pushed the add-centos7-oj17-openldap-active-directory branch 6 times, most recently from 30624d6 to 4502c44 Compare June 27, 2023 23:48
@eformat
Copy link
Member Author

eformat commented Jun 28, 2023

@Praveen2112 - done. thx.

@Praveen2112
Copy link
Member

The failure is not related to the changes - it is a blocker for releasing this image - I'll try to fix the failure in a different PR.

@Praveen2112
Copy link
Member

Can you rebase the PR to the recent changes in the master ?

@eformat
Copy link
Member Author

eformat commented Jul 3, 2023

Done

Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Can we remove the merge commit (last commit) and instead do a rebase - so the history could be a bit clear

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
testing/centos7-oj17-openldap-base/Dockerfile Show resolved Hide resolved
@@ -0,0 +1,4298 @@
# $OpenLDAP$
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the next commit right ?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to wget or download them from a specific url ?

Copy link
Member Author

@eformat eformat Jul 4, 2023

Choose a reason for hiding this comment

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

as documented in the Dockerfile its from here

you cant easily wget it, as there are some minor modifications required to make sure it loads properly.
its pretty old, so wont load as-is without commenting out a couple of duplicate definitions that were clearly loaded ok at some time in the distant past.

Copy link
Member

Choose a reason for hiding this comment

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

This should be in the next commit, and it should be in directory relative to AD

@eformat eformat force-pushed the add-centos7-oj17-openldap-active-directory branch 3 times, most recently from dae0079 to 2ad1117 Compare July 4, 2023 06:53
@Praveen2112
Copy link
Member

Let me know if you need any help on moving the changes across commits. Happy to help

@eformat eformat force-pushed the add-centos7-oj17-openldap-active-directory branch 2 times, most recently from c254981 to 6155136 Compare July 4, 2023 09:58
@eformat eformat force-pushed the add-centos7-oj17-openldap-active-directory branch 4 times, most recently from f145488 to e13cded Compare July 4, 2023 10:08
@eformat
Copy link
Member Author

eformat commented Jul 4, 2023

changes moved commits as requested. cheers.

@@ -0,0 +1,4298 @@
# $OpenLDAP$
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the next commit, and it should be in directory relative to AD

testing/centos7-oj17-openldap-base/Dockerfile Show resolved Hide resolved
@eformat eformat force-pushed the add-centos7-oj17-openldap-active-directory branch from 77f8830 to f08e5e6 Compare July 6, 2023 22:17
@eformat
Copy link
Member Author

eformat commented Jul 6, 2023

@Praveen2112 - can we get this merged please ? i dont have a tonne of time to keep making small changes in commit ordering. Those are cosmetic changes really to the functionality. I am happy to squash it all in one commit if that is easier for you to review.

@Praveen2112 Praveen2112 force-pushed the add-centos7-oj17-openldap-active-directory branch from f08e5e6 to 7110a2f Compare July 7, 2023 08:24
@Praveen2112 Praveen2112 merged commit 3427d29 into trinodb:master Jul 10, 2023
35 checks passed
@Praveen2112
Copy link
Member

Thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants