Skip to content

fix(config): support dynamic map keys with dots #8096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
lidel wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

fix(config): support dynamic map keys with dots #8096

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
05232af
fix(config): support dynamic map keys with dots
lidel Apr 28, 2021
0a2336e
docs: ipfs config --help
lidel Apr 28, 2021
732adeb
refactor: addressing code review
lidel Apr 28, 2021
660cc66
fix: sanitize secrets accessed via map key notation
lidel Apr 28, 2021
6d295eb
test(config): service secrets with json notation
lidel Apr 28, 2021
e1cbee7
test(ci): deterministic jq
lidel Apr 29, 2021
36b62c1
fix: more strict regex
lidel Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions repo/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,50 @@ package common

import (
"fmt"
"regexp"
"strings"
)

// Find dynamic map key names passed as Parent["foo"] notation
var bracketsRe = regexp.MustCompile(`\[([^\[\]]*)\]`)

// Normalization for supporting arbitrary dynamic keys with dots:
// Gateway.PublicGateways["gw.example.com"].UseSubdomains
// Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable
Comment on lines +15 to +16
Copy link
Contributor

@aschmahmann aschmahmann Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this notation standard/used in other software or are we just making this up this form of escaping? Either way we'd need to document this in the config command.

Copy link
Member Author

@lidel lidel Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how keys in JSON objects are addressed on the web, and since we use JSON for config anyway, means we don't invent anything new, but follow existing JSON convention.:

( {"foo.bar":{"a": "buz"}} )["foo.bar"].a → "buz"

I've added note about this notation to ipfs config --help in 0a2336e

Copy link
Contributor

@petar petar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value is not documented accurately on a public method. What about dynamicKeys?

func keyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]string) {
Copy link
Contributor

@aschmahmann aschmahmann Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting this here isn't sufficient since we need to deal with things like https://github.com/ipfs/go-ipfs/blob/eea198f0811f50ab14c4f713de07ce93dffec9af/core/commands/config.go#L90 that exist above this level.

We'll either need to export some of this behavior from here, or move the other behavior here and export it.

Copy link
Member Author

@lidel lidel Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in 660cc66 and added tests in 6d295eb

ps. I restored %s in error messages because entire error string gets sanitized already upstream, so we want to avoid double escaping.

Copy link
Member Author

@lidel lidel Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharness tests fail on CI but pass locally, I'm suspecting different versions of jq, needs another look.

Copy link
Member Author

@lidel lidel Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made our CI use the latest jq instead of old 1.5, and it is green now.

bracketedKeys := bracketsRe.FindAllString(key, -1)
dynamicKeys = make(map[string]string, len(bracketedKeys))
normalizedKey = key
for i, mapKeySegment := range bracketedKeys {
mapKey := strings.TrimLeft(mapKeySegment, "[\"")
mapKey = strings.TrimRight(mapKey, "\"]")
placeholder := fmt.Sprintf("mapKey%d", i)
dynamicKeys[placeholder] = mapKey
normalizedKey = strings.Replace(normalizedKey, mapKeySegment, fmt.Sprintf(".%s", placeholder), 1)
Copy link
Contributor

@petar petar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will convert a glob string like: ["abc"]["def"] into .UUID1.UUID2
Is this your intention? Specifically, I am referring to the first key.

}
return normalizedKey, dynamicKeys
}

func MapGetKV(v map[string]interface{}, key string) (interface{}, error) {
var ok bool
var mcursor map[string]interface{}
var cursor interface{} = v

parts := strings.Split(key, ".")
normalizedKey, dynamicKeys := keyToLookupData(key)
parts := strings.Split(normalizedKey, ".")
Copy link
Contributor

@petar petar Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails of normalizedKey starts with a ".", which technically it can.

for i, part := range parts {
sofar := strings.Join(parts[:i], ".")

mcursor, ok = cursor.(map[string]interface{})
if !ok {
return nil, fmt.Errorf("%s key is not a map", sofar)
return nil, fmt.Errorf("%q key is not a map", sofar)
}
if dynamicPart, ok := dynamicKeys[part]; ok {
part = dynamicPart
}

cursor, ok = mcursor[part]
if !ok {
return nil, fmt.Errorf("%s key has no attributes", sofar)
return nil, fmt.Errorf("%q key has no attribute %q", sofar, part)
}
}
return cursor, nil
Expand All @@ -32,12 +56,16 @@ func MapSetKV(v map[string]interface{}, key string, value interface{}) error {
var mcursor map[string]interface{}
var cursor interface{} = v

parts := strings.Split(key, ".")
normalizedKey, dynamicKeys := keyToLookupData(key)
parts := strings.Split(normalizedKey, ".")
for i, part := range parts {
mcursor, ok = cursor.(map[string]interface{})
if !ok {
sofar := strings.Join(parts[:i], ".")
return fmt.Errorf("%s key is not a map", sofar)
return fmt.Errorf("%q key is not a map", sofar)
}
if dynamicPart, ok := dynamicKeys[part]; ok {
part = dynamicPart
}

// last part? set here
Expand Down
7 changes: 7 additions & 0 deletions test/sharness/t0021-config.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,13 @@ test_config_cmd() {
test_cmp replconfig.json newconfig.json
'

# Dynamic keys with dot in their names
test_config_cmd_set "--json" "Gateway.PublicGateways[\"some.example.com\"].UseSubdomains" "true"
test_expect_success "'ipfs config' output for dynamic keys looks good" '
ipfs config Gateway.PublicGateways["some.example.com"].UseSubdomains > dynkey_out &&
grep true dynkey_out
'

# SECURITY
# Those tests are here to prevent exposing the PrivKey on the network

Expand Down