code review feedback

Signed-off-by: Irbe Krumina <irbe@tailscale.com>
pull/11019/head
Irbe Krumina 6 months ago
parent 3f8e1377e5
commit 0bfaaa5d04

@ -17,7 +17,7 @@ spec:
versions: versions:
- additionalPrinterColumns: - additionalPrinterColumns:
- description: Service IP address of the nameserver - description: Service IP address of the nameserver
jsonPath: .status.nameserverStatus.ip jsonPath: .status.nameserver.ip
name: NameserverIP name: NameserverIP
type: string type: string
name: v1alpha1 name: v1alpha1
@ -85,7 +85,7 @@ spec:
x-kubernetes-list-map-keys: x-kubernetes-list-map-keys:
- type - type
x-kubernetes-list-type: map x-kubernetes-list-type: map
nameserverStatus: nameserver:
type: object type: object
properties: properties:
ip: ip:

@ -5,5 +5,5 @@ metadata:
spec: spec:
nameserver: nameserver:
image: image:
repo: gcr.io/csi-test-290908/nameserver repo: tailscale/k8s-nameserver
tag: v0.0.1ns tag: unstable-v1.65

@ -176,7 +176,7 @@ spec:
versions: versions:
- additionalPrinterColumns: - additionalPrinterColumns:
- description: Service IP address of the nameserver - description: Service IP address of the nameserver
jsonPath: .status.nameserverStatus.ip jsonPath: .status.nameserver.ip
name: NameserverIP name: NameserverIP
type: string type: string
name: v1alpha1 name: v1alpha1
@ -240,7 +240,7 @@ spec:
x-kubernetes-list-map-keys: x-kubernetes-list-map-keys:
- type - type
x-kubernetes-list-type: map x-kubernetes-list-type: map
nameserverStatus: nameserver:
properties: properties:
ip: ip:
type: string type: string

@ -40,9 +40,9 @@ const (
// - For tailscale Ingress, a mapping of the Ingress's MagicDNSName to the IP address of // - For tailscale Ingress, a mapping of the Ingress's MagicDNSName to the IP address of
// the ingress proxy Pod. // the ingress proxy Pod.
// - For egress proxies configured via tailscale.com/tailnet-fqdn annotation, a // - 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 // tailscale.com/v1alpha1.DNSConfig instance in the cluster (so that we know
// that there is a ts.net nameserver deployed in the cluster). // that there is a ts.net nameserver deployed in the cluster).
type dnsRecordsReconciler struct { 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 // Check that there is a ts.net nameserver deployed to the cluster by
// checking that there is a ready tailscale.com/v1alpha1.DNSConfig // checking that there is tailscale.com/v1alpha1.DNSConfig resource in a
// resource. // Ready state.
dnsCfgLst := new(tsapi.DNSConfigList) dnsCfgLst := new(tsapi.DNSConfigList)
if err = dnsRR.List(ctx, dnsCfgLst); err != nil { if err = dnsRR.List(ctx, dnsCfgLst); err != nil {
return reconcile.Result{}, fmt.Errorf("error listing DNSConfigs: %w", err) 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) isEgressFQDNSvc, err := dnsRR.isSvcForFQDNEgressProxy(ctx, headlessSvc)
if err != nil { 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")) { if !(isEgressFQDNSvc || isManagedByType(headlessSvc, "ingress")) {
logger.Debug("Service is not fronting a proxy that we create DNS records for; do nothing") 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 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) eps, err := getSingleObject[discoveryv1.EndpointSlice](ctx, dnsRR.Client, dnsRR.tsNamespace, labels)
if err != nil { 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 { 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 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) ips := make([]string, 0)
for _, ep := range eps.Endpoints { for _, ep := range eps.Endpoints {
for _, ip := range ips { for _, ip := range ep.Addresses {
if !net.IsIPv4String(ip) { if !net.IsIPv4String(ip) {
logger.Infof("EndpointSlice contains IP address %q that is not IPv4, ignoring. Currently only IPv4 is supported", ip) logger.Infof("EndpointSlice contains IP address %q that is not IPv4, ignoring. Currently only IPv4 is supported", ip)
} else { } else {
ips = append(ips, ip) ips = append(ips, ip)
} }
} }
ips = append(ips, ep.Addresses...)
} }
if len(ips) == 0 { 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 return nil
} }
updateFunc := func(rec *operatorutils.Records) { 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 // maybeCleanup ensures that the DNS record for the proxy has been removed from
// dnsrecords ConfigMap and the tailscale.com/dns-records-reconciler finalizer // dnsrecords ConfigMap and the tailscale.com/dns-records-reconciler finalizer
// has been removed from the Service. If the record is not found in the // 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. // tailscale.com/magic-dnsname annotation, just remove the finalizer.
func (h *dnsRecordsReconciler) maybeCleanup(ctx context.Context, headlessSvc *corev1.Service, logger *zap.SugaredLogger) error { func (h *dnsRecordsReconciler) maybeCleanup(ctx context.Context, headlessSvc *corev1.Service, logger *zap.SugaredLogger) error {
ix := slices.Index(headlessSvc.Finalizers, dnsRecordsRecocilerFinalizer) ix := slices.Index(headlessSvc.Finalizers, dnsRecordsRecocilerFinalizer)
@ -219,23 +221,23 @@ func (h *dnsRecordsReconciler) maybeCleanup(ctx context.Context, headlessSvc *co
cm := &corev1.ConfigMap{} cm := &corev1.ConfigMap{}
err := h.Client.Get(ctx, types.NamespacedName{Name: operatorutils.DNSRecordsCMName, Namespace: h.tsNamespace}, cm) err := h.Client.Get(ctx, types.NamespacedName{Name: operatorutils.DNSRecordsCMName, Namespace: h.tsNamespace}, cm)
if apierrors.IsNotFound(err) { if apierrors.IsNotFound(err) {
logger.Debug("dsnrecords ConfigMap not found") logger.Debug("'dsnrecords' ConfigMap not found")
return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) return h.removeHeadlessSvcFinalizer(ctx, headlessSvc)
} }
if err != nil { 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 { if cm.Data == nil {
logger.Debug("dnsrecords ConfigMap contains no records") logger.Debug("'dnsrecords' ConfigMap contains no records")
return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) return h.removeHeadlessSvcFinalizer(ctx, headlessSvc)
} }
_, ok := cm.Data[operatorutils.DNSRecordsCMKey] _, ok := cm.Data[operatorutils.DNSRecordsCMKey]
if !ok { if !ok {
logger.Debug("dnsrecords ConfigMap contains no records") logger.Debug("'dnsrecords' ConfigMap contains no records")
return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) return h.removeHeadlessSvcFinalizer(ctx, headlessSvc)
} }
fqdn, ok := headlessSvc.GetAnnotations()[annotationTSMagicDNSName] fqdn, _ := headlessSvc.GetAnnotations()[annotationTSMagicDNSName]
if !ok || fqdn == "" { if fqdn == "" {
return h.removeHeadlessSvcFinalizer(ctx, headlessSvc) return h.removeHeadlessSvcFinalizer(ctx, headlessSvc)
} }
logger.Infof("removing DNS record for MagicDNS name %s", fqdn) logger.Infof("removing DNS record for MagicDNS name %s", fqdn)

@ -141,7 +141,7 @@ func (a *NameserverReconciler) Reconcile(ctx context.Context, req reconcile.Requ
return res, fmt.Errorf("error getting Service: %w", err) return res, fmt.Errorf("error getting Service: %w", err)
} }
if ip := svc.Spec.ClusterIP; ip != "" && ip != "None" { if ip := svc.Spec.ClusterIP; ip != "" && ip != "None" {
dnsCfg.Status.NameserverStatus = &tsapi.NameserverStatus{ dnsCfg.Status.Nameserver = &tsapi.NameserverStatus{
IP: ip, IP: ip,
} }
return setStatus(&dnsCfg, tsapi.NameserverReady, metav1.ConditionTrue, reasonNameserverCreated, reasonNameserverCreated) return setStatus(&dnsCfg, tsapi.NameserverReady, metav1.ConditionTrue, reasonNameserverCreated, reasonNameserverCreated)

@ -77,7 +77,7 @@ func TestNameserverReconciler(t *testing.T) {
svc.Spec.ClusterIP = "1.2.3.4" svc.Spec.ClusterIP = "1.2.3.4"
}) })
expectReconciled(t, nr, "", "test") expectReconciled(t, nr, "", "test")
dnsCfg.Status.NameserverStatus = &tsapi.NameserverStatus{ dnsCfg.Status.Nameserver = &tsapi.NameserverStatus{
IP: "1.2.3.4", IP: "1.2.3.4",
} }
dnsCfg.Finalizers = []string{FinalizerName} dnsCfg.Finalizers = []string{FinalizerName}

@ -427,7 +427,7 @@ ConnectorCondition contains condition information for a Connector.
</td> </td>
<td>false</td> <td>false</td>
</tr><tr> </tr><tr>
<td><b><a href="#dnsconfigstatusnameserverstatus">nameserverStatus</a></b></td> <td><b><a href="#dnsconfigstatusnameserver">nameserver</a></b></td>
<td>object</td> <td>object</td>
<td> <td>
<br/> <br/>
@ -503,7 +503,7 @@ ConnectorCondition contains condition information for a Connector.
</table> </table>
### DNSConfig.status.nameserverStatus ### DNSConfig.status.nameserver
<sup><sup>[↩ Parent](#dnsconfigstatus)</sup></sup> <sup><sup>[↩ Parent](#dnsconfigstatus)</sup></sup>

@ -17,7 +17,7 @@ var DNSConfigKind = "DNSConfig"
// +kubebuilder:object:root=true // +kubebuilder:object:root=true
// +kubebuilder:subresource:status // +kubebuilder:subresource:status
// +kubebuilder:resource:scope=Cluster,shortName=dc // +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 { type DNSConfig struct {
metav1.TypeMeta `json:",inline"` metav1.TypeMeta `json:",inline"`
@ -60,7 +60,7 @@ type DNSConfigStatus struct {
// +optional // +optional
Conditions []ConnectorCondition `json:"conditions"` Conditions []ConnectorCondition `json:"conditions"`
// +optional // +optional
NameserverStatus *NameserverStatus `json:"nameserverStatus"` Nameserver *NameserverStatus `json:"nameserver"`
} }
type NameserverStatus struct { type NameserverStatus struct {

@ -252,8 +252,8 @@ func (in *DNSConfigStatus) DeepCopyInto(out *DNSConfigStatus) {
(*in)[i].DeepCopyInto(&(*out)[i]) (*in)[i].DeepCopyInto(&(*out)[i])
} }
} }
if in.NameserverStatus != nil { if in.Nameserver != nil {
in, out := &in.NameserverStatus, &out.NameserverStatus in, out := &in.Nameserver, &out.Nameserver
*out = new(NameserverStatus) *out = new(NameserverStatus)
**out = **in **out = **in
} }

Loading…
Cancel
Save