Implement support for API Key metadata by aleksmaus · Pull Request #195 · elastic/fleet-server

aleksmaus · 2021-03-31T18:10:09Z

What does this PR do?

Adds metadata to they api keys.

      "metadata" : {
        "application" : "fleet-agent",
        "agent_id" : "887f1001-5dc8-45e2-aa61-413123c8013b",
        "type" : "access"
      }

Not sure about the type field, wanted to capture the possible values output and access.
Maybe a better name? or can remove if not needed. @scunningham @ruflin

P.S. If you are having issues running integration tests locally, try delete the elasticsearch docker image and pull the latest one that implements the new api.

Why is it important?

Adopting new ES Api elastic/elasticsearch#48182 for 7.13.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas

Related issues

Closes Implement support for API Key metadata #193

Screenshots

elasticmachine · 2021-03-31T18:14:54Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #195 updated
Start Time: 2021-04-01T13:31:08.909+0000
Duration: 4 min 11 sec
Commit: 64e94bd

Test stats 🧪

Test	Results
Failed	0
Passed	66
Skipped	0
Total	66

Trends 🧪

scunningham · 2021-03-31T20:49:01Z

internal/pkg/apikey/metadata.go

+	TypeOutput
+)
+
+func (t Type) String() string {


This is some magic. Switch too boring?

that the Go way

ruflin

Left some comments on naming and content, but overall LGTM. As soon as we get this in, we should open the backport but AFAIK the feature is not backported yet in ES to 7.13 so testing might / should fail at first.

ruflin · 2021-04-01T08:20:05Z

cmd/fleet/handleEnroll.go

+		apikey.Metadata{
+			Application: apikey.FleetAgentApplication,
+			AgentId:     agentId,
+			Type:        apikey.TypeAccess.String(),


I like the idea to have type as part of the metadata to differentiate between the output and "access" key. I stumbled over the name "access", may communication or similar? Anyone other ideas?

ruflin · 2021-04-01T08:20:42Z

cmd/fleet/handleEnroll.go

+	return apikey.Create(ctx, client, agentId, "", []byte(kFleetAccessRolesJSON),
+		apikey.Metadata{
+			Application: apikey.FleetAgentApplication,
+			AgentId:     agentId,


Torn if we should call it agent_id or agent.id. to be consistent with ECS. agent.id makes the code more complicated I guess?

@tsg FYI, we already add the agent id. Any opinion on format?

Ah, that's awesome, thanks for the ping. Also FYI @andrewkroh.

I'd say that we don't really need the . here, since we expect this to be internal. Unless it can be added without headaches.

just had similar debate on the other PR elastic/kibana#95935
user.id vs user_id .... was asked to change to user_id.
can do either way, just let me know

😆 Can't see the discussion on the commit change. Was the reason there? In the end agree with @tsg it probably does not matter here, especially if in kibana we also use user_id. I always have an ECS preference but I probably spent too much time with ECS :-)

it was a discussion on Slack with one of the reviewers, thus few commits there iterating on the final shape

ruflin · 2021-04-01T08:21:41Z

cmd/fleet/handleEnroll.go

 func generateAccessApiKey(ctx context.Context, client *elasticsearch.Client, agentId string) (*apikey.ApiKey, error) {
-	return apikey.Create(ctx, client, agentId, "", []byte(kFleetAccessRolesJSON))
+	return apikey.Create(ctx, client, agentId, "", []byte(kFleetAccessRolesJSON),
+		apikey.Metadata{


One metadata we have for other assets is "managed": true to indicate that this is managed by tooling internally. I think we should add it.

ruflin · 2021-04-01T08:22:52Z

cmd/fleet/handleEnroll.go

-	return apikey.Create(ctx, client, agentId, "", []byte(kFleetAccessRolesJSON))
+	return apikey.Create(ctx, client, agentId, "", []byte(kFleetAccessRolesJSON),
+		apikey.Metadata{
+			Application: apikey.FleetAgentApplication,


Naming: Should we call this application? Service? ...

naming is hard. the original ES ticket used "application" elastic/elasticsearch#48182
easy to change, I have no preference. let me know.

@tvernum I think you were the one using application there. Any preference?

aleksmaus · 2021-04-01T12:54:46Z

Left some comments on naming and content, but overall LGTM. As soon as we get this in, we should open the backport but AFAIK the feature is not backported yet in ES to 7.13 so testing might / should fail at first.

Yep. I added the integration test that should fail on 7.x until this API is backported.

ruflin · 2021-04-01T13:04:19Z

@aleksmaus I would get this PR in independent of the naming discussions. It will be easy to update it later on.

aleksmaus · 2021-04-01T13:30:43Z

Updated the metadata format based on more recent conversations

    {
      "id" : "jWeejXgBOfmSk3GHgDWJ",
      "name" : "e4dede19-759e-45d5-b08f-0e78dec888e5",
      "creation" : 1617283678345,
      "invalidated" : false,
      "username" : "elastic",
      "realm" : "reserved",
      "metadata" : {
        "agent_id" : "e4dede19-759e-45d5-b08f-0e78dec888e5",
        "managed_by" : "fleet-server",
        "managed" : true,
        "type" : "access"
      }
    },
    {
      "id" : "j2eejXgBOfmSk3GHlDXj",
      "name" : "e4dede19-759e-45d5-b08f-0e78dec888e5:default",
      "creation" : 1617283683555,
      "invalidated" : false,
      "username" : "elastic",
      "realm" : "reserved",
      "metadata" : {
        "agent_id" : "e4dede19-759e-45d5-b08f-0e78dec888e5",
        "managed_by" : "fleet-server",
        "managed" : true,
        "type" : "output"
      }
    }

aleksmaus · 2021-04-01T13:32:08Z

@aleksmaus I would get this PR in independent of the naming discussions. It will be easy to update it later on.

just need one 👍

* Implement support for API Key metadata * Adjust apikey.Create to make the metadata functional options * Address code review feedback * Make metadata properties json omitempty * Additional changes to the metatadata format (cherry picked from commit 82ea1e7)

* Implement support for API Key metadata * Adjust apikey.Create to make the metadata functional options * Address code review feedback * Make metadata properties json omitempty * Additional changes to the metatadata format (cherry picked from commit 82ea1e7) Co-authored-by: Aleksandr Maus <aleksandr.maus@elastic.co>

Implement support for API Key metadata

b8a3a3c

aleksmaus added the enhancement New feature or request label Mar 31, 2021

aleksmaus requested review from blakerouse, ruflin and scunningham March 31, 2021 18:10

aleksmaus added 2 commits March 31, 2021 16:15

Adjust apikey.Create to make the metadata functional options

53e5683

Address code review feedback

e8c51d1

scunningham reviewed Mar 31, 2021

View reviewed changes

Make metadata properties json omitempty

c60f378

ruflin reviewed Apr 1, 2021

View reviewed changes

Additional changes to the metatadata format

64e94bd

blakerouse approved these changes Apr 1, 2021

View reviewed changes

aleksmaus merged commit 82ea1e7 into elastic:master Apr 1, 2021

ruflin mentioned this pull request Apr 8, 2021

[Fleet] Create enrollment API keys as current user elastic/kibana#96464

Merged

aleksmaus added the v7.13.0 label Apr 13, 2021

mergify bot mentioned this pull request Apr 13, 2021

Implement support for API Key metadata (backport #195) #232

Merged

Conversation

aleksmaus commented Mar 31, 2021

What does this PR do?

Why is it important?

Checklist

Related issues

Screenshots

Uh oh!

elasticmachine commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksmaus commented Apr 1, 2021

Uh oh!

ruflin commented Apr 1, 2021

Uh oh!

aleksmaus commented Apr 1, 2021

Uh oh!

aleksmaus commented Apr 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

elasticmachine commented Mar 31, 2021 •

edited

Loading