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

protect against negative dupe window via negative max age #2546

Merged
merged 1 commit into from Sep 20, 2021

Conversation

ripienaar
Copy link
Contributor

Signed-off-by: R.I.Pienaar rip@devco.net

/cc @nats-io/core

Signed-off-by: R.I.Pienaar <rip@devco.net>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@ripienaar ripienaar merged commit 3be0b23 into nats-io:main Sep 20, 2021
@ripienaar ripienaar deleted the negative_dupe_window_protection branch September 20, 2021 16:09
@ripienaar
Copy link
Contributor Author

ripienaar commented Sep 20, 2021

I considered doing this, which if we go this way would probably remove over 1KLOC of boiler plate, thoughts @derekcollison ?

(after the release though)

type testServer struct {
	s  *Server
	nc *nats.Conn
	js nats.JetStreamContext
}

func withBasicJetStreamServer(t *testing.T, f func(*testing.T, *testServer)) {
	t.Helper()

	s := RunBasicJetStreamServer()
	defer s.Shutdown()

	config := s.JetStreamConfig()
	if config != nil {
		defer removeDir(t, config.StoreDir)
	}

	nc, js := jsClientConnect(t, s)
	defer nc.Close()

	f(t, &testServer{s, nc, js})
}

func TestNegativeDupeWindow(t *testing.T) {
	withBasicJetStreamServer(t, func(t *testing.T, srv *testServer) {
		// we incorrectly set MaxAge to -1 which then as a side effect sets dupe window to -1 which should fail
		_, err := srv.js.AddStream(&nats.StreamConfig{
			Name:              "TEST",
			Subjects:          nil,
			Retention:         nats.WorkQueuePolicy,
			MaxConsumers:      1,
			MaxMsgs:           -1,
			MaxBytes:          -1,
			Discard:           nats.DiscardNew,
			MaxAge:            -1,
			MaxMsgsPerSubject: -1,
			MaxMsgSize:        -1,
			Storage:           nats.FileStorage,
			Replicas:          1,
			NoAck:             false,
		})
		if err == nil || err.Error() != "duplicates window can not be negative" {
			t.Fatalf("Expected dupe window error got: %v", err)
		}
	})
}

@derekcollison
Copy link
Member

Great idea! Let's get 2.5.1 out but I like the approach.

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.

None yet

2 participants