[azure_application_insights] add client secret#17899
[azure_application_insights] add client secret#17899jakubgalecki0 merged 14 commits intoelastic:mainfrom
Conversation
✅ Vale Linting ResultsNo issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
There was a problem hiding this comment.
I think this constraint should be refined to the versions of Beats which include the api key change onwards elastic/beats#48880.
| conditions: | ||
| kibana: | ||
| version: "^8.13.0 || ^9.0.0" | ||
| version: "^8.19.14 || ^9.2.8" |
There was a problem hiding this comment.
As we have back-ported to 8.19.0 and 9.2.0. Should we use them instead?
CC: @tommyers-elastic
There was a problem hiding this comment.
we should use these constraints:
conditions:
kibana:
version: "~8.19.14 || ~9.2.8 || ^9.3.3"for major.minor.patch:
- the
~modifier allows newer patch releases - the
^modifier also allows new minor releases
|
|
||
| `Api Key`:: (_[]string_) The API key which will be generated. See [Azure Monitor Log Analytics API Overview](https://learn.microsoft.com/en-us/azure/azure-monitor/logs/api/overview) for more information. | ||
| `Authentication Type`:: (_string_) Optional. The authentication method to use. Accepted values: `api_key` or `client_secret`. Defaults to `api_key` if not set. | ||
|
|
There was a problem hiding this comment.
Can we add a note in the README highlighting that users currently authenticating via API Key should migrate to Client Secret?
|
|
||
| `Api Key`:: (_[]string_) The API key which will be generated. See [Azure Monitor Log Analytics API Overview](https://learn.microsoft.com/en-us/azure/azure-monitor/logs/api/overview) for more information. | ||
| `Authentication Type`:: (_string_) Optional. The authentication method to use. Accepted values: `api_key` or `client_secret`. Defaults to `api_key` if not set. | ||
|
|
| conditions: | ||
| kibana: | ||
| version: "^8.13.0 || ^9.0.0" | ||
| version: "^8.19.0 || ^9.2.0" |
There was a problem hiding this comment.
This would be actually:
-
= 8.19.31
-
= 9.2.8
-
= 9.3.3
There was a problem hiding this comment.
Except if we add a comment on the authentication method that selecting Authentication Type is allowed only on 8.19.31, 9.2.8, 9.3.3 onwards
| conditions: | ||
| kibana: | ||
| version: "^8.13.0 || ^9.0.0" | ||
| version: "^8.19.0 || ^9.2.0" |
There was a problem hiding this comment.
We probably need something like this:
conditions:
kibana:
version: "~8.19.14 || ~9.2.8 || ^9.3.3"| conditions: | ||
| kibana: | ||
| version: "^8.13.0 || ^9.0.0" | ||
| version: "^8.19.0 || ^9.2.0" |
There was a problem hiding this comment.
Except if we add a comment on the authentication method that selecting Authentication Type is allowed only on 8.19.31, 9.2.8, 9.3.3 onwards
| - name: auth_type | ||
| type: select | ||
| title: Authentication Type | ||
| description: The authentication type to use. Either "api_key" or "client_secret". |
There was a problem hiding this comment.
Can we put a warning that this setting can be used only on 8.19.14, 9.2.8, 9.3.3 onwards?
kibana.version is only a "proxy" of the recommended version and checks only Kibana.
Maybe it is better to be explicit here.
There was a problem hiding this comment.
| description: The authentication type to use. Either "api_key" or "client_secret". | |
| description: Only for Elastic Agent 8.19.14, 9.2.8, 9.3.3 or more recent: the authentication type to use. Either "api_key" or "client_secret". |
This is a suggestion, not the actual "text" to display.
886b196 to
df6a643
Compare
zmoog
left a comment
There was a problem hiding this comment.
LGTM overall.
I suggest replacing "Azure AD" with "Entra ID" as MSFT moved away from the old service name.
Also consider default the authentication type to client secret for new installs since we're so close to the EOL for API key support.
| `Application ID`:: (_[]string_) ID of the application. This is Application ID from the API Access settings blade in the Azure portal. | ||
|
|
||
| `Api Key`:: (_[]string_) The API key which will be generated, more on the steps here https://dev.applicationinsights.io/documentation/Authorization/API-key-and-App-ID. | ||
| `Authentication Type`:: (_string_) Optional. The authentication method to use. Accepted values: `api_key` or `client_secret`. Defaults to `api_key` if not set. |
There was a problem hiding this comment.
nit: should we flip the default auth to client_secret for new installs? Microsoft is discontinuing API key support soon, right?
| `Api Key`:: (_[]string_) The API key which will be generated. See [Azure Monitor Log Analytics API Overview](https://learn.microsoft.com/en-us/azure/azure-monitor/logs/api/overview) for more information. | ||
| `Authentication Type`:: (_string_) Optional. The authentication method to use. Accepted values: `api_key` or `client_secret`. Defaults to `api_key` if not set. | ||
|
|
||
| NOTE: API Key authentication is deprecated. Users currently authenticating via API Key are encouraged to migrate to Client Secret authentication. Client Secret authentication uses Azure Active Directory (Azure AD) and provides more robust security through short-lived tokens. |
There was a problem hiding this comment.
Is MSFT still actively using the "Azure Active Directory"? I guess they moved to Entra ID naming now.
| @@ -1,3 +1,8 @@ | |||
| - version: "1.10.0-next" | |||
| changes: | |||
| - description: Add support for Azure AD client secret authentication. New optional parameters `auth_type`, `client_id`, `client_secret`, and `tenant_id` have been added. `api_key` is now optional. | |||
There was a problem hiding this comment.
As before, i suggest to replace "Azure AD" with Entra ID.
| description: The client ID for Azure AD authentication. | ||
| multi: false | ||
| required: false | ||
| show_user: true | ||
| - name: client_secret | ||
| type: password | ||
| title: Client Secret | ||
| description: The client secret for Azure AD authentication. | ||
| secret: true | ||
| multi: false | ||
| required: false | ||
| show_user: true | ||
| - name: tenant_id | ||
| type: text | ||
| title: Tenant ID | ||
| description: The tenant ID for Azure AD authentication. | ||
| multi: false | ||
| required: false |
There was a problem hiding this comment.
I suggest replacing Azure AD with Entra ID.
💚 Build Succeeded
History
|
|
Package azure_application_insights - 1.10.0 containing this change is available at https://epr.elastic.co/package/azure_application_insights/1.10.0/ |
Proposed commit message
This pull request adds support for client secret authentication elastic/beats#48880
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots
API KEY