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

Kibana api key #372

Merged
merged 22 commits into from
Nov 23, 2023
Merged

Kibana api key #372

merged 22 commits into from
Nov 23, 2023

Conversation

andrianjardan
Copy link
Contributor

Fixes #364

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 30, 2023

💚 CLA has been signed

@andrianjardan
Copy link
Contributor Author

I guess disaster37/go-kibana-rest#5 has to be merged first for this to work.

@andrianjardan
Copy link
Contributor Author

Seems like there's no activity in disaster37/go-kibana-rest#5, and the project looks abandoned

@andrianjardan
Copy link
Contributor Author

andrianjardan commented Aug 9, 2023

https://github.com/disaster37/go-kibana-rest is abandoned since 2022, @tobio since this is a library crucial for the plugin functionality, does it make sense to fork it under Elastic orga ?

@tobio
Copy link
Member

tobio commented Aug 15, 2023

does it make sense to fork it under Elastic orga

Yes it does, there's been some discussions about this previously. I'm not clear where that stands right now. @Kushmaro FYI on having official client support for Kibana.

@andrianjardan IIUC Kibana API's should come with an OpenAPI spec in the future which reduces the need for a hand-written client library. I wonder if the interim solution here is to fork the client inside this repo rather than by itself.

@andrianjardan
Copy link
Contributor Author

@tobio I could do so, should I ?

@tobio
Copy link
Member

tobio commented Aug 17, 2023

That would be amazing if you've got an opportunity to do so!

@andrianjardan
Copy link
Contributor Author

@tobio aaand it's done ;)

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Can you update the existing provider test to verify that Kibana API keys are working too.

@@ -508,6 +514,9 @@ func buildKibanaConfig(d *schema.ResourceData, baseConfig BaseConfig) (kibana.Co
if password := os.Getenv("KIBANA_PASSWORD"); password != "" {
config.Password = strings.TrimSpace(password)
}
if api_key := os.Getenv("KIBANA_API_KEY"); api_key != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make sure this is added to the provider docs as well.

Copy link
Member

Choose a reason for hiding this comment

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

API key support also needs to be added to connectors, as defaults for the fleet client and the SLO auth context

That's too many places :( There must be a better way.

Copy link
Contributor

@jloleysens jloleysens Nov 20, 2023

Choose a reason for hiding this comment

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

I agree, it would be nice refactor some of this functionality. I'm hopeful we'll have a single client for the entire Kibana surface area soon so that we don't have to implement glue code around all the existing clients.

@@ -414,6 +414,11 @@ func (c *APIClient) prepareRequest(
localVarRequest.SetBasicAuth(auth.UserName, auth.Password)
}

// APIKey Authentication
Copy link
Member

Choose a reason for hiding this comment

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

This has been manually added to the generated client? I think that's fine for now, these openapi clients are a bit of a mess right now. We need to tidy this up.

Can we add a comment calling out that this is inserted after generation.

@jloleysens
Copy link
Contributor

Just wanted to ^bump this as adding support for Kibana API keys in the terraform provider would be very useful!

@andrianjardan
Copy link
Contributor Author

@jloleysens I'll try to come back to it next week

@tobio tobio mentioned this pull request Oct 1, 2023
@electra06
Copy link

@andrianjardan - any update on this?

@andrianjardan
Copy link
Contributor Author

@electra06 I am unfortunately busy with other tasks right now and the progress here is on hold, but I will do my best to continue working on it next week.

@electra06
Copy link

@electra06 I am unfortunately busy with other tasks right now and the progress here is on hold, but I will do my best to continue working on it next week.

Thank you @andrianjardan - one of our developers has this requirement and it would be helpful once this has been pushed thru then.

@electra06
Copy link

@andrianjardan - bumping on this once again, hope you got the time to work on this one this week :-)

@electra06
Copy link

@andrianjardan - sorry to bump this again, our developer is following up as this one is very useful for them. Appreciate your time and effort.

@andrianjardan
Copy link
Contributor Author

@electra06 absolutely no worries, I understand this is useful, and we also need it, but I got sick last week and the amount of work is not getting less...

Sorry for dragging it for too long, but it was intended as a small and quick fix, and turned into a longer journey, as my experience with Golang is quite limited.

I am happy to hand it over to someone else who has more time and expertise, if needed.

@jloleysens
Copy link
Contributor

@andrianjardan , I've attempted to address outstanding items, hopefully we can get this over the line!

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

self-review

@@ -583,7 +583,7 @@ func (a *AlertingApiService) CreateRuleExecute(r ApiCreateRuleRequest) (*RuleRes
} else {
key = apiKey.Key
}
localVarHeaderParams["ApiKey"] = key
localVarHeaderParams["Authorization"] = key
Copy link
Contributor

Choose a reason for hiding this comment

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

This generated Kibana client code is buggy :(

We will have to revisit this and ensure we can generate functioning HTTP requests from the OpenAPI specs. I think this task is beyond this PR so I've modified the generated code directly. We should open an issue for fixing this!

generated/alerting/api/openapi.yaml Show resolved Hide resolved
@andrianjardan
Copy link
Contributor Author

@jloleysens thanks a lot for jumping on this, I am sure it is going to be appreciated by everyone waiting :)!

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Thanks for moving this forward, couple of comments on the code.

generated/alerting/api/openapi.yaml Show resolved Hide resolved
internal/kibana/alerting_test.go Show resolved Hide resolved
internal/schema/connection.go Show resolved Hide resolved
internal/clients/config/kibana.go Outdated Show resolved Hide resolved
internal/schema/connection.go Show resolved Hide resolved
@jloleysens
Copy link
Contributor

jloleysens commented Nov 22, 2023

@tobio I've updated the docs in 0885614, would you mind doing another review?

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM. I see a weird issue in running the acceptance tests locally with an API key here, but it looks unrelated, I'll open an issue for it. The tests pass if run individually which is the main thing, but one of the tests (probably the API key test but I haven't looked at all) is cleaning up existing keys so when the entire suite is run, the key based tests fail since the key is removed in an earlier test.

@tobio
Copy link
Member

tobio commented Nov 23, 2023

FYI #490

@jloleysens jloleysens merged commit 6fcbbaa into elastic:main Nov 23, 2023
14 checks passed
@electra06
Copy link

Hi All,
Thank you for moving this forward :-), looks like there is still an issue?

@tobio
Copy link
Member

tobio commented Nov 24, 2023

looks like there is still an issue?

What's the issue?

@electra06
Copy link

What's the issue?
@tobio just confirming :-), can we share this now to the customer?

@tobio
Copy link
Member

tobio commented Nov 24, 2023

We'll need a new provider release including this PR first. I don't have a timeframe to share right now, we're discussing that internally at the moment.

daemitus pushed a commit to daemitus/terraform-provider-elasticstack that referenced this pull request Nov 30, 2023
* Adding api key functionality for Kibana

* Adding support for api_key for Kibana

* Adding go-kibana-rest lib locally as it is abandoned by authors

* update docs

* added support for SLO client to use API keys

* add auth interceptor to connectors client

* added test case for Kibana API keys via provider

* slight fix to table test

* added fwschema

* fix copy-pasta

* some typos, generate docs

* manually patch generated code for  now

* update specs

* update new kb config from framework

* added api key to provider tests

* use built in jq

* remove kibana api key env

* API keys should cascade from ES->Kibana->Fleet

* update rules doc

---------

Co-authored-by: Jean-Louis Leysens <[email protected]>
Co-authored-by: Toby Brain <[email protected]>
daemitus pushed a commit to daemitus/terraform-provider-elasticstack that referenced this pull request Nov 30, 2023
* Adding api key functionality for Kibana

* Adding support for api_key for Kibana

* Adding go-kibana-rest lib locally as it is abandoned by authors

* update docs

* added support for SLO client to use API keys

* add auth interceptor to connectors client

* added test case for Kibana API keys via provider

* slight fix to table test

* added fwschema

* fix copy-pasta

* some typos, generate docs

* manually patch generated code for  now

* update specs

* update new kb config from framework

* added api key to provider tests

* use built in jq

* remove kibana api key env

* API keys should cascade from ES->Kibana->Fleet

* update rules doc

---------

Co-authored-by: Jean-Louis Leysens <[email protected]>
Co-authored-by: Toby Brain <[email protected]>
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.

[Feature] Add API Key authentication to kibana
4 participants