From 9cc648941cb7954196886b5b7e42df9c568341dc Mon Sep 17 00:00:00 2001 From: Yi Tao Date: Wed, 8 Feb 2023 14:30:25 +0800 Subject: [PATCH] address comments --- internal/adminapi/kong.go | 6 +++-- internal/konnect/client.go | 49 +++++++++++++--------------------- internal/konnect/node_agent.go | 28 +++++++++---------- internal/konnect/protocol.go | 5 ++-- internal/manager/run.go | 5 +--- internal/util/tls/tls_cert.go | 8 +++--- 6 files changed, 46 insertions(+), 55 deletions(-) diff --git a/internal/adminapi/kong.go b/internal/adminapi/kong.go index dea6eb7c56..40665cbb3c 100644 --- a/internal/adminapi/kong.go +++ b/internal/adminapi/kong.go @@ -105,12 +105,14 @@ func MakeHTTPClient(opts *HTTPClientOpts) (*http.Client, error) { tlsConfig.RootCAs = certPool } - clientCertificates, err := tlsutil.ExtractClientCertificates( + clientCertificate, err := tlsutil.ExtractClientCertificates( []byte(opts.TLSClient.Cert), opts.TLSClient.CertFile, []byte(opts.TLSClient.Key), opts.TLSClient.KeyFile) if err != nil { return nil, fmt.Errorf("failed to extract client certificates: %w", err) } - tlsConfig.Certificates = append(tlsConfig.Certificates, clientCertificates...) + if clientCertificate != nil { + tlsConfig.Certificates = append(tlsConfig.Certificates, *clientCertificate) + } transport := http.DefaultTransport.(*http.Transport).Clone() transport.TLSClientConfig = &tlsConfig diff --git a/internal/konnect/client.go b/internal/konnect/client.go index 390091fee2..7909fc2bfe 100644 --- a/internal/konnect/client.go +++ b/internal/konnect/client.go @@ -12,66 +12,55 @@ import ( tlsutil "github.com/kong/kubernetes-ingress-controller/v2/internal/util/tls" ) -// AdminClient is used for sending requests to Konnect APIs which are not included +// Client is used for sending requests to Konnect APIs which are not included // in Kong Admin APIs, like node registration APIs or runtime group operation APIs. // TODO(naming): give a better type name to this client? -type AdminClient struct { +type Client struct { Address string RuntimeGroupID string Client *http.Client } var ( - // KicAPIPathPattern is the pattern of paths to API for - // operating runtime group with ID in AdminClient. - KicAPIPathPattern = "%s/kic/api/runtime_groups/%s" // KicNodeAPIPathPattern is the path pattern for KIC node operations. KicNodeAPIPathPattern = "%s/kic/api/runtime_groups/%s/v1/kic-nodes" ) -// NewAdminClient creates a Konnect client. -func NewAdminClient(cfg adminapi.KonnectConfig) (*AdminClient, error) { - tlsClientCert, err := tlsutil.ValueFromVariableOrFile([]byte(cfg.TLSClient.Cert), cfg.TLSClient.CertFile) - if err != nil { - return nil, fmt.Errorf("could not extract TLS client cert: %w", err) - } - tlsClientKey, err := tlsutil.ValueFromVariableOrFile([]byte(cfg.TLSClient.Key), cfg.TLSClient.KeyFile) - if err != nil { - return nil, fmt.Errorf("could not extract TLS client key: %w", err) - } +// NewClient creates a Konnect client. +func NewClient(cfg adminapi.KonnectConfig) (*Client, error) { tlsConfig := tls.Config{ //nolint:gosec Certificates: []tls.Certificate{}, } - if len(tlsClientCert) > 0 && len(tlsClientKey) > 0 { - // Read the key pair to create certificate - cert, err := tls.X509KeyPair(tlsClientCert, tlsClientKey) - if err != nil { - return nil, fmt.Errorf("failed to load client certificate: %w", err) - } - tlsConfig.Certificates = []tls.Certificate{cert} + cert, err := tlsutil.ExtractClientCertificates([]byte(cfg.TLSClient.Cert), cfg.TLSClient.CertFile, []byte(cfg.TLSClient.Key), cfg.TLSClient.KeyFile) + if err != nil { + return nil, fmt.Errorf("failed to extract client certificates: %w", err) + } + if cert != nil { + tlsConfig.Certificates = append(tlsConfig.Certificates, *cert) } + c := &http.Client{} defaultTransport := http.DefaultTransport.(*http.Transport) defaultTransport.TLSClientConfig = &tlsConfig c.Transport = defaultTransport - return &AdminClient{ + return &Client{ Address: cfg.Address, RuntimeGroupID: cfg.RuntimeGroupID, Client: c, }, nil } -func (c *AdminClient) kicNodeAPIEndpoint() string { +func (c *Client) kicNodeAPIEndpoint() string { return fmt.Sprintf(KicNodeAPIPathPattern, c.Address, c.RuntimeGroupID) } -func (c *AdminClient) kicNodeAPIEndpointWithNodeID(nodeID string) string { +func (c *Client) kicNodeAPIEndpointWithNodeID(nodeID string) string { return fmt.Sprintf(KicNodeAPIPathPattern, c.Address, c.RuntimeGroupID) + "/" + nodeID } -func (c *AdminClient) CreateNode(req *CreateNodeRequest) (*CreateNodeResponse, error) { +func (c *Client) CreateNode(req *CreateNodeRequest) (*CreateNodeResponse, error) { buf, err := json.Marshal(req) if err != nil { return nil, fmt.Errorf("failed to marshal create node request: %w", err) @@ -107,7 +96,7 @@ func (c *AdminClient) CreateNode(req *CreateNodeRequest) (*CreateNodeResponse, e return resp, nil } -func (c *AdminClient) UpdateNode(nodeID string, req *UpdateNodeRequest) (*UpdateNodeResponse, error) { +func (c *Client) UpdateNode(nodeID string, req *UpdateNodeRequest) (*UpdateNodeResponse, error) { buf, err := json.Marshal(req) if err != nil { return nil, fmt.Errorf("failed to marshal update node request: %w", err) @@ -142,7 +131,7 @@ func (c *AdminClient) UpdateNode(nodeID string, req *UpdateNodeRequest) (*Update return resp, nil } -func (c *AdminClient) ListNodes() (*ListNodeResponse, error) { +func (c *Client) ListNodes() (*ListNodeResponse, error) { url := c.kicNodeAPIEndpoint() httpResp, err := c.Client.Get(url) if err != nil { @@ -168,7 +157,7 @@ func (c *AdminClient) ListNodes() (*ListNodeResponse, error) { return resp, nil } -func (c *AdminClient) DeleteNode(nodeID string) error { +func (c *Client) DeleteNode(nodeID string) error { url := c.kicNodeAPIEndpointWithNodeID(nodeID) httpReq, err := http.NewRequest("DELETE", url, nil) if err != nil { @@ -187,7 +176,7 @@ func (c *AdminClient) DeleteNode(nodeID string) error { return nil } -// returns true if the input HTTP status code is 2xx, in [200,300). +// isOKStatusCode returns true if the input HTTP status code is 2xx, in [200,300). func isOKStatusCode(code int) bool { return code >= 200 && code < 300 } diff --git a/internal/konnect/node_agent.go b/internal/konnect/node_agent.go index 8c0118d1de..b9da517c2e 100644 --- a/internal/konnect/node_agent.go +++ b/internal/konnect/node_agent.go @@ -9,7 +9,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v2/internal/util" ) -var defaultRefreshNodeInterval = 15 * time.Second +const defaultRefreshNodeInterval = 30 * time.Second type NodeAgent struct { NodeID string @@ -18,17 +18,17 @@ type NodeAgent struct { Logger logr.Logger - adminClient *AdminClient + konnectClient *Client refreshInterval time.Duration } -func NewNodeAgent(hostname string, version string, logger logr.Logger, adminClient *AdminClient) *NodeAgent { +func NewNodeAgent(hostname string, version string, logger logr.Logger, client *Client) *NodeAgent { return &NodeAgent{ Hostname: hostname, Version: version, Logger: logger. - WithName("konnect-node").WithValues("runtime_group_id", adminClient.RuntimeGroupID), - adminClient: adminClient, + WithName("konnect-node").WithValues("runtime_group_id", client.RuntimeGroupID), + konnectClient: client, // TODO: set refresh interval by flags/envvar refreshInterval: defaultRefreshNodeInterval, } @@ -42,27 +42,26 @@ func (a *NodeAgent) createNode() error { Type: NodeTypeIngressController, LastPing: time.Now().Unix(), } - resp, err := a.adminClient.CreateNode(createNodeReq) + resp, err := a.konnectClient.CreateNode(createNodeReq) if err != nil { - a.Logger.Error(err, "failed to create node") - return err + return fmt.Errorf("failed to update node, hostname %s: %w", a.Hostname, err) } a.NodeID = resp.Item.ID - a.Logger.Info("updated last ping time of node for KIC", "node_id", a.NodeID) + a.Logger.V(util.DebugLevel).Info("created node for KIC", "node_id", a.NodeID, "hostname", a.Hostname) return nil } func (a *NodeAgent) clearOutdatedNodes() error { - nodes, err := a.adminClient.ListNodes() + nodes, err := a.konnectClient.ListNodes() if err != nil { return fmt.Errorf("failed to list nodes: %w", err) } for _, node := range nodes.Items { if node.Type == NodeTypeIngressController && node.Hostname != a.Hostname { - a.Logger.V(util.DebugLevel).Info("remove KIC node", "node_id", node.ID, "hostname", node.Hostname) - err := a.adminClient.DeleteNode(node.ID) + a.Logger.V(util.DebugLevel).Info("remove outdated KIC node", "node_id", node.ID, "hostname", node.Hostname) + err := a.konnectClient.DeleteNode(node.ID) if err != nil { return fmt.Errorf("failed to delete node %s: %w", node.ID, err) } @@ -89,17 +88,18 @@ func (a *NodeAgent) updateNode() error { Status: string(ingressControllerStatus), } - _, err = a.adminClient.UpdateNode(a.NodeID, updateNodeReq) + _, err = a.konnectClient.UpdateNode(a.NodeID, updateNodeReq) if err != nil { a.Logger.Error(err, "failed to update node for KIC") return err } - a.Logger.V(util.DebugLevel).Info("updated last ping time of node for KIC", "node_id", a.NodeID) + a.Logger.V(util.DebugLevel).Info("updated last ping time of node for KIC", "node_id", a.NodeID, "hostname", a.Hostname) return nil } func (a *NodeAgent) updateNodeLoop() { ticker := time.NewTicker(a.refreshInterval) + defer ticker.Stop() for range ticker.C { err := a.updateNode() if err != nil { diff --git a/internal/konnect/protocol.go b/internal/konnect/protocol.go index 00b259c87c..42d833a8e1 100644 --- a/internal/konnect/protocol.go +++ b/internal/konnect/protocol.go @@ -1,9 +1,10 @@ package konnect const ( + // NodeTypeIngressController is the type of nodes representing KIC instances. NodeTypeIngressController = "ingress-controller" - NodeTypeKongProxy = "kong-proxy" - NodeTypeIngressProxy = "ingress-proxy" + // NodeTypeKongProxy is the type of nodes representing (KIC controlled) kong gateway instances + NodeTypeKongProxy = "kong-proxy" ) type NodeItem struct { diff --git a/internal/manager/run.go b/internal/manager/run.go index 0a4da35b38..a6e776a90b 100644 --- a/internal/manager/run.go +++ b/internal/manager/run.go @@ -175,15 +175,12 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d if c.Konnect.ConfigSynchronizationEnabled { setupLog.Info("Start Konnect client to register runtime instances to Konnect") - konnectClient, err := konnect.NewAdminClient(c.Konnect) + konnectClient, err := konnect.NewClient(c.Konnect) if err != nil { - setupLog.Error(err, "failed to create konnect client") return fmt.Errorf("failed to create konnect client: %w", err) } hostname, _ := os.Hostname() version := metadata.Release - // REVIEW: here we used version of KIC itself as version of KIC node.' - // Which version is the proper one to use? agent := konnect.NewNodeAgent(hostname, version, setupLog, konnectClient) agent.Run() } diff --git a/internal/util/tls/tls_cert.go b/internal/util/tls/tls_cert.go index bfa22954f1..40efc670df 100644 --- a/internal/util/tls/tls_cert.go +++ b/internal/util/tls/tls_cert.go @@ -7,8 +7,10 @@ import ( ) // ExtractClientCertificates extracts tls.Certificates from TLSClientConfig. -// It returns an empty slice in case there was no client cert and/or client key provided. -func ExtractClientCertificates(cert []byte, certFile string, key []byte, keyFile string) ([]tls.Certificate, error) { +// It returns nil in case there was no client cert and/or client key provided. + +// REVIEW: in case of no certs specified, return nil, nil, OR return non-nil error, OR add a boolean return value? +func ExtractClientCertificates(cert []byte, certFile string, key []byte, keyFile string) (*tls.Certificate, error) { clientCert, err := ValueFromVariableOrFile(cert, certFile) if err != nil { return nil, fmt.Errorf("could not extract TLS client cert") @@ -23,7 +25,7 @@ func ExtractClientCertificates(cert []byte, certFile string, key []byte, keyFile if err != nil { return nil, fmt.Errorf("failed to load client certificate: %w", err) } - return []tls.Certificate{cert}, nil + return &cert, nil } return nil, nil