-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Support Kibana Spaces #8045
Support Kibana Spaces #8045
Conversation
Note that there is currently a bug in the Kibana Spaces implementation which allows a non-existent space ID to be specified without causing any errors: elastic/kibana#21408 (comment). Once this bug is resolved, I'd like to re-test this PR with a non-existent space ID to make sure we fail with a good error message. |
I think it would be worth to add a system test for this to make sure it works. There are already tests loading dashboards which could be copied and adjusted. |
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 suggest to add the system tests directly as part of this PR? Code looks good but would be nice if we have automated tests around it so we also know in case functionality breaks in the Kibana side.
Yes, that’s my plan. I just haven’t got to it yet since your previous
comment because of support live fire followed by being on holiday now. I
plan to get to it next week when I’m back from holiday.
…On Thu, Aug 23, 2018 at 07:25 Nicolas Ruflin ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I suggest to add the system tests directly as part of this PR? Code looks
good but would be nice if we have automated tests around it so we also know
in case functionality breaks in the Kibana side.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8045 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADHdXv_WAJJ7B4xxSET2WgTC_oYblXCks5uTpEYgaJpZM4WGuzK>
.
|
@@ -48,11 +49,14 @@ func makeURL(url, path string, params url.Values) string { | |||
return strings.Join([]string{url, path, "?", params.Encode()}, "") | |||
} | |||
|
|||
func Export(client *http.Client, conn string, dashboard string, out string) error { | |||
func Export(client *http.Client, conn string, spaceID string, dashboard string, out string) error { |
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.
exported function Export should have comment or be unexported
45598ca
to
f8260de
Compare
@ycombinator @ruflin I'm pretty familiar with the Kibana dev environment but this is the first time I've been asked to be a reviewer for any beats changes 😄 What's the best way to go about testing this PR? Does this seem about right? Also, does make start the beat? How would I test with
Sorry for the n00b questions |
@alexfrancoeur Ahhhh, sorry about that, but the steps you outlined are 👌. After the last step (
|
b10d78d
to
69f53b7
Compare
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 changes overall look good to me.
Is CI not happy yet because the Kibana PR is not merged yet? Please ping me as soon as Kibana PR is in so I can manually test this against master.
@ruflin Yeah, CI is failing on this PR at the moment because the Kibana Spaces PR (elastic/kibana#21408) hasn't been merged into So I'm going to leave this PR here as-is until then and resolve any conflicts, etc. all at once after the Kibana Spaces PR has landed in |
@legrego I would strongly prefer if we could get this rather sooner then later into master so we have enough time on our side to test the feature and integrate with it. |
Understood. We'd love to get it in as soon as possible, but we are also trying to be realistic with our timeline. It's a large PR that will likely have a long review cycle, but we will do everything we can to get it into master in a reasonable timeframe. In the meantime, is there anything we can do to facilitate testing? |
@legrego Thanks for the details. I think so far we are covered as we manually test against the feature branch. |
69f53b7
to
a4d4d84
Compare
Now that the Kibana Spaces PR is merged, I've rebased this PR on |
4e38414
to
0f6feb8
Compare
0f6feb8
to
30ed549
Compare
CI is failing because the latest snapshot of Kibana being used by system tests is actually 3+ days old and therefore does not yet contain Spaces in it. |
Jenkins, test this. |
1 similar comment
Jenkins, test this. |
@ruflin CI is green now, after Kibana Spaces PR was merged into |
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.
Did not test it locally but code and system tests LGTM.
} | ||
|
||
headers = { | ||
"kbn-xsrf": "1" |
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.
out of curiosity: Why is this needed?
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.
It's for protecting against CSRF attacks: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Protecting_REST_Services:_Use_of_Custom_Request_Headers
Cherry-pick of PR #8045 to 6.x branch. Original message: Resolves #7942. Kibana is implementing a new feature called Spaces, in which Kibana saved objects (such as dashboards) and advanced settings can be restricted to a user-defined namespace. Spaces are identified by a unique ID, e.g. `my-space`. There is also a notion of a Default space which corresponds to how Kibana worked up until the Spaces feature was introduced. Beats have the ability to import dashboards into Kibana as well as export dashboards out of Kibana. With Spaces, dashboards may belong to a specific space. So Beats must learn to accept an optional Space ID and operate against it when importing or exporting dashboards. This PR teaches Beats to do just that. Concretely, if a user wishes to import or export dashboards from a specific space, say with ID = `my-space`, they must either: * Edit `<beat>.yml` and set `setup.kibana.space.id: my-space`, _or_ * Run `<beat> setup` or `<beat> export dashboard` along with the `-E setup.kibana.space.id=my-space` option. Similarly, if a Beat _developer_ wishes to export dashboards from a specific space, say with ID = `my-space`, they must run: ```sh go run dev-tools/cmd/dashboards/export_dashboards.go -space-id my-space [other options] ```
The docs missing from elastic#8045
Resolves #7942.
Kibana is implementing a new feature called Spaces, in which Kibana saved objects (such as dashboards) and advanced settings can be restricted to a user-defined namespace. Spaces are identified by a unique ID, e.g.
my-space
. There is also a notion of a Default space which corresponds to how Kibana worked up until the Spaces feature was introduced.Beats have the ability to import dashboards into Kibana as well as export dashboards out of Kibana. With Spaces, dashboards may belong to a specific space. So Beats must learn to accept an optional Space ID and operate against it when importing or exporting dashboards. This PR teaches Beats to do just that.
Concretely, if a user wishes to import or export dashboards from a specific space, say with ID =
my-space
, they must either:<beat>.yml
and setsetup.kibana.space.id: my-space
, or<beat> setup
or<beat> export dashboard
along with the-E setup.kibana.space.id=my-space
option.Similarly, if a Beat developer wishes to export dashboards from a specific space, say with ID =
my-space
, they must run: