Skip to content

Commit 013b2c7

Browse files
emosbaughemosbaugh
authored
fix(kubernetes): RESTClientGetter concurrency issue (#2427)
* fix(kubernetes): RESTClientGetter concurrency issue * f
1 parent 44b2959 commit 013b2c7

File tree

8 files changed

+123
-96
lines changed

8 files changed

+123
-96
lines changed

‎api/controllers/kubernetes/install/controller.go

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,23 @@ type Controller interface {
3333
var _ Controller = (*InstallController)(nil)
3434

3535
type InstallController struct {
36-
installationManager installation.InstallationManager
37-
infraManager infra.InfraManager
38-
appConfigManager appconfig.AppConfigManager
39-
metricsReporter metrics.ReporterInterface
40-
restClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
41-
releaseData *release.ReleaseData
42-
password string
43-
tlsConfig types.TLSConfig
44-
license []byte
45-
airgapBundle string
46-
configValues string
47-
endUserConfig *ecv1beta1.Config
48-
store store.Store
49-
ki kubernetesinstallation.Installation
50-
stateMachine statemachine.Interface
51-
logger logrus.FieldLogger
52-
mu sync.RWMutex
36+
installationManager installation.InstallationManager
37+
infraManager infra.InfraManager
38+
appConfigManager appconfig.AppConfigManager
39+
metricsReporter metrics.ReporterInterface
40+
restClientGetter genericclioptions.RESTClientGetter
41+
releaseData *release.ReleaseData
42+
password string
43+
tlsConfig types.TLSConfig
44+
license []byte
45+
airgapBundle string
46+
configValues string
47+
endUserConfig *ecv1beta1.Config
48+
store store.Store
49+
ki kubernetesinstallation.Installation
50+
stateMachine statemachine.Interface
51+
logger logrus.FieldLogger
52+
mu sync.RWMutex
5353
}
5454

5555
type InstallControllerOption func(*InstallController)
@@ -72,9 +72,9 @@ func WithMetricsReporter(metricsReporter metrics.ReporterInterface) InstallContr
7272
}
7373
}
7474

75-
func WithRESTClientGetterFactory(restClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter) InstallControllerOption {
75+
func WithRESTClientGetter(restClientGetter genericclioptions.RESTClientGetter) InstallControllerOption {
7676
return func(c *InstallController) {
77-
c.restClientGetterFactory = restClientGetterFactory
77+
c.restClientGetter = restClientGetter
7878
}
7979
}
8080

@@ -181,7 +181,7 @@ func NewInstallController(opts ...InstallControllerOption) (*InstallController,
181181
controller.infraManager = infra.NewInfraManager(
182182
infra.WithLogger(controller.logger),
183183
infra.WithInfraStore(controller.store.LinuxInfraStore()),
184-
infra.WithRESTClientGetterFactory(controller.restClientGetterFactory),
184+
infra.WithRESTClientGetter(controller.restClientGetter),
185185
infra.WithPassword(controller.password),
186186
infra.WithTLSConfig(controller.tlsConfig),
187187
infra.WithLicense(controller.license),

‎api/internal/handlers/kubernetes/kubernetes.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func New(cfg types.APIConfig, opts ...Option) (*Handler, error) {
5555
installController, err := install.NewInstallController(
5656
install.WithLogger(h.logger),
5757
install.WithMetricsReporter(h.metricsReporter),
58-
install.WithRESTClientGetterFactory(h.cfg.RESTClientGetterFactory),
58+
install.WithRESTClientGetter(h.cfg.RESTClientGetter),
5959
install.WithReleaseData(h.cfg.ReleaseData),
6060
install.WithEndUserConfig(h.cfg.EndUserConfig),
6161
install.WithPassword(h.cfg.Password),

‎api/internal/managers/kubernetes/infra/manager.go

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,21 +27,21 @@ type InfraManager interface {
2727

2828
// infraManager is an implementation of the InfraManager interface
2929
type infraManager struct {
30-
infraStore infrastore.Store
31-
password string
32-
tlsConfig types.TLSConfig
33-
license []byte
34-
airgapBundle string
35-
configValues string
36-
releaseData *release.ReleaseData
37-
endUserConfig *ecv1beta1.Config
38-
logger logrus.FieldLogger
39-
kcli client.Client
40-
mcli metadata.Interface
41-
hcli helm.Client
42-
restClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
43-
kotsInstaller func() error
44-
mu sync.RWMutex
30+
infraStore infrastore.Store
31+
password string
32+
tlsConfig types.TLSConfig
33+
license []byte
34+
airgapBundle string
35+
configValues string
36+
releaseData *release.ReleaseData
37+
endUserConfig *ecv1beta1.Config
38+
logger logrus.FieldLogger
39+
kcli client.Client
40+
mcli metadata.Interface
41+
hcli helm.Client
42+
restClientGetter genericclioptions.RESTClientGetter
43+
kotsInstaller func() error
44+
mu sync.RWMutex
4545
}
4646

4747
type InfraManagerOption func(*infraManager)
@@ -118,9 +118,9 @@ func WithHelmClient(hcli helm.Client) InfraManagerOption {
118118
}
119119
}
120120

121-
func WithRESTClientGetterFactory(restClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter) InfraManagerOption {
121+
func WithRESTClientGetter(restClientGetter genericclioptions.RESTClientGetter) InfraManagerOption {
122122
return func(c *infraManager) {
123-
c.restClientGetterFactory = restClientGetterFactory
123+
c.restClientGetter = restClientGetter
124124
}
125125
}
126126

‎api/internal/managers/kubernetes/infra/util.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,10 @@ func (m *infraManager) helmClient(_ kubernetesinstallation.Installation) (helm.C
6060
airgapChartsPath = "" // rc.EmbeddedClusterChartsSubDir()
6161
}
6262
hcli, err := helm.NewClient(helm.HelmOptions{
63-
RESTClientGetterFactory: m.restClientGetterFactory,
64-
K0sVersion: versions.K0sVersion,
65-
AirgapPath: airgapChartsPath,
66-
LogFn: m.logFn("helm"),
63+
RESTClientGetter: m.restClientGetter,
64+
K0sVersion: versions.K0sVersion,
65+
AirgapPath: airgapChartsPath,
66+
LogFn: m.logFn("helm"),
6767
})
6868
if err != nil {
6969
return nil, fmt.Errorf("create helm client: %w", err)

‎api/types/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ type LinuxConfig struct {
2929
}
3030

3131
type KubernetesConfig struct {
32-
RESTClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
33-
Installation kubernetesinstallation.Installation
32+
RESTClientGetter genericclioptions.RESTClientGetter
33+
Installation kubernetesinstallation.Installation
3434
}

‎cmd/installer/cli/install.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ type installConfig struct {
9595
tlsCertBytes []byte
9696
tlsKeyBytes []byte
9797

98-
kubernetesRESTClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
98+
kubernetesRESTClientGetter genericclioptions.RESTClientGetter
9999
}
100100

101101
// webAssetsFS is the filesystem to be used by the web component. Defaults to nil allowing the web server to use the default assets embedded in the binary. Useful for testing.
@@ -540,11 +540,7 @@ func preRunInstallKubernetes(_ *cobra.Command, flags *InstallCmdFlags, _ kuberne
540540
return fmt.Errorf("failed to connect to kubernetes api server: %w", err)
541541
}
542542

543-
flags.installConfig.kubernetesRESTClientGetterFactory = func(namespace string) genericclioptions.RESTClientGetter {
544-
// TODO: this is not thread safe
545-
flags.kubernetesEnvSettings.SetNamespace(namespace)
546-
return flags.kubernetesEnvSettings.RESTClientGetter()
547-
}
543+
flags.installConfig.kubernetesRESTClientGetter = flags.kubernetesEnvSettings.RESTClientGetter()
548544

549545
return nil
550546
}
@@ -655,8 +651,8 @@ func runManagerExperienceInstall(
655651
AllowIgnoreHostPreflights: flags.ignoreHostPreflights,
656652
},
657653
KubernetesConfig: apitypes.KubernetesConfig{
658-
RESTClientGetterFactory: flags.installConfig.kubernetesRESTClientGetterFactory,
659-
Installation: ki,
654+
RESTClientGetter: flags.installConfig.kubernetesRESTClientGetter,
655+
Installation: ki,
660656
},
661657
},
662658

‎operator/controllers/suite_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
. "github.com/onsi/gomega"
2525

2626
"k8s.io/client-go/kubernetes/scheme"
27-
"k8s.io/client-go/rest"
27+
restclient "k8s.io/client-go/rest"
2828
"sigs.k8s.io/controller-runtime/pkg/client"
2929
"sigs.k8s.io/controller-runtime/pkg/envtest"
3030
logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -37,7 +37,7 @@ import (
3737
// These tests use Ginkgo (BDD-style Go testing framework). Refer to
3838
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
3939

40-
var cfg *rest.Config
40+
var cfg *restclient.Config
4141
var k8sClient client.Client
4242
var testEnv *envtest.Environment
4343

‎pkg/helm/client.go

Lines changed: 73 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,12 @@ import (
2828
"helm.sh/helm/v3/pkg/storage/driver"
2929
"helm.sh/helm/v3/pkg/uploader"
3030
"k8s.io/cli-runtime/pkg/genericclioptions"
31+
restclient "k8s.io/client-go/rest"
32+
"k8s.io/client-go/tools/clientcmd"
33+
clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
3134
k8syaml "sigs.k8s.io/yaml"
3235
)
3336

34-
type RESTClientGetterFactory func(namespace string) genericclioptions.RESTClientGetter
35-
3637
var (
3738
// getters is a list of known getters for both http and
3839
// oci schemes.
@@ -79,24 +80,30 @@ func newClient(opts HelmOptions) (*HelmClient, error) {
7980
if err != nil {
8081
return nil, fmt.Errorf("create registry client: %w", err)
8182
}
83+
if opts.RESTClientGetter == nil {
84+
cfgFlags := &genericclioptions.ConfigFlags{}
85+
if opts.KubeConfig != "" {
86+
cfgFlags.KubeConfig = &opts.KubeConfig
87+
}
88+
opts.RESTClientGetter = cfgFlags
89+
}
8290
return &HelmClient{
83-
tmpdir: tmpdir,
84-
kubeconfig: opts.KubeConfig,
85-
kversion: kversion,
86-
regcli: regcli,
87-
logFn: opts.LogFn,
88-
getterFactory: opts.RESTClientGetterFactory,
89-
airgapPath: opts.AirgapPath,
91+
tmpdir: tmpdir,
92+
kversion: kversion,
93+
restClientGetter: opts.RESTClientGetter,
94+
regcli: regcli,
95+
logFn: opts.LogFn,
96+
airgapPath: opts.AirgapPath,
9097
}, nil
9198
}
9299

93100
type HelmOptions struct {
94-
KubeConfig string
95-
K0sVersion string
96-
AirgapPath string
97-
Writer io.Writer
98-
LogFn action.DebugLog
99-
RESTClientGetterFactory RESTClientGetterFactory
101+
KubeConfig string
102+
RESTClientGetter genericclioptions.RESTClientGetter
103+
K0sVersion string
104+
AirgapPath string
105+
Writer io.Writer
106+
LogFn action.DebugLog
100107
}
101108

102109
type InstallOptions struct {
@@ -128,16 +135,15 @@ type UninstallOptions struct {
128135
}
129136

130137
type HelmClient struct {
131-
tmpdir string
132-
kversion *semver.Version
133-
kubeconfig string
134-
regcli *registry.Client
135-
repocfg string
136-
repos []*repo.Entry
137-
reposChanged bool
138-
logFn action.DebugLog
139-
getterFactory RESTClientGetterFactory
140-
airgapPath string
138+
tmpdir string
139+
kversion *semver.Version
140+
restClientGetter genericclioptions.RESTClientGetter
141+
regcli *registry.Client
142+
repocfg string
143+
repos []*repo.Entry
144+
reposChanged bool
145+
logFn action.DebugLog
146+
airgapPath string
141147
}
142148

143149
func (h *HelmClient) prepare() error {
@@ -500,35 +506,23 @@ func (h *HelmClient) Render(ctx context.Context, opts InstallOptions) ([][]byte,
500506
}
501507

502508
func (h *HelmClient) getActionCfg(namespace string) (*action.Configuration, error) {
503-
getter := h.getRESTClientGetter(namespace)
504-
505509
cfg := &action.Configuration{}
506510
var logFn action.DebugLog
507511
if h.logFn != nil {
508512
logFn = h.logFn
509513
} else {
510514
logFn = _logFn
511515
}
512-
if err := cfg.Init(getter, namespace, "secret", logFn); err != nil {
516+
restClientGetter := &namespacedRESTClientGetter{
517+
RESTClientGetter: h.restClientGetter,
518+
namespace: namespace,
519+
}
520+
if err := cfg.Init(restClientGetter, namespace, "secret", logFn); err != nil {
513521
return nil, fmt.Errorf("init helm configuration: %w", err)
514522
}
515523
return cfg, nil
516524
}
517525

518-
func (h *HelmClient) getRESTClientGetter(namespace string) genericclioptions.RESTClientGetter {
519-
if h.getterFactory != nil {
520-
return h.getterFactory(namespace)
521-
}
522-
523-
cfgFlags := &genericclioptions.ConfigFlags{
524-
Namespace: &namespace,
525-
}
526-
if h.kubeconfig != "" {
527-
cfgFlags.KubeConfig = &h.kubeconfig
528-
}
529-
return cfgFlags
530-
}
531-
532526
func (h *HelmClient) loadChart(ctx context.Context, releaseName, chartPath, chartVersion string) (*chart.Chart, error) {
533527
var localPath string
534528
if h.airgapPath != "" {
@@ -579,3 +573,40 @@ func _logFn(format string, args ...interface{}) {
579573
log := logrus.WithField("component", "helm")
580574
log.Debugf(format, args...)
581575
}
576+
577+
type namespacedRESTClientGetter struct {
578+
genericclioptions.RESTClientGetter
579+
namespace string
580+
}
581+
582+
func (n *namespacedRESTClientGetter) ToRawKubeConfigLoader() clientcmd.ClientConfig {
583+
cfg := n.RESTClientGetter.ToRawKubeConfigLoader()
584+
return &namespacedClientConfig{
585+
cfg: cfg,
586+
namespace: n.namespace,
587+
}
588+
}
589+
590+
type namespacedClientConfig struct {
591+
cfg clientcmd.ClientConfig
592+
namespace string
593+
}
594+
595+
func (n *namespacedClientConfig) RawConfig() (clientcmdapi.Config, error) {
596+
return n.cfg.RawConfig()
597+
}
598+
599+
func (n *namespacedClientConfig) ClientConfig() (*restclient.Config, error) {
600+
return n.cfg.ClientConfig()
601+
}
602+
603+
func (n *namespacedClientConfig) Namespace() (string, bool, error) {
604+
if n.namespace == "" {
605+
return n.cfg.Namespace()
606+
}
607+
return n.namespace, true, nil
608+
}
609+
610+
func (n *namespacedClientConfig) ConfigAccess() clientcmd.ConfigAccess {
611+
return n.cfg.ConfigAccess()
612+
}

0 commit comments

Comments
 (0)