-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Load elasticsearch auth token from file #1319
Load elasticsearch auth token from file #1319
Conversation
plugin/storage/es/options.go
Outdated
flagSet.String( | ||
nsConfig.namespace+suffixTokenPath, | ||
nsConfig.TokenFilePath, | ||
"Path to a file containing bearer token. This flag also uses "+suffixCA+" if it is specified") |
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.
suffixCA
or suffixTokenPath
? I think it might be implicit, but the token is the whole content of the file, right? Perhaps we could make it more explicit?
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.
suffixCA
, it's not a bad thing to have more explanatory docs. I didn't know that we need root CA at all when using token.
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 wouldn't use the constant in the help message. It starts with a dot and will look weird. Just say CA.
plugin/storage/es/options.go
Outdated
flagSet.String( | ||
nsConfig.namespace+suffixTokenPath, | ||
nsConfig.TokenFilePath, | ||
"Path to a file containing bearer token. This flag also uses "+suffixCA+" if it is specified") |
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 wouldn't use the constant in the help message. It starts with a dot and will look weird. Just say CA.
plugin/storage/es/factory.go
Outdated
@@ -76,11 +76,13 @@ func (f *Factory) Initialize(metricsFactory metrics.Factory, logger *zap.Logger) | |||
|
|||
primaryClient, err := f.primaryConfig.NewClient(logger, metricsFactory) | |||
if err != nil { | |||
f.logger.Error("failed to create primary Elasticsearch client", zap.Error(err)) |
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.
Logging should happen in main. We may want to call errors.Wrap() here
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
b2fa5f9
to
fad3068
Compare
Codecov Report
@@ Coverage Diff @@
## master #1319 +/- ##
======================================
Coverage 100% 100%
======================================
Files 162 162
Lines 7277 7282 +5
======================================
+ Hits 7277 7282 +5
Continue to review full report at Codecov.
|
* Load elasticsearch auth token from file Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Wrap errors Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix lint Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix test Signed-off-by: Pavol Loffay <ploffay@redhat.com> Signed-off-by: Iori Yoneji <fivo.11235813@gmail.com>
* Load elasticsearch auth token from file Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Wrap errors Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix lint Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix test Signed-off-by: Pavol Loffay <ploffay@redhat.com>
* Load elasticsearch auth token from file Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Wrap errors Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix lint Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix test Signed-off-by: Pavol Loffay <ploffay@redhat.com>
In k8s the auth token (serviceaccount) is injected to pod.
Signed-off-by: Pavol Loffay ploffay@redhat.com