-
Notifications
You must be signed in to change notification settings - Fork 578
fix: address PR review comments #1912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ import ( | |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/validation" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
|
|
@@ -130,6 +131,7 @@ type AdkApiTranslator interface { | |
| inputs *AgentManifestInputs, | ||
| ) (*AgentOutputs, error) | ||
| GetOwnedResourceTypes() []client.Object | ||
| WithClusterDomain(clusterDomain string) AdkApiTranslator | ||
| } | ||
|
|
||
| // probeConfig holds readiness probe timing configuration | ||
|
|
@@ -177,16 +179,30 @@ func NewAdkApiTranslatorWithWatchedNamespaces(kube client.Client, watchedNamespa | |
| defaultModelConfig: defaultModelConfig, | ||
| plugins: plugins, | ||
| globalProxyURL: globalProxyURL, | ||
| clusterDomain: "cluster.local", | ||
| sandboxBackend: sandboxBackend, | ||
| } | ||
| } | ||
|
|
||
| func (a *adkApiTranslator) WithClusterDomain(clusterDomain string) AdkApiTranslator { | ||
| if a == nil { | ||
| return a | ||
| } | ||
| clusterDomain = strings.TrimSpace(clusterDomain) | ||
| if clusterDomain == "" { | ||
| clusterDomain = "cluster.local" | ||
| } | ||
| a.clusterDomain = clusterDomain | ||
| return a | ||
| } | ||
|
|
||
| type adkApiTranslator struct { | ||
| kube client.Client | ||
| watchedNamespaces []string | ||
| defaultModelConfig types.NamespacedName | ||
| plugins []TranslatorPlugin | ||
| globalProxyURL string | ||
| clusterDomain string | ||
| sandboxBackend sandboxbackend.Backend | ||
| } | ||
|
|
||
|
|
@@ -953,34 +969,68 @@ func (a *adkApiTranslator) isInternalK8sURL(ctx context.Context, urlStr, namespa | |
| return false | ||
| } | ||
|
|
||
| // Check if it ends with .svc.cluster.local (definitely internal) | ||
| if strings.HasSuffix(hostname, ".svc.cluster.local") { | ||
| clusterDomain := strings.TrimSpace(a.clusterDomain) | ||
| if clusterDomain == "" { | ||
| clusterDomain = "cluster.local" | ||
| } | ||
| svcDomain := ".svc." + clusterDomain | ||
|
|
||
| // Only treat fully qualified service hostnames as internal if they match | ||
| // a valid Kubernetes service DNS pattern for the current cluster domain. | ||
| // Valid forms are: | ||
| // service.namespace.svc.<clusterDomain> | ||
| // service.namespace.svc | ||
| // service.namespace | ||
| if strings.HasSuffix(hostname, svcDomain) { | ||
| prefix := strings.TrimSuffix(hostname, svcDomain) | ||
| if prefix == "" { | ||
| return false | ||
| } | ||
| parts := strings.Split(prefix, ".") | ||
| if len(parts) != 2 { | ||
| return false | ||
| } | ||
| if len(validation.IsDNS1123Label(parts[0])) > 0 || len(validation.IsDNS1123Label(parts[1])) > 0 { | ||
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // Extract namespace from hostname pattern: {name}.{namespace} | ||
| // Examples: test-mcp-server.kagent -> namespace is "kagent" | ||
|
Comment on lines
-961
to
-962
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we keep this comment? this seems to still be relevant so it would be good context. |
||
| parts := strings.Split(hostname, ".") | ||
| if len(parts) == 2 { | ||
| service := parts[0] | ||
| potentialNamespace := parts[1] | ||
|
|
||
| // Check if this namespace exists in the cluster | ||
| ns := &corev1.Namespace{} | ||
| err := a.kube.Get(ctx, types.NamespacedName{Name: potentialNamespace}, ns) | ||
| if err == nil { | ||
| // Namespace exists, so this is an internal k8s URL | ||
| return true | ||
| if len(validation.IsDNS1123Label(service)) > 0 || len(validation.IsDNS1123Label(potentialNamespace)) > 0 { | ||
| return false | ||
| } | ||
| // Controller is using namespaced RBAC, so check if the namespace is watched | ||
| if (apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err)) && len(a.watchedNamespaces) > 0 { | ||
| return slices.Contains(a.watchedNamespaces, potentialNamespace) | ||
| return a.namespaceExistsOrWatched(ctx, potentialNamespace) | ||
| } | ||
|
|
||
| if len(parts) == 3 && parts[2] == "svc" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: would be helpful to add a comment that this conditional specifically addressesing a hostname using the |
||
| service := parts[0] | ||
| potentialNamespace := parts[1] | ||
| if len(validation.IsDNS1123Label(service)) > 0 || len(validation.IsDNS1123Label(potentialNamespace)) > 0 { | ||
| return false | ||
| } | ||
| // If namespace doesn't exist, it's likely a TLD or external domain | ||
| return a.namespaceExistsOrWatched(ctx, potentialNamespace) | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| func (a *adkApiTranslator) namespaceExistsOrWatched(ctx context.Context, potentialNamespace string) bool { | ||
| ns := &corev1.Namespace{} | ||
| err := a.kube.Get(ctx, types.NamespacedName{Name: potentialNamespace}, ns) | ||
| if err == nil { | ||
| return true | ||
| } | ||
| if (apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err)) && len(a.watchedNamespaces) > 0 { | ||
| return slices.Contains(a.watchedNamespaces, potentialNamespace) | ||
| } | ||
| return false | ||
| } | ||
|
|
||
|
|
||
| func applyProxyURL(originalURL, proxyURL string, headers map[string]string) (targetURL string, updatedHeaders map[string]string, err error) { | ||
| // Parse original URL to extract path and hostname | ||
| originalURLParsed, err := url.Parse(originalURL) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package agent | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "github.com/kagent-dev/kagent/go/api/v1alpha2" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| k8sruntime "k8s.io/apimachinery/pkg/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| ) | ||
|
|
||
| func TestAdkApiTranslator_IsInternalK8sURL(t *testing.T) { | ||
| scheme := k8sruntime.NewScheme() | ||
| require.NoError(t, corev1.AddToScheme(scheme)) | ||
| require.NoError(t, v1alpha2.AddToScheme(scheme)) | ||
|
|
||
| namespace := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "kagent"}, | ||
| } | ||
|
|
||
| kubeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(namespace).Build() | ||
| translatorImpl := NewAdkApiTranslatorWithWatchedNamespaces(kubeClient, nil, types.NamespacedName{Name: "default-model"}, nil, "", nil).WithClusterDomain("cluster.local").(*adkApiTranslator) | ||
|
|
||
| require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.cluster.local:3000/api", "kagent"), "should recognize fully qualified service DNS as internal") | ||
| require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc:3000/api", "kagent"), "should recognize service.namespace.svc shorthand as internal") | ||
| require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent:3000/api", "kagent"), "should recognize service.namespace shorthand as internal") | ||
|
|
||
| require.False(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.cluster.local.evil.com:3000/api", "kagent"), "should reject external domains that only contain the cluster suffix") | ||
| require.False(t, translatorImpl.isInternalK8sURL(context.Background(), "http://example.com:8080", "kagent"), "should reject normal external URLs") | ||
|
|
||
| translatorImpl.WithClusterDomain("custom.internal") | ||
| require.True(t, translatorImpl.isInternalK8sURL(context.Background(), "http://grafana.kagent.svc.custom.internal:3000/api", "kagent"), "should honor custom cluster-domain values") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer using
a != nilinstead for slightly better readability