[full-ci] adapt for new LDAP config #3476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
rhafer
force-pushed
the
share-ldap-con
branch
11 times, most recently
from
e531a86 to
8190db8
Compare
Allow to pass comma-separated strings via Enviroment variables and store them in a string slice.
This adapts the storage service to the recent changes of the LDAP settings in reva. Also we define a new set of LDAP env variables that can be shared between all LDAP related ocis services (graph, idp, storage-auth-basic, storage-userprovider, storage-groupprovider, idm). This should simplify the most LDAP based configurations considerably.
webUISharingInternalGroupsEdgeCases/shareWithGroupsEdgeCases.feature:41 no longer fails as cs3org/reva#2708 fixed some issue with LDAP filter escaping.
rhafer
requested review from
wkloucek,
micbar,
phil-davis and
individual-it
as code owners
|
Kudos, SonarCloud Quality Gate passed! |
|
You fixed a test scenario regarding group sharing. There is good stuff in that PR 👍 |
butonic
reviewed
Apr 11, 2022
| UUIDAttribute: "uid", | ||
| UUIDAttributeType: "text", | ||
| Filter: "(objectClass=posixaccount)", | ||
| Filter: "", | ||
| ObjectClass: "posixAccount", |
There was a problem hiding this comment.
Hm, posixaccount has no displayName, inetOrgPerson MAY have a displayName. And only inetOrgPerson may have a mail attribute. sn is a MUST for the STRUCTURAL person objectclass.
Maybe switch from posixAccount to inetOrgPerson?
Suggested change
| ObjectClass: "posixAccount", | |
| ObjectClass: "inetOrgPerson", |
butonic
reviewed
Apr 11, 2022
| func waitForLDAPCA(log log.Logger, cfg *config.LDAP) error { | ||
| if !cfg.Insecure && cfg.CACert != "" { | ||
| if _, err := os.Stat(cfg.CACert); errors.Is(err, os.ErrNotExist) { | ||
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf("File does not exist. Waiting %d seconds for it to appear.", caTimeout) |
There was a problem hiding this comment.
This is just my personal vendetta against Msgf which should be avoided in favor of a structured log property. In addition to better machine readibility people will c'n'p it to other places where it might actually start to hurt.
Suggested change
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf("File does not exist. Waiting %d seconds for it to appear.", caTimeout) | |
| log.Warn().Str("LDAP CACert", cfg.CACert).Int("seconds", caTimeout).Msg("File does not exist. Waiting for it to appear.", caTimeout) |
butonic
reviewed
Apr 11, 2022
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf("File does not exist. Waiting %d seconds for it to appear.", caTimeout) | ||
| time.Sleep(caTimeout * time.Second) | ||
| if _, err := os.Stat(cfg.CACert); errors.Is(err, os.ErrNotExist) { | ||
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf("File does still not exist after Timeout") |
There was a problem hiding this comment.
Suggested change
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf("File does still not exist after Timeout") | |
| log.Warn().Str("LDAP CACert", cfg.CACert).Msgf"File does still not exist after Timeout") |
butonic
reviewed
Apr 11, 2022
| logger.Debug(). | ||
| Str("server", "users"). | ||
| Interface("reva-config", rcfg). | ||
| Msg("config") | ||
|
|
||
| if cfg.Reva.Users.Driver == "ldap" { | ||
| if err := waitForLDAPCA(logger, &cfg.Reva.LDAP); err != nil { | ||
| logger.Error().Err(err).Msg("The configured LDAP CA cert does not exist") |
There was a problem hiding this comment.
Suggested change
| logger.Error().Err(err).Msg("The configured LDAP CA cert does not exist") | |
| logger.Error().Err(err).Str("LDAP CACert", cfg.Reva.LDAP.CACert).Msg("The configured LDAP CA cert does not exist") |
butonic
approved these changes
Apr 11, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Unify LDAP config settings accross services
The storage services where updated to adapt for the recent changes of the LDAP
settings in reva.
Also we allow now to use a new set of top-level LDAP environment variables that
are shared between all LDAP-using services in ocis (graph, idp,
storage-auth-basic, storage-userprovider, storage-groupprovider, idm). This
should simplify the most LDAP based configurations considerably.
Here is a list of the new environment variables:
LDAP_URI
LDAP_INSECURE
LDAP_CACERT
LDAP_BIND_DN
LDAP_BIND_PASSWORD
LDAP_LOGIN_ATTRIBUTES
LDAP_USER_BASE_DN
LDAP_USER_SCOPE
LDAP_USER_FILTER
LDAP_USER_OBJECTCLASS
LDAP_USER_SCHEMA_MAIL
LDAP_USER_SCHEMA_DISPLAY_NAME
LDAP_USER_SCHEMA_USERNAME
LDAP_USER_SCHEMA_ID
LDAP_USER_SCHEMA_ID_IS_OCTETSTRING
LDAP_GROUP_BASE_DN
LDAP_GROUP_SCOPE
LDAP_GROUP_FILTER
LDAP_GROUP_OBJECTCLASS
LDAP_GROUP_SCHEMA_GROUPNAME
LDAP_GROUP_SCHEMA_ID
LDAP_GROUP_SCHEMA_ID_IS_OCTETSTRING
Where need these can be overwritten by service specific variables. E.g. it is possible
to use STORAGE_LDAP_URI to overide the top-level LDAP_URI variable.
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: