Refactor RecordConfig: Add getters/setters by tlimoncelli · Pull Request #314 · StackExchange/dnscontrol

tlimoncelli · 2018-02-01T13:27:32Z

No description provided.

tlimoncelli · 2018-02-01T17:19:24Z

I set up getters/setters for .Name and .Target and split the functions to their own file.

What do you think about their names?

pmoroney · 2018-02-01T17:26:48Z

models/record.go

+//   This is
+
+// SetLabel sets the .Name/.NameFQDN fields given a short name and origin.
+// origin may not have a trailing dot.


I think it would be better to say origin MUST not have a trailing dot.

captncraig · 2018-02-02T21:31:21Z

models/dns.go


+// CheckDomainIntegrity performs sanity checks on a DomainConfig
+// and panics if problems are found.
+func (dc DomainConfig) CheckDomainIntegrity() {


Nothing should ever panic. Return an error.

Should more of the normalize package be moved into models or vice versa? There are some poorly defined package boundaries here.

captncraig · 2018-02-02T21:31:36Z

models/dns.go

+}
+
+// checkNameFQDN panics if there is a Name/NameFQDN mismatch.
+func checkNameFQDN(recs []*RecordConfig, origin string) {


this needa a better name

captncraig · 2018-02-02T21:34:50Z

models/record.go

+	Target           string            `json:"target"` // If a name, must end with "."
+	TTL              uint32            `json:"ttl,omitempty"`
+	Metadata         map[string]string `json:"meta,omitempty"`
+	NameFQDN         string            `json:"-"` // Must end with ".$origin". See above.


If Record kept a pointer to its' parent domain, maybe GetNameFQDN could just be a pure function, so we don't even need to store fqdn seperately at all? Just the short name. Would be a larger change to break each provider, but compiler would help us out.

captncraig · 2018-02-02T21:35:32Z

models/record.go

+// It does not need further shortening (i.e. if it returns "foo.com" and the
+//   domain is "foo.com" then the FQDN is actually "foo.com.foo.com").
+// It will never be "" (the apex is returned as "@").
+func (rc *RecordConfig) Label() string {


Getters should probably be named GetLabel etc... easier to find stuff in autocomplete.

captncraig · 2018-02-02T21:38:07Z

I have a few concerns about naming conventions and some of the specifics of how it works. Its kinda hard to put precicely what those are into clear words though.

I think it is a step in the right direction to try and enforce conventions at some higher level than each provider deciding their own way of representing things.

I made some initial comments. Only got through some of the files though. Will continue a deeper review later tonight.

tlimoncelli · 2018-02-03T13:17:44Z

I renamed Label* to GetLabel* as suggested.

I converted a few more providers, which was very educational and I found a few ways to improve the functions.

I agree that the checkIntegrity stuff should be moved. Let's do another PR later that refactors the normalization/validation stuff.

I think you'll be happy with one recent change: You know how every "switch rtype" tends to have a default clause that calls panic()? Well, now the default can just call rc.PopulateFromString() and it handles most situations. In fact, move nativeToRecord() methods now only need to handle a few types (the ones that they handle in a non-standard way) then call rc.PopulateFromString(). Done.

tlimoncelli · 2018-02-03T13:23:24Z

MergeToTarget() This is now eliminated in all providers with official support. I could remove it from dnsimple but I don't have a test account. Do you?

RecordConfig.Content() is now unexported. This found a lot of bad behavior.

RecordConfig.CombinedTarget has been eliminated from all official providers. I need a test account for ovh and ns1 to be able to completely eliminate it.

Once PopulateFromString() was written, converting providers became a lot easier.

tlimoncelli · 2018-02-03T17:49:35Z

I'll remove the panic() calls soon.

tlimoncelli · 2018-02-04T04:03:50Z

question for @pmoroney:
How does the name.com API expect a TXT record's strings to be encoded/decoded?

More specifically: In the name.com API, when a TXT record has multiple strings, it looks like we send (or receive) them all in r.Answer as one big string. However, it isn't documented how that string is to be encoded (or decoded).

Could I ask you a favor and request that you look at dnscontrol/providers/namedotcom/records_test.go and see what I'm doing wrong?

Oddly enough the code in the master branch works fine. The code in this branch looks exactly the same. I can't figure out how I broke it. Maybe I just haven't moved the code over properly or the test cases are wrong. (I admit I get the \ escaping and quoting mixed up some times).

Any help would be greatly appreciated!

pmoroney · 2018-02-04T06:00:20Z

I'll take a look right now :)

pmoroney · 2018-02-04T06:51:44Z

Since I don't have access to push to this branch, I created a pull request here: #315

pmoroney · 2018-02-04T07:04:47Z

models/record.go

+//   Official:
+//     A
+//     AAAA
+//     ANAME


ANAME is still just a draft and I don't think any providers support it outright.
Name.com's ANAME record is really just an ALIAS until there is more support for the real ANAME draft.

tlimoncelli · 2018-02-04T15:15:45Z

Once we fix ovh, we're ready to remove the RecordConfig.CombinedTarget field and the CombineMXs() function.

At that point, we can consider unexporting .Name/.NameFQDN/.Target.

* Replace fmt.Errorf with errors.Errorf * Finish removing CombinedTarget

tlimoncelli · 2018-02-04T21:13:51Z

Finished with global changes for now. NOTE TO SELF: Merge everything but the community providers, then create a PR for each provider for each maintainer to approve.

captncraig · 2018-02-14T18:46:36Z

models/record.go

+// SetLabelFQDN sets the .Name/.NameFQDN fields given a FQDN and origin.
+// fqdn may have a trailing "." but it is not required.
+// origin may not have a trailing dot.
+func (rc *RecordConfig) SetLabelFQDN(fqdn, origin string) {


This name is a little confusing to me. Would SetLabelFromFQDN make a little more sense?

captncraig · 2018-02-14T18:50:04Z

models/record.go

+	if strings.HasSuffix(origin, ".") {
+		panic(errors.Errorf("origin (%s) is not supposed to end with a dot", origin))
+	}
+	if strings.HasSuffix(fqdn, "..") {


There is a point where the user is just trying to shoot themselves in the foot. We don't check double dots in the middle for example.

I feel like we should either do more rigorous validation here ((\w+\.)+ maybe), or test fewer things.

In the future, I think maybe the validation package being separate is a mistake and we should just have a single call Validate on dnsconfig that cascades down to domains and records.

The panics are only for assertions failures. i.e. the code is being used wrong. I could change them to errors later, but these are literally "this should never happen" panics.

As far as validation... yes, that needs to be refactored some day.

captncraig · 2018-02-14T18:52:15Z

models/t_caa.go

+)
+
+// SetTargetCAA sets the CAA fields.
+func (rc *RecordConfig) SetTargetCAA(flag uint8, tag string, target string) error {


Would SetCAATarget be a more appropriate name? This applies to almost all methods with that pattern.

It is more searchable to have SetTarget*

captncraig · 2018-02-14T18:55:00Z

models/t_parse.go

+//     }
+func (r *RecordConfig) PopulateFromString(rtype, contents, origin string) error {
+	if r.Type != "" && r.Type != rtype {
+		panic(errors.Errorf("assertion failed: rtype already set (%s) (%s)", rtype, r.Type))


I'm even more confused by panic here. We return an error, but also panic sometimes? What is going on?

captncraig · 2018-02-14T18:56:19Z

models/t_srv.go

+)
+
+// SetTargetSRV sets the SRV fields.
+func (rc *RecordConfig) SetTargetSRV(priority, weight, port uint16, target string) error {


Are all of these variants actually used? I hate dead code, and would rather only add as we need to.

I commented out all the SetTarget$RTYPE functions and only brought back the ones that are in use. They're all in use :-)

captncraig · 2018-02-14T18:57:49Z

models/target.go

+
+// GetTargetSingle returns the target for types that have a single value target
+// and panics for all others.
+func (rc *RecordConfig) GetTargetSingle() string {


This confuses me, and appears to be unused. Whitelisting might be a better approach than blacklisting if we need to keep it.

I've commented out a lot of these functions and only brought back the ones that are in use.

captncraig · 2018-02-14T18:59:43Z

models/target.go

+	// Sadly String() always includes a header, which we must strip out.
+	// TODO(tlim): Request the dns project add a function that returns
+	// the string without the header.
+	rr := rc.ToRR()


Ug. If we are cheating by converting to RR, maybe we should bite the bullet and use RR as our internal representation. Or some type that embeds or encapsulates an RR for metadata and stuff. And maybe some additional implementations for custom stuff.

Getting kinda frustrated at the complexity we've built to avoid it.

captncraig · 2018-02-14T19:00:33Z

models/target.go

+}
+
+// GetTargetDebug returns a string with the various fields spelled out.
+func (rc *RecordConfig) GetTargetDebug() string {


Not sure about leaving debug code in.

captncraig · 2018-02-14T19:01:13Z

models/util.go

+}
+
+// atou32 converts a string  to uint32 or panics.
+// DEPRECATED: This will go away when SOA record handling is rewritten.


* Replace RecordConfig.Name and .NameFQDN with getters and setters. * Replace RecordConfig.Target with getters and setters. * Eliminate the CombinedTarget concept. * Add RecordConfig.PopulateFromString to reduce code in all providers. * encode and decode name.com txt records (StackExchange#315) * Replace fmt.Errorf with errors.Errorf

tlimoncelli added 2 commits February 1, 2018 08:24

Improve testing: Panic if .Name/.NameFQDN are inconsistent.

8123bf1

Merge branch 'master' into namefqdn

a7aed4a

tlimoncelli mentioned this pull request Feb 1, 2018

Make IGNORE work with all providers #313

Merged

tlimoncelli added 4 commits February 1, 2018 10:38

wip!

b4277d7

wip!

daf7bbe

BIND: Use the new getters/settters.

4074e9f

Merge branch 'master' into namefqdn

d5fc64d

tlimoncelli changed the title ~~Improve testing: Panic if .Name/.NameFQDN are inconsistent.~~ Refactor RecordConfig: Add getters/setters Feb 1, 2018

tlimoncelli requested a review from mhenderson-so February 1, 2018 17:18

tlimoncelli assigned captncraig Feb 1, 2018

tlimoncelli requested review from captncraig and removed request for mhenderson-so February 1, 2018 17:18

pmoroney reviewed Feb 1, 2018

View reviewed changes

tlimoncelli added 5 commits February 1, 2018 13:19

may -> must

7d642b9

Fix tests

d10448d

Add helper functions and start converting BIND and GCLOUD.

9407584

Refactor BIND and GCLOUD away from Name/NameFQDN/Target.

8785738

CLOUDFLAREAPI: Refactor to new abstraction

567e974

captncraig reviewed Feb 2, 2018

View reviewed changes

wip!

507a893

wip!

27963d5

tlimoncelli added 3 commits February 3, 2018 17:16

wip!

16b70fe

wip!

bbdc6b4

wip!

ff42bf8

pmoroney reviewed Feb 4, 2018

View reviewed changes

pmoroney and others added 3 commits February 4, 2018 09:35

encode and decode name.com txt records (#315)

bdeda4f

More consistent names

ea65b2c

Remove the need for CombinedTarget in all but ovh

542d8af

tlimoncelli added 2 commits February 4, 2018 14:53

error checking remove CombinedTarget

6bd9538

A lot of changes

90c8166

* Replace fmt.Errorf with errors.Errorf * Finish removing CombinedTarget

tlimoncelli added 8 commits February 5, 2018 10:29

Merge branch 'master' into namefqdn

04ea10f

wip!

f411d86

Switch from fmt.Error* to errors.Error*

b1da24b

Merge branch 'errorserrorf' into namefqdn

fb850b5

Start removing dead code

cbaccc0

more cleanups

6c484aa

DO: Fix TXT record bug.

cedf32e

Merge branch 'master' into namefqdn

548b63b

captncraig reviewed Feb 14, 2018

View reviewed changes

Reply to code review

6aad2ba

tlimoncelli merged commit de44559 into master Feb 15, 2018

tlimoncelli deleted the namefqdn branch February 15, 2018 21:10

pgaskin mentioned this pull request Oct 8, 2020

VULTR: AddOrigin is unnecessary #896

Merged

tlimoncelli pushed a commit that referenced this pull request Oct 8, 2020

VULTR: AddOrigin is unnecessary (#314) (#896)

38d9f05

Conversation

tlimoncelli commented Feb 1, 2018

Uh oh!

tlimoncelli commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captncraig commented Feb 2, 2018

Uh oh!

tlimoncelli commented Feb 3, 2018

Uh oh!

tlimoncelli commented Feb 3, 2018

Uh oh!

tlimoncelli commented Feb 3, 2018

Uh oh!

tlimoncelli commented Feb 4, 2018

Uh oh!

pmoroney commented Feb 4, 2018

Uh oh!

pmoroney commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlimoncelli commented Feb 4, 2018

Uh oh!

tlimoncelli commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants