address comments

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

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
 }

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 {

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 {

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()
 	}

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