Skip to content

fix problem with disabling tls #2295

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix problem with disabling tls #2295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion internal/locality/locality.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,9 +364,10 @@
continue
}

mixedTls := cluster.Status.Generations.HasExtraListeners == cluster.Generation

Check failure on line 367 in internal/locality/locality.go

View workflow job for this annotation

GitHub Actions / Lint go code 

var-naming: var mixedTls should be mixedTLS (revive)
var ipAddress fdbv1beta2.ProcessAddress
for _, addr := range addresses {
if addr.Flags["tls"] == cluster.Spec.MainContainer.EnableTLS {
if mixedTls || (addr.Flags["tls"] == cluster.Spec.MainContainer.EnableTLS) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not totally sure in which cases that should help. In cased of "mixed TLS" the processes should have the right listen address anyways (either tls or non-tls, depending on the migration path). Would be nice if you could add some additional information why this change is needed. What are the risks of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry still very much a WIP, I want to do a few more tests to be sure this solves it on some edge cases. I haven't looked much at the risks yet (I did figure the generation condition would protect against most of them) - wanted to make sure it works first haha.

When I was doing some tests disabling TLS, the listen address was still using TLS when the maincontainer.enableTls was set to false, and this TLS-match-condition caused the coordinator addresses to be seen as invalid, and the operator wasn't able to select coordinators. It might be a different or FDB bug that the addresses were still on TLS though

ipAddress = addr
break
}
Expand Down
17 changes: 16 additions & 1 deletion internal/locality/locality_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,7 @@ var _ = Describe("Localities", func() {
}

status = generateDefaultStatus(false)
cluster.Status.Generations.HasExtraListeners = -1
})

JustBeforeEach(func() {
Expand Down Expand Up @@ -1243,7 +1244,21 @@ var _ = Describe("Localities", func() {
})
})

When("the coordinators are listening on TLS", func() {
When("the coordinators are listening on TLS, and this generation is noted HasExtraListeners", func() {
BeforeEach(func() {
cluster.Spec.MainContainer.EnableTLS = true
cluster.Status.Generations.HasExtraListeners = cluster.Generation
})

It("should report the coordinators as valid", func() {
coordinatorsValid, addressesValid, err := CheckCoordinatorValidity(logr.Discard(), cluster, status, coordinatorStatus)
Expect(coordinatorsValid).To(BeTrue())
Expect(addressesValid).To(BeTrue())
Expect(err).NotTo(HaveOccurred())
})
})

When("the coordinators are listening on TLS, HasExtraListeners not set", func() {
BeforeEach(func() {
cluster.Spec.MainContainer.EnableTLS = true
})
Expand Down
Loading