Validate TXT prefix #1507

ruudk · 2020-04-10T13:38:54Z

To prevent mistakes I think it's good to validate the txt prefix on startup.

k8s-ci-robot · 2020-04-10T13:39:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ruudk
To complete the pull request process, please assign njuettner
You can assign the PR to them by writing /assign @njuettner in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ruudk · 2020-04-10T13:39:29Z

/assign @njuettner

jgrumboe · 2020-04-10T16:05:58Z

If you want I can incorporate your validations into #1483.

ruudk · 2020-04-13T09:33:24Z

@jgrumboe Thanks I think better for the maintainers to review this separately

fejta-bot · 2020-07-12T09:44:15Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

ruudk · 2020-07-14T08:23:37Z

Rebased the PR because #1483 was merged.

fejta-bot · 2020-08-13T09:11:14Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

seanmalloy · 2020-08-13T12:44:27Z

/remove-lifecycle rotten
/kind bug

seanmalloy · 2020-08-25T02:46:57Z

/assign

seanmalloy · 2020-08-25T02:55:36Z

/lgtm

@tariq1890 and @vinny-sabatini please take a look when you have some time. After you review I'd like to assign this to the external-dns approvers, and see if we can get this merged. Thanks!

seanmalloy · 2020-09-01T18:37:35Z

/cc @tariq1890

vinny-sabatini

I believe the rest of this code change (except the one comment about the error creation) looks good to me.

One thing I am unsure of is if it matters if the txt record starts or ends with . or - characters.

registry/txt.go

Signed-off-by: Ruud Kamphuis <ruudk@users.noreply.github.com>

k8s-ci-robot · 2020-09-02T07:06:12Z

New changes are detected. LGTM label has been removed.

ruudk · 2020-09-02T07:06:38Z

Applied the requested changes.

Raffo

I added a couple of comments, I am not sure that the check that we are doing is accurate or enough. In case it is not accurate or complete I would rather not having a check.

Raffo · 2020-09-02T08:01:04Z

registry/txt_test.go

+	require.Error(t, err, `invalid TXT prefix provided, expected "[a-z0-9-.]+" got "@invalid#"`)
+	_, err = NewTXTRegistry(p, "", "@invalid#", "owner", time.Hour)
+	require.Error(t, err, `invalid TXT suffix provided, expected "[a-z0-9-.]+" got "@invalid#"`)
+


If I add

_, err = NewTXTRegistry(p, "valid....", "", "owner", time.Hour) require.Error(t, err)

That also looks invalid to me, but the test fails. Are you sure the check that we are doing is fine?

Then we need to improve the regex that is used. Any suggestions?

^([a-z0-9]+(-[a-z0-9]+)*)$ maybe?

@ruudk any thoughts on the suggestion from @Raffo

Raffo · 2020-09-02T08:01:29Z

registry/txt_test.go

@@ -60,6 +60,26 @@ func testTXTRegistryNew(t *testing.T) {
 	_, err = NewTXTRegistry(p, "txt", "txt", "owner", time.Hour)
 	require.Error(t, err)

+	_, err = NewTXTRegistry(p, "@invalid#", "", "owner", time.Hour)
+	require.Error(t, err, `invalid TXT prefix provided, expected "[a-z0-9-.]+" got "@invalid#"`)


This message and the one on like 66 are not accurate, I recommend changing it like:

Suggested change

require.Error(t, err, `invalid TXT prefix provided, expected "[a-z0-9-.]+" got "@invalid#"`)

require.Error(t, err)

Why are they not accurate? By comparing the error string I make sure that the guard that returns the error works. If I just accept any error, it would make it less good, right?

Because what is happening when you have an error is not that the TXT prefix is invalid, rather than it is expected to be invalid, but there is no error. The string you are providing is only changing the messaging to report, not changing any behavior at all. The default message instead, is correct.

@ruudk any thoughts on the suggestion from @Raffo

Raffo · 2020-10-09T14:38:37Z

Given that this had no follow up, I'm going to close this, to keep a bit of clarity in the PRs that we have to review. Please reopen if you feel you can follow up on this @ruudk .

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 10, 2020

k8s-ci-robot requested review from hjacobs and linki April 10, 2020 13:39

k8s-ci-robot assigned njuettner Apr 10, 2020

ruudk force-pushed the validate-txt-prefix branch from 6723319 to dea703c Compare April 10, 2020 14:49

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 12, 2020

ruudk force-pushed the validate-txt-prefix branch from dea703c to 3b685da Compare July 14, 2020 08:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2020

ruudk force-pushed the validate-txt-prefix branch from 3b685da to e5fce25 Compare July 14, 2020 08:31

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 13, 2020

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Aug 13, 2020

k8s-ci-robot assigned seanmalloy Aug 25, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2020

k8s-ci-robot requested a review from tariq1890 September 1, 2020 18:37

vinny-sabatini suggested changes Sep 1, 2020

View reviewed changes

registry/txt.go Outdated Show resolved Hide resolved

registry/txt.go Outdated Show resolved Hide resolved

Validate TXT prefix and suffix

6e19628

Signed-off-by: Ruud Kamphuis <ruudk@users.noreply.github.com>

ruudk force-pushed the validate-txt-prefix branch from e5fce25 to 6e19628 Compare September 2, 2020 07:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2020

Raffo reviewed Sep 2, 2020

View reviewed changes

vinny-sabatini mentioned this pull request Sep 3, 2020

REQUEST: New membership for vinny-sabatini kubernetes/org#2176

Closed

6 tasks

Raffo closed this Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate TXT prefix #1507

Validate TXT prefix #1507

ruudk commented Apr 10, 2020 •

edited

Loading

k8s-ci-robot commented Apr 10, 2020

ruudk commented Apr 10, 2020

jgrumboe commented Apr 10, 2020

ruudk commented Apr 13, 2020

fejta-bot commented Jul 12, 2020

ruudk commented Jul 14, 2020

fejta-bot commented Aug 13, 2020

seanmalloy commented Aug 13, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Sep 1, 2020

vinny-sabatini left a comment

k8s-ci-robot commented Sep 2, 2020

ruudk commented Sep 2, 2020

Raffo left a comment

Raffo Sep 2, 2020

ruudk Sep 2, 2020

Raffo Sep 4, 2020

seanmalloy Sep 16, 2020

Raffo Sep 2, 2020

ruudk Sep 2, 2020

Raffo Sep 4, 2020

seanmalloy Sep 16, 2020

Raffo commented Oct 9, 2020

	require.Error(t, err, `invalid TXT prefix provided, expected "[a-z0-9-.]+" got "@invalid#"`)
	require.Error(t, err)

Validate TXT prefix #1507

Validate TXT prefix #1507

Conversation

ruudk commented Apr 10, 2020 • edited Loading

k8s-ci-robot commented Apr 10, 2020

ruudk commented Apr 10, 2020

jgrumboe commented Apr 10, 2020

ruudk commented Apr 13, 2020

fejta-bot commented Jul 12, 2020

ruudk commented Jul 14, 2020

fejta-bot commented Aug 13, 2020

seanmalloy commented Aug 13, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Aug 25, 2020

seanmalloy commented Sep 1, 2020

vinny-sabatini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 2, 2020

ruudk commented Sep 2, 2020

Raffo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raffo commented Oct 9, 2020

ruudk commented Apr 10, 2020 •

edited

Loading