sqlproxyccl: add support for expiring a denylist entry based on timestamp

[meridional]

Previously, there's no easy way to specify an expiration
time for each entry. This PR adds a new denylist config format
that allows encoding of expiration time in the config file.

Additionally, the old config file format is essentially "free-form",
and the parsing is offloaded to viper. This PR "standardizes"
the config file format in yaml.

Also, the old Deny takes an "untyped" string as input,
this PR added a "type" field to the input to better distinguish
between IP and cluster, and other potentially future expansions.

The main API of the new code is written as such that it won't
report an error the caller when it cannot parse the config file;
instead it logs it and moves on in the hope that human operators can
know that something is broken and fix it. This is done this way
mostly to avoid unnecessary runtime failures due to old config
file formats.

Original code is mostly left untouched for testing backward
and forward tolerance of the config files.

Release note: None

[cockroach-teamcity]

This change is 

[andy-kimball]

, with a few small comments. Nice work!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @darinpp and @meridional)

---

pkg/ccl/sqlproxyccl/denylist/file.go, line 67 at r1 (raw file):

		*syncutil.RWMutex
	}
	pollingInterval time.Duration

Maybe add a TODO somewhere to switch to a filewatcher rather than use polling.

---

pkg/ccl/sqlproxyccl/denylist/file.go, line 68 at r1 (raw file):

	}
	pollingInterval time.Duration
	timeSource      timeutil.TimeSource

The timeutil package comes from CRDB. I think we should avoid a dependency on that package (we try to minimize dependencies on CRDB, as they can be a real pain to maintain over time), and just use timeutils.Now() in the managed-service repo. As long as you call timeutils.Now instead of time.Now in your code, you can then hook that function in your tests, like this:

	now := time.Date(2021, 1, 29, 12, 0, 0, 0, time.UTC)
	defer testutils.HookGlobal(&timeutils.Now, func() time.Time { return now })()

Then you'd get rid of timeSource. This is the pattern we use all over managed-service.

---

pkg/ccl/sqlproxyccl/denylist/local_file_test.go, line 29 at r1 (raw file):

		Seq: 3,
		Denylist: []*DenyEntry{
			{DenyEntity{"1.1.1.1", IPAddrType}, timeutil.Now().Add(time.Hour), "reason"},

Maybe a few more test cases would be interesting, such as a ClusterType case. I'd usually structure a test like this similar to how TestCockroachStatefulSet works, so that it's easy to add a bunch of test cases.

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 28 at r1 (raw file):

}

// Type is the type of the denied entity

NIT: Use period at end of comments here and elsewhere.

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 35 at r1 (raw file):

	IPAddrType Type = iota + 1
	ClusterType
	UnknonwType

SP: UnknownType

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 49 at r1 (raw file):

// UnmarshalYAML implements yaml.Unmarshaler interface for type
func (typ *Type) UnmarshalYAML(unmarshal func(interface{}) error) error {

It'd be best for this new code to have at least one simple unit test.

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 72 at r1 (raw file):

}

// String() implements Stringer interface for type

NIT: Remove parentheses at end of String().

[meridional]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @darinpp, and @meridional)

---

pkg/ccl/sqlproxyccl/denylist/file.go, line 68 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

The timeutil package comes from CRDB. I think we should avoid a dependency on that package (we try to minimize dependencies on CRDB, as they can be a real pain to maintain over time), and just use timeutils.Now() in the managed-service repo. As long as you call timeutils.Now instead of time.Now in your code, you can then hook that function in your tests, like this:

	now := time.Date(2021, 1, 29, 12, 0, 0, 0, time.UTC)
	defer testutils.HookGlobal(&timeutils.Now, func() time.Time { return now })()

Then you'd get rid of timeSource. This is the pattern we use all over managed-service.

This is the CRDB repo. :)

Though your comment makes me worried.. My plan is once we vendor this patch back, our control plane can just construct the deny list File and use Serialize to get the config map content, and push that; and use Deserialize to check the sequence number, etc.. I assumed that the usage of this library in the managed-service is ok.

[meridional]

Dismissed @andy-kimball from 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @darinpp)

---

pkg/ccl/sqlproxyccl/denylist/file.go, line 67 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Maybe add a TODO somewhere to switch to a filewatcher rather than use polling.

Added a TODO in the function where the polling happens.

---

pkg/ccl/sqlproxyccl/denylist/local_file_test.go, line 29 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Maybe a few more test cases would be interesting, such as a ClusterType case. I'd usually structure a test like this similar to how TestCockroachStatefulSet works, so that it's easy to add a bunch of test cases.

Done.

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 35 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

SP: UnknownType

Done.

---

pkg/ccl/sqlproxyccl/denylist/service.go, line 72 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

NIT: Remove parentheses at end of String().

Done.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @darinpp, and @meridional)

---

pkg/ccl/sqlproxyccl/denylist/file.go, line 68 at r1 (raw file):

Previously, meridional (Ye Ji) wrote…

This is the CRDB repo. :)

Though your comment makes me worried.. My plan is once we vendor this patch back, our control plane can just construct the deny list File and use Serialize to get the config map content, and push that; and use Deserialize to check the sequence number, etc.. I assumed that the usage of this library in the managed-service is ok.

Ha, yeah, my mistake. I think this is fine, then. The principle is to make reasonable efforts to make sure all our dependencies are worth it. Since this is in the CRDB repo, it's very reasonable to use this package.

[meridional]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball, @darinpp, and @meridional)

pkg/ccl/sqlproxyccl/denylist/file.go, line 68 at r1 (raw file):

Previously, meridional (Ye Ji) wrote…
Ha, yeah, my mistake. I think this is fine, then. The principle is to make reasonable efforts to make sure all our dependencies are worth it. Since this is in the CRDB repo, it's very reasonable to use this package.

Thanks for the review!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)