Skip to content

Commit

Permalink
fix: reduce unnecessary quotations in logging (#925)
Browse files Browse the repository at this point in the history
  • Loading branch information
@favonia
favonia committed Sep 12, 2024
1 parent 76e08f9 commit dc3a26b
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 93 deletions.
7 changes: 5 additions & 2 deletions docs/DESIGN.markdown
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@ The source code follows the [standard Go project layout](https://github.com/gola

Here is some arbitrary coding convention that I chose to follow. It may change in the future, but the whole codebase should be consistent with it at any time:

1. Cloudflare IDs (zone IDs, DNS record IDs, etc.) are already designed to use only “very safe” characters and should not be quoted. The formatter `%s` should be used instead of `%q`.
2. A variable name of type `map[..]...` is not in a plural form just because it is of type `map[...]...`. For example, a mapping from IP networks to detected IPs should be named `detectedIP` not `detectedIPs`.
1. These are in general not quoted in the logging because they use only “safe” characters and usually do not cause confusion in a textual context; the formatter `%s` should be used instead of `%q`:
- Cloudflare IDs (DNS zone IDs, DNS record IDs, WAF list IDs, etc.)
- Domain names
- Full list references (`account/name`)
2. A variable of type `map[..]...` should not be named in a plural form just because it is of type `map[...]...`. For example, a mapping from IP networks to detected IPs should be named `detectedIP` not `detectedIPs`.

## Network Security Threat Model

Expand Down
29 changes: 14 additions & 15 deletions internal/api/cloudflare_records.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func (h CloudflareHandle) ListZones(ctx context.Context, ppfmt pp.PP, name strin

res, err := h.cf.ListZonesContext(ctx, cloudflare.WithZoneFilters(name, "", ""))
if err != nil {
ppfmt.Noticef(pp.EmojiError, "Failed to check the existence of a zone named %q: %v", name, err)
ppfmt.Noticef(pp.EmojiError, "Failed to check the existence of a zone named %s: %v", name, err)
hintRecordPermission(ppfmt, err)
return nil, false
}
Expand All @@ -62,14 +62,13 @@ func (h CloudflareHandle) ListZones(ctx context.Context, ppfmt pp.PP, name strin
"initializing", // the setup was just started?
"moved", // domain registrar not pointing to Cloudflare
"pending": // the setup was not completed
ppfmt.Noticef(pp.EmojiWarning, "Zone %q is %q; your Cloudflare setup is incomplete; some features (e.g., proxying) might not work as expected", name, zone.Status) //nolint:lll
ppfmt.Noticef(pp.EmojiWarning, "DNS zone %s is %q in your Cloudflare account; some features (e.g., proxying) might not work as expected", name, zone.Status) //nolint:lll
ids = append(ids, ID(zone.ID))
case
"deleted": // archived, pending/moved for too long
ppfmt.Infof(pp.EmojiWarning, "Zone %q is %q and thus skipped", name, zone.Status)
// skip these
ppfmt.Infof(pp.EmojiWarning, "DNS zone %s is %q in your Cloudflare account and thus skipped", name, zone.Status)
default:
ppfmt.Noticef(pp.EmojiImpossible, "Zone %q is in an undocumented status %q; please report this at %s",
ppfmt.Noticef(pp.EmojiImpossible, "DNS zone %s is in an undocumented status %q in your Cloudflare account; please report this at %s", //nolint:lll
name, zone.Status, pp.IssueReportingURL)
ids = append(ids, ID(zone.ID))
}
Expand Down Expand Up @@ -103,13 +102,13 @@ zoneSearch:
return zones[0], true
default: // len(zones) > 1
ppfmt.Noticef(pp.EmojiImpossible,
"Found multiple active zones named %q (IDs: %s); please report this at %s",
"Found multiple active zones named %s (IDs: %s); please report this at %s",
zoneName, pp.EnglishJoinMap(ID.String, zones), pp.IssueReportingURL)
return "", false
}
}

ppfmt.Noticef(pp.EmojiError, "Failed to find the zone of %q", domain.Describe())
ppfmt.Noticef(pp.EmojiError, "Failed to find the zone of %s", domain.Describe())

return "", false
}
Expand All @@ -135,7 +134,7 @@ func (h CloudflareHandle) ListRecords(ctx context.Context, ppfmt pp.PP, ipNet ip
})
if err != nil {
ppfmt.Noticef(pp.EmojiError,
"Failed to retrieve %s records of %q: %v",
"Failed to retrieve %s records of %s: %v",
ipNet.RecordType(), domain.Describe(), err)
hintRecordPermission(ppfmt, err)
return nil, false, false
Expand All @@ -146,7 +145,7 @@ func (h CloudflareHandle) ListRecords(ctx context.Context, ppfmt pp.PP, ipNet ip
ip, err := netip.ParseAddr(r.Content)
if err != nil {
ppfmt.Noticef(pp.EmojiImpossible,
"Failed to parse the IP address in an %s record of %q (ID: %s): %v",
"Failed to parse the IP address in an %s record of %s (ID: %s): %v",
ipNet.RecordType(), domain.Describe(), r.ID, err)
return nil, false, false
}
Expand All @@ -171,7 +170,7 @@ func (h CloudflareHandle) DeleteRecord(ctx context.Context, ppfmt pp.PP,
}

if err := h.cf.DeleteDNSRecord(ctx, cloudflare.ZoneIdentifier(string(zone)), string(id)); err != nil {
ppfmt.Noticef(pp.EmojiError, "Failed to delete a stale %s record of %q (ID: %s): %v",
ppfmt.Noticef(pp.EmojiError, "Failed to delete a stale %s record of %s (ID: %s): %v",
ipNet.RecordType(), domain.Describe(), id, err)
hintRecordPermission(ppfmt, err)
if mode == RegularDelitionMode {
Expand Down Expand Up @@ -205,7 +204,7 @@ func (h CloudflareHandle) UpdateRecord(ctx context.Context, ppfmt pp.PP,

r, err := h.cf.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier(string(zone)), params)
if err != nil {
ppfmt.Noticef(pp.EmojiError, "Failed to update a stale %s record of %q (ID: %s): %v",
ppfmt.Noticef(pp.EmojiError, "Failed to update a stale %s record of %s (ID: %s): %v",
ipNet.RecordType(), domain.Describe(), id, err)
hintRecordPermission(ppfmt, err)

Expand All @@ -216,7 +215,7 @@ func (h CloudflareHandle) UpdateRecord(ctx context.Context, ppfmt pp.PP,

if TTL(r.TTL) != expectedTTL {
ppfmt.Infof(pp.EmojiUserWarning,
"The TTL of the %s record of %q (ID: %s) to be updated differs from the value of TTL (%s) and will be kept",
"The TTL of the %s record of %s (ID: %s) to be updated differs from the value of TTL (%s) and will be kept",
ipNet.RecordType(), domain.Describe(), id, expectedTTL.Describe(),
)
hintMismatchedRecordAttributes(ppfmt)
Expand All @@ -225,14 +224,14 @@ func (h CloudflareHandle) UpdateRecord(ctx context.Context, ppfmt pp.PP,
if r.Proxied == nil && expectedProxied ||
r.Proxied != nil && *r.Proxied != expectedProxied {
ppfmt.Infof(pp.EmojiUserWarning,
"The proxy status of the %s record of %q (ID: %s) to be updated differs from the value of PROXIED (%v for this domain) and will be kept", //nolint:lll
"The proxy status of the %s record of %s (ID: %s) to be updated differs from the value of PROXIED (%v for this domain) and will be kept", //nolint:lll
ipNet.RecordType(), domain.Describe(), id, expectedProxied,
)
hintMismatchedRecordAttributes(ppfmt)
}
if r.Comment != expectedRecordComment {
ppfmt.Infof(pp.EmojiUserWarning,
"The comment of the %s record of %q (ID: %s) to be updated differs from the value of RECORD_COMMENT (%q) and will be kept", //nolint:lll
"The comment of the %s record of %s (ID: %s) to be updated differs from the value of RECORD_COMMENT (%q) and will be kept", //nolint:lll
ipNet.RecordType(), domain.Describe(), id, expectedRecordComment,
)
hintMismatchedRecordAttributes(ppfmt)
Expand Down Expand Up @@ -270,7 +269,7 @@ func (h CloudflareHandle) CreateRecord(ctx context.Context, ppfmt pp.PP,

res, err := h.cf.CreateDNSRecord(ctx, cloudflare.ZoneIdentifier(string(zone)), params)
if err != nil {
ppfmt.Noticef(pp.EmojiError, "Failed to add a new %s record of %q: %v",
ppfmt.Noticef(pp.EmojiError, "Failed to add a new %s record of %s: %v",
ipNet.RecordType(), domain.Describe(), err)
hintRecordPermission(ppfmt, err)

Expand Down
52 changes: 26 additions & 26 deletions internal/api/cloudflare_records_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func TestListZonesTwo(t *testing.T) {
mockPP = mocks.NewMockPP(mockCtrl)
mockPP.EXPECT().Noticef(
pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"test.org",
gomock.Any(),
)
Expand Down Expand Up @@ -169,15 +169,15 @@ func TestZoneIDOfDomain(t *testing.T) {
map[string][]string{},
3, "", false,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %q", "sub.test.org")
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %s", "sub.test.org")
},
},
"none/wildcard": {
"test.org", domain.Wildcard("test.org"),
map[string][]string{},
2, "", false,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %q", "*.test.org")
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %s", "*.test.org")
},
},
"multiple": {
Expand All @@ -188,7 +188,7 @@ func TestZoneIDOfDomain(t *testing.T) {
gomock.InOrder(
m.EXPECT().Noticef(
pp.EmojiImpossible,
"Found multiple active zones named %q (IDs: %s); please report this at %s",
"Found multiple active zones named %s (IDs: %s); please report this at %s",
"test.org", pp.EnglishJoin(mockIDsAsStrings("test.org", 0, 1)), pp.IssueReportingURL,
),
)
Expand All @@ -202,7 +202,7 @@ func TestZoneIDOfDomain(t *testing.T) {
gomock.InOrder(
m.EXPECT().Noticef(
pp.EmojiImpossible,
"Found multiple active zones named %q (IDs: %s); please report this at %s",
"Found multiple active zones named %s (IDs: %s); please report this at %s",
"test.org", pp.EnglishJoin(mockIDsAsStrings("test.org", 0, 1)), pp.IssueReportingURL,
),
)
Expand All @@ -214,8 +214,8 @@ func TestZoneIDOfDomain(t *testing.T) {
2, "", false,
func(m *mocks.MockPP) {
gomock.InOrder(
m.EXPECT().Infof(pp.EmojiWarning, "Zone %q is %q and thus skipped", "test.org", "deleted"),
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %q", "test.org"),
m.EXPECT().Infof(pp.EmojiWarning, "DNS zone %s is %q in your Cloudflare account and thus skipped", "test.org", "deleted"), //nolint:lll
m.EXPECT().Noticef(pp.EmojiError, "Failed to find the zone of %s", "test.org"),
)
},
},
Expand All @@ -225,7 +225,7 @@ func TestZoneIDOfDomain(t *testing.T) {
1, mockID("test.org", 0), true,
func(m *mocks.MockPP) {
gomock.InOrder(
m.EXPECT().Noticef(pp.EmojiWarning, "Zone %q is %q; your Cloudflare setup is incomplete; some features (e.g., proxying) might not work as expected", "test.org", "pending"), //nolint:lll
m.EXPECT().Noticef(pp.EmojiWarning, "DNS zone %s is %q in your Cloudflare account; some features (e.g., proxying) might not work as expected", "test.org", "pending"), //nolint:lll
)
},
},
Expand All @@ -236,7 +236,7 @@ func TestZoneIDOfDomain(t *testing.T) {
func(m *mocks.MockPP) {
gomock.InOrder(
m.EXPECT().Noticef(pp.EmojiWarning,
"Zone %q is %q; your Cloudflare setup is incomplete; some features (e.g., proxying) might not work as expected",
"DNS zone %s is %q in your Cloudflare account; some features (e.g., proxying) might not work as expected",
"test.org", "initializing"),
)
},
Expand All @@ -247,7 +247,7 @@ func TestZoneIDOfDomain(t *testing.T) {
1, mockID("test.org", 0), true,
func(m *mocks.MockPP) {
m.EXPECT().Noticef(pp.EmojiImpossible,
"Zone %q is in an undocumented status %q; please report this at %s",
"DNS zone %s is in an undocumented status %q in your Cloudflare account; please report this at %s",
"test.org", "some-undocumented-status", pp.IssueReportingURL)
},
},
Expand Down Expand Up @@ -292,7 +292,7 @@ func TestZoneIDOfDomainInvalid(t *testing.T) {

mockPP.EXPECT().Noticef(
pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org",
gomock.Any(),
)
Expand Down Expand Up @@ -415,7 +415,7 @@ func TestListRecords(t *testing.T) {
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiError,
"Failed to retrieve %s records of %q: %v", "AAAA", "sub.test.org", gomock.Any(),
"Failed to retrieve %s records of %s: %v", "AAAA", "sub.test.org", gomock.Any(),
)
ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
"Double check your API token. "+
Expand All @@ -425,7 +425,7 @@ func TestListRecords(t *testing.T) {
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiError,
"Failed to retrieve %s records of %q: %v", "AAAA", "sub.test.org", gomock.Any(),
"Failed to retrieve %s records of %s: %v", "AAAA", "sub.test.org", gomock.Any(),
)
ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
"Double check your API token. "+
Expand All @@ -441,15 +441,15 @@ func TestListRecords(t *testing.T) {
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org", gomock.Any(),
)
},
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org", gomock.Any(),
)
},
Expand All @@ -466,15 +466,15 @@ func TestListRecords(t *testing.T) {
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiImpossible,
"Failed to parse the IP address in an %s record of %q (ID: %s): %v",
"Failed to parse the IP address in an %s record of %s (ID: %s): %v",
"AAAA", "sub.test.org", "record2", gomock.Any(),
)
},
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(
pp.EmojiError,
"Failed to retrieve %s records of %q: %v",
"Failed to retrieve %s records of %s: %v",
"AAAA", "sub.test.org", gomock.Any(),
)
ppfmt.EXPECT().Hintf(pp.HintRecordPermission,
Expand Down Expand Up @@ -579,7 +579,7 @@ func TestDeleteRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org", gomock.Any(),
)
},
Expand All @@ -589,7 +589,7 @@ func TestDeleteRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to delete a stale %s record of %q (ID: %s): %v",
"Failed to delete a stale %s record of %s (ID: %s): %v",
"AAAA", "sub.test.org", api.ID("record1"), gomock.Any(),
)
},
Expand Down Expand Up @@ -703,7 +703,7 @@ func TestUpdateRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org", gomock.Any(),
)
},
Expand All @@ -714,7 +714,7 @@ func TestUpdateRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to update a stale %s record of %q (ID: %s): %v",
"Failed to update a stale %s record of %s (ID: %s): %v",
"AAAA", "sub.test.org", api.ID("record1"), gomock.Any(),
)
},
Expand All @@ -729,17 +729,17 @@ func TestUpdateRecord(t *testing.T) {

gomock.InOrder(
ppfmt.EXPECT().Infof(pp.EmojiUserWarning,
"The TTL of the %s record of %q (ID: %s) to be updated differs from the value of TTL (%s) and will be kept", //nolint:lll
"The TTL of the %s record of %s (ID: %s) to be updated differs from the value of TTL (%s) and will be kept", //nolint:lll
"AAAA", "sub.test.org", api.ID("record1"), "1 (auto)",
),
ppfmt.EXPECT().Hintf(pp.HintMismatchedRecordAttributes, hintText),
ppfmt.EXPECT().Infof(pp.EmojiUserWarning,
"The proxy status of the %s record of %q (ID: %s) to be updated differs from the value of PROXIED (%v for this domain) and will be kept", //nolint:lll
"The proxy status of the %s record of %s (ID: %s) to be updated differs from the value of PROXIED (%v for this domain) and will be kept", //nolint:lll
"AAAA", "sub.test.org", api.ID("record1"), true,
),
ppfmt.EXPECT().Hintf(pp.HintMismatchedRecordAttributes, hintText),
ppfmt.EXPECT().Infof(pp.EmojiUserWarning,
"The comment of the %s record of %q (ID: %s) to be updated differs from the value of RECORD_COMMENT (%q) and will be kept", //nolint:lll
"The comment of the %s record of %s (ID: %s) to be updated differs from the value of RECORD_COMMENT (%q) and will be kept", //nolint:lll
"AAAA", "sub.test.org", api.ID("record1"), "hello",
),
ppfmt.EXPECT().Hintf(pp.HintMismatchedRecordAttributes, hintText),
Expand Down Expand Up @@ -859,7 +859,7 @@ func TestCreateRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to check the existence of a zone named %q: %v",
"Failed to check the existence of a zone named %s: %v",
"sub.test.org", gomock.Any(),
).Times(2)
},
Expand All @@ -869,7 +869,7 @@ func TestCreateRecord(t *testing.T) {
false,
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiError,
"Failed to add a new %s record of %q: %v",
"Failed to add a new %s record of %s: %v",
"AAAA", "sub.test.org", gomock.Any(),
)
},
Expand Down
2 changes: 1 addition & 1 deletion internal/api/cloudflare_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func mockResponse() cloudflare.Response {
const (
mockToken = "token123"
mockAuthString = "Bearer " + mockToken
mockAccountID = "account456"
mockAccountID = api.ID("account456")
)

func newServerAuth(t *testing.T) (*http.ServeMux, api.CloudflareAuth) {
Expand Down
4 changes: 2 additions & 2 deletions internal/api/cloudflare_waf.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ func (h CloudflareHandle) WAFListID(ctx context.Context, ppfmt pp.PP, list WAFLi
count++
if count > 1 {
ppfmt.Noticef(pp.EmojiImpossible,
"Found multiple lists named %q (IDs: %s and %s); please report this at %s",
l.Name, listID, l.ID, pp.IssueReportingURL,
"Found multiple lists named %q within the account %s (IDs: %s and %s); please report this at %s",
list.Name, list.AccountID, listID, l.ID, pp.IssueReportingURL,
)
return "", false, false
}
Expand Down
10 changes: 5 additions & 5 deletions internal/api/cloudflare_waf_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,8 @@ func TestWAFListID(t *testing.T) {
false, false, "",
func(ppfmt *mocks.MockPP) {
ppfmt.EXPECT().Noticef(pp.EmojiImpossible,
"Found multiple lists named %q (IDs: %s and %s); please report this at %s",
"list", mockID("list", 0), mockID("list", 2), pp.IssueReportingURL,
"Found multiple lists named %q within the account %s (IDs: %s and %s); please report this at %s",
"list", mockAccountID, mockID("list", 0), mockID("list", 2), pp.IssueReportingURL,
)
},
},
Expand Down Expand Up @@ -226,7 +226,7 @@ func TestWAFListID(t *testing.T) {
ppfmt.EXPECT().Hintf(pp.HintMismatchedWAFListAttributes,
"The updater will not overwrite WAF list descriptions; "+
"you can change them at https://dash.cloudflare.com/%s/configurations/lists",
api.ID("account456"),
mockAccountID,
),
)
},
Expand Down Expand Up @@ -356,8 +356,8 @@ func TestFindWAFList(t *testing.T) {
func(ppfmt *mocks.MockPP) {
gomock.InOrder(
ppfmt.EXPECT().Noticef(pp.EmojiImpossible,
"Found multiple lists named %q (IDs: %s and %s); please report this at %s",
"list", mockID("list", 0), mockID("list", 2), pp.IssueReportingURL,
"Found multiple lists named %q within the account %s (IDs: %s and %s); please report this at %s",
"list", mockAccountID, mockID("list", 0), mockID("list", 2), pp.IssueReportingURL,
),
ppfmt.EXPECT().Noticef(pp.EmojiError, "Failed to find the list %s", "account456/list"),
)
Expand Down
Loading

0 comments on commit dc3a26b

Please sign in to comment.