Add syncrepl (rfc-4533) consumer (persistent search) #447

t2y · 2023-07-19T13:30:16Z

After I discussed it with @cpuschma, I changed the design for maintainability.

Design

I added the syncrepl feature as one of the asynchronous search functions. The syncrepl is available on slapd in OpenLDAP project and it provides a persistent search. The syncrepl is proposed by rfc-4533.

(the below description is the original when I created this PR)

(Old) Design

At first, I tried to implement syncrepl feature into the same source. For example, the searchResponse struct has an asynchronous search process, DecodeControl() function has to decode various controls. However, I am concerned that they are challenging to handle syncrepl communications with other generic LDAP communications.

It has a complicated specification because syncrepl is proposed by rfc-4533. I think dividing existent control.go/response.go and related syncrepl code is stable and maintainable (Of course both belong to the same ldap package). Additionally, they won't put a bug on each other.

control_syncrepl.go
response_syncrepl.go

Data flow

syncrepl has 4 Controls:

Sync Request Control is used for client requests
Sync State Control is used for server responses
- request with mode=RefreshAndPersist, then get a response with ApplicationSearchResultEntry (4)
Sync Done Control is used for server responses
- request with mode=RefreshOnly, then get a response with ApplicationSearchResultDone (5)
Sync Info Control is used for server responses
- request with mode=RefreshAndPersist, then get a response with ApplicationIntermediateResponse (25)

Client                                                  Server
  |                                                      |
  |------ Sync Request Control ------------------------> |
  |                                                      |
  | <-------- Sync Info Message (optional) ------------- |
  |                                                      |
  |      (One or more times for each entry)              |
  | <----- Sync State Control (with Entry) ------------- |
  |                                                      |
  | <-------- Sync Info Message (optional) ------------- |
  |                                                      |
  | <----------- Sync Done Control --------------------- |
  |                                                      |

Controls

Sync Request Control as the following.

Control Type: Sync Request ("1.3.6.1.4.1.4203.1.9.1.1") Criticality: true Mode: 3 Cookie: rid=000,csn=20230719002941.016654Z#000000#000#000000 ReloadHint: false

Sync Request with mode=RefreshAndPersist

Got entries and Sync Info Control as the following.

