sql: Fix bug with negative parsing validation for zone constraints

[rohany]

Fixes #39801.

This PR fixes the parsing issues brought up in #39801, and refactors the test cases for zone validation to specifically test this behavior. Previously this behavior was tested, but falsely passed due to a logic error.

Release note: None

[cockroach-teamcity]

This change is 

[solongordon]

Logic looks good to me. A couple points of confusion that could be cleaned up.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 611 at r1 (raw file):

		// logic below fails to validate negative constraints if a node
		// in the cluster has the constraint being prohibited, and
		// falsely validates prohibited constraints in certain cases as well.

This comment is confusing to me. Based on the comment on the function definition it seems like we don't want to validate negative constraints anyway, right?

// Note that this really only catches typos in required constraints -- we don't
// want to reject prohibited constraints whose attributes/localities don't
// match any of the current nodes because it's a reasonable use case to add
// prohibited constraints for a new set of nodes before adding the new nodes to
// the cluster. If you had to first add one of the nodes before creating the
// constraints, data could be replicated there that shouldn't be.

So it seems like your comment isn't really necessary.

---

pkg/sql/set_zone_config_test.go, line 136 at r1 (raw file):

		cfg       string
		expectErr int
		nodes     nodeGetter

Nit: To me the way this is set up is confusing, with all these functions floating around. It would be clearer if the test case just had the store definitions. Then the test body could do the work of converting that to a nodeGetter.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @solongordon)

---

pkg/sql/set_zone_config.go, line 611 at r1 (raw file):

Previously, solongordon (Solon) wrote…

This comment is confusing to me. Based on the comment on the function definition it seems like we don't want to validate negative constraints anyway, right?

// Note that this really only catches typos in required constraints -- we don't
// want to reject prohibited constraints whose attributes/localities don't
// match any of the current nodes because it's a reasonable use case to add
// prohibited constraints for a new set of nodes before adding the new nodes to
// the cluster. If you had to first add one of the nodes before creating the
// constraints, data could be replicated there that shouldn't be.

So it seems like your comment isn't really necessary.

I think my wording isn't clear enough then. When I say "fail to validate" i mean incorrectly declares negative constraints in certain cases as invalid.

---

pkg/sql/set_zone_config_test.go, line 136 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Nit: To me the way this is set up is confusing, with all these functions floating around. It would be clearer if the test case just had the store definitions. Then the test body could do the work of converting that to a nodeGetter.

I think it is a bit clearer to do this, as the zone configuration function also expects a "nodeGetter" function. I don't see the difference in readability between passing the closures (getters around the structs) around vs passing the structs as variables directly.

[solongordon]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 611 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I think my wording isn't clear enough then. When I say "fail to validate" i mean incorrectly declares negative constraints in certain cases as invalid.

I think what confuses me is that your comment says we want to ignore validation on negative constraints because the logic can't handle it. But from the pre-existing comment, it sounds like we want to ignore validation on negative constraints anyway for business reasons. Is that right?

---

pkg/sql/set_zone_config_test.go, line 136 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

I think it is a bit clearer to do this, as the zone configuration function also expects a "nodeGetter" function. I don't see the difference in readability between passing the closures (getters around the structs) around vs passing the structs as variables directly.

OK, I don't feel strongly about it.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @solongordon)

---

pkg/sql/set_zone_config.go, line 611 at r1 (raw file):

Previously, solongordon (Solon) wrote…

I think what confuses me is that your comment says we want to ignore validation on negative constraints because the logic can't handle it. But from the pre-existing comment, it sounds like we want to ignore validation on negative constraints anyway for business reasons. Is that right?

Yeah, it's a bit weird. The comment at the top of the function says we want to ignore it, but the comment/code at the validation step still performs some sort of validation on negative constraints. And it so happens that the validation it performs on negative constraints is incorrect.

To make it a bit clearer what is going on, look at config.StoreMatchesConstraint and config.StoreHasConstraint. If the constraint is a negative constraint, and the store actually has the constraint, then StoreMatchesConstraint returns false. However, if the constraint is negative and the store doesn't match the constraint, then StoreMatchesConstraint returns true. Based on it erroneously returning true here, this bug wasn't seen in a majority of cases (multi node clusters -- the loop here loops over all nodes, so when a negative constraint was checked against a node in the cluster that didn't have the constraint, the validation would be successful).

[solongordon]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @rohany, and @solongordon)

---

pkg/sql/set_zone_config.go, line 611 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Yeah, it's a bit weird. The comment at the top of the function says we want to ignore it, but the comment/code at the validation step still performs some sort of validation on negative constraints. And it so happens that the validation it performs on negative constraints is incorrect.

To make it a bit clearer what is going on, look at config.StoreMatchesConstraint and config.StoreHasConstraint. If the constraint is a negative constraint, and the store actually has the constraint, then StoreMatchesConstraint returns false. However, if the constraint is negative and the store doesn't match the constraint, then StoreMatchesConstraint returns true. Based on it erroneously returning true here, this bug wasn't seen in a majority of cases (multi node clusters -- the loop here loops over all nodes, so when a negative constraint was checked against a node in the cluster that didn't have the constraint, the validation would be successful).

Yeah that makes sense, but still, we don't even want to validate negative constraints, so I don't care if the logic handles them or not. My personal preference would be to just change this comment to // We skip validation for negative constraints. See the function-level comment., but I'll leave it up to you to choose what you think is clearest.

[rohany]

bors r=solongordon

[craig]

Canceled

[rohany]

bors r=solongordon

[rohany]

bors retry

[rohany]

bors r+

[rohany]

alright @solongordon got any idea whats going on here? bors is just not happy :(

[rohany]

bors r=solongordon

[craig]

Build succeeded

• GitHub CI (Cockroach)