-
Notifications
You must be signed in to change notification settings - Fork 721
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
Introduce Kibana config field in stack config policy #7324
Introduce Kibana config field in stack config policy #7324
Conversation
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.
\"test-stack-config-policy\" is invalid: spec.elasticsearch: Required value: Elasticsearch settings are mandatory and must not be empty"
I guess we should add something along those lines in the validation logic:
if policy.Spec.Kibana.Config != nil {
settingsCount += len(policy.Spec.Kibana.Config.Data)
}
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.
Haven't gotten fully through this, or tested locally, but some initial things I saw.
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.
Still yet to test this locally, but have gotten through looking at all of the changes...
for i := range secrets.Items { | ||
secret := secrets.Items[i] |
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 know this isn't your code, but do you see any reason this shouldn't be re-written as:
if _, secret := range secrets.Items {
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 it is because the linter will complain when we try send the memory address of the secret here
err := c.Delete(ctx, &secret)
if err != nil && !apierrors.IsNotFound(err) {
return err
}
referencing memory of temporary loop variables
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.
Code looks good to me. I have not gotten round to actually testing it though.
pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go
Outdated
Show resolved
Hide resolved
pkg/controller/stackconfigpolicy/elasticsearch_config_settings.go
Outdated
Show resolved
Hide resolved
80523cc
to
5a54010
Compare
buildkite test this -f p=gke |
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.
LGTM. Nice work! I did some very basic tests only. We already discussed that we need some documentation updates. Are you also going to adjust/extend our e2e tests for the stack config policy feature?
Yeah I can look into adding/ adjusting existing e2e tests, can that be a separate PR or should be part of this ? |
Can be separate |
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.
LGTM, great job 👍
default: | ||
// Just return empty since there are no other resource type monitored by the stack config policy | ||
log := ulog.FromContext(ctx) | ||
log.Info("Unknown resource kind, stackconfigpolicy only monitors Elasticsearch and Kibana resources", "resource kind", resourceKind) |
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.
Should we return an error here?
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.
This function is called from secureSettingsVolume
in the common keystore package, which I think could be called from the apm, beat logstash etc, that is why I just decided to log and return.
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Introduce a new Kibana field in StackConfigPolicy that has two sub fields, config and secureSettings. These fields can be used to configure Kibana configurations that go into kibana.yml and to store secrets in Kibana keystore --------- Co-authored-by: Michael Morello <michael.morello@gmail.com> Co-authored-by: Michael Montgomery <mmontg1@gmail.com> Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com> Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
What does this change?
Introduce Kibana config Field in StackConfigPolicy. This field can be used to manage multiple kibana instances using stackConfigPolicy. The only configuration that can be configured for kibana using the StackConfigPolicy is the kibanaConfig which are the configurations that go into the kibana.yml file.
Testing
I Have tested with all four auth config policies that we intend to support , JWT, OIDC, SAML and LDAP. I was able to successfully set these up with the StackConfigPolicy and also login into Kibana(make API calls against Elasticsearch in case of JWT) successfully.
However there are limitations here. With JWT and LDAP it is straightforward to configure multiple elasticsearch clusters with the same auth config. With OIDC we generate client secret and application ID and while doing so in the identity provider we enter a redirect url which is generally
KIBANA_ENDPOINT_URL/api/security/oidc/callback
now to be able to configure multiple KIbana instances to work with this, there are some identity providers like Google that allow you enter more than on redirect url. So it is possible to configure OIDC for multiple elasticsearch clusters as long as the provider being used allows entering multiple urls for the redirect url. However for SAML I could not find a similar option and to me it seems like we might not be able to configure multiple instances using the same SAML config as the SAML config is generated using a redirect url that points to a specific kibana instance.JWT
I tested configuring JWT auth policy and it worked as expected. I add the jwks secret, elasticsearch config, role mapping and shared secret to the stackconfigpolicy, and then used the JWT and shared secret in headers to call Elasticsearch
Output
I used the elastic-agent tokens so that I don't have to create a new role. But I found this was causing issues to Kibana due to the secure settings being applied to both Kibana and Elasticsearch. Working on a fix for this
OIDC
StackConfigPolicy
SAML with Okta
StackConfigPolicy
LDAP
I set up a LDAP server using the openLDAP helm chart on my dev kubernetes cluster and then configured two elasticsearch clusters via stack config policy to interact with it. I was able to sucessfully login via kibana using a LDAP user.
stackConfigPolicy