Control Type: Sync Info ("1.3.6.1.4.1.4203.1.9.1.4") Criticality: false Value: 1 RefreshDelete[Cookie: rid=000,csn=20230719074010.612296Z#000000#000#000000 RefreshDone: true]

Control Type: Sync Info ("1.3.6.1.4.1.4203.1.9.1.4") Criticality: false Value: 3 SyncIdSet[Cookie: rid=000,csn=20230719072347.153542Z#000000#000#000000 RefreshDeletes: true SyncUUIDs: [f1688779-d7f3-4c20-9580-24dcb21149a9 1ee6cb16-86c5-41a7-a52a-9168f1f24703 3d11877e-a030-4007-aafe-c94bb0284fdb]]

When a user is added

Got the entry and a Sync State Control such as the following.

Control Type: Sync State ("1.3.6.1.4.1.4203.1.9.1.2") Criticality: false State: 1 EntryUUID: c033cfc4-89cb-4571-9088-6e62be597266 Cookie: rid=000,csn=20230719062607.286081Z#000000#000#000000

When a user is modified

Got the entry and a Sync State Control such as the following.

Control Type: Sync State ("1.3.6.1.4.1.4203.1.9.1.2") Criticality: false State: 2 EntryUUID: 33a88143-1334-4f6b-853c-27239af30c54 Cookie: rid=000,csn=20230719062553.176851Z#000000#000#000000

When a user is deleted

Got the entry and a Sync State Control such as the following.

Control Type: Sync State ("1.3.6.1.4.1.4203.1.9.1.2") Criticality: false State: 3 EntryUUID: 1ee6cb16-86c5-41a7-a52a-9168f1f24703 Cookie: rid=000,csn=20230719071122.501306Z#000000#000#000000

Sync Request with mode=RefreshOnly

Got entries and a Sync Done Control such as the following.

Control Type: Sync Done ("1.3.6.1.4.1.4203.1.9.1.3") Criticality: false Cookie: rid=000,csn=20230719002941.016654Z#000000#000#000000 RefreshDeletes: true

How to test

To enable syncrepl on the server side, add the below configuration into slapd.conf.

overlay syncprov

Then, call Syncrepl() method described in examples_test.go. I confirmed syncrepl feature works with OpenLDAP server 2.4/2.5 in my environment.

I need more test patterns or more considerations since I'm new to LDAP protocol. Could you review it?

References

t2y · 2023-07-19T13:54:38Z

#446 will fix DATA RACE with GetLastError(). It is not an error due to this PR.

t2y · 2023-07-22T13:18:52Z

I rebased to include #446 for CI (github actions).

…ldap#447

…-ldap#447

t2y · 2023-07-30T23:40:37Z

@cpuschma @johnweldon I've been waiting for this feature since March 2023 (#422). I have added the syncrepl feature so that the existing process has no side effects. What do you think?

cpuschma

Once again, thank you very much for your PR and your constant help improving the project. Overall, thinks are looking good but we need to stay focused to not derivate too much from existing patterns. I'm always open for feedback if you disagree. @go-ldap/committers any comments?

cpuschma · 2023-07-31T13:23:25Z

response_syncrepl.go

+	return r.sr.Next()
+}
+
+func (r *syncreplResponse) start(ctx context.Context, searchRequest *SearchRequest) {


This mostly a copy of the ´startmethod forSearchAsync`. I think we should minize any code duplication, as we already have a copy of basically everythig because of Go module compatibility.

I suggest to remove this duplicate and use a control with SearchAsync instead. This would reduce the LOC dramatically, the amount of work required when thinks need to update and is more in line with the approaches used for regular search or modify requests.

The only think that needs to change is this addition to the start method of SearchAsync, to account for the syncrepl control:

case ApplicationIntermediateResponse: decoded, err := DecodeSyncreplControl(packet.Children[1]) if err != nil { werr := fmt.Errorf("failed to decode intermediate response: %w", err) r.sr.ch <- &SearchSingleResult{Error: werr} return } result := &SearchSingleResult{} result.Controls = append(result.Controls, decoded) r.sr.ch <- result

I suggest to remove this duplicate and use a control with SearchAsync instead.

I also tried to implement the syncrepl feature into the searchResponse struct with SearchAsync() method. And then, I found we have to change several code places. Because existent search response packets and syncrepl search response packets are a little different. Not only ApplicationIntermediateResponse.

For example, compare handling ApplicationSearchResultEntry of the response packet. The searchResponse doesn't handle controls.

searchResponse

switch packet.Children[1].Tag { case ApplicationSearchResultEntry: r.ch <- &SearchSingleResult{ Entry: &Entry{ DN: packet.Children[1].Children[0].Value.(string), Attributes: unpackAttributes(packet.Children[1].Children[1].Children), }, }

But, the syncreplResponse expects an entry with a control. To implement the syncrepl functions, I thought it might change existing behavior (I'm not sure whether the existing behavior is correct or not). Do you think this is acceptable?

syncreplResponse

case ApplicationSearchResultEntry: result := &SearchSingleResult{ Entry: &Entry{ DN: packet.Children[1].Children[0].Value.(string), Attributes: unpackAttributes(packet.Children[1].Children[1].Children), }, } if len(packet.Children) != 3 { r.sr.ch <- result continue } controlPacket := packet.Children[2].Children[0] decoded, err := DecodeSyncreplControl(controlPacket) if err != nil { werr := fmt.Errorf("failed to decode search result entry: %w", err) result.Error = werr r.sr.ch <- result return } result.Controls = append(result.Controls, decoded) r.sr.ch <- result

I think adding the syncrepl feature makes increasing some check condition codes to work them together (both normal search and syncrepl consumer). I can implement them, but I'm not sure whether some side effects affect a normal search or not. This is a trade-off between maintenance cost and compatibility/efficiency.

I agree that reducing unnecessary code duplication is a worthy goal, in this case I'm okay with a distinct syncreplResponse

@cpuschma I integrated the source code for maintainability. I did change the existing process carefully, so it is almost an additional function. There are only three parts to be changed. That is few than I expected. So I think you are right about maintainability.

Could you review it again?

johnweldon · 2023-08-03T16:49:24Z

I agree with @cpuschma; thank you for your work on this feature.

My involvement with this project is very limited now, so I'm okay with deferring to the other @go-ldap/committers as far as requested changes.

My overarching values are:

Keeping the public surface area of this library as limited as possible (without sacrificing new, useful features)
Striving for comprehensive code test coverage

With this in mind, my vote is to approve this MR

t2y · 2023-08-04T05:07:25Z

@johnweldon Thank you for reviewing! I agree with your idea.

And then, I will try to implement this PR without duplicating the code this weekend. I think it's possible, but it might be a little complex. I will carefully integrate my new code to prevent the side effect.

…aintainability go-ldap#447

cpuschma

LGTM, please see the two notes. After that we can merge the PR

response.go

v3/response.go

…arch process

t2y force-pushed the add-syncrepl-consumer branch 2 times, most recently from da18de8 to 91f9494 Compare July 19, 2023 13:49

cpuschma added the enhancement label Jul 20, 2023

feat: add syncrepl (rfc-4533) consumer feature go-ldap#422

87ae55c

t2y force-pushed the add-syncrepl-consumer branch from 91f9494 to 87ae55c Compare July 22, 2023 13:17

t2y added 2 commits July 22, 2023 22:55

refactor: change Syncrepl interface to be able to pass reloadHint go-…

513ab5a

…ldap#447

refactor: unify syncrepl (Syncrepl) instead of syncRepl (SyncRepl) go…

3c7b473

…-ldap#447

cpuschma requested changes Jul 31, 2023

View reviewed changes

johnweldon approved these changes Aug 3, 2023

View reviewed changes

refactor: integrate xxx_syncrepl.go into control.go/response.go for m…

a9912a0

…aintainability go-ldap#447

cpuschma requested changes Aug 5, 2023

View reviewed changes

response.go Outdated Show resolved Hide resolved

v3/response.go Outdated Show resolved Hide resolved

refactor: return an error if an unknown tag is received during the se…

81d8b25

…arch process

cpuschma approved these changes Aug 5, 2023

View reviewed changes

cpuschma merged commit 9306bae into go-ldap:master Aug 5, 2023
16 checks passed

t2y deleted the add-syncrepl-consumer branch August 5, 2023 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syncrepl (rfc-4533) consumer (persistent search) #447

Add syncrepl (rfc-4533) consumer (persistent search) #447

t2y commented Jul 19, 2023 •

edited

t2y commented Jul 19, 2023

t2y commented Jul 22, 2023

t2y commented Jul 30, 2023 •

edited

cpuschma left a comment

cpuschma Jul 31, 2023

t2y Jul 31, 2023 •

edited

johnweldon Aug 3, 2023 •

edited

t2y Aug 5, 2023

johnweldon commented Aug 3, 2023

t2y commented Aug 4, 2023

cpuschma left a comment

Add syncrepl (rfc-4533) consumer (persistent search) #447

Add syncrepl (rfc-4533) consumer (persistent search) #447

Conversation

t2y commented Jul 19, 2023 • edited

Design

(Old) Design

Data flow

Controls

Sync Request with mode=RefreshAndPersist

When a user is added

When a user is modified

When a user is deleted

Sync Request with mode=RefreshOnly

How to test

References

t2y commented Jul 19, 2023

t2y commented Jul 22, 2023

t2y commented Jul 30, 2023 • edited

cpuschma left a comment

Choose a reason for hiding this comment

cpuschma Jul 31, 2023

Choose a reason for hiding this comment

t2y Jul 31, 2023 • edited

Choose a reason for hiding this comment

johnweldon Aug 3, 2023 • edited

Choose a reason for hiding this comment

t2y Aug 5, 2023

Choose a reason for hiding this comment

johnweldon commented Aug 3, 2023

t2y commented Aug 4, 2023

cpuschma left a comment

Choose a reason for hiding this comment

t2y commented Jul 19, 2023 •

edited

t2y commented Jul 30, 2023 •

edited

t2y Jul 31, 2023 •

edited

johnweldon Aug 3, 2023 •

edited