-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat(kuma-cp) expose config #454
Conversation
@@ -10,7 +10,7 @@ type GuiServerConfig struct { | |||
// Port on which the server is exposed | |||
Port uint32 `yaml:"port" envconfig:"kuma_gui_server_port"` | |||
// Config of the GUI itself | |||
GuiConfig *GuiConfig | |||
GuiConfig *GuiConfig `yaml:"-"` |
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.
I don't understand. Why don't we want to see this field in YAML ?
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.
Otherwise the json/yaml looks wierd. Like this:
{
"guiServer": {
"port": 1234,
"GuiConfig": {
"ApiUrl": "",
"Environment": "",
}
}
}
everything in config is lowercase instead of this. If we write yaml
tags, we will expose this config to the users.
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.
Up to you.
Personally, I see the value in exposing the entire config even if it comes at a cost.
pkg/config/api-server/config.go
Outdated
@@ -16,7 +16,7 @@ type ApiServerConfig struct { | |||
// If true, then API Server will operate in read only mode (serving GET requests) | |||
ReadOnly bool `yaml:"readOnly" envconfig:"kuma_api_server_read_only"` | |||
// API Catalogue | |||
Catalogue *catalogue.CatalogueConfig | |||
Catalogue *catalogue.CatalogueConfig `yaml:"-"` |
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.
I don't understand. Why don't we want to see this field in YAML ?
"github.com/kelseyhightower/envconfig" | ||
"github.com/pkg/errors" | ||
"gopkg.in/yaml.v2" // todo(jakubdyszkiewicz) switch to "github.com/ghodss/yaml" when solved problems with loading xdsServer | ||
// we use gopkg.in/yaml.v2 because it supports time.Duration |
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.
I don't understand this.
Here is an example of ghodds/yaml
doing marshalling/unmarshalling of time.Duration
- https://play.golang.org/p/NdpmTyqitmw
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.
Sorry I wasn't clear enough. ghodss
supports time.Duration
, but you cannot deserialize 2s
, you have to write 2000000000
which I'd say is not user friendly at all
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.
There are workarounds for this, e.g. https://stackoverflow.com/questions/48050945/how-to-unmarshal-json-into-durations
I'm not sure I get this. Here is a working example - https://play.golang.org/p/NdpmTyqitmw
Let's go ahead with it. A more conventional name for that func would be
Sub-configs are pointers if (and only if) they're optional. It wouldn't be right to break this convention for the sake of |
ba8f651
to
787be60
Compare
"github.com/kelseyhightower/envconfig" | ||
"github.com/pkg/errors" | ||
"gopkg.in/yaml.v2" // todo(jakubdyszkiewicz) switch to "github.com/ghodss/yaml" when solved problems with loading xdsServer | ||
// we use gopkg.in/yaml.v2 because it supports time.Duration |
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.
There are workarounds for this, e.g. https://stackoverflow.com/questions/48050945/how-to-unmarshal-json-into-durations
runLog.Error(err, "unable to prepare config for display") | ||
return err | ||
} | ||
runLog.Info(fmt.Sprintf("Current config %s", cfgBytes)) |
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.
I think, logging API is designed in away to avoid explicit fmt.Sprintf()
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.
The problem is that if I the bytes the whole json is escaped which is not readable at all.
not sure if custom duration time is better. |
Summary
Starting as draft, because of open questions.
Library
I tried to switch from
gopkg.in/yaml.v2
togithub.com/ghodss/yaml
. It required to changeyaml
tojson
annotations, but it turned out thatghodss
does not supporttime.Duration
, therefore you cannot put in config stuff likexdsConnectTimeout: 1s
. I'd say this is a decrease in UX.Secret values
For now, we only want to hide the Postgres password, but who knows what will come next. I see 2 ways to implement this.
secret:"true"
next toenvconfig
andyaml
. Then scan struct recursivly via reflection and replace it via placeholder. Extra tricky part, the value can be of many types.func HideSecretValues()
method onConfig
, call that for every subconfig and explicitly replace the values.I like the second option better. Although probably a bit more code, it is explicit and less magical.
Pointers in config
We have to replace secrets in config so we have to copy a config. It's a problem since subconfigs are pointers. We can "hack" it with marshal -> unmarshal, but I'd say it's better to get rid of pointers. It's kind of uncomfortable to think that any component can change the config at any given time of running app.