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

[APM] Add Agent Keys in APM settings - Create agent keys #120373

Merged
merged 17 commits into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions x-pack/plugins/apm/common/agent_key_types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export interface CreateApiKeyResponse {
api_key: string;
expiration?: number;
id: string;
name: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import { ConfirmDeleteModal } from './confirm_delete_modal';

interface Props {
agentKeys: ApiKey[];
refetchAgentKeys: () => void;
onKeyDelete: () => void;
}

export function AgentKeysTable({ agentKeys, refetchAgentKeys }: Props) {
export function AgentKeysTable({ agentKeys, onKeyDelete }: Props) {
const [agentKeyToBeDeleted, setAgentKeyToBeDeleted] = useState<ApiKey>();

const columns: Array<EuiBasicTableColumn<ApiKey>> = [
Expand Down Expand Up @@ -159,7 +159,7 @@ export function AgentKeysTable({ agentKeys, refetchAgentKeys }: Props) {
agentKey={agentKeyToBeDeleted}
onConfirm={() => {
setAgentKeyToBeDeleted(undefined);
refetchAgentKeys();
onKeyDelete();
}}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { useState, useEffect } from 'react';
import { i18n } from '@kbn/i18n';
import {
EuiButton,
EuiFlyout,
EuiFlyoutHeader,
EuiTitle,
EuiFlyoutBody,
EuiFlexGroup,
EuiFlexItem,
EuiFlyoutFooter,
EuiButtonEmpty,
EuiForm,
EuiFormRow,
EuiSpacer,
EuiFieldText,
EuiText,
EuiFormFieldset,
EuiCheckbox,
htmlIdGenerator,
} from '@elastic/eui';
import { isEmpty } from 'lodash';
import { callApmApi } from '../../../../services/rest/createCallApmApi';
import { useKibana } from '../../../../../../../../src/plugins/kibana_react/public';
import { ApmPluginStartDeps } from '../../../../plugin';
import { CreateApiKeyResponse } from '../../../../../common/agent_key_types';

interface Props {
onCancel: () => void;
onSuccess: (agentKey: CreateApiKeyResponse) => void;
onError: (keyName: string) => void;
}

export function CreateAgentKeyFlyout({ onCancel, onSuccess, onError }: Props) {
const {
services: { security },
} = useKibana<ApmPluginStartDeps>();

const [username, setUsername] = useState('');

const [formTouched, setFormTouched] = useState(false);
const [keyName, setKeyName] = useState('');
const [agentConfigChecked, setAgentConfigChecked] = useState(true);
const [eventWriteChecked, setEventWriteChecked] = useState(true);
const [sourcemapChecked, setSourcemapChecked] = useState(true);
Comment on lines +49 to +52
Copy link
Member

@sorenlouv sorenlouv Dec 7, 2021

Choose a reason for hiding this comment

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

WDYT about combining these into a single state?

Suggested change
const [keyName, setKeyName] = useState('');
const [agentConfigChecked, setAgentConfigChecked] = useState(true);
const [eventWriteChecked, setEventWriteChecked] = useState(true);
const [sourcemapChecked, setSourcemapChecked] = useState(true);
const [agentKeyBody, setAgentKeyBody] = useState<AgentKeyBody>({
name: '',
sourcemap: true,
event: true,
agentConfig: true
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a matter of preference and code consistency when working with forms, I normally use different states but happy to change it if a single state is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to single state in #120765


const isInputInvalid = isEmpty(keyName);
const isFormInvalid = formTouched && isInputInvalid;

const formError = i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.name.placeholder',
{ defaultMessage: 'Enter a name' }
);

useEffect(() => {
const getCurrentUser = async () => {
try {
const authenticatedUser = await security?.authc.getCurrentUser();
gbamparop marked this conversation as resolved.
Show resolved Hide resolved
setUsername(authenticatedUser?.username || '');
} catch {
setUsername('');
}
};
getCurrentUser();
}, [security?.authc]);
Comment on lines +62 to +72
Copy link
Member

@sorenlouv sorenlouv Dec 7, 2021

Choose a reason for hiding this comment

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

Can you extract this (and the useState) to a hook like:

const authenticatedUser = useAuthenticatedUser()

Copy link
Contributor Author

@gbamparop gbamparop Dec 7, 2021

Choose a reason for hiding this comment

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

Sure, could use something like this. Wouldn't be better to convert it to a hook if it's used another time?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, could use something like this

Yes, but let's just make our own. That one look pretty big.

Wouldn't be better to convert it to a hook if it's used another time?

No, I'd do it already now for better encapsulation. It's not about DRY-ness but readability (and maintainability).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a follow up PR with these changes


const createAgentKeyTitle = i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.createAgentKey',
{ defaultMessage: 'Create agent key' }
);

const createAgentKey = async () => {
setFormTouched(true);
if (isInputInvalid) {
return;
}

try {
const { agentKey } = await callApmApi({
endpoint: 'POST /apm/agent_keys',
signal: null,
params: {
body: {
name: keyName,
sourcemap: sourcemapChecked,
event: eventWriteChecked,
agentConfig: agentConfigChecked,
},
},
});

onSuccess(agentKey);
} catch (error) {
onError(keyName);
}
};

return (
<EuiFlyout onClose={onCancel} size="s">
<EuiFlyoutHeader hasBorder>
<EuiTitle>
<h2>{createAgentKeyTitle}</h2>
</EuiTitle>
</EuiFlyoutHeader>

<EuiFlyoutBody>
<EuiForm isInvalid={isFormInvalid} error={formError}>
{username && (
<EuiFormRow
label={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.userTitle',
{ defaultMessage: 'User' }
)}
>
<EuiText>{username}</EuiText>
</EuiFormRow>
)}
<EuiFormRow
label={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.nameTitle',
{
defaultMessage: 'Name',
}
)}
helpText={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.nameHelpText',
{
defaultMessage: 'What is this key used for?',
}
)}
isInvalid={isFormInvalid}
error={formError}
>
<EuiFieldText
name="name"
placeholder={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.namePlaceholder',
{
defaultMessage: 'e.g. apm-key',
}
)}
onChange={(e) => setKeyName(e.target.value)}
isInvalid={isFormInvalid}
onBlur={() => setFormTouched(true)}
/>
</EuiFormRow>
<EuiSpacer size="m" />
<EuiFormFieldset
legend={{
children: i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.privilegesFieldset',
{
defaultMessage: 'Assign privileges',
}
),
}}
>
<EuiFormRow
helpText={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.agentConfigHelpText',
{
defaultMessage:
'Required for agents to read agent configuration remotely.',
}
)}
>
<EuiCheckbox
id={htmlIdGenerator()()}
Copy link
Contributor

@cauemarcondes cauemarcondes Dec 6, 2021

Choose a reason for hiding this comment

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

Do you really need this htmlIdGenerator here and on the other components?

label="config_agent:read"
checked={agentConfigChecked}
onChange={() => setAgentConfigChecked((state) => !state)}
/>
</EuiFormRow>
<EuiSpacer size="s" />
<EuiFormRow
helpText={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.ingestAgentEvents',
{
defaultMessage: 'Required for ingesting events.',
}
)}
>
<EuiCheckbox
id={htmlIdGenerator()()}
label="event:write"
checked={eventWriteChecked}
onChange={() => setEventWriteChecked((state) => !state)}
/>
</EuiFormRow>
<EuiSpacer size="s" />
<EuiFormRow
helpText={i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.sourcemaps',
{
defaultMessage: 'Required for uploading sourcemaps.',
}
)}
>
<EuiCheckbox
id={htmlIdGenerator()()}
label="sourcemap:write"
checked={sourcemapChecked}
onChange={() => setSourcemapChecked((state) => !state)}
/>
</EuiFormRow>
<EuiSpacer size="s" />
</EuiFormFieldset>
</EuiForm>
</EuiFlyoutBody>

<EuiFlyoutFooter>
<EuiFlexGroup justifyContent="spaceBetween">
<EuiFlexItem grow={false}>
<EuiButtonEmpty onClick={onCancel}>
{i18n.translate(
'xpack.apm.settings.agentKeys.createKeyFlyout.cancelButton',
{
defaultMessage: 'Cancel',
}
)}
</EuiButtonEmpty>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton
fill={true}
onClick={createAgentKey}
type="submit"
disabled={isFormInvalid}
>
{createAgentKeyTitle}
gbamparop marked this conversation as resolved.
Show resolved Hide resolved
</EuiButton>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlyoutFooter>
</EuiFlyout>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { i18n } from '@kbn/i18n';
import {
EuiSpacer,
EuiCallOut,
EuiButtonIcon,
EuiCopy,
EuiFormControlLayout,
} from '@elastic/eui';

interface Props {
name: string;
token: string;
}

export function AgentKeyCallOut({ name, token }: Props) {
return (
<>
<EuiCallOut
gbamparop marked this conversation as resolved.
Show resolved Hide resolved
title={i18n.translate(
'xpack.apm.settings.agentKeys.copyAgentKeyField.title',
{
defaultMessage: 'Created "{name}" key',
values: { name },
}
)}
color="success"
iconType="check"
>
<p>
{i18n.translate(
'xpack.apm.settings.agentKeys.copyAgentKeyField.message',
{
defaultMessage:
'Copy this key now. You will not be able to view it again.',
}
)}
</p>
<EuiFormControlLayout
style={{ backgroundColor: 'transparent' }}
readOnly
prepend="Base64"
append={
<EuiCopy textToCopy={token}>
{(copy) => (
<EuiButtonIcon
iconType="copyClipboard"
onClick={copy}
color="success"
style={{ backgroundColor: 'transparent' }}
aria-label={i18n.translate(
'xpack.apm.settings.agentKeys.copyAgentKeyField.copyButton',
{
defaultMessage: 'Copy to clipboard',
}
)}
/>
)}
</EuiCopy>
}
>
<input
Copy link
Contributor

@cauemarcondes cauemarcondes Dec 6, 2021

Choose a reason for hiding this comment

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

Any reason why you didn't use <EuiFieldText/> here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the UI pattern from api keys management with a custom EuiFormControlLayout so we can use the prepend and append elements. I could change it if you think there's a better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is addressed in #120765

type="text"
className="euiFieldText euiFieldText--inGroup"
readOnly
value={token}
aria-label={i18n.translate(
'xpack.apm.settings.agentKeys.copyAgentKeyField.agentKeyLabel',
{
defaultMessage: 'Agent key',
}
)}
/>
</EuiFormControlLayout>
</EuiCallOut>
<EuiSpacer size="m" />
</>
);
}
Loading