Ingress status update fixes (#2136)

* Updated samples to reflect needed permissions and config properties for ingress status change * Describe required YarpOptions fields * Call async method instead of sync and pass cancellation token * Do not update statuses of ingresses not handled by the controller
microsoft · May 22, 2023 · ad6ee6a · ad6ee6a
1 parent 09680d9
commit ad6ee6a
Show file tree

Hide file tree

Showing 9 changed files with 46 additions and 25 deletions.
diff --git a/samples/KubernetesIngress.Sample/Combined/ingress-controller.yaml b/samples/KubernetesIngress.Sample/Combined/ingress-controller.yaml
@@ -14,7 +14,9 @@ data:
       "Yarp": {
         "ControllerClass": "microsoft.com/ingress-yarp",
         "ServerCertificates": false,
-        "DefaultSslCertificate": "yarp/yarp-ingress-tls"
+        "DefaultSslCertificate": "yarp/yarp-ingress-tls",
+        "ControllerServiceName": "ingress-yarp-controller",
+        "ControllerServiceNamespace": "yarp"
       }
     }
 ---
@@ -55,6 +57,12 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - services/status
+  verbs:
+  - get
 - apiGroups:
   - networking.k8s.io
   - extensions
@@ -80,6 +88,7 @@ rules:
   resources:
   - ingresses/status
   verbs:
+  - get
   - update
 ---
 apiVersion: rbac.authorization.k8s.io/v1

diff --git a/samples/KubernetesIngress.Sample/Monitor/ingress-monitor.yaml b/samples/KubernetesIngress.Sample/Monitor/ingress-monitor.yaml
@@ -12,7 +12,9 @@ data:
   yarp.json: |
     {
       "Yarp": {
-        "ControllerClass": "microsoft.com/ingress-yarp"
+        "ControllerClass": "microsoft.com/ingress-yarp",
+        "ControllerServiceName": "yarp-controller",
+        "ControllerServiceNamespace": "yarp"
       }
     }
 ---
@@ -53,6 +55,12 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - ""
+  resources:
+  - services/status
+  verbs:
+  - get
 - apiGroups:
   - networking.k8s.io
   - extensions
@@ -78,6 +86,7 @@ rules:
   resources:
   - ingresses/status
   verbs:
+  - get
   - update
 ---
 apiVersion: rbac.authorization.k8s.io/v1

diff --git a/src/Kubernetes.Controller/Caching/ICache.cs b/src/Kubernetes.Controller/Caching/ICache.cs
@@ -23,5 +23,4 @@ public interface ICache
     bool TryGetReconcileData(NamespacedName key, out ReconcileData data);
     void GetKeys(List<NamespacedName> keys);
     IEnumerable<IngressData> GetIngresses();
-    bool IsYarpIngress(IngressData ingress);
 }
diff --git a/src/Kubernetes.Controller/Caching/IngressCache.cs b/src/Kubernetes.Controller/Caching/IngressCache.cs
@@ -151,14 +151,14 @@ public IEnumerable<IngressData> GetIngresses()
         {
             foreach (var ns in _namespaceCaches)
             {
-                ingresses.AddRange(ns.Value.GetIngresses());
+                ingresses.AddRange(ns.Value.GetIngresses().Where(IsYarpIngress));
             }
         }
 
         return ingresses;
     }
 
-    public bool IsYarpIngress(IngressData ingress)
+    private bool IsYarpIngress(IngressData ingress)
     {
         if (ingress.Spec.IngressClassName is null)
         {

diff --git a/src/Kubernetes.Controller/Client/IIngressResourceStatusUpdater.cs b/src/Kubernetes.Controller/Client/IIngressResourceStatusUpdater.cs
@@ -1,10 +1,6 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
-using System.Collections.Generic;
-using k8s;
-using k8s.Models;
-using Microsoft.Extensions.Hosting;
-using System;
+
 using System.Threading;
 using System.Threading.Tasks;
 
@@ -15,5 +11,5 @@ public interface IIngressResourceStatusUpdater
     /// <summary>
     /// Updates the status of cached ingresses.
     /// </summary>
-    Task UpdateStatusAsync();
+    Task UpdateStatusAsync(CancellationToken cancellationToken);
 }
diff --git a/src/Kubernetes.Controller/Client/V1IngressResourceStatusUpdater.cs b/src/Kubernetes.Controller/Client/V1IngressResourceStatusUpdater.cs
@@ -8,6 +8,7 @@
 using Microsoft.Extensions.Options;
 using System.Threading.Tasks;
 using Yarp.Kubernetes.Controller.Caching;
+using System.Threading;
 
 namespace Yarp.Kubernetes.Controller.Client;
 
@@ -31,19 +32,19 @@ internal sealed class V1IngressResourceStatusUpdater : IIngressResourceStatusUpd
     }
 
 
-    public async Task UpdateStatusAsync()
+    public async Task UpdateStatusAsync(CancellationToken cancellationToken)
     {
-        var service = _client.CoreV1.ReadNamespacedServiceStatus(_options.ControllerServiceName, _options.ControllerServiceNamespace);
+        var service = await _client.CoreV1.ReadNamespacedServiceStatusAsync(_options.ControllerServiceName, _options.ControllerServiceNamespace, cancellationToken: cancellationToken);
         if (service.Status?.LoadBalancer?.Ingress is { } loadBalancerIngresses)
         {
             var status = new V1IngressStatus(new V1LoadBalancerStatus(loadBalancerIngresses));
             var ingresses = _cache.GetIngresses().ToArray();
             foreach (var ingress in ingresses)
             {
                 _logger.LogInformation("Updating ingress {IngressClassNamespace}/{IngressClassName} status.", ingress.Metadata.NamespaceProperty, ingress.Metadata.Name);
-                var ingressStatus = await _client.NetworkingV1.ReadNamespacedIngressStatusAsync(ingress.Metadata.Name, ingress.Metadata.NamespaceProperty);
+                var ingressStatus = await _client.NetworkingV1.ReadNamespacedIngressStatusAsync(ingress.Metadata.Name, ingress.Metadata.NamespaceProperty, cancellationToken: cancellationToken);
                 ingressStatus.Status = status;
-                await _client.NetworkingV1.ReplaceNamespacedIngressStatusAsync(ingressStatus, ingress.Metadata.Name, ingress.Metadata.NamespaceProperty);
+                await _client.NetworkingV1.ReplaceNamespacedIngressStatusAsync(ingressStatus, ingress.Metadata.Name, ingress.Metadata.NamespaceProperty, cancellationToken: cancellationToken);
                 _logger.LogInformation("Updated ingress {IngressClassNamespace}/{IngressClassName} status.", ingress.Metadata.NamespaceProperty, ingress.Metadata.Name);
             }
         }

diff --git a/src/Kubernetes.Controller/Services/Reconciler.cs b/src/Kubernetes.Controller/Services/Reconciler.cs
@@ -45,11 +45,6 @@ public async Task ProcessAsync(CancellationToken cancellationToken)
             {
                 try
                 {
-                    if (!_cache.IsYarpIngress(ingress))
-                    {
-                        continue;
-                    }
-
                     if (_cache.TryGetReconcileData(new NamespacedName(ingress.Metadata.NamespaceProperty, ingress.Metadata.Name), out var data))
                     {
                         var ingressContext = new YarpIngressContext(ingress, data.ServiceList, data.EndpointsList);
@@ -68,7 +63,7 @@ public async Task ProcessAsync(CancellationToken cancellationToken)
             _logger.LogInformation(JsonSerializer.Serialize(clusters));
 
             await _updateConfig.UpdateAsync(configContext.Routes, clusters, cancellationToken).ConfigureAwait(false);
-            await _ingressResourceStatusUpdater.UpdateStatusAsync();
+            await _ingressResourceStatusUpdater.UpdateStatusAsync(cancellationToken);
         }
         catch (Exception ex)
         {

diff --git a/src/Kubernetes.Controller/YarpOptions.cs b/src/Kubernetes.Controller/YarpOptions.cs
@@ -5,13 +5,25 @@ namespace Yarp.Kubernetes.Controller;
 
 public class YarpOptions
 {
+    /// <summary>
+    /// Defines a name of the ingress controller. IngressClass ".spec.controller" field should match this.
+    /// This field is required.
+    /// </summary>
     public string ControllerClass { get; set; }
 
     public bool ServerCertificates { get; set; }
 
     public string DefaultSslCertificate { get; set; }
 
+    /// <summary>
+    /// Name of the Kubernetes Service the ingress controller is running in.
+    /// This field is required.
+    /// </summary>
     public string ControllerServiceName { get; set; }
 
+    /// <summary>
+    /// Namespace of the Kubernetes Service the ingress controller is running in.
+    /// This field is required.
+    /// </summary>
     public string ControllerServiceNamespace { get; set; }
 }
diff --git a/test/Kubernetes.Tests/IngressCacheTests.cs b/test/Kubernetes.Tests/IngressCacheTests.cs
@@ -64,7 +64,7 @@ public void IngressWithClassAnnotationTests(string ingressClassName, string cont
         _cacheUnderTest.Update(WatchEventType.Added, ingress);
 
         // Assert
-        var ingresses = _cacheUnderTest.GetIngresses().Where(_cacheUnderTest.IsYarpIngress).ToArray();
+        var ingresses = _cacheUnderTest.GetIngresses().ToArray();
 
         Assert.Equal(expectedIngressCount, ingresses.Length);
     }
@@ -90,7 +90,7 @@ public void IngressWithoutClassAnnotationTests(string ingressClassName, string c
         _cacheUnderTest.Update(WatchEventType.Added, ingress);
 
         // Assert
-        var ingresses = _cacheUnderTest.GetIngresses().Where(_cacheUnderTest.IsYarpIngress).ToArray();
+        var ingresses = _cacheUnderTest.GetIngresses().ToArray();
 
         Assert.Equal(expectedIngressCount, ingresses.Length);
     }
@@ -111,7 +111,7 @@ public void IngressModifiedToRemoveClass()
 
         // Assert
         var ingresses = _cacheUnderTest.GetIngresses().ToArray();
-        Assert.All(ingresses, i => Assert.False(_cacheUnderTest.IsYarpIngress(i)));
+        Assert.Empty(ingresses);
     }
 
     [Fact]
@@ -149,7 +149,7 @@ public void IngressClassDeleted()
 
         // Assert
         var ingresses = _cacheUnderTest.GetIngresses().ToArray();
-        Assert.All(ingresses, i => Assert.False(_cacheUnderTest.IsYarpIngress(i)));
+        Assert.Empty(ingresses);
     }
 
     [Fact]