Reject invalid endpoint

[rmweir]

Problem:
Prior, a non-empty endpoint of the incorrect format would cause an embedded sqlite backend to be used. By passing a non-empty endpoint the user is intending to use an external backend.

Solution:
Now, the endpoint will be validated and passing a non-empty endpoint of an incorrect format will result in an error.

Issue:
#319

[galal-hussein]

LGTM

[galal-hussein]

@rmweir Looks like the test is failing tho in testing sqlite:

[sqlite] #- Tail: /tmp/GSt1E0/kine/1/logs/system.log
[sqlite] time="2024-09-03T18:48:42.444315289Z" level=fatal msg="parsing storage endpoint: invalid storage endpoint [sqlite]. Non-empty storage endpoint should be of the format <network>://<address>"

passing just an endpoint without a url should be allowed as well, like passing just sqlite https://github.com/k3s-io/kine/blob/master/scripts/test-run-sqlite#L3C23-L3C45

[rmweir]

@galal-hussein can we either change the test or explicity check for "sqlite"? Because the value gets totally discarded, its not evaluated.

[rmweir]

@galal-hussein I added an explicit check in a new commit so you can review it. If you're okay with that I can squash.

[brandond]

I would be willing to tighten this down to require a valid scheme. So if you want sqlite for example, you have to do at least sqlite:// instead of just sqlite. I don't think we have documented anywhere that we support sqlite, and if you want to pass any configuration to the sqlite backend you already have to use a proper URI, as seen at k3s-io/k3s#8234 (comment)

Also, please do use the proper RFC3986 labels for the URI components in the error, not network and address.

[rmweir]

@brandond I think passing an empty storage endpoint is how one usually uses sqlite. I think expecting either a valid DSN URI or empty to imply an embedded backend is required, makes enough sense. Can we go with that?

[brandond]

yeah thats what I meant. Either leave it empty, or pass a URI with a valid scheme and separator. The URI can have an empty authority/path/parameters to get whatever the defaults are for that backend.

[rmweir]

@brandond I just updated the latest commit to what I was thinking. Lmk if that's fine and I will squash. If you want me too add some option for "sqlite://" I can do that too but I think we're okay without it.

[brandond]

1. looks like you need to sign off for DCO
2. I think you'll need to fix the sqlite test that was linked above:

   kine/scripts/test-run-sqlite

   Line 3 in cb8c874

   KINE_IMAGE=$IMAGE KINE_ENDPOINT="sqlite" provision-kine

[brandond]

lgtm!

[rmweir]

@brandond fixed test, also had to move validation to its own function as we use that network/address parsing function in multiple places where string could be empty, squashed all commits. Just take another pass on everything please as I don't want to sneak anything in.

[brandond]

[sqlite] time="2024-09-03T21:27:16Z" level=fatal msg="flag needs an argument: -endpoint"

You can leave --endpoint unset, but you can't currently set it to an empty string... so you should either fix the CLI flag, or explicitly set it.

Suggested change

	KINE_IMAGE=$IMAGE KINE_ENDPOINT="" provision-kine
	KINE_IMAGE=$IMAGE KINE_ENDPOINT="sqlite://" provision-kine

[brandond]

Actually I suspect that when the var is empty the script is just running kine --endpoint instead of kine --endpoint "" so the CLI framework is complaining that the flag doesn't have a value - which is valid.

You could change this

kine/scripts/test-helpers

Line 254 in cb8c874

--endpoint $KINE_ENDPOINT

to --endpoint=$KINE_ENDPOINT

[rmweir]

@brandond @galal-hussein thanks for the fast replies! Will probably need someone to merge this for me as well once everything passes 😬.

[brandond]

I'll wait for @galal-hussein to review again, then either one of us can merge and tag!