feat: deprecate --gateway-discovery-dns-strategy - make discovery a…

…lways work (#7033)

CHANGELOG.md

@@ -114,6 +114,9 @@ Adding a new version? You'll need three changes:
   For more information on this migration please consult
   [kubernetes-sigs/kubebuilder#3907][kubebuilder_3907].
   [#6861](https://github.com/Kong/kubernetes-ingress-controller/pull/6861)
+- Deprecate flag `--gateway-discovery-dns-strategy` and remove it from the help message,
+  this setting is not needed anymore, discovery always works without any adjustments.
+  [#7033](https://github.com/Kong/kubernetes-ingress-controller/pull/7033)
 
 [kubebuilder_3907]: https://github.com/kubernetes-sigs/kubebuilder/discussions/3907
 

docs/cli-arguments.md

@@ -43,7 +43,6 @@
 | `--enable-reverse-sync` | `bool` | Send configuration to Kong even if the configuration checksum has not changed since previous update. | `false` |
 | `--feature-gates` | `list of string=bool` | A set of comma separated key=value pairs that describe feature gates for alpha/beta/experimental features. See the Feature Gates documentation for information and available options: https://github.com/Kong/kubernetes-ingress-controller/blob/main/FEATURE_GATES.md. |  |
 | `--gateway-api-controller-name` | `string` | The controller name to match on Gateway API resources. | `konghq.com/kic-gateway-controller` |
-| `--gateway-discovery-dns-strategy` | `dns-strategy` | DNS strategy to use when creating Gateway's Admin API addresses. One of: ip, service, pod. | `"ip"` |
 | `--gateway-discovery-readiness-check-interval` | `duration` | Interval of readiness checks on gateway admin API clients for discovery. | `10s` |
 | `--gateway-discovery-readiness-check-timeout` | `duration` | Timeout of readiness checks on gateway admin clients. | `5s` |
 | `--gateway-to-reconcile` | `namespaced-name` | Gateway namespaced name in "namespace/name" format. Makes KIC reconcile only the specified Gateway. |  |

internal/adminapi/client.go

@@ -7,10 +7,12 @@ import (
 	"net/http"
 	"sync"
 
+	"github.com/go-logr/logr"
 	"github.com/kong/go-kong/kong"
 	"github.com/samber/lo"
 	k8stypes "k8s.io/apimachinery/pkg/types"
 
+	"github.com/kong/kubernetes-ingress-controller/v3/internal/logging"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/store"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
 	"github.com/kong/kubernetes-ingress-controller/v3/internal/util/clock"
@@ -204,25 +206,30 @@ func (c *Client) PodReference() (k8stypes.NamespacedName, bool) {
 }
 
 type ClientFactory struct {
-	workspace      string
-	httpClientOpts HTTPClientOpts
-	adminToken     string
+	logger     logr.Logger
+	workspace  string
+	opts       ClientOpts
+	adminToken string
 }
 
-func NewClientFactoryForWorkspace(workspace string, httpClientOpts HTTPClientOpts, adminToken string) ClientFactory {
+func NewClientFactoryForWorkspace(logger logr.Logger, workspace string, clientOpts ClientOpts, adminToken string) ClientFactory {
 	return ClientFactory{
-		workspace:      workspace,
-		httpClientOpts: httpClientOpts,
-		adminToken:     adminToken,
+		logger:     logger,
+		workspace:  workspace,
+		opts:       clientOpts,
+		adminToken: adminToken,
 	}
 }
 
 func (cf ClientFactory) CreateAdminAPIClient(ctx context.Context, discoveredAdminAPI DiscoveredAdminAPI) (*Client, error) {
-	httpclient, err := MakeHTTPClient(&cf.httpClientOpts, cf.adminToken)
-	if err != nil {
-		return nil, err
-	}
-	cl, err := NewKongClientForWorkspace(ctx, discoveredAdminAPI.Address, cf.workspace, httpclient)
+	cf.logger.V(logging.DebugLevel).Info(
+		"Creating Kong Gateway Admin API client",
+		"address", discoveredAdminAPI.Address, "tlsServerName", discoveredAdminAPI.TLSServerName,
+	)
+	opts := cf.opts
+	opts.TLSServerName = discoveredAdminAPI.TLSServerName
+
+	cl, err := NewKongClientForWorkspace(ctx, discoveredAdminAPI.Address, cf.workspace, opts, cf.adminToken)
 	if err != nil {
 		return nil, err
 	}

internal/adminapi/client_test.go

@@ -5,6 +5,7 @@ import (
 	"net/http/httptest"
 	"testing"
 
+	"github.com/go-logr/logr"
 	"github.com/stretchr/testify/require"
 	k8stypes "k8s.io/apimachinery/pkg/types"
 
@@ -13,7 +14,7 @@ import (
 )
 
 func TestClientFactory_CreateAdminAPIClientAttachesPodReference(t *testing.T) {
-	factory := adminapi.NewClientFactoryForWorkspace("workspace", adminapi.HTTPClientOpts{}, "")
+	factory := adminapi.NewClientFactoryForWorkspace(logr.Discard(), "workspace", adminapi.ClientOpts{}, "")
 
 	adminAPIHandler := mocks.NewAdminAPIHandler(t)
 	adminAPIServer := httptest.NewServer(adminAPIHandler)

internal/adminapi/endpoints.go

@@ -3,48 +3,42 @@ package adminapi
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	discoveryv1 "k8s.io/api/discovery/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
 	k8stypes "k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-
-	cfgtypes "github.com/kong/kubernetes-ingress-controller/v3/internal/manager/config/types"
 )
 
 // DiscoveredAdminAPI represents an Admin API discovered from a Kubernetes Service.
+// For field Address use format https://<podIP>:<port>, and for field TLSServerName
+// use format pod.<serviceName>.<namespace>.svc.
 type DiscoveredAdminAPI struct {
+	// Address format is https://10.68.0.5:8444.
 	Address string
-	PodRef  k8stypes.NamespacedName
+	// TLSServerName format is pod.dataplane-admin-kong-rqwr9-sc49t.default.svc.
+	TLSServerName string
+	// PodRef is the reference to the Pod with the above IP address.
+	PodRef k8stypes.NamespacedName
 }
 
 type Discoverer struct {
 	// portNames is the set of port names that Admin API Service ports will be
 	// matched against.
 	portNames sets.Set[string]
-
-	// dnsStrategy is the DNS strategy to use when resolving Admin API Service
-	// addresses.
-	dnsStrategy cfgtypes.DNSStrategy
 }
 
 func NewDiscoverer(
 	adminAPIPortNames sets.Set[string],
-	dnsStrategy cfgtypes.DNSStrategy,
 ) (*Discoverer, error) {
 	if adminAPIPortNames.Len() == 0 {
 		return nil, fmt.Errorf("no admin API port names provided")
 	}
-	if err := dnsStrategy.Validate(); err != nil {
-		return nil, fmt.Errorf("invalid dns strategy: %w", err)
-	}
 
 	return &Discoverer{
-		portNames:   adminAPIPortNames,
-		dnsStrategy: dnsStrategy,
+		portNames: adminAPIPortNames,
 	}, nil
 }
 
@@ -140,7 +134,7 @@ func (d *Discoverer) AdminAPIsFromEndpointSlice(
 				Namespace: endpoints.Namespace,
 			}
 
-			adminAPI, err := adminAPIFromEndpoint(e, p, svc, d.dnsStrategy, endpoints.AddressType)
+			adminAPI, err := adminAPIFromEndpoint(e, p, svc, endpoints.AddressType)
 			if err != nil {
 				return nil, err
 			}
@@ -154,7 +148,6 @@ func adminAPIFromEndpoint(
 	endpoint discoveryv1.Endpoint,
 	port discoveryv1.EndpointPort,
 	service k8stypes.NamespacedName,
-	dnsStrategy cfgtypes.DNSStrategy,
 	addressFamily discoveryv1.AddressType,
 ) (DiscoveredAdminAPI, error) {
 	podNN := k8stypes.NamespacedName{
@@ -165,49 +158,38 @@ func adminAPIFromEndpoint(
 	// NOTE: Endpoint's addresses are assumed to be fungible, therefore we pick
 	// only the first one.
 	// For the context please see the `Endpoint.Addresses` godoc.
-	eAddress := endpoint.Addresses[0]
+	podIPAddr := endpoint.Addresses[0]
+	if addressFamily == discoveryv1.AddressTypeIPv6 {
+		podIPAddr = fmt.Sprintf("[%s]", podIPAddr)
+	}
+
+	if service.Name == "" {
+		return DiscoveredAdminAPI{}, fmt.Errorf(
+			"service name is empty for an endpoint with TargetRef %s/%s",
+			endpoint.TargetRef.Namespace, endpoint.TargetRef.Name,
+		)
+	}
 
 	// NOTE: We assume https below because the referenced Admin API
 	// server will live in another Pod/elsewhere so allowing http would
 	// not be considered best practice.
-
-	switch dnsStrategy {
-	case cfgtypes.ServiceScopedPodDNSStrategy:
-		if service.Name == "" {
-			return DiscoveredAdminAPI{}, fmt.Errorf(
-				"service name is empty for an endpoint with TargetRef %s/%s",
-				endpoint.TargetRef.Namespace, endpoint.TargetRef.Name,
-			)
-		}
-
-		ipAddr := strings.ReplaceAll(eAddress, ".", "-")
-		address := fmt.Sprintf("%s.%s.%s.svc", ipAddr, service.Name, service.Namespace)
-
-		return DiscoveredAdminAPI{
-			Address: fmt.Sprintf("https://%s:%d", address, *port.Port),
-			PodRef:  podNN,
-		}, nil
-
-	case cfgtypes.NamespaceScopedPodDNSStrategy:
-		ipAddr := strings.ReplaceAll(eAddress, ".", "-")
-		address := fmt.Sprintf("%s.%s.pod", ipAddr, service.Namespace)
-
-		return DiscoveredAdminAPI{
-			Address: fmt.Sprintf("https://%s:%d", address, *port.Port),
-			PodRef:  podNN,
-		}, nil
-
-	case cfgtypes.IPDNSStrategy:
-		bounded := eAddress
-		if addressFamily == discoveryv1.AddressTypeIPv6 {
-			bounded = fmt.Sprintf("[%s]", bounded)
-		}
-		return DiscoveredAdminAPI{
-			Address: fmt.Sprintf("https://%s:%d", bounded, *port.Port),
-			PodRef:  podNN,
-		}, nil
-
-	default:
-		return DiscoveredAdminAPI{}, fmt.Errorf("unknown dns strategy: %s", dnsStrategy)
-	}
+	return DiscoveredAdminAPI{
+		// Address format:
+		//   - ipv4 - https://10.244.0.16:8444
+		//   - ipv6 - https://[fd00:10:244::d]:8444
+		Address: fmt.Sprintf("https://%s:%d", podIPAddr, *port.Port),
+		// TLSServerName format doesn't need to include the IP address part, it's the same for
+		// ipv4 and ipv6: pod.dataplane-admin-kong-rqwr9-sc49t.default.svc.
+		// Currently for KGO certificates are generated like that:
+		// *.<service-name>.<namespace>.svc e.g.: *.dataplane-admin-kong-rqwr9-sc49t.default.svc, see
+		// https://github.com/Kong/gateway-operator/blob/8f009b49b622197ac083abae1c3606bc2c35114f/controller/dataplane/owned_resources.go#L30-L53
+		// In case of Ingress Chart TLS never worked out of the box (by default it's disabled), due to hardcoded service name in the
+		// https://github.com/Kong/charts/blob/1903dd5370e2d028ec31f6b3cec1414a55462ebd/charts/kong/templates/service-kong-admin.yaml#L35-L37
+		// Let's follow the same 4-parts pattern here, to satisfy wildcard. The first part
+		// can be arbitral so let's use "pod". Ditching the first part (wildcard certificate) is
+		// problematic, because this requires changes in the certificate generation logic and may
+		// break existing users' setups, but it can be done one day.
+		TLSServerName: fmt.Sprintf("pod.%s.%s.svc", service.Name, service.Namespace),
+		PodRef:        podNN,
+	}, nil
 }