PMM-11714 Don't return an error if API key can't be created. (#2469)

* PMM-11714 Don't return error if API key can't be created. * PMM-11714 Revert removed files. * PMM-11714 Fix tests. * PMM-11714 Fix tests. * PMM-11714 Fix format. * PMM-11714 Fix tests. * Update managed/services/grafana/client.go Co-authored-by: Alex Tymchuk <[email protected]> * Update docker-compose.yml * PMM-11714 add license header * PMM-11714 add extra info to logs. --------- Co-authored-by: Alex Tymchuk <[email protected]>
percona · Nov 13, 2023 · 9fa78dd · 9fa78dd
1 parent 80e4c8b
commit 9fa78dd
Show file tree

Hide file tree

Showing 15 changed files with 199 additions and 38 deletions.
diff --git a/.gitignore b/.gitignore
@@ -38,3 +38,6 @@ compose.yml
 
 cli-tests/node_modules/
 cli-tests/playwright-report/
+
+api-tests/pmm-api-tests-output.txt
+api-tests/pmm-api-tests-junit-report.xml
diff --git a/admin/commands/management/register.go b/admin/commands/management/register.go
@@ -28,12 +28,16 @@ var registerResultT = commands.ParseTemplate(`
 pmm-agent registered.
 pmm-agent ID: {{ .PMMAgent.AgentID }}
 Node ID     : {{ .PMMAgent.RunsOnNodeID }}
+{{ if .Warning }}
+Warning: {{ .Warning }}
+{{- end -}}
 `)
 
 type registerResult struct {
 	GenericNode   *node.RegisterNodeOKBodyGenericNode   `json:"generic_node"`
 	ContainerNode *node.RegisterNodeOKBodyContainerNode `json:"container_node"`
 	PMMAgent      *node.RegisterNodeOKBodyPMMAgent      `json:"pmm_agent"`
+	Warning       string                                `json:"warning"`
 }
 
 func (res *registerResult) Result() {}
@@ -96,5 +100,6 @@ func (cmd *RegisterCommand) RunCmd() (commands.Result, error) {
 		GenericNode:   resp.Payload.GenericNode,
 		ContainerNode: resp.Payload.ContainerNode,
 		PMMAgent:      resp.Payload.PMMAgent,
+		Warning:       resp.Payload.Warning,
 	}, nil
 }
diff --git a/admin/commands/management/register_test.go b/admin/commands/management/register_test.go
@@ -0,0 +1,67 @@
+// Copyright (C) 2023 Percona LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//  http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package management
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/percona/pmm/api/managementpb/json/client/node"
+)
+
+func TestRegisterResult(t *testing.T) {
+	tests := []struct {
+		name   string
+		result registerResult
+		want   string
+	}{
+		{
+			name: "Success",
+			result: registerResult{
+				PMMAgent: &node.RegisterNodeOKBodyPMMAgent{
+					AgentID:      "/agent_id/new_id",
+					RunsOnNodeID: "/node_id/second_id",
+				},
+				Warning: "",
+			},
+			want: `pmm-agent registered.
+pmm-agent ID: /agent_id/new_id
+Node ID     : /node_id/second_id
+`,
+		},
+		{
+			name: "With warning",
+			result: registerResult{
+				PMMAgent: &node.RegisterNodeOKBodyPMMAgent{
+					AgentID:      "/agent_id/warning",
+					RunsOnNodeID: "/node_id/warning_node",
+				},
+				Warning: "Couldn't create Admin API Key",
+			},
+			want: `pmm-agent registered.
+pmm-agent ID: /agent_id/warning
+Node ID     : /node_id/warning_node
+
+Warning: Couldn't create Admin API Key
+`,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, tt.result.String(), "String()")
+		})
+	}
+}
diff --git a/api/managementpb/json/client/node/register_node_responses.go b/api/managementpb/json/client/node/register_node_responses.go
diff --git a/api/managementpb/json/managementpb.json b/api/managementpb/json/managementpb.json
@@ -3899,6 +3899,11 @@
                   "description": "Token represents token for vmagent auth config.",
                   "type": "string",
                   "x-order": 3
+                },
+                "warning": {
+                  "description": "Warning message.",
+                  "type": "string",
+                  "x-order": 4
                 }
               }
             }

diff --git a/api/managementpb/node.pb.go b/api/managementpb/node.pb.go
diff --git a/api/managementpb/node.pb.validate.go b/api/managementpb/node.pb.validate.go
diff --git a/api/managementpb/node.proto b/api/managementpb/node.proto
@@ -52,6 +52,8 @@ message RegisterNodeResponse {
   inventory.PMMAgent pmm_agent = 3;
   // Token represents token for vmagent auth config.
   string token = 4;
+  // Warning message.
+  string warning = 5;
 }
 
 // Node service provides public Management API methods for Nodes.

diff --git a/api/swagger/swagger-dev.json b/api/swagger/swagger-dev.json
@@ -27492,6 +27492,11 @@
                   "description": "Token represents token for vmagent auth config.",
                   "type": "string",
                   "x-order": 3
+                },
+                "warning": {
+                  "description": "Warning message.",
+                  "type": "string",
+                  "x-order": 4
                 }
               }
             }

diff --git a/api/swagger/swagger.json b/api/swagger/swagger.json
@@ -18661,6 +18661,11 @@
                   "description": "Token represents token for vmagent auth config.",
                   "type": "string",
                   "x-order": 3
+                },
+                "warning": {
+                  "description": "Warning message.",
+                  "type": "string",
+                  "x-order": 4
                 }
               }
             }

diff --git a/managed/services/grafana/auth_server.go b/managed/services/grafana/auth_server.go
@@ -540,15 +540,7 @@ func cleanPath(p string) (string, error) {
 
 func (s *AuthServer) getAuthUser(ctx context.Context, req *http.Request, l *logrus.Entry) (*authUser, *authError) {
 	// check Grafana with some headers from request
-	authHeaders := make(http.Header)
-	for _, k := range []string{
-		"Authorization",
-		"Cookie",
-	} {
-		if v := req.Header.Get(k); v != "" {
-			authHeaders.Set(k, v)
-		}
-	}
+	authHeaders := s.authHeaders(req)
 	j, err := json.Marshal(authHeaders)
 	if err != nil {
 		l.Warnf("%s", err)
@@ -565,6 +557,19 @@ func (s *AuthServer) getAuthUser(ctx context.Context, req *http.Request, l *logr
 	return s.retrieveRole(ctx, hash, authHeaders, l)
 }
 
+func (s *AuthServer) authHeaders(req *http.Request) http.Header {
+	authHeaders := make(http.Header)
+	for _, k := range []string{
+		"Authorization",
+		"Cookie",
+	} {
+		if v := req.Header.Get(k); v != "" {
+			authHeaders.Set(k, v)
+		}
+	}
+	return authHeaders
+}
+
 func (s *AuthServer) retrieveRole(ctx context.Context, hash string, authHeaders http.Header, l *logrus.Entry) (*authUser, *authError) {
 	authUser, err := s.c.getAuthUser(ctx, authHeaders)
 	if err != nil {

diff --git a/managed/services/grafana/client.go b/managed/services/grafana/client.go
@@ -219,7 +219,7 @@ func (c *Client) GetUserID(ctx context.Context) (int, error) {
 // Ctx is used only for cancelation.
 func (c *Client) getAuthUser(ctx context.Context, authHeaders http.Header) (authUser, error) {
 	// Check if it's API Key
-	if c.isAPIKeyAuth(authHeaders.Get("Authorization")) {
+	if c.IsAPIKeyAuth(authHeaders) {
 		role, err := c.getRoleForAPIKey(ctx, authHeaders)
 		return authUser{
 			role:   role,
@@ -277,7 +277,9 @@ func (c *Client) getAuthUser(ctx context.Context, authHeaders http.Header) (auth
 	}, nil
 }
 
-func (c *Client) isAPIKeyAuth(authHeader string) bool {
+// IsAPIKeyAuth checks if the request is made using an API Key.
+func (c *Client) IsAPIKeyAuth(headers http.Header) bool {
+	authHeader := headers.Get("Authorization")
 	switch {
 	case strings.HasPrefix(authHeader, "Bearer"):
 		return true

diff --git a/managed/services/management/mock_api_key_provider_test.go b/managed/services/management/mock_api_key_provider_test.go