Skip to content
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

control-plane: bootstrap configuration server #129

Merged
merged 3 commits into from
Aug 28, 2019

Conversation

jakubdyszkiewicz
Copy link
Contributor

This PR introduces server that provides an initial bootstrap configuration for Envoy.

The client of this server provides Node ID, for this Node ID we look for Dataplane definition in our resources store and seek required data. We also define a XDS Server location on the server side.

components/konvoy-control-plane/pkg/config/loader_test.go Outdated Show resolved Hide resolved
@@ -23,6 +23,8 @@ type XdsServerConfig struct {
DataplaneConfigurationRefreshInterval time.Duration `yaml:"dataplaneConfigurationRefreshInterval" envconfig:"konvoy_xds_server_dataplane_configuration_refresh_interval"`
// Interval for flushing status of Dataplanes connected to the Control Plane
DataplaneStatusFlushInterval time.Duration `yaml:"dataplaneStatusFlushInterval" envconfig:"konvoy_xds_server_dataplane_status_flush_interval"`
// Configuration of Bootstrap Server, which provides bootstrap config to Dataplanes
Bootstrap *XdsBootstrapServerConfig `yaml:"bootstrap"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, bootstrapServer configuration should be separate from xdsServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't sure about this. Logically in code the bootstrap logic is placed in xds package. But from user perspective it makes more sense to put it outside. Changed.

Btw, shouldn't the diagnostic port also be separated from XdsConfig?

@jakubdyszkiewicz jakubdyszkiewicz force-pushed the feature/bootstrap-configuration-server branch from 884f875 to 6450d6a Compare August 27, 2019 15:15
Copy link
Contributor

@yskopets yskopets left a comment

Choose a reason for hiding this comment

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

I've left a few comments on log messages.

Regarding "diagnostics port", let's keep it for now in the context of XDS server.

@@ -66,6 +69,9 @@ func (c *Config) Validate() error {
if err := c.XdsServer.Validate(); err != nil {
return errors.Wrap(err, "Xds Server validation failed")
}
if err := c.BootstrapServer.Validate(); err != nil {
return errors.Wrap(err, "Bootstrap validation failed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change it to Bootstrap Server validation failed


func (b *BootstrapParamsConfig) Validate() error {
if b.AdminPort < 0 {
return errors.New("Port cannot be negative")
Copy link
Contributor

Choose a reason for hiding this comment

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

AdminPort cannot be negative

return errors.New("Port cannot be negative")
}
if b.XdsHost == "" {
return errors.New("Host cannot be empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change it to XdsHost cannot be empty

return errors.New("Host cannot be empty")
}
if b.XdsPort < 0 {
return errors.New("Port cannot be negative")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change it to XdsPort cannot be negative

"net/http"
)

var log = core.Log.WithName("xds-server").WithName("bootstrap")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change it to core.Log.WithName("bootstrap-server")

@jakubdyszkiewicz
Copy link
Contributor Author

minor changes fixed in another PR, so I won't have to rebase and do both CI checks and stuff.

@jakubdyszkiewicz jakubdyszkiewicz merged commit 1b0dd55 into master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants