From ea76aade3580d6d48fc2649a8e44ba7e1ee2941f Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Fri, 5 Jul 2024 08:22:21 +0200 Subject: [PATCH] Configurable git client timeout Fixes #2188 Also fixes an issue in the config reconciler which has prevented the config from being reloaded when it was changed. --- charts/fleet/templates/configmap.yaml | 3 ++- charts/fleet/values.yaml | 4 ++++ .../gitjob/controller/controller_test.go | 4 ++++ integrationtests/gitjob/git/git_test.go | 10 ++++---- internal/cmd/controller/gitops/operator.go | 23 ++++++++++++++++--- .../reconciler/config_controller.go | 1 + internal/config/config.go | 5 ++++ pkg/git/fetch.go | 2 ++ pkg/git/fetch_test.go | 4 ++++ pkg/git/netutils.go | 6 ++--- pkg/git/netutils_test.go | 17 ++++++++------ pkg/git/remote.go | 2 ++ pkg/git/vendor.go | 7 +++++- pkg/git/vendor_test.go | 10 ++++---- 14 files changed, 73 insertions(+), 25 deletions(-) diff --git a/charts/fleet/templates/configmap.yaml b/charts/fleet/templates/configmap.yaml index a801cb6024..4477402111 100644 --- a/charts/fleet/templates/configmap.yaml +++ b/charts/fleet/templates/configmap.yaml @@ -22,5 +22,6 @@ data: "agentNamespace": "{{.Values.bootstrap.agentNamespace}}" }, "webhookReceiverURL": "{{.Values.webhookReceiverURL}}", - "githubURLPrefix": "{{.Values.githubURLPrefix}}" + "githubURLPrefix": "{{.Values.githubURLPrefix}}", + "gitClientTimeout": "{{.Values.gitClientTimeout}}" } diff --git a/charts/fleet/values.yaml b/charts/fleet/values.yaml index 8ce206f4ce..1f08168609 100644 --- a/charts/fleet/values.yaml +++ b/charts/fleet/values.yaml @@ -37,6 +37,10 @@ ignoreClusterRegistrationLabels: false # comma separated list of domains or ip addresses that will not use the proxy noProxy: 127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,.svc,.cluster.local +# The amount of time to wait for a response from the server before canceling the +# request. Used to retrieve the latest commit of configured git repositories. +gitClientTimeout: 30s + bootstrap: enabled: true # The namespace that will be autocreated and the local cluster will be registered in diff --git a/integrationtests/gitjob/controller/controller_test.go b/integrationtests/gitjob/controller/controller_test.go index 7c73fe2305..20ac22ffa4 100644 --- a/integrationtests/gitjob/controller/controller_test.go +++ b/integrationtests/gitjob/controller/controller_test.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/rancher/fleet/integrationtests/utils" + "github.com/rancher/fleet/internal/config" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/wrangler/v3/pkg/name" @@ -43,6 +44,9 @@ var _ = Describe("GitJob controller", func() { ) JustBeforeEach(func() { + config.Set(&config.Config{ + GitClientTimeout: metav1.Duration{Duration: 10 * time.Second}, + }) gitRepo = createGitRepo(gitRepoName) Expect(k8sClient.Create(ctx, &gitRepo)).ToNot(HaveOccurred()) Eventually(func() string { diff --git a/integrationtests/gitjob/git/git_test.go b/integrationtests/gitjob/git/git_test.go index a633fdbf64..c2989ea037 100644 --- a/integrationtests/gitjob/git/git_test.go +++ b/integrationtests/gitjob/git/git_test.go @@ -17,7 +17,7 @@ import ( dockercontainer "github.com/docker/docker/api/types/container" dockermount "github.com/docker/docker/api/types/mount" gogit "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/config" + gitconfig "github.com/go-git/go-git/v5/config" "github.com/go-git/go-git/v5/plumbing/object" "github.com/go-git/go-git/v5/plumbing/transport" httpgit "github.com/go-git/go-git/v5/plumbing/transport/http" @@ -30,6 +30,7 @@ import ( "go.uber.org/mock/gomock" "golang.org/x/crypto/ssh" + "github.com/rancher/fleet/internal/config" "github.com/rancher/fleet/internal/mocks" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/fleet/pkg/git" @@ -111,6 +112,7 @@ func TestLatestCommit_NoAuth(t *testing.T) { }, } + config.Set(&config.Config{GitClientTimeout: metav1.Duration{Duration: 30 * time.Second}}) for name, test := range tests { t.Run(name, func(t *testing.T) { f := git.Fetch{} @@ -466,7 +468,7 @@ func initRepo(url string, name string, private bool) (string, error) { if err != nil { return "", err } - cfg.Remotes["upstream"] = &config.RemoteConfig{ + cfg.Remotes["upstream"] = &gitconfig.RemoteConfig{ Name: "upstream", URLs: []string{repoURL}, } @@ -529,7 +531,7 @@ func addRepoCommitAndTag(url string, name string, tag string, tagMessage string) if err != nil { return "", err } - cfg.Remotes["upstream"] = &config.RemoteConfig{ + cfg.Remotes["upstream"] = &gitconfig.RemoteConfig{ Name: "upstream", URLs: []string{url}, } @@ -574,7 +576,7 @@ func addRepoCommitAndTag(url string, name string, tag string, tagMessage string) Username: gogsUser, Password: gogsPass, }, - RefSpecs: []config.RefSpec{config.RefSpec("refs/tags/*:refs/tags/*")}, + RefSpecs: []gitconfig.RefSpec{gitconfig.RefSpec("refs/tags/*:refs/tags/*")}, }) if err != nil { return "", nil diff --git a/internal/cmd/controller/gitops/operator.go b/internal/cmd/controller/gitops/operator.go index b22d04f155..133b32d03f 100644 --- a/internal/cmd/controller/gitops/operator.go +++ b/internal/cmd/controller/gitops/operator.go @@ -10,6 +10,7 @@ import ( command "github.com/rancher/fleet/internal/cmd" "github.com/rancher/fleet/internal/cmd/controller/gitops/reconciler" + fcreconciler "github.com/rancher/fleet/internal/cmd/controller/reconciler" "github.com/rancher/fleet/internal/metrics" fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/fleet/pkg/git/poll" @@ -121,7 +122,7 @@ func (g *GitOperator) Run(cmd *cobra.Command, args []string) error { workers = w } - reconciler := &reconciler.GitJobReconciler{ + gitJobReconciler := &reconciler.GitJobReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Image: g.Image, @@ -131,14 +132,30 @@ func (g *GitOperator) Run(cmd *cobra.Command, args []string) error { ShardID: g.ShardID, } + configReconciler := &fcreconciler.ConfigReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + SystemNamespace: namespace, + ShardID: g.ShardID, + } + + if err := fcreconciler.Load(ctx, mgr.GetAPIReader(), namespace); err != nil { + setupLog.Error(err, "failed to load config") + return err + } + group, ctx := errgroup.WithContext(ctx) group.Go(func() error { return startWebhook(ctx, namespace, g.Listen, mgr.GetClient(), mgr.GetCache()) }) group.Go(func() error { - setupLog.Info("starting manager") + setupLog.Info("starting config manager") + if err = configReconciler.SetupWithManager(mgr); err != nil { + return err + } - if err = reconciler.SetupWithManager(mgr); err != nil { + setupLog.Info("starting gitops manager") + if err = gitJobReconciler.SetupWithManager(mgr); err != nil { return err } diff --git a/internal/cmd/controller/reconciler/config_controller.go b/internal/cmd/controller/reconciler/config_controller.go index 84bb779adc..e1efa47e2e 100644 --- a/internal/cmd/controller/reconciler/config_controller.go +++ b/internal/cmd/controller/reconciler/config_controller.go @@ -58,6 +58,7 @@ func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { object.GetName() == config.ManagerConfigName }), predicate.Or( + predicate.ResourceVersionChangedPredicate{}, predicate.GenerationChangedPredicate{}, predicate.AnnotationChangedPredicate{}, predicate.LabelChangedPredicate{}, diff --git a/internal/config/config.go b/internal/config/config.go index 8629327759..d7f92fe9a0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -108,6 +108,11 @@ type Config struct { // to trust CA bundles from the operating system's store. If set to `strict`, then the agent shall only connect // to a server which uses the exact CA configured when creating/updating the agent. AgentTLSMode string `json:"agentTLSMode,omitempty"` + + // The amount of time to wait for a response from the server before + // canceling the request. Used to retrieve the latest commit of configured + // git repositories. + GitClientTimeout metav1.Duration `json:"gitClientTimeout,omitempty"` } type Bootstrap struct { diff --git a/pkg/git/fetch.go b/pkg/git/fetch.go index 708cf73d9d..b141973508 100644 --- a/pkg/git/fetch.go +++ b/pkg/git/fetch.go @@ -3,6 +3,7 @@ package git import ( "context" + "github.com/rancher/fleet/internal/config" v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" corev1 "k8s.io/api/core/v1" @@ -46,6 +47,7 @@ func (f *Fetch) LatestCommit(ctx context.Context, gitrepo *v1alpha1.GitRepo, cli CABundle: gitrepo.Spec.CABundle, Credential: &secret, InsecureTLSVerify: gitrepo.Spec.InsecureSkipTLSverify, + Timeout: config.Get().GitClientTimeout.Duration, log: log.FromContext(ctx), }) if err != nil { diff --git a/pkg/git/fetch_test.go b/pkg/git/fetch_test.go index 8aa4f437cf..f9341529ea 100644 --- a/pkg/git/fetch_test.go +++ b/pkg/git/fetch_test.go @@ -8,6 +8,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/rancher/fleet/internal/config" fleetv1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1" "github.com/rancher/fleet/pkg/git" corev1 "k8s.io/api/core/v1" @@ -76,6 +77,9 @@ var _ = Describe("git fetch's LatestCommit tests", func() { }) It("returns the commit for the expected revision", func() { + config.Set(&config.Config{ + GitClientTimeout: metav1.Duration{Duration: 0}, + }) secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "test-secret", diff --git a/pkg/git/netutils.go b/pkg/git/netutils.go index 2b399dac89..be7c0b5e6e 100644 --- a/pkg/git/netutils.go +++ b/pkg/git/netutils.go @@ -16,8 +16,6 @@ import ( corev1 "k8s.io/api/core/v1" ) -var clientTimeout = time.Duration(30) - // GetAuthFromSecret returns the AuthMethod calculated from the given secret // The credentials secret is expected to be either basic-auth or ssh-auth (with extra known_hosts data option) func GetAuthFromSecret(url string, creds *corev1.Secret) (transport.AuthMethod, error) { @@ -59,7 +57,7 @@ func GetAuthFromSecret(url string, creds *corev1.Secret) (transport.AuthMethod, // GetHTTPClientFromSecret returns a HTTP client filled from the information in the given secret // and optional CABundle and insecureTLSVerify -func GetHTTPClientFromSecret(creds *corev1.Secret, CABundle []byte, insecureTLSVerify bool) (*http.Client, error) { +func GetHTTPClientFromSecret(creds *corev1.Secret, CABundle []byte, insecureTLSVerify bool, timeout time.Duration) (*http.Client, error) { var ( username string password string @@ -99,7 +97,7 @@ func GetHTTPClientFromSecret(creds *corev1.Secret, CABundle []byte, insecureTLSV client := &http.Client{ Transport: transport, - Timeout: clientTimeout * time.Second, + Timeout: timeout, } if username != "" || password != "" { client.Transport = &basicRoundTripper{ diff --git a/pkg/git/netutils_test.go b/pkg/git/netutils_test.go index 65064bf6a0..331f782a92 100644 --- a/pkg/git/netutils_test.go +++ b/pkg/git/netutils_test.go @@ -3,6 +3,7 @@ package git_test import ( "encoding/pem" "net/http" + "time" httpgit "github.com/go-git/go-git/v5/plumbing/transport/http" gossh "github.com/go-git/go-git/v5/plumbing/transport/ssh" @@ -13,6 +14,8 @@ import ( corev1 "k8s.io/api/core/v1" ) +const gitClientTimeout = time.Second * 30 + var _ = Describe("git's GetAuthFromSecret tests", func() { var ( secret *corev1.Secret @@ -252,7 +255,7 @@ YcwLYudAztZeA/A4aM5Y0MA6PlNIeoHohuMkSZNOBcvkNEWdzGBpKb34yLfMarNm var _ = Describe("git's GetHTTPClientFromSecret tests", func() { When("using a nil secret, no caBudle and InsecureSkipVerify = false", func() { var caBundle []byte - client, err := git.GetHTTPClientFromSecret(nil, caBundle, false) + client, err := git.GetHTTPClientFromSecret(nil, caBundle, false, gitClientTimeout) Expect(err).ToNot(HaveOccurred()) Expect(client).ToNot(BeNil()) expectedTransport, ok := client.Transport.(*http.Transport) @@ -278,7 +281,7 @@ var _ = Describe("git's GetHTTPClientFromSecret tests", func() { When("using a nil secret, no caBudle and InsecureSkipVerify = true", func() { var caBundle []byte - client, err := git.GetHTTPClientFromSecret(nil, caBundle, true) + client, err := git.GetHTTPClientFromSecret(nil, caBundle, true, gitClientTimeout) Expect(err).ToNot(HaveOccurred()) Expect(client).ToNot(BeNil()) expectedTransport, ok := client.Transport.(*http.Transport) @@ -320,7 +323,7 @@ DXZDjC5Ty3zfDBeWUA== block, _ := pem.Decode([]byte(caBundlePEM)) Expect(block).ToNot(BeNil()) - client, err := git.GetHTTPClientFromSecret(nil, block.Bytes, true) + client, err := git.GetHTTPClientFromSecret(nil, block.Bytes, true, gitClientTimeout) Expect(err).ToNot(HaveOccurred()) Expect(client).ToNot(BeNil()) expectedTransport, ok := client.Transport.(*http.Transport) @@ -348,7 +351,7 @@ DXZDjC5Ty3zfDBeWUA== caBundle := []byte(`-----BEGIN CERTIFICATE----- SUPER FAKE CERT -----END CERTIFICATE-----`) - client, err := git.GetHTTPClientFromSecret(nil, caBundle, true) + client, err := git.GetHTTPClientFromSecret(nil, caBundle, true, gitClientTimeout) It("returns an error", func() { Expect(err).To(HaveOccurred()) Expect(client).To(BeNil()) @@ -366,7 +369,7 @@ SUPER FAKE CERT corev1.BasicAuthPasswordKey: password, }, } - client, err := git.GetHTTPClientFromSecret(secret, nil, false) + client, err := git.GetHTTPClientFromSecret(secret, nil, false, gitClientTimeout) Expect(err).ToNot(HaveOccurred()) Expect(client).ToNot(BeNil()) @@ -403,7 +406,7 @@ MC4CAQAwBQYDK2VwBCIEINifzf07d9qx3d44e0FSbV4mC/xQxT644RRbpgNpin7I corev1.TLSPrivateKeyKey: []byte(keyPEM), }, } - client, err := git.GetHTTPClientFromSecret(secret, nil, false) + client, err := git.GetHTTPClientFromSecret(secret, nil, false, gitClientTimeout) Expect(err).ToNot(HaveOccurred()) Expect(client).ToNot(BeNil()) @@ -431,7 +434,7 @@ THIS IS NOT A VALID KEY corev1.TLSPrivateKeyKey: []byte(keyPEM), }, } - _, err := git.GetHTTPClientFromSecret(secret, nil, false) + _, err := git.GetHTTPClientFromSecret(secret, nil, false, gitClientTimeout) It("returns an error", func() { Expect(err).To(HaveOccurred()) diff --git a/pkg/git/remote.go b/pkg/git/remote.go index 11ddefe4c0..3f00c97f16 100644 --- a/pkg/git/remote.go +++ b/pkg/git/remote.go @@ -2,6 +2,7 @@ package git import ( "fmt" + "time" gogit "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/config" @@ -16,6 +17,7 @@ type options struct { CABundle []byte InsecureTLSVerify bool Headers map[string]string + Timeout time.Duration log logr.Logger } diff --git a/pkg/git/vendor.go b/pkg/git/vendor.go index cee68a1fd1..e9c6c1cdf5 100644 --- a/pkg/git/vendor.go +++ b/pkg/git/vendor.go @@ -56,7 +56,12 @@ func getRancherCommitsURL(url *neturl.URL, branch string) string { // latestCommitFromCommitsURL returns the latest commit using the given commits url func latestCommitFromCommitsURL(commitsUrl string, opts *options) (string, error) { - client, err := GetHTTPClientFromSecret(opts.Credential, opts.CABundle, opts.InsecureTLSVerify) + client, err := GetHTTPClientFromSecret( + opts.Credential, + opts.CABundle, + opts.InsecureTLSVerify, + opts.Timeout, + ) if err != nil { return "", err } diff --git a/pkg/git/vendor_test.go b/pkg/git/vendor_test.go index 5c247d94fc..0daa5108c1 100644 --- a/pkg/git/vendor_test.go +++ b/pkg/git/vendor_test.go @@ -77,25 +77,25 @@ var _ = Describe("git's vendor specific functions tests", func() { }) It("returns an error when the server timeouts", func() { - clientTimeout = 1 + clientTimeout := time.Second svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - time.Sleep(time.Second*clientTimeout + 1) + time.Sleep(clientTimeout + 1) w.WriteHeader(http.StatusGatewayTimeout) })) - commit, err := latestCommitFromCommitsURL(svr.URL, &options{}) + commit, err := latestCommitFromCommitsURL(svr.URL, &options{Timeout: clientTimeout}) Expect(err).To(HaveOccurred()) Expect(commit).To(BeEmpty()) }) It("returns no error when cannot get a valid client, and changed returned is true", func() { - clientTimeout = 1 + clientTimeout := time.Second svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusGatewayTimeout) })) caBundle := []byte(`-----BEGIN CERTIFICATE----- SUPER FAKE CERT -----END CERTIFICATE-----`) - commit, err := latestCommitFromCommitsURL(svr.URL, &options{CABundle: caBundle}) + commit, err := latestCommitFromCommitsURL(svr.URL, &options{CABundle: caBundle, Timeout: clientTimeout}) // no error and returns true, so the client is forced to run the List to get results Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("x509: malformed certificate"))