-
Notifications
You must be signed in to change notification settings - Fork 15
add advertised endpoint to cluster / active fo #192
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.
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
Changes from all commits
511d49c
888cbff
c59497b
239ee0b
55c33dd
796cf30
82c286e
c960e9c
dc4d1f9
a6284ee
e5cdde2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "io/ioutil" | ||
| "net/url" | ||
| "os" | ||
| "os/signal" | ||
| "path/filepath" | ||
|
|
@@ -86,6 +87,7 @@ var ( | |
| logService logging.Service | ||
| showVersion bool | ||
| id string | ||
| advertisedEndpoint string | ||
| agencySize int | ||
| arangodPath string | ||
| arangodJSPath string | ||
|
|
@@ -192,7 +194,7 @@ func init() { | |
| pf.StringVar(&logDir, "log.dir", getEnvVar("LOG_DIR", ""), "Custom log file directory.") | ||
| f.IntVar(&logRotateFilesToKeep, "log.rotate-files-to-keep", defaultLogRotateFilesToKeep, "Number of files to keep when rotating log files") | ||
| f.DurationVar(&logRotateInterval, "log.rotate-interval", defaultLogRotateInterval, "Time between log rotations (0 disables log rotation)") | ||
|
|
||
| f.StringVar(&advertisedEndpoint, "cluster.advertised-endpoint", "", "An external endpoint for the servers started by this Starter") | ||
| f.IntVar(&agencySize, "cluster.agency-size", 3, "Number of agents in the cluster") | ||
| f.BoolSliceVar(&startAgent, "cluster.start-agent", nil, "should an agent instance be started") | ||
| f.BoolSliceVar(&startDBserver, "cluster.start-dbserver", nil, "should a dbserver instance be started") | ||
|
|
@@ -568,6 +570,11 @@ func mustPrepareService(generateAutoKeyFile bool) (*service.Service, service.Boo | |
| log.Fatal().Err(err).Msgf("Unsupport image pull policy '%s'", dockerImagePullPolicy) | ||
| } | ||
|
|
||
| // Sanity checking URL scheme on advertised endpoints | ||
| if _, err := url.Parse(advertisedEndpoint); err != nil { | ||
| log.Fatal().Err(err).Msgf("Advertised cluster endpoint %s does not meet URL standards", advertisedEndpoint) | ||
| } | ||
|
|
||
| // Expand home-dis (~) in paths | ||
| arangodPath = mustExpand(arangodPath) | ||
| arangodJSPath = mustExpand(arangodJSPath) | ||
|
|
@@ -707,6 +714,7 @@ func mustPrepareService(generateAutoKeyFile bool) (*service.Service, service.Boo | |
| ArangodPath: arangodPath, | ||
| ArangoSyncPath: arangoSyncPath, | ||
| ArangodJSPath: arangodJSPath, | ||
| AdvertisedEndpoint: advertisedEndpoint, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Secondly I suggest to allow the use of "http" & "https" schemes in here and automatically transform them to "tcp" & "ssl"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We had that discussion and decided to the contrary. It had to suffice the enpoint scheme. Keep in mind that this was supposed to not be interpreted in any way and match the endpoint specification and was the reason we decided to not call it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly disagree on this. The starter is supposed to make life easier and having think about "tcp://" or "ssl://" where everyone knows about "http://" and "https://" does not help. |
||
| MasterPort: masterPort, | ||
| RrPath: rrPath, | ||
| DataDir: dataDir, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the advertised endpoint should be checked (when non-empty) to make sure it is a valid format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, also here we had decided to not do any sanity checks. It it does not suffice the endpoint specification, the affected services will complain. Effectively like wrong passthrough options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so sure on the "no sanity checks". If we decided to that, it sounds like a bad decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now test was added. Problem solved.