Skip to content

Commit

Permalink
Configurable git client timeout
Browse files Browse the repository at this point in the history
Fixes #2188

Also fixes an issue in the config reconciler which has prevented the
config from being reloaded when it was changed.
  • Loading branch information
p-se committed Jul 9, 2024
1 parent 489d4c5 commit ea76aad
Show file tree
Hide file tree
Showing 14 changed files with 73 additions and 25 deletions.
3 changes: 2 additions & 1 deletion charts/fleet/templates/configmap.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@ data:
"agentNamespace": "{{.Values.bootstrap.agentNamespace}}"
},
"webhookReceiverURL": "{{.Values.webhookReceiverURL}}",
"githubURLPrefix": "{{.Values.githubURLPrefix}}"
"githubURLPrefix": "{{.Values.githubURLPrefix}}",
"gitClientTimeout": "{{.Values.gitClientTimeout}}"
}
4 changes: 4 additions & 0 deletions charts/fleet/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions integrationtests/gitjob/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions integrationtests/gitjob/git/git_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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},
}
Expand Down Expand Up @@ -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},
}
Expand Down Expand Up @@ -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
Expand Down
23 changes: 20 additions & 3 deletions internal/cmd/controller/gitops/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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
}

Expand Down
1 change: 1 addition & 0 deletions internal/cmd/controller/reconciler/config_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand Down
5 changes: 5 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions pkg/git/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 4 additions & 0 deletions pkg/git/fetch_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand Down
6 changes: 2 additions & 4 deletions pkg/git/netutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down
17 changes: 10 additions & 7 deletions pkg/git/netutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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())
Expand All @@ -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())

Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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())
Expand Down
2 changes: 2 additions & 0 deletions pkg/git/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -16,6 +17,7 @@ type options struct {
CABundle []byte
InsecureTLSVerify bool
Headers map[string]string
Timeout time.Duration
log logr.Logger
}

Expand Down
7 changes: 6 additions & 1 deletion pkg/git/vendor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
10 changes: 5 additions & 5 deletions pkg/git/vendor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down

0 comments on commit ea76aad

Please sign in to comment.