swarm: add config file #15548

holisticode · 2017-11-23T17:35:39Z

This PR adds a TOML configuration option to swarm.
It reuses the TOML configuration structure used in geth with swarm customized items.

The PR:

Adds a dumpconfig command to the swarm executable which allows printing the (default!) configuration to STDOUT, which then can be redirected to a file in order to customize it
Adds a config <file> option to the swarm executable which will allow to load a configuration file in TOML format from the specified location in order to initialize the Swarm node

The override priorities are like follows:
ENVIRONMENT VARIABLES override COMMAND LINE ARGUMENTS override CONFIG FILE override DEFAULT CONFIG.

nonsense · 2017-11-24T09:57:55Z

cmd/swarm/main.go

@@ -325,6 +327,7 @@ DEPRECATED: use 'swarm db clean'.
 		CorsStringFlag,
 		EnsAPIFlag,
 		EnsAddrFlag,
+		SwarmTomlConfigPathFlag,


wrong rename: cmd/swarm/main.go:330:3: undefined: SwarmTomlConfigPathFlag. This should refer to swarmTomlConfigFileFlag I guess.

Something went terribly wrong after rebasing. Hang on

nonsense · 2017-11-24T09:58:20Z

cmd/swarm/config.go

+	return stack, cfg
+}
+
+func makeFullNode(ctx *cli.Context) *node.Node {


this doesn't appear to be used anywhere. I suggest we don't add it, unless it becomes needed.

nonsense · 2017-11-24T10:00:42Z

cmd/swarm/config.go

+	return cfg
+}
+
+func makeConfigNode(ctx *cli.Context) (*node.Node, bzzConfig) {


this is not used either. I am not sure if we plan to use it very soon, but if not - probably best to add it only when we need it.

nolash

These changes also touch on the same territory as the swarm binary fix I did earlier.

Have we had a comprehensive analysis that all the params that need to be there are addressed properly? And maybe we should take the opportunity to weed out if there are any redundant or obsolete ones?

nolash · 2017-11-27T17:17:10Z

cmd/swarm/main.go

+	//a few steps need to be done after the config phase is completed,
+	//due to overriding behavior
+	initSwarmNode(bzzconfig, stack, ctx)
+	//now we can boot the Swarm node


Is the config.go file the right place to actually create the key?

As opposed to...? It was previously done in main.go right in registerBzzService, but this is now not possible anymore because we first need to evaluate the final config, as the key needs the bzzaccount parameter, which could have been overridden. As the config is built up in config.go, that's where the key is created now.

As opposed to the buildConfig caller, I would say was more natural. I'm not sure how to argue strongly for it, just having it in the config routine seems too trivial somehow.

nolash · 2017-11-27T17:21:04Z

swarm/api/config.go

-	if networkId == 0 {
-		self.NetworkId = network.NetworkId
-	}
-


there goes the first change I made to the go-ethereum codebase ;')

nolash · 2017-11-29T13:39:07Z

cmd/swarm/config.go

+	FieldToKey: func(rt reflect.Type, field string) string {
+		return field
+	},
+	MissingField: func(rt reflect.Type, field string) error {


Are NormFieldName and FieldToKey just supposed to return the second param?

I literally copied this section from the geth code, which apparently is needed to match go structures to TOML , check cmd/geth/config.go

If it works don't fix it :)

nolash · 2017-11-29T13:40:48Z

cmd/swarm/config.go

+		link := ""
+		if unicode.IsUpper(rune(rt.Name()[0])) && rt.PkgPath() != "main" {
+			link = fmt.Sprintf(", see https://godoc.org/%s#%s for available fields", rt.PkgPath(), rt.Name())
+		}


godoc.org is not relevant here?

same here, literally copied from geth assuming this is needed to match go code to TOML

We should probably change this as the godoc.org link will point to an empty location?

nolash · 2017-11-29T13:42:04Z

cmd/swarm/config.go

+	// exit if the deprecated --ethapi flag is set
+	if ctx.GlobalString(DeprecatedEthAPIFlag.Name) != "" {
+		utils.Fatalf("--ethapi is no longer a valid command line flag, please use --ens-api and/or --swap-api.")
+	}


why and/OR --swap-api? Can it replace --ens-api?

The error message is the same as currently on master. I understand it to indicate that the user may have been wanting to use swap-api instead of ethapi and just mistyped, but it feels rather confusing, I agree

Maybe we should clear this up in the orange-lounge?

Previously --ethapi was used for all eth interactions. This has now been split up into ENS and SWAP allowing us to use mainnet ENS while testing SWAP on ropsten for example.
Thus 'and/or' is correct. If you were using your --ethapi just for ENS (most cases), then you should now use --ens-api

nolash · 2017-11-29T14:07:48Z

cmd/swarm/config.go

+		//note that we are decoding into the existing defaultConfig;
+		//if an entry is not present in the file, the default entry is kept
+		err = tomlSettings.NewDecoder(bufio.NewReader(f)).Decode(&conf)
+		// Add file name to errors that have a line number.


Do you need buffered reader? Can't you just pass f?

The interface for NewDecoder takes a io.Reader, so I guess using directly f should work.

nolash · 2017-11-29T14:08:57Z

cmd/swarm/config.go

+	config = cmdLineOverride(config, ctx)
+	//override settings provided by environment variables
+	config = envVarsOverride(config)
+


Shouldn't command line vars override envvars?

good question. I thought to follow @lmars implementing this:

Lewis Marshall @lmars Nov 15 12:06
so typically, command line flags take precedence, then env vars, then config file, all for the same set of parameters

But I may have indeed misinterpreted him, in that he seems to say that command line would override envvars. If you can confirm you understand this like that, I will change it

He seems to concur that cmd line vars would take precedence. When you think about it, it would be strange otherwise. It's the "easiest" switch to flip, somehow.

nolash · 2017-11-29T15:40:00Z

cmd/swarm/config.go

+	cfg, err := buildConfig(ctx)
+	if err != nil {
+		utils.Fatalf(fmt.Sprintf("Uh oh - dumpconfig bumped triggered an error %v", err))
+	}


bumped triggered? :D

nolash · 2017-11-29T15:47:47Z

swarm/api/config.go


-	self.Swap.SetKey(prvKey)
+	self.PublicKey = pubkeyhex
+	self.BzzKey = keyhex



I was under the impression that we did not want to be able to infer the overlay address from predictable underlying data?

?
I should have not changed any logic in this respect, just reorganizing stuff which was there before, but only allowing things to happen after the complete config with the overrides has been evaluated.

This is the old way, where the config was initialized in swarm/api/config.go with NewConfig():

60 func NewConfig(path string, contract common.Address, prvKey *ecdsa.PrivateKey, networkId uint64) (self *Config, err error) { 61 address := crypto.PubkeyToAddress(prvKey.PublicKey) // default beneficiary address 62 dirpath := filepath.Join(path, "bzz-"+common.Bytes2Hex(address.Bytes())) 63 err = os.MkdirAll(dirpath, os.ModePerm) 64 if err != nil { 65 return 66 } 67 confpath := filepath.Join(dirpath, "config.json") 68 var data []byte 69 pubkey := crypto.FromECDSAPub(&prvKey.PublicKey) 70 pubkeyhex := common.ToHex(pubkey) 71 keyhex := crypto.Keccak256Hash(pubkey).Hex() 72 73 self = &Config{ 74 SyncParams: network.NewSyncParams(dirpath), 75 HiveParams: network.NewHiveParams(dirpath), 76 ChunkerParams: storage.NewChunkerParams(), 77 StoreParams: storage.NewStoreParams(dirpath), 78 ListenAddr: DefaultHTTPListenAddr, 79 Port: DefaultHTTPPort, 80 Path: dirpath, 81 Swap: swap.DefaultSwapParams(contract, prvKey), 82 PublicKey: pubkeyhex, 83 BzzKey: keyhex, 84 EnsRoot: ens.TestNetAddress, 85 NetworkId: networkId, 86 }

Or I did misunderstand your comment :)

No from my understanding of the ultimate intention, the old method is also wrong. I though the overlay address should be totally independent / random. We've discussed this erratically a few times, but never really had a firm discussion about it, which maybe we should. We can address it in a separate issue, so I suggest leave this for now as it is.

nolash · 2017-11-29T15:49:42Z

swarm/api/config.go

-		self.EnsRoot = ens.TestNetAddress
+	//create the directory if it doesn't exist
+	if _, err := os.Stat(self.Path); os.IsNotExist(err) {
+		os.MkdirAll(self.Path, os.ModePerm)
 	}



Should this create the <datadir>/swarm/bzz-<key> datadir? Now it's set to the same as the node's?

Only if it doesn't exist?

It would actually create --datadir if it wouldn't exist - which should not happen at this point, but added this just to be safe.

I actually added this last minute, after some running tests were producing errors (which then turned out not being related to the datadir). I wonder if this is needed indeed?

well it could happen if we never ran geth before and are running swarm --ens-api ''?

My point was where is the swarm/bzz-xxxx subdir created? I missed it maybe?

This is a spot-on remark! Made me realize quite a few things! Thanks @nolash ! (And bzz-xxxx wasn't even being created at all!)

… overrides

holisticode · 2017-12-11T16:33:14Z

Retest this please

This commit adds a TOML configuration option to swarm. It reuses the TOML configuration structure used in geth with swarm customized items. The commit: * Adds a "dumpconfig" command to the swarm executable which allows printing the (default) configuration to stdout, which then can be redirected to a file in order to customize it. * Adds a "--config <file>" option to the swarm executable which will allow to load a configuration file in TOML format from the specified location in order to initialize the Swarm node The override priorities are like follows: environment variables override command line arguments override config file override default config.

nonsense reviewed Nov 24, 2017

View reviewed changes

nolash reviewed Nov 29, 2017

View reviewed changes

swarm: New TOML config file option,default conf,cmd line and env vars…

07a3c65

… overrides

cobordism approved these changes Dec 11, 2017

View reviewed changes

janos mentioned this pull request Dec 11, 2017

swarm: repeated --ens-api CLI flag (#14386) ethersphere/swarm#142

Closed

fjl merged commit 32516c7 into ethereum:master Dec 11, 2017

gbalint mentioned this pull request Dec 13, 2017

reduce the number of errors in http access ethersphere/swarm#112

Closed

7 tasks

karalabe added this to the 1.8.0 milestone Dec 14, 2017

christopherdro mentioned this pull request Jan 14, 2018

[Store] Allow parameters to be set via cli and env vars ethersphere/swarm#195

Merged

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm: add config file #15548

swarm: add config file #15548

holisticode commented Nov 23, 2017

nonsense Nov 24, 2017

holisticode Nov 24, 2017

nonsense Nov 24, 2017

nonsense Nov 24, 2017

nolash left a comment

nolash Nov 27, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

nolash Nov 27, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

cobordism Dec 11, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017 •

edited

Loading

nolash Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 29, 2017

nolash Nov 29, 2017

holisticode Nov 30, 2017

holisticode commented Dec 11, 2017

swarm: add config file #15548

swarm: add config file #15548

Conversation

holisticode commented Nov 23, 2017

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

nolash 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

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

holisticode Nov 29, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holisticode commented Dec 11, 2017

holisticode Nov 29, 2017 •

edited

Loading