From eb196e9a8309ab760ae827a87e1405e1b8bca308 Mon Sep 17 00:00:00 2001 From: Kevin McDermott Date: Thu, 29 Oct 2026 21:30:43 +0000 Subject: [PATCH] Apply upstream GitOpsSets bug fixes This applies several changes from the weaveworks-gitopssets controler. * API-Client destinations can be restricted to not allow RFC1918 addresses and redirects. * Suspended GitOpsSets are now finalised on deletion. * Watch credential and header changes. * Pull-Request pagination * toYAML errors are now surfaced * Remove null creationTimestamp from rendered objects Co-Authored-By: Charles Sibbald --- api/v1alpha1/gitopsset_types.go | 6 + api/v1alpha1/zz_generated.deepcopy.go | 5 + .../crd/bases/sets.gitops.pro_gitopssets.yaml | 14 +- controllers/gitopsset_controller.go | 62 ++++---- controllers/gitopsset_controller_test.go | 75 +++++++++- controllers/templates/renderer.go | 13 +- controllers/templates/renderer_test.go | 35 +++++ docs/api/gitopsset.md | 14 ++ pkg/generators/apiclient/address_policy.go | 120 +++++++++++++++ .../apiclient/address_policy_test.go | 138 ++++++++++++++++++ pkg/generators/apiclient/api_client.go | 12 +- pkg/generators/apiclient/api_client_test.go | 4 + pkg/generators/pullrequests/pull_requests.go | 29 +++- .../pullrequests/pull_requests_test.go | 73 +++++++++ tests/e2e/gitopsset_controller_test.go | 2 +- tests/e2e/main_test.go | 2 + tests/e2e/output.log | 0 17 files changed, 569 insertions(+), 35 deletions(-) create mode 100644 pkg/generators/apiclient/address_policy.go create mode 100644 pkg/generators/apiclient/address_policy_test.go create mode 100644 tests/e2e/output.log diff --git a/api/v1alpha1/gitopsset_types.go b/api/v1alpha1/gitopsset_types.go index d4ab5758..5fdfa515 100644 --- a/api/v1alpha1/gitopsset_types.go +++ b/api/v1alpha1/gitopsset_types.go @@ -144,6 +144,12 @@ type APIClientGenerator struct { // Reference to Secret in same namespace with a field "caFile" which // provides the Certificate Authority to trust when making API calls. SecretRef *LocalObjectReference `json:"secretRef,omitempty"` + + // AllowClusterNetwork permits requests to loopback, private, and in-cluster + // addresses when true. If omitted or false, those destinations are denied. + // Link-local, unspecified, and multicast addresses are always denied. + // +optional + AllowClusterNetwork *bool `json:"allowClusterNetwork,omitempty"` } // HeadersReference references either a Secret or ConfigMap to be used for diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index ff70e9f6..d90bd040 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -33,6 +33,11 @@ func (in *APIClientGenerator) DeepCopyInto(out *APIClientGenerator) { *out = new(LocalObjectReference) **out = **in } + if in.AllowClusterNetwork != nil { + in, out := &in.AllowClusterNetwork, &out.AllowClusterNetwork + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new APIClientGenerator. diff --git a/config/crd/bases/sets.gitops.pro_gitopssets.yaml b/config/crd/bases/sets.gitops.pro_gitopssets.yaml index dc90caaa..f9cbc0bb 100644 --- a/config/crd/bases/sets.gitops.pro_gitopssets.yaml +++ b/config/crd/bases/sets.gitops.pro_gitopssets.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.19.0 + controller-gen.kubebuilder.io/version: v0.22.0 name: gitopssets.sets.gitops.pro spec: group: sets.gitops.pro @@ -63,6 +63,12 @@ spec: APIClientGenerator defines a generator that queries an API endpoint and uses that to generate data. properties: + allowClusterNetwork: + description: |- + AllowClusterNetwork permits requests to loopback, private, and in-cluster + addresses when true. If omitted or false, those destinations are denied. + Link-local, unspecified, and multicast addresses are always denied. + type: boolean body: description: |- Body is set as the body in a POST request. @@ -284,6 +290,12 @@ spec: APIClientGenerator defines a generator that queries an API endpoint and uses that to generate data. properties: + allowClusterNetwork: + description: |- + AllowClusterNetwork permits requests to loopback, private, and in-cluster + addresses when true. If omitted or false, those destinations are denied. + Link-local, unspecified, and multicast addresses are always denied. + type: boolean body: description: |- Body is set as the body in a POST request. diff --git a/controllers/gitopsset_controller.go b/controllers/gitopsset_controller.go index b14d9cc8..36f0286a 100644 --- a/controllers/gitopsset_controller.go +++ b/controllers/gitopsset_controller.go @@ -124,12 +124,6 @@ func (r *GitOpsSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{Requeue: true}, nil } - // Skip reconciliation if the GitOpsSet is suspended. - if gitOpsSet.Spec.Suspend { - logger.Info("Reconciliation is suspended for this GitOpsSet") - return ctrl.Result{}, nil - } - k8sClient := r.Client if gitOpsSet.Spec.ServiceAccountName != "" || r.DefaultServiceAccount != "" { serviceAccountName := r.DefaultServiceAccount @@ -147,6 +141,12 @@ func (r *GitOpsSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.finalize(ctx, &gitOpsSet, k8sClient) } + // Skip reconciliation if the GitOpsSet is suspended. + if gitOpsSet.Spec.Suspend { + logger.Info("Reconciliation is suspended for this GitOpsSet") + return ctrl.Result{}, nil + } + // Set the value of the reconciliation request in status. if v, ok := fluxMeta.ReconcileAnnotationValue(gitOpsSet.GetAnnotations()); ok { gitOpsSet.Status.LastHandledReconcileAt = v @@ -370,7 +370,7 @@ func (r *GitOpsSetReconciler) SetupWithManager(mgr ctrl.Manager) error { handler.EnqueueRequestsFromMapFunc(r.secretToGitOpsSet), ) - if r.Generators["Config"] != nil { + if r.Generators["Config"] != nil || r.Generators["APIClient"] != nil { builder.Watches( &corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.configMapToGitOpsSet), @@ -438,8 +438,7 @@ func (r *GitOpsSetReconciler) finalize(ctx context.Context, gs *templatesv1.GitO logger := ctrl.LoggerFrom(ctx) logger.Info("finalizing resources") - if !gs.Spec.Suspend && - gs.Status.Inventory != nil && + if gs.Status.Inventory != nil && gs.Status.Inventory.Entries != nil { if err := r.removeResourceRefs(ctx, k8sClient, gs.Status.Inventory.Entries); err != nil { @@ -623,27 +622,40 @@ func indexConfig(kind string) func(o client.Object) []string { panic(fmt.Sprintf("Expected a GitOpsSet, got %T", o)) } - referencedResources := []*templatesv1.ConfigGenerator{} - for _, gen := range ks.Spec.Generators { - if gen.Config != nil && gen.Config.Kind == kind { - referencedResources = append(referencedResources, gen.Config) + referencedNames := []string{} + addConfig := func(config *templatesv1.ConfigGenerator) { + if config != nil && config.Kind == kind { + referencedNames = append(referencedNames, fmt.Sprintf("%s/%s", ks.GetNamespace(), config.Name)) } - if gen.Matrix != nil && gen.Matrix.Generators != nil { - for _, matrixGen := range gen.Matrix.Generators { - if matrixGen.Config != nil && matrixGen.Config.Kind == kind { - referencedResources = append(referencedResources, matrixGen.Config) - } - } + } + addSecret := func(secret *templatesv1.LocalObjectReference) { + if kind == "Secret" && secret != nil { + referencedNames = append(referencedNames, fmt.Sprintf("%s/%s", ks.GetNamespace(), secret.Name)) } } - - if len(referencedResources) == 0 { - return nil + addHeaders := func(headers *templatesv1.HeadersReference) { + if headers != nil && headers.Kind == kind { + referencedNames = append(referencedNames, fmt.Sprintf("%s/%s", ks.GetNamespace(), headers.Name)) + } + } + addGeneratorReferences := func(pullRequests *templatesv1.PullRequestGenerator, apiClient *templatesv1.APIClientGenerator, config *templatesv1.ConfigGenerator) { + addConfig(config) + if pullRequests != nil { + addSecret(pullRequests.SecretRef) + } + if apiClient != nil { + addSecret(apiClient.SecretRef) + addHeaders(apiClient.HeadersRef) + } } - referencedNames := []string{} - for _, grg := range referencedResources { - referencedNames = append(referencedNames, fmt.Sprintf("%s/%s", ks.GetNamespace(), grg.Name)) + for _, gen := range ks.Spec.Generators { + addGeneratorReferences(gen.PullRequests, gen.APIClient, gen.Config) + if gen.Matrix != nil && gen.Matrix.Generators != nil { + for _, matrixGen := range gen.Matrix.Generators { + addGeneratorReferences(matrixGen.PullRequests, matrixGen.APIClient, matrixGen.Config) + } + } } return referencedNames diff --git a/controllers/gitopsset_controller_test.go b/controllers/gitopsset_controller_test.go index 78db2c48..55a0c7fb 100644 --- a/controllers/gitopsset_controller_test.go +++ b/controllers/gitopsset_controller_test.go @@ -1,6 +1,7 @@ package controllers import ( + "context" "encoding/json" "path/filepath" "sort" @@ -735,6 +736,35 @@ func TestReconciliation(t *testing.T) { assertNoKustomizationsExistInNamespace(t, k8sClient, "default") }) + t.Run("deleting a suspended set cleans its inventory", func(t *testing.T) { + ctx := t.Context() + gs := createAndReconcileToFinalizedState(t, k8sClient, reconciler, makeTestGitOpsSet(t)) + deleted := false + defer func() { + if !deleted { + deleteGitOpsSetAndFinalize(t, k8sClient, reconciler, gs) + } + }() + + _, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gs)}) + test.AssertNoError(t, err) + test.AssertNoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(gs), gs)) + assertKustomizationsExist(t, k8sClient, "default", "engineering-dev-demo", "engineering-prod-demo", "engineering-preprod-demo") + + gs.Spec.Suspend = true + test.AssertNoError(t, k8sClient.Update(ctx, gs)) + test.AssertNoError(t, k8sClient.Delete(ctx, gs)) + + _, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(gs)}) + test.AssertNoError(t, err) + + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(gs), &templatesv1.GitOpsSet{}); !apierrors.IsNotFound(err) { + t.Fatalf("GitOpsSet still exists after finalization: %v", err) + } + deleted = true + assertNoKustomizationsExistInNamespace(t, k8sClient, "default") + }) + t.Run("reconciling when gitrepository has no artifact", func(t *testing.T) { ctx := t.Context() emptyGR := test.NewGitRepository() @@ -887,6 +917,47 @@ func TestGetClusterSelectors(t *testing.T) { } } +func TestIndexConfigIncludesGeneratorReferences(t *testing.T) { + gitOpsSet := &templatesv1.GitOpsSet{ + ObjectMeta: metav1.ObjectMeta{Namespace: "demo"}, + Spec: templatesv1.GitOpsSetSpec{ + Generators: []templatesv1.GitOpsSetGenerator{ + { + PullRequests: &templatesv1.PullRequestGenerator{ + SecretRef: &templatesv1.LocalObjectReference{Name: "pr-token"}, + }, + APIClient: &templatesv1.APIClientGenerator{ + SecretRef: &templatesv1.LocalObjectReference{Name: "api-ca"}, + HeadersRef: &templatesv1.HeadersReference{Kind: "Secret", Name: "api-auth"}, + }, + }, + { + Matrix: &templatesv1.MatrixGenerator{ + Generators: []templatesv1.GitOpsSetNestedGenerator{ + { + PullRequests: &templatesv1.PullRequestGenerator{ + SecretRef: &templatesv1.LocalObjectReference{Name: "matrix-pr-token"}, + }, + APIClient: &templatesv1.APIClientGenerator{ + SecretRef: &templatesv1.LocalObjectReference{Name: "matrix-api-ca"}, + HeadersRef: &templatesv1.HeadersReference{Kind: "ConfigMap", Name: "matrix-api-headers"}, + }, + }, + }, + }, + }, + }, + }, + } + + if got, want := indexConfig("Secret")(gitOpsSet), []string{"demo/pr-token", "demo/api-ca", "demo/api-auth", "demo/matrix-pr-token", "demo/matrix-api-ca"}; cmp.Diff(want, got) != "" { + t.Fatalf("Secret index mismatch (-want +got):\n%s", cmp.Diff(want, got)) + } + if got, want := indexConfig("ConfigMap")(gitOpsSet), []string{"demo/matrix-api-headers"}; cmp.Diff(want, got) != "" { + t.Fatalf("ConfigMap index mismatch (-want +got):\n%s", cmp.Diff(want, got)) + } +} + func TestMatchCluster(t *testing.T) { gitopsCluster := &clustersv1.GitopsCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -1135,7 +1206,9 @@ func assertInventoryHasNoItems(t *testing.T, gs *templatesv1.GitOpsSet) { func deleteObject(t *testing.T, cl client.Client, obj client.Object) { t.Helper() - if err := cl.Delete(t.Context(), obj); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := cl.Delete(ctx, obj); err != nil { t.Fatal(err) } } diff --git a/controllers/templates/renderer.go b/controllers/templates/renderer.go index 626d8d91..657c6b3d 100644 --- a/controllers/templates/renderer.go +++ b/controllers/templates/renderer.go @@ -161,6 +161,9 @@ func renderTemplateParams(index int, tmpl templatesv1.GitOpsSetTemplate, params return nil, fmt.Errorf("failed convert parsed template: %w", err) } delete(unstructuredMap, "status") + if metadata, ok := unstructuredMap["metadata"].(map[string]any); ok && metadata["creationTimestamp"] == nil { + delete(metadata, "creationTimestamp") + } uns := &unstructured.Unstructured{Object: unstructuredMap} if IsNamespacedObject(uns) { @@ -240,6 +243,9 @@ func makeTemplateFunctions() template.FuncMap { f := sprig.TxtFuncMap() unwanted := []string{ "env", "expandenv", "getHostByName", "genPrivateKey", "derivePassword", "sha256sum", + "genCA", "genSelfSignedCert", "genSignedCert", "genCAWithKey", "genSelfSignedCertWithKey", "genSignedCertWithKey", + "bcrypt", "htpasswd", "uuidv4", "now", "date", "dateInZone", "dateModify", "ago", "unixEpoch", + "randInt", "randBytes", "randNumeric", "randAlpha", "randAlphaNum", "randAscii", "base", "dir", "ext", "clean", "isAbs", "osBase", "osDir", "osExt", "osClean", "osIsAbs"} for _, v := range unwanted { @@ -254,13 +260,12 @@ func makeTemplateFunctions() template.FuncMap { return def } - f["toYaml"] = func(v interface{}) string { + f["toYaml"] = func(v interface{}) (string, error) { data, err := syaml.Marshal(v) if err != nil { - // Swallow errors inside of a template. - return "" + return "", err } - return strings.TrimSuffix(string(data), "\n") + return strings.TrimSuffix(string(data), "\n"), nil } return f diff --git a/controllers/templates/renderer_test.go b/controllers/templates/renderer_test.go index 902f8122..43b53548 100644 --- a/controllers/templates/renderer_test.go +++ b/controllers/templates/renderer_test.go @@ -29,6 +29,41 @@ const ( testNS = "demo" ) +func TestTemplateFunctionsAreStable(t *testing.T) { + for _, name := range []string{ + "genCA", "genSelfSignedCert", "genSignedCert", "genCAWithKey", "genSelfSignedCertWithKey", "genSignedCertWithKey", + "bcrypt", "htpasswd", "uuidv4", "now", "date", "dateInZone", "dateModify", "ago", "unixEpoch", + "randInt", "randBytes", "randNumeric", "randAlpha", "randAlphaNum", "randAscii", + } { + if _, ok := makeTemplateFunctions()[name]; ok { + t.Errorf("template function %q should be unavailable", name) + } + } +} + +func TestToYAMLReturnsMarshalErrors(t *testing.T) { + _, err := render([]byte(`value: {{ toYaml .Value }}`), map[string]any{"Value": make(chan int)}, templatesv1.GitOpsSet{}) + if err == nil { + t.Fatal("render() succeeded for a value that cannot be marshaled as YAML") + } +} + +func TestRenderTemplateParamsDropsNullCreationTimestamp(t *testing.T) { + objects, err := renderTemplateParams(0, templatesv1.GitOpsSetTemplate{ + Content: runtime.RawExtension{Raw: []byte(`{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"example","creationTimestamp":null}}`)}, + }, map[string]any{}, templatesv1.GitOpsSet{ObjectMeta: metav1.ObjectMeta{Name: "example-set", Namespace: "demo"}}) + if err != nil { + t.Fatal(err) + } + metadata, ok := objects[0].Object["metadata"].(map[string]any) + if !ok { + t.Fatalf("metadata has unexpected type: %T", objects[0].Object["metadata"]) + } + if _, exists := metadata["creationTimestamp"]; exists { + t.Fatal("rendered metadata still contains creationTimestamp") + } +} + func TestRender(t *testing.T) { testGenerators := map[string]generators.Generator{ "List": list.NewGenerator(logr.Discard()), diff --git a/docs/api/gitopsset.md b/docs/api/gitopsset.md index 8d0ab951..fac2d444 100644 --- a/docs/api/gitopsset.md +++ b/docs/api/gitopsset.md @@ -266,6 +266,20 @@ LocalObjectReference provides the Certificate Authority to trust when making API calls.

+ + +allowClusterNetwork
+ +bool + + + +(Optional) +

AllowClusterNetwork permits requests to loopback, private, and in-cluster +addresses when true. If omitted or false, those destinations are denied. +Link-local, unspecified, and multicast addresses are always denied.

+ +

ClusterGenerator diff --git a/pkg/generators/apiclient/address_policy.go b/pkg/generators/apiclient/address_policy.go new file mode 100644 index 00000000..28170eb4 --- /dev/null +++ b/pkg/generators/apiclient/address_policy.go @@ -0,0 +1,120 @@ +package apiclient + +import ( + "context" + "fmt" + "net" + "net/http" + "net/url" + "strings" + "time" +) + +func validateEndpointAddress(ctx context.Context, endpoint *url.URL, allowClusterNetwork bool) error { + host := strings.TrimSuffix(strings.ToLower(endpoint.Hostname()), ".") + if host == "" { + return fmt.Errorf("refusing API endpoint with an empty host") + } + if isClusterDNS(host) { + if !allowClusterNetwork { + return fmt.Errorf("refusing cluster address %s", host) + } + return nil + } + if ip := net.ParseIP(host); ip != nil { + return checkEndpointIP(ip, allowClusterNetwork, host) + } + + ips, err := net.DefaultResolver.LookupIP(ctx, "ip", host) + if err != nil { + return fmt.Errorf("failed to resolve API endpoint host %s: %w", host, err) + } + if len(ips) == 0 { + return fmt.Errorf("API endpoint host %s resolved to no addresses", host) + } + for _, ip := range ips { + if err := checkEndpointIP(ip, allowClusterNetwork, host); err != nil { + return err + } + } + return nil +} + +func isClusterDNS(host string) bool { + return host == "kubernetes" || strings.HasSuffix(host, ".svc") || strings.HasSuffix(host, ".cluster.local") +} + +func checkEndpointIP(ip net.IP, allowClusterNetwork bool, host string) error { + if ip == nil || ip.IsUnspecified() || ip.IsMulticast() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { + return fmt.Errorf("refusing restricted API endpoint address %s", host) + } + if ip.IsPrivate() || ip.IsLoopback() { + if !allowClusterNetwork { + return fmt.Errorf("refusing private or loopback API endpoint address %s", host) + } + } + if !ip.IsGlobalUnicast() && !ip.IsPrivate() && !ip.IsLoopback() { + return fmt.Errorf("refusing non-unicast API endpoint address %s", host) + } + return nil +} + +func applyAddressPolicy(httpClient *http.Client, allowClusterNetwork bool) error { + if httpClient == nil { + return fmt.Errorf("API client factory returned a nil HTTP client") + } + + transport := http.DefaultTransport.(*http.Transport) + if httpClient.Transport != nil { + var ok bool + transport, ok = httpClient.Transport.(*http.Transport) + if !ok { + return fmt.Errorf("API client transport %T cannot enforce destination policy", httpClient.Transport) + } + } + transport = transport.Clone() + // A proxy hides the request destination from the dialer, so policy-enforced + // clients connect directly to the validated endpoint. + transport.Proxy = nil + + baseDial := transport.DialContext + if baseDial == nil { + dialer := &net.Dialer{Timeout: 30 * time.Second} + baseDial = dialer.DialContext + } + transport.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { + conn, err := baseDial(ctx, network, address) + if err != nil { + return nil, err + } + remote, ok := conn.RemoteAddr().(*net.TCPAddr) + if !ok { + conn.Close() + return nil, fmt.Errorf("refusing non-TCP API endpoint connection") + } + if err := checkEndpointIP(remote.IP, allowClusterNetwork, remote.IP.String()); err != nil { + conn.Close() + return nil, err + } + return conn, nil + } + httpClient.Transport = transport + + previousCheckRedirect := httpClient.CheckRedirect + httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) > 0 && !strings.EqualFold(req.URL.Host, via[0].URL.Host) { + return fmt.Errorf("refusing API endpoint redirect to a different host") + } + if err := validateEndpointAddress(req.Context(), req.URL, allowClusterNetwork); err != nil { + return err + } + if previousCheckRedirect != nil { + return previousCheckRedirect(req, via) + } + if len(via) >= 10 { + return fmt.Errorf("stopped after 10 API endpoint redirects") + } + return nil + } + return nil +} diff --git a/pkg/generators/apiclient/address_policy_test.go b/pkg/generators/apiclient/address_policy_test.go new file mode 100644 index 00000000..c3164df5 --- /dev/null +++ b/pkg/generators/apiclient/address_policy_test.go @@ -0,0 +1,138 @@ +package apiclient + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/go-logr/logr" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + templatesv1 "github.com/gitops-tools/gitopssets-controller/api/v1alpha1" +) + +func TestValidateEndpointAddress(t *testing.T) { + testCases := []struct { + name string + endpoint string + allowCluster bool + wantErrorSubstring string + }{ + {name: "public address", endpoint: "https://8.8.8.8"}, + {name: "private address denied", endpoint: "https://10.0.0.1", wantErrorSubstring: "private"}, + {name: "private address allowed explicitly", endpoint: "https://10.0.0.1", allowCluster: true}, + {name: "loopback denied", endpoint: "http://127.0.0.1", wantErrorSubstring: "loopback"}, + {name: "loopback allowed explicitly", endpoint: "http://127.0.0.1", allowCluster: true}, + {name: "link local always denied", endpoint: "http://169.254.169.254", allowCluster: true, wantErrorSubstring: "restricted"}, + {name: "unspecified always denied", endpoint: "http://0.0.0.0", allowCluster: true, wantErrorSubstring: "restricted"}, + {name: "multicast always denied", endpoint: "http://224.0.0.1", allowCluster: true, wantErrorSubstring: "restricted"}, + {name: "cluster DNS denied", endpoint: "https://kubernetes.default.svc", wantErrorSubstring: "cluster"}, + {name: "cluster DNS allowed explicitly", endpoint: "https://kubernetes.default.svc", allowCluster: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + endpoint, err := url.Parse(tc.endpoint) + if err != nil { + t.Fatal(err) + } + err = validateEndpointAddress(context.Background(), endpoint, tc.allowCluster) + if tc.wantErrorSubstring == "" { + if err != nil { + t.Fatalf("validateEndpointAddress() error = %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tc.wantErrorSubstring) { + t.Fatalf("validateEndpointAddress() error = %v, want containing %q", err, tc.wantErrorSubstring) + } + }) + } +} + +func TestAllowsClusterNetworkDefaultsToFalse(t *testing.T) { + if allowsClusterNetwork(nil) { + t.Fatal("nil API client allowed cluster network access") + } + if allowsClusterNetwork(&templatesv1.APIClientGenerator{}) { + t.Fatal("omitted allowClusterNetwork allowed cluster network access") + } + allow := false + if allowsClusterNetwork(&templatesv1.APIClientGenerator{AllowClusterNetwork: &allow}) { + t.Fatal("explicit false allowed cluster network access") + } + allow = true + if !allowsClusterNetwork(&templatesv1.APIClientGenerator{AllowClusterNetwork: &allow}) { + t.Fatal("explicit true did not allow cluster network access") + } +} + +func TestGenerateRejectsPrivateEndpointBeforeRequest(t *testing.T) { + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + _, _ = w.Write([]byte("[]")) + })) + defer server.Close() + + gen := NewGenerator(logr.Discard(), fake.NewFakeClient(), DefaultClientFactory) + _, err := gen.Generate(t.Context(), &templatesv1.GitOpsSetGenerator{ + APIClient: &templatesv1.APIClientGenerator{Endpoint: server.URL}, + }, &templatesv1.GitOpsSet{}) + if err == nil || !strings.Contains(err.Error(), "private or loopback") { + t.Fatalf("Generate() error = %v, want private endpoint error", err) + } + if requests != 0 { + t.Fatalf("server received %d requests, want none", requests) + } +} + +func TestAddressPolicyRejectsPrivateDialPeer(t *testing.T) { + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + _, _ = w.Write([]byte("[]")) + })) + defer server.Close() + + httpClient := &http.Client{Transport: http.DefaultTransport.(*http.Transport).Clone()} + if err := applyAddressPolicy(httpClient, false); err != nil { + t.Fatal(err) + } + _, err := httpClient.Get(server.URL) + if err == nil || !strings.Contains(err.Error(), "private or loopback") { + t.Fatalf("Get() error = %v, want private peer error", err) + } + if requests != 0 { + t.Fatalf("server received %d requests, want none", requests) + } +} + +func TestAddressPolicyRejectsCrossHostRedirect(t *testing.T) { + targetRequests := 0 + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + targetRequests++ + _, _ = w.Write([]byte("[]")) + })) + defer target.Close() + + redirect := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + http.Redirect(w, req, target.URL, http.StatusFound) + })) + defer redirect.Close() + + allowClusterNetwork := true + gen := NewGenerator(logr.Discard(), fake.NewFakeClient(), DefaultClientFactory) + _, err := gen.Generate(t.Context(), &templatesv1.GitOpsSetGenerator{ + APIClient: &templatesv1.APIClientGenerator{Endpoint: redirect.URL, AllowClusterNetwork: &allowClusterNetwork}, + }, &templatesv1.GitOpsSet{}) + if err == nil || !strings.Contains(err.Error(), "different host") { + t.Fatalf("Generate() error = %v, want cross-host redirect error", err) + } + if targetRequests != 0 { + t.Fatalf("redirect target received %d requests, want none", targetRequests) + } +} diff --git a/pkg/generators/apiclient/api_client.go b/pkg/generators/apiclient/api_client.go index 3114e01f..b2c9387f 100644 --- a/pkg/generators/apiclient/api_client.go +++ b/pkg/generators/apiclient/api_client.go @@ -85,6 +85,9 @@ func (g *APIClientGenerator) Generate(ctx context.Context, sg *templatesv1.GitOp } client := g.ClientFactory(tlsConfig) + if err := applyAddressPolicy(client, allowsClusterNetwork(sg.APIClient)); err != nil { + return nil, err + } resp, err := client.Do(req) if err != nil { @@ -137,7 +140,7 @@ func (g *APIClientGenerator) createRequest(ctx context.Context, ac *templatesv1. body = bytes.NewReader(ac.Body.Raw) } - // Validate scheme to http/https only and ensure URL parses correctly. + // Validate scheme and destination before loading credentials or sending a request. u, err := url.Parse(ac.Endpoint) if err != nil { return nil, fmt.Errorf("invalid endpoint URL %q: %w", ac.Endpoint, err) @@ -145,6 +148,9 @@ func (g *APIClientGenerator) createRequest(ctx context.Context, ac *templatesv1. if u.Scheme != "http" && u.Scheme != "https" { return nil, fmt.Errorf("unsupported URL scheme %q for endpoint %s", u.Scheme, ac.Endpoint) } + if err := validateEndpointAddress(ctx, u, allowsClusterNetwork(ac)); err != nil { + return nil, err + } req, err := http.NewRequestWithContext(ctx, method, ac.Endpoint, body) if err != nil { @@ -167,6 +173,10 @@ func (g *APIClientGenerator) createRequest(ctx context.Context, ac *templatesv1. return req, nil } +func allowsClusterNetwork(apiClient *templatesv1.APIClientGenerator) bool { + return apiClient != nil && apiClient.AllowClusterNetwork != nil && *apiClient.AllowClusterNetwork +} + func (g *APIClientGenerator) createTLSConfig(ctx context.Context, ac *templatesv1.APIClientGenerator, namespace string) (*tls.Config, error) { if ac.SecretRef == nil { return nil, nil diff --git a/pkg/generators/apiclient/api_client_test.go b/pkg/generators/apiclient/api_client_test.go index 556202bc..52dea601 100644 --- a/pkg/generators/apiclient/api_client_test.go +++ b/pkg/generators/apiclient/api_client_test.go @@ -219,6 +219,8 @@ func TestGenerate(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { + allowClusterNetwork := true + tt.apiClient.AllowClusterNetwork = &allowClusterNetwork factory := func(_ *tls.Config) *http.Client { return ts.Client() } @@ -342,6 +344,8 @@ func TestGenerate_errors(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { + allowClusterNetwork := true + tt.apiClient.AllowClusterNetwork = &allowClusterNetwork factory := func(_ *tls.Config) *http.Client { return ts.Client() } diff --git a/pkg/generators/pullrequests/pull_requests.go b/pkg/generators/pullrequests/pull_requests.go index 7c2cf893..07477910 100644 --- a/pkg/generators/pullrequests/pull_requests.go +++ b/pkg/generators/pullrequests/pull_requests.go @@ -80,9 +80,9 @@ func (g *PullRequestGenerator) Generate(ctx context.Context, sg *templatesv1.Git return nil, fmt.Errorf("failed to create client: %w", err) } - prs, _, err := scmClient.PullRequests.List(ctx, sg.PullRequests.Repo, listOptionsFromConfig(sg.PullRequests)) + prs, err := g.listPullRequests(ctx, scmClient, sg.PullRequests) if err != nil { - return nil, fmt.Errorf("failed to list pull requests: %w", err) + return nil, err } g.Logger.Info("queried pull requests", "repo", sg.PullRequests.Repo, "count", len(prs)) @@ -120,6 +120,31 @@ func (g *PullRequestGenerator) Interval(sg *templatesv1.GitOpsSetGenerator) time return sg.PullRequests.Interval.Duration } +const maxPullRequestPages = 50 + +func (g *PullRequestGenerator) listPullRequests(ctx context.Context, scmClient *scm.Client, config *templatesv1.PullRequestGenerator) ([]*scm.PullRequest, error) { + options := listOptionsFromConfig(config) + options.Page = 1 + allPullRequests := []*scm.PullRequest{} + + for range maxPullRequestPages { + pullRequests, response, err := scmClient.PullRequests.List(ctx, config.Repo, options) + if err != nil { + return nil, fmt.Errorf("failed to list pull requests: %w", err) + } + allPullRequests = append(allPullRequests, pullRequests...) + if response == nil || response.Page.Next == 0 { + return allPullRequests, nil + } + if response.Page.Next <= options.Page { + return nil, fmt.Errorf("failed to list pull requests: provider returned non-advancing page %d after %d", response.Page.Next, options.Page) + } + options.Page = response.Page.Next + } + + return nil, fmt.Errorf("failed to list pull requests: exceeded %d pages", maxPullRequestPages) +} + // label filtering is only supported by GitLab (that I'm aware of) // The fetched PRs are filtered on labels across all providers, but providing // the labels optimises the load from GitLab. diff --git a/pkg/generators/pullrequests/pull_requests_test.go b/pkg/generators/pullrequests/pull_requests_test.go index 954e0523..27d84ea5 100644 --- a/pkg/generators/pullrequests/pull_requests_test.go +++ b/pkg/generators/pullrequests/pull_requests_test.go @@ -1,7 +1,9 @@ package pullrequests import ( + "context" "fmt" + "strings" "testing" "time" @@ -326,6 +328,77 @@ func TestGenerate(t *testing.T) { } } +func TestGenerateFollowsPullRequestPages(t *testing.T) { + client, _ := fakescm.NewDefault() + service := &paginatedPullRequestService{ + pages: [][]*scm.PullRequest{ + {{Number: 1, Fork: "test-org/my-repo"}}, + {{Number: 2, Fork: "test-org/my-repo"}}, + }, + } + client.PullRequests = service + gen := NewGenerator(logr.Discard(), fake.NewFakeClient()) + gen.clientFactory = func(_, _, _ string, _ ...factory.ClientOptionFunc) (*scm.Client, error) { + return client, nil + } + + generator := &templatesv1.GitOpsSetGenerator{PullRequests: &templatesv1.PullRequestGenerator{ + Driver: "fake", + Repo: "test-org/my-repo", + }} + got, err := gen.Generate(t.Context(), generator, &templatesv1.GitOpsSet{}) + test.AssertNoError(t, err) + if len(got) != 2 || got[0]["Number"] != "1" || got[1]["Number"] != "2" { + t.Fatalf("generated pull requests = %#v, want PRs 1 and 2", got) + } + if diff := cmp.Diff([]int{1, 2}, service.calls); diff != "" { + t.Fatalf("requested pages mismatch (-want +got):\n%s", diff) + } +} + +func TestGenerateStopsAtPullRequestPageLimit(t *testing.T) { + client, _ := fakescm.NewDefault() + service := &paginatedPullRequestService{alwaysNext: true} + client.PullRequests = service + gen := NewGenerator(logr.Discard(), fake.NewFakeClient()) + gen.clientFactory = func(_, _, _ string, _ ...factory.ClientOptionFunc) (*scm.Client, error) { + return client, nil + } + + generator := &templatesv1.GitOpsSetGenerator{PullRequests: &templatesv1.PullRequestGenerator{ + Driver: "fake", + Repo: "test-org/my-repo", + }} + _, err := gen.Generate(t.Context(), generator, &templatesv1.GitOpsSet{}) + if err == nil || !strings.Contains(err.Error(), "exceeded 50 pages") { + t.Fatalf("Generate() error = %v, want page-limit error", err) + } + if len(service.calls) != 50 { + t.Fatalf("requested %d pages, want 50", len(service.calls)) + } +} + +type paginatedPullRequestService struct { + scm.PullRequestService + pages [][]*scm.PullRequest + calls []int + alwaysNext bool +} + +func (s *paginatedPullRequestService) List(_ context.Context, _ string, options *scm.PullRequestListOptions) ([]*scm.PullRequest, *scm.Response, error) { + page := options.Page + s.calls = append(s.calls, page) + var pulls []*scm.PullRequest + if page > 0 && page <= len(s.pages) { + pulls = s.pages[page-1] + } + response := &scm.Response{} + if s.alwaysNext || page < len(s.pages) { + response.Page.Next = page + 1 + } + return pulls, response, nil +} + func TestGenerate_errors(t *testing.T) { testCases := []struct { name string diff --git a/tests/e2e/gitopsset_controller_test.go b/tests/e2e/gitopsset_controller_test.go index d8c786a0..88f71c72 100644 --- a/tests/e2e/gitopsset_controller_test.go +++ b/tests/e2e/gitopsset_controller_test.go @@ -361,7 +361,7 @@ func TestReconcilingUpdatingImagePolicy_in_matrix(t *testing.T) { ctx := t.Context() ip := test.NewImagePolicy() - test.AssertNoError(t, testEnv.Create(ctx, test.ToUnstructured(t, ip))) + test.AssertNoError(t, testEnv.CreateAndWait(ctx, test.ToUnstructured(t, ip))) defer deleteObject(t, testEnv, ip) test.AssertNoError(t, testEnv.Get(ctx, client.ObjectKeyFromObject(ip), ip)) diff --git a/tests/e2e/main_test.go b/tests/e2e/main_test.go index 8a3e4795..bc3975a7 100644 --- a/tests/e2e/main_test.go +++ b/tests/e2e/main_test.go @@ -16,6 +16,7 @@ import ( sourcev1 "github.com/fluxcd/source-controller/api/v1" sourcev1beta2 "github.com/fluxcd/source-controller/api/v1beta2" "github.com/gitops-tools/gitopssets-controller/test" + "github.com/go-logr/logr" clustersv1 "github.com/weaveworks/cluster-controller/api/v1alpha1" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes/scheme" @@ -54,6 +55,7 @@ func TestMain(m *testing.M) { utilruntime.Must(imagev1.AddToScheme(scheme.Scheme)) utilruntime.Must(kustomizev1.AddToScheme(scheme.Scheme)) fetcher := fetch.NewArchiveFetcher(1, tar.UnlimitedUntarSize, tar.UnlimitedUntarSize, "") + ctrl.SetLogger(logr.Discard()) testEnv = testenv.New(testenv.WithCRDPath(filepath.Join("..", "..", "config", "crd", "bases"), filepath.Join("..", "..", "controllers", "testdata", "crds"), diff --git a/tests/e2e/output.log b/tests/e2e/output.log new file mode 100644 index 00000000..e69de29b