Skip to content

Commit 007aa78

Browse files
steveiliop56codex
andcommitted
fix: coderabbit comments
Co-authored-by: Codex <noreply@openai.com>
1 parent 11a60fe commit 007aa78

3 files changed

Lines changed: 136 additions & 2 deletions

File tree

‎.coderabbit.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
# yaml-language-server: $schema=https://www.coderabbit.ai/integrations/schema.v2.json
12
issue_enrichment:
23
auto_enrich:
34
enabled: false
5+
reviews:
6+
profile: chill

‎internal/service/kubernetes_service.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,22 @@ var supportedResources = []watchedResource{
5050
}
5151

5252
func hostMatchesHostname(host string, hostname string) bool {
53+
if host == "" {
54+
return true
55+
}
5356
host = normalizeDomain(host)
5457
hostname = normalizeDomain(hostname)
5558
if suffix, ok := strings.CutPrefix(host, "*."); ok {
56-
return strings.HasSuffix(hostname, "."+suffix)
59+
prefix, matches := strings.CutSuffix(hostname, "."+suffix)
60+
return matches && prefix != "" && !strings.Contains(prefix, ".")
5761
}
5862
return host == hostname
5963
}
6064

6165
func hostCoversName(host string, name string) bool {
66+
if host == "" {
67+
return true
68+
}
6269
host = strings.ToLower(host)
6370
if strings.HasPrefix(host, "*.") {
6471
return true
@@ -312,14 +319,26 @@ func (k *KubernetesService) resyncGVR(res watchedResource, ctx context.Context)
312319
k.log.App.Warn().Err(err).Str("res", res.pretty()).Msg("Failed to list resources for resync")
313320
return err
314321
}
322+
seen := make(map[resourceKey]struct{}, len(list.Items))
315323
for _, item := range list.Items {
324+
seen[resourceKey{typ: res.typ, namespace: item.GetNamespace(), name: item.GetName()}] = struct{}{}
316325
newTypedItem, err := new(typedItem).fromUnstructured(res.typ, &item)
317326
if err != nil {
318327
k.log.App.Warn().Err(err).Str("res", res.pretty()).Msg("Failed to decode resource, skipping")
319328
continue
320329
}
321330
k.updateFromItem(res, newTypedItem)
322331
}
332+
k.mu.Lock()
333+
for key := range k.apps {
334+
if key.typ != res.typ {
335+
continue
336+
}
337+
if _, ok := seen[key]; !ok {
338+
delete(k.apps, key)
339+
}
340+
}
341+
k.mu.Unlock()
323342
k.log.App.Debug().Str("res", res.pretty()).Int("count", len(list.Items)).Msg("Resync complete")
324343
return nil
325344
}

‎internal/service/kubernetes_service_test.go‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package service
22

33
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
48
"strings"
59
"testing"
610

@@ -11,6 +15,8 @@ import (
1115
networking "k8s.io/api/networking/v1"
1216
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1317
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
18+
"k8s.io/client-go/dynamic"
19+
"k8s.io/client-go/rest"
1420
)
1521

1622
func watchedResourceForTest(t *testing.T, typ ResourceType) watchedResource {
@@ -88,6 +94,23 @@ func TestKubernetesServiceUpdateFromItem(t *testing.T) {
8894
}, "Dashboard.example.com"),
8995
domain: "dashboard.example.com", allow: "alice",
9096
},
97+
{
98+
name: "Hostless Ingress matches a configured domain",
99+
resource: ResourceTypeIngress,
100+
item: testIngress("ingress", map[string]string{
101+
"tinyauth.apps.dashboard.config.domain": "dashboard.example.com",
102+
"tinyauth.apps.dashboard.users.allow": "alice",
103+
}, ""),
104+
domain: "dashboard.example.com", wantConfigDomain: "dashboard.example.com", allow: "alice",
105+
},
106+
{
107+
name: "Hostless Ingress matches an app name",
108+
resource: ResourceTypeIngress,
109+
item: testIngress("ingress", map[string]string{
110+
"tinyauth.apps.dashboard.users.allow": "alice",
111+
}, ""),
112+
domain: "dashboard.example.com", allow: "alice",
113+
},
91114
}
92115

93116
for _, test := range tests {
@@ -99,6 +122,10 @@ func TestKubernetesServiceUpdateFromItem(t *testing.T) {
99122
require.NotNil(t, app)
100123
assert.Equal(t, test.allow, app.Users.Allow)
101124
assert.Equal(t, test.wantConfigDomain, app.Config.Domain)
125+
if test.item.ingress.Spec.Rules[0].Host == "" {
126+
key := resourceKey{typ: test.resource, namespace: "default", name: "ingress"}
127+
assert.Equal(t, []string{""}, service.apps[key].hosts)
128+
}
102129
})
103130
}
104131
}
@@ -132,6 +159,68 @@ func TestKubernetesServiceUpdateFromItemRemovesStaleEntries(t *testing.T) {
132159
}
133160
}
134161

162+
func TestKubernetesServiceResyncRemovesMissingResources(t *testing.T) {
163+
log := logger.NewLogger().WithTestConfig()
164+
log.Init()
165+
service := newKubernetesServiceForTest(log)
166+
res := watchedResourceForTest(t, ResourceTypeIngress)
167+
168+
live := testIngress("keep", map[string]string{
169+
"tinyauth.apps.keep.config.domain": "keep.example.com",
170+
}, "keep.example.com")
171+
items := []networking.Ingress{*live.ingress}
172+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
173+
w.Header().Set("Content-Type", "application/json")
174+
if err := json.NewEncoder(w).Encode(networking.IngressList{
175+
TypeMeta: metav1.TypeMeta{APIVersion: "networking.k8s.io/v1", Kind: "IngressList"},
176+
Items: items,
177+
}); err != nil {
178+
t.Errorf("encode ingress list: %v", err)
179+
}
180+
}))
181+
defer server.Close()
182+
client, err := dynamic.NewForConfig(&rest.Config{Host: server.URL})
183+
require.NoError(t, err)
184+
service.client = client
185+
stale := resourceKey{typ: res.typ, namespace: "default", name: "gone"}
186+
otherNamespace := resourceKey{typ: res.typ, namespace: "other", name: "keep"}
187+
otherType := resourceKey{typ: ResourceType("other"), namespace: "default", name: "gone"}
188+
for _, key := range []resourceKey{stale, otherNamespace, otherType} {
189+
service.addResourceEntries(key, nil, []resourceEntry{{name: "app"}})
190+
}
191+
192+
require.NoError(t, service.resyncGVR(res, context.Background()))
193+
assert.Contains(t, service.apps, resourceKey{typ: res.typ, namespace: "default", name: "keep"})
194+
assert.NotContains(t, service.apps, stale)
195+
assert.NotContains(t, service.apps, otherNamespace)
196+
assert.Contains(t, service.apps, otherType)
197+
198+
items = nil
199+
require.NoError(t, service.resyncGVR(res, context.Background()))
200+
assert.NotContains(t, service.apps, resourceKey{typ: res.typ, namespace: "default", name: "keep"})
201+
assert.Contains(t, service.apps, otherType)
202+
}
203+
204+
func TestKubernetesServiceResyncKeepsResourcesOnListFailure(t *testing.T) {
205+
log := logger.NewLogger().WithTestConfig()
206+
log.Init()
207+
service := newKubernetesServiceForTest(log)
208+
res := watchedResourceForTest(t, ResourceTypeIngress)
209+
key := resourceKey{typ: res.typ, namespace: "default", name: "keep"}
210+
service.addResourceEntries(key, nil, []resourceEntry{{name: "app"}})
211+
212+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
213+
http.Error(w, "list failed", http.StatusServiceUnavailable)
214+
}))
215+
defer server.Close()
216+
client, err := dynamic.NewForConfig(&rest.Config{Host: server.URL})
217+
require.NoError(t, err)
218+
service.client = client
219+
220+
require.Error(t, service.resyncGVR(res, context.Background()))
221+
assert.Contains(t, service.apps, key)
222+
}
223+
135224
func TestTypedItemFromUnstructured(t *testing.T) {
136225
tests := []struct {
137226
name string
@@ -227,9 +316,14 @@ func TestKubernetesHostMatching(t *testing.T) {
227316
}{
228317
{"Exact host", "app.example.com", "app.example.com", true},
229318
{"Case insensitive exact host", "App.Example.com", "app.example.com", true},
230-
{"Wildcard host", "*.example.com", "deep.app.example.com", true},
319+
{"Wildcard host", "*.example.com", "app.example.com", true},
320+
{"Wildcard host is case insensitive", "*.Example.com", "App.example.com", true},
231321
{"Wildcard does not match its apex", "*.example.com", "example.com", false},
322+
{"Wildcard rejects empty label", "*.example.com", ".example.com", false},
323+
{"Wildcard rejects multiple labels", "*.example.com", "deep.app.example.com", false},
324+
{"Wildcard rejects other suffix", "*.example.com", "app.other.com", false},
232325
{"Different host", "app.example.com", "other.example.com", false},
326+
{"Empty host matches any hostname", "", "other.example.com", true},
233327
}
234328

235329
for _, test := range tests {
@@ -238,3 +332,21 @@ func TestKubernetesHostMatching(t *testing.T) {
238332
})
239333
}
240334
}
335+
336+
func TestKubernetesHostCoversName(t *testing.T) {
337+
tests := []struct {
338+
host string
339+
name string
340+
want bool
341+
}{
342+
{"", "dashboard", true},
343+
{"dashboard.example.com", "dashboard", true},
344+
{"Dashboard.example.com", "dashboard", true},
345+
{"*.example.com", "dashboard", true},
346+
{"other.example.com", "dashboard", false},
347+
}
348+
349+
for _, test := range tests {
350+
assert.Equal(t, test.want, hostCoversName(test.host, test.name))
351+
}
352+
}

0 commit comments

Comments
 (0)