From 0bfaaa5d04b5fee657b4fe0baac4cdd1a1d0380c Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Thu, 2 May 2024 13:38:58 +0100 Subject: [PATCH] code review feedback Signed-off-by: Irbe Krumina --- .../deploy/crds/tailscale.com_dnsconfigs.yaml | 4 +-- .../deploy/examples/dnsconfig.yaml | 4 +-- .../deploy/manifests/operator.yaml | 4 +-- cmd/k8s-operator/dnsrecords.go | 36 ++++++++++--------- cmd/k8s-operator/nameserver.go | 2 +- cmd/k8s-operator/nameserver_test.go | 2 +- k8s-operator/api.md | 4 +-- .../apis/v1alpha1/types_tsdnsconfig.go | 4 +-- .../apis/v1alpha1/zz_generated.deepcopy.go | 4 +-- 9 files changed, 33 insertions(+), 31 deletions(-) diff --git a/cmd/k8s-operator/deploy/crds/tailscale.com_dnsconfigs.yaml b/cmd/k8s-operator/deploy/crds/tailscale.com_dnsconfigs.yaml index ba4a66c98..083c587b2 100644 --- a/cmd/k8s-operator/deploy/crds/tailscale.com_dnsconfigs.yaml +++ b/cmd/k8s-operator/deploy/crds/tailscale.com_dnsconfigs.yaml @@ -17,7 +17,7 @@ spec: versions: - additionalPrinterColumns: - description: Service IP address of the nameserver - jsonPath: .status.nameserverStatus.ip + jsonPath: .status.nameserver.ip name: NameserverIP type: string name: v1alpha1 @@ -85,7 +85,7 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - nameserverStatus: + nameserver: type: object properties: ip: diff --git a/cmd/k8s-operator/deploy/examples/dnsconfig.yaml b/cmd/k8s-operator/deploy/examples/dnsconfig.yaml index 401946ed5..eae6486db 100644 --- a/cmd/k8s-operator/deploy/examples/dnsconfig.yaml +++ b/cmd/k8s-operator/deploy/examples/dnsconfig.yaml @@ -5,5 +5,5 @@ metadata: spec: nameserver: image: - repo: gcr.io/csi-test-290908/nameserver - tag: v0.0.1ns + repo: tailscale/k8s-nameserver + tag: unstable-v1.65 diff --git a/cmd/k8s-operator/deploy/manifests/operator.yaml b/cmd/k8s-operator/deploy/manifests/operator.yaml index 1dbfd4323..2bbd0357c 100644 --- a/cmd/k8s-operator/deploy/manifests/operator.yaml +++ b/cmd/k8s-operator/deploy/manifests/operator.yaml @@ -176,7 +176,7 @@ spec: versions: - additionalPrinterColumns: - description: Service IP address of the nameserver - jsonPath: .status.nameserverStatus.ip + jsonPath: .status.nameserver.ip name: NameserverIP type: string name: v1alpha1 @@ -240,7 +240,7 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map - nameserverStatus: + nameserver: properties: ip: type: string diff --git a/cmd/k8s-operator/dnsrecords.go b/cmd/k8s-operator/dnsrecords.go index 87b2ca206..2287a458c 100644 --- a/cmd/k8s-operator/dnsrecords.go +++ b/cmd/k8s-operator/dnsrecords.go @@ -40,9 +40,9 @@ const ( // - For tailscale Ingress, a mapping of the Ingress's MagicDNSName to the IP address of // the ingress proxy Pod. // - For egress proxies configured via tailscale.com/tailnet-fqdn annotation, a -// mapping of the the tailnet FQDN to the IP address of the egress proxy Pod. +// mapping of the tailnet FQDN to the IP address of the egress proxy Pod. // -// Records will only created if there is exactly one ready +// Records will only be created if there is exactly one ready // tailscale.com/v1alpha1.DNSConfig instance in the cluster (so that we know // that there is a ts.net nameserver deployed in the cluster). type dnsRecordsReconciler struct { @@ -80,8 +80,8 @@ func (dnsRR *dnsRecordsReconciler) Reconcile(ctx context.Context, req reconcile. } // Check that there is a ts.net nameserver deployed to the cluster by - // checking that there is a ready tailscale.com/v1alpha1.DNSConfig - // resource. + // checking that there is tailscale.com/v1alpha1.DNSConfig resource in a + // Ready state. dnsCfgLst := new(tsapi.DNSConfigList) if err = dnsRR.List(ctx, dnsCfgLst); err != nil { return reconcile.Result{}, fmt.Errorf("error listing DNSConfigs: %w", err) @@ -128,7 +128,7 @@ func (dnsRR *dnsRecordsReconciler) maybeProvision(ctx context.Context, headlessS } isEgressFQDNSvc, err := dnsRR.isSvcForFQDNEgressProxy(ctx, headlessSvc) if err != nil { - return fmt.Errorf("error checking whether Service is for an egress proxy: %w", err) + return fmt.Errorf("error checking whether the Service is for an egress proxy: %w", err) } if !(isEgressFQDNSvc || isManagedByType(headlessSvc, "ingress")) { logger.Debug("Service is not fronting a proxy that we create DNS records for; do nothing") @@ -175,25 +175,27 @@ func (dnsRR *dnsRecordsReconciler) maybeProvision(ctx context.Context, headlessS labels := map[string]string{discoveryv1.LabelServiceName: headlessSvc.Name} // https://kubernetes.io/docs/concepts/services-networking/endpoint-slices/#ownership eps, err := getSingleObject[discoveryv1.EndpointSlice](ctx, dnsRR.Client, dnsRR.tsNamespace, labels) if err != nil { - return fmt.Errorf("error getting proxy EndpointSlice: %w", err) + return fmt.Errorf("error getting the EndpointSlice for the proxy's headless Service: %w", err) } if eps == nil { - logger.Debugf("proxy EndpointSlice does not yet exist. We will reconcile again once it's created") + logger.Debugf("proxy's headless Service EndpointSlice does not yet exist. We will reconcile again once it's created") return nil } + // An EndpointSlice for a Service can have a list of endpoints that each + // can have multiple addresses - these are the IP addresses of any Pods + // selected by that Service. Pick all the IPv4 addresses. ips := make([]string, 0) for _, ep := range eps.Endpoints { - for _, ip := range ips { + for _, ip := range ep.Addresses { if !net.IsIPv4String(ip) { logger.Infof("EndpointSlice contains IP address %q that is not IPv4, ignoring. Currently only IPv4 is supported", ip) } else { ips = append(ips, ip) } } - ips = append(ips, ep.Addresses...) } if len(ips) == 0 { - logger.Debugf("No endpoint addresses found. We will reconcile again once they are created.") + logger.Debugf("EndpointSlice for the Service contains no IPv4 addresses. We will reconcile again once they are created.") return nil } updateFunc := func(rec *operatorutils.Records) { @@ -208,7 +210,7 @@ func (dnsRR *dnsRecordsReconciler) maybeProvision(ctx context.Context, headlessS // maybeCleanup ensures that the DNS record for the proxy has been removed from // dnsrecords ConfigMap and the tailscale.com/dns-records-reconciler finalizer // has been removed from the Service. If the record is not found in the -// ConfigMap, the ConfigMap does not exist or the Service does not have +// ConfigMap, the ConfigMap does not exist, or the Service does not have // tailscale.com/magic-dnsname annotation, just remove the finalizer. func (h *dnsRecordsReconciler) maybeCleanup(ctx context.Context, headlessSvc *corev1.Service, logger *zap.SugaredLogger) error { ix := slices.Index(headlessSvc.Finalizers, dnsRecordsRecocilerFinalizer) @@ -219,23 +221,23 @@ func (h *dnsRecordsReconciler) maybeCleanup(ctx context.Context, headlessSvc *co cm := &corev1.ConfigMap{} err := h.Client.Get(ctx, types.NamespacedName{Name: operatorutils.DNSRecordsCMName, Namespace: h.tsNamespace}, cm) if apierrors.IsNotFound(err) { - logger.Debug("dsnrecords ConfigMap not found") + logger.Debug("'dsnrecords' ConfigMap not found") return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) } if err != nil { - return fmt.Errorf("error retrieving dnsrecords ConfigMap: %w", err) + return fmt.Errorf("error retrieving 'dnsrecords' ConfigMap: %w", err) } if cm.Data == nil { - logger.Debug("dnsrecords ConfigMap contains no records") + logger.Debug("'dnsrecords' ConfigMap contains no records") return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) } _, ok := cm.Data[operatorutils.DNSRecordsCMKey] if !ok { - logger.Debug("dnsrecords ConfigMap contains no records") + logger.Debug("'dnsrecords' ConfigMap contains no records") return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) } - fqdn, ok := headlessSvc.GetAnnotations()[annotationTSMagicDNSName] - if !ok || fqdn == "" { + fqdn, _ := headlessSvc.GetAnnotations()[annotationTSMagicDNSName] + if fqdn == "" { return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) } logger.Infof("removing DNS record for MagicDNS name %s", fqdn) diff --git a/cmd/k8s-operator/nameserver.go b/cmd/k8s-operator/nameserver.go index d0be65503..0a1d21bb3 100644 --- a/cmd/k8s-operator/nameserver.go +++ b/cmd/k8s-operator/nameserver.go @@ -141,7 +141,7 @@ func (a *NameserverReconciler) Reconcile(ctx context.Context, req reconcile.Requ return res, fmt.Errorf("error getting Service: %w", err) } if ip := svc.Spec.ClusterIP; ip != "" && ip != "None" { - dnsCfg.Status.NameserverStatus = &tsapi.NameserverStatus{ + dnsCfg.Status.Nameserver = &tsapi.NameserverStatus{ IP: ip, } return setStatus(&dnsCfg, tsapi.NameserverReady, metav1.ConditionTrue, reasonNameserverCreated, reasonNameserverCreated) diff --git a/cmd/k8s-operator/nameserver_test.go b/cmd/k8s-operator/nameserver_test.go index 8e6580b4a..cd89444a4 100644 --- a/cmd/k8s-operator/nameserver_test.go +++ b/cmd/k8s-operator/nameserver_test.go @@ -77,7 +77,7 @@ func TestNameserverReconciler(t *testing.T) { svc.Spec.ClusterIP = "1.2.3.4" }) expectReconciled(t, nr, "", "test") - dnsCfg.Status.NameserverStatus = &tsapi.NameserverStatus{ + dnsCfg.Status.Nameserver = &tsapi.NameserverStatus{ IP: "1.2.3.4", } dnsCfg.Finalizers = []string{FinalizerName} diff --git a/k8s-operator/api.md b/k8s-operator/api.md index 1e7c9879b..3a881428a 100644 --- a/k8s-operator/api.md +++ b/k8s-operator/api.md @@ -427,7 +427,7 @@ ConnectorCondition contains condition information for a Connector. false - nameserverStatus + nameserver object
@@ -503,7 +503,7 @@ ConnectorCondition contains condition information for a Connector. -### DNSConfig.status.nameserverStatus +### DNSConfig.status.nameserver [↩ Parent](#dnsconfigstatus) diff --git a/k8s-operator/apis/v1alpha1/types_tsdnsconfig.go b/k8s-operator/apis/v1alpha1/types_tsdnsconfig.go index 7104d3c13..62dad3f68 100644 --- a/k8s-operator/apis/v1alpha1/types_tsdnsconfig.go +++ b/k8s-operator/apis/v1alpha1/types_tsdnsconfig.go @@ -17,7 +17,7 @@ var DNSConfigKind = "DNSConfig" // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster,shortName=dc -// +kubebuilder:printcolumn:name="NameserverIP",type="string",JSONPath=`.status.nameserverStatus.ip`,description="Service IP address of the nameserver" +// +kubebuilder:printcolumn:name="NameserverIP",type="string",JSONPath=`.status.nameserver.ip`,description="Service IP address of the nameserver" type DNSConfig struct { metav1.TypeMeta `json:",inline"` @@ -60,7 +60,7 @@ type DNSConfigStatus struct { // +optional Conditions []ConnectorCondition `json:"conditions"` // +optional - NameserverStatus *NameserverStatus `json:"nameserverStatus"` + Nameserver *NameserverStatus `json:"nameserver"` } type NameserverStatus struct { diff --git a/k8s-operator/apis/v1alpha1/zz_generated.deepcopy.go b/k8s-operator/apis/v1alpha1/zz_generated.deepcopy.go index 419a763fd..3d5840ad2 100644 --- a/k8s-operator/apis/v1alpha1/zz_generated.deepcopy.go +++ b/k8s-operator/apis/v1alpha1/zz_generated.deepcopy.go @@ -252,8 +252,8 @@ func (in *DNSConfigStatus) DeepCopyInto(out *DNSConfigStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.NameserverStatus != nil { - in, out := &in.NameserverStatus, &out.NameserverStatus + if in.Nameserver != nil { + in, out := &in.Nameserver, &out.Nameserver *out = new(NameserverStatus) **out = **in }