Skip to content

Commit 7f4edfd

Browse files
manusamanusa
authored
refactor(kubernetes): Provider implementations deal with Manager instantiations (#379)
* refactor(kubernetes): Provider implementations deal with Manager instantiations Removed `*Manager` parameter from `ProviderFactory`. Provider implementations should deal with the appropriate (base) Manager instantiation if needed at all. Manager creation function divided into two explicit functions: - NewKubeconfigManager: to be used when using KubeConfig files - NewInClusterManager: to be used inside a cluster New functions contain validations to ensure they are used in the expected places. This ensures that the right manager is used by the provider implementation. Fake kubeconfig for in-cluster Manager is now generated when the Manager is created. This kubeconfig has the "magic" strings (inClusterKubeConfigDefaultContext) that are used by the MCP server and tool mutators. Signed-off-by: Marc Nuri <[email protected]> * review: Provider implementation refactor Signed-off-by: Marc Nuri <[email protected]> --------- Signed-off-by: Marc Nuri <[email protected]>
1 parent 9da29f4 commit 7f4edfd

File tree

10 files changed

+258
-174
lines changed

10 files changed

+258
-174
lines changed

‎pkg/kubernetes/configuration.go‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,6 @@ func (k *Kubernetes) NamespaceOrDefault(namespace string) string {
3838
// ConfigurationContextsDefault returns the current context name
3939
// TODO: Should be moved to the Provider level ?
4040
func (k *Kubernetes) ConfigurationContextsDefault() (string, error) {
41-
if k.manager.inCluster {
42-
return inClusterKubeConfigDefaultContext, nil
43-
}
4441
cfg, err := k.manager.clientCmdConfig.RawConfig()
4542
if err != nil {
4643
return "", err
@@ -51,9 +48,6 @@ func (k *Kubernetes) ConfigurationContextsDefault() (string, error) {
5148
// ConfigurationContextsList returns the list of available context names
5249
// TODO: Should be moved to the Provider level ?
5350
func (k *Kubernetes) ConfigurationContextsList() (map[string]string, error) {
54-
if k.manager.inCluster {
55-
return map[string]string{inClusterKubeConfigDefaultContext: ""}, nil
56-
}
5751
cfg, err := k.manager.clientCmdConfig.RawConfig()
5852
if err != nil {
5953
return nil, err
@@ -77,21 +71,7 @@ func (k *Kubernetes) ConfigurationContextsList() (map[string]string, error) {
7771
func (k *Kubernetes) ConfigurationView(minify bool) (runtime.Object, error) {
7872
var cfg clientcmdapi.Config
7973
var err error
80-
if k.manager.inCluster {
81-
cfg = *clientcmdapi.NewConfig()
82-
cfg.Clusters["cluster"] = &clientcmdapi.Cluster{
83-
Server: k.manager.cfg.Host,
84-
InsecureSkipTLSVerify: k.manager.cfg.Insecure,
85-
}
86-
cfg.AuthInfos["user"] = &clientcmdapi.AuthInfo{
87-
Token: k.manager.cfg.BearerToken,
88-
}
89-
cfg.Contexts[inClusterKubeConfigDefaultContext] = &clientcmdapi.Context{
90-
Cluster: "cluster",
91-
AuthInfo: "user",
92-
}
93-
cfg.CurrentContext = inClusterKubeConfigDefaultContext
94-
} else if cfg, err = k.manager.clientCmdConfig.RawConfig(); err != nil {
74+
if cfg, err = k.manager.clientCmdConfig.RawConfig(); err != nil {
9575
return nil, err
9676
}
9777
if minify {

‎pkg/kubernetes/kubernetes_derived_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ users:
4747
kubeconfig = "` + strings.ReplaceAll(kubeconfigPath, `\`, `\\`) + `"
4848
`)))
4949
s.Run("without authorization header returns original manager", func() {
50-
testManager, err := NewManager(testStaticConfig, "")
50+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
5151
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
5252
s.T().Cleanup(testManager.Close)
5353

@@ -58,7 +58,7 @@ users:
5858
})
5959

6060
s.Run("with invalid authorization header returns original manager", func() {
61-
testManager, err := NewManager(testStaticConfig, "")
61+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
6262
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
6363
s.T().Cleanup(testManager.Close)
6464

@@ -70,7 +70,7 @@ users:
7070
})
7171

7272
s.Run("with valid bearer token creates derived manager with correct configuration", func() {
73-
testManager, err := NewManager(testStaticConfig, "")
73+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
7474
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
7575
s.T().Cleanup(testManager.Close)
7676

@@ -138,7 +138,7 @@ users:
138138
`)))
139139

140140
s.Run("with no authorization header returns oauth token required error", func() {
141-
testManager, err := NewManager(testStaticConfig, "")
141+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
142142
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
143143
s.T().Cleanup(testManager.Close)
144144

@@ -149,7 +149,7 @@ users:
149149
})
150150

151151
s.Run("with invalid authorization header returns oauth token required error", func() {
152-
testManager, err := NewManager(testStaticConfig, "")
152+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
153153
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
154154
s.T().Cleanup(testManager.Close)
155155

@@ -161,7 +161,7 @@ users:
161161
})
162162

163163
s.Run("with valid bearer token creates derived manager", func() {
164-
testManager, err := NewManager(testStaticConfig, "")
164+
testManager, err := NewKubeconfigManager(testStaticConfig, "")
165165
s.Require().NoErrorf(err, "failed to create test manager: %v", err)
166166
s.T().Cleanup(testManager.Close)
167167

‎pkg/kubernetes/manager.go‎

Lines changed: 58 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
type Manager struct {
2626
cfg *rest.Config
2727
clientCmdConfig clientcmd.ClientConfig
28-
inCluster bool
2928
discoveryClient discovery.CachedDiscoveryInterface
3029
accessControlClientSet *AccessControlClientset
3130
accessControlRESTMapper *AccessControlRESTMapper
@@ -38,33 +37,77 @@ type Manager struct {
3837
var _ helm.Kubernetes = (*Manager)(nil)
3938
var _ Openshift = (*Manager)(nil)
4039

41-
func NewManager(config *config.StaticConfig, kubeconfigContext string) (*Manager, error) {
42-
k8s := &Manager{
43-
staticConfig: config,
40+
var (
41+
ErrorKubeconfigInClusterNotAllowed = errors.New("kubeconfig manager cannot be used in in-cluster deployments")
42+
ErrorInClusterNotInCluster = errors.New("in-cluster manager cannot be used outside of a cluster")
43+
)
44+
45+
func NewKubeconfigManager(config *config.StaticConfig, kubeconfigContext string) (*Manager, error) {
46+
if IsInCluster(config) {
47+
return nil, ErrorKubeconfigInClusterNotAllowed
4448
}
49+
4550
pathOptions := clientcmd.NewDefaultPathOptions()
46-
if k8s.staticConfig.KubeConfig != "" {
47-
pathOptions.LoadingRules.ExplicitPath = k8s.staticConfig.KubeConfig
51+
if config.KubeConfig != "" {
52+
pathOptions.LoadingRules.ExplicitPath = config.KubeConfig
4853
}
49-
k8s.clientCmdConfig = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
54+
clientCmdConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
5055
pathOptions.LoadingRules,
5156
&clientcmd.ConfigOverrides{
5257
ClusterInfo: clientcmdapi.Cluster{Server: ""},
5358
CurrentContext: kubeconfigContext,
5459
})
55-
var err error
56-
if IsInCluster(k8s.staticConfig) {
57-
k8s.cfg, err = InClusterConfig()
58-
k8s.inCluster = true
59-
} else {
60-
k8s.cfg, err = k8s.clientCmdConfig.ClientConfig()
60+
61+
restConfig, err := clientCmdConfig.ClientConfig()
62+
if err != nil {
63+
return nil, fmt.Errorf("failed to create kubernetes rest config from kubeconfig: %v", err)
6164
}
62-
if err != nil || k8s.cfg == nil {
63-
return nil, fmt.Errorf("failed to create kubernetes rest config: %v", err)
65+
66+
return newManager(config, restConfig, clientCmdConfig)
67+
}
68+
69+
func NewInClusterManager(config *config.StaticConfig) (*Manager, error) {
70+
if config.KubeConfig != "" {
71+
return nil, fmt.Errorf("kubeconfig file %s cannot be used with the in-cluster deployments: %v", config.KubeConfig, ErrorKubeconfigInClusterNotAllowed)
72+
}
73+
74+
if !IsInCluster(config) {
75+
return nil, ErrorInClusterNotInCluster
76+
}
77+
78+
restConfig, err := InClusterConfig()
79+
if err != nil {
80+
return nil, fmt.Errorf("failed to create in-cluster kubernetes rest config: %v", err)
81+
}
82+
83+
// Create a dummy kubeconfig clientcmdapi.Config for in-cluster config to be used in places where clientcmd.ClientConfig is required
84+
clientCmdConfig := clientcmdapi.NewConfig()
85+
clientCmdConfig.Clusters["cluster"] = &clientcmdapi.Cluster{
86+
Server: restConfig.Host,
87+
InsecureSkipTLSVerify: restConfig.Insecure,
88+
}
89+
clientCmdConfig.AuthInfos["user"] = &clientcmdapi.AuthInfo{
90+
Token: restConfig.BearerToken,
91+
}
92+
clientCmdConfig.Contexts[inClusterKubeConfigDefaultContext] = &clientcmdapi.Context{
93+
Cluster: "cluster",
94+
AuthInfo: "user",
95+
}
96+
clientCmdConfig.CurrentContext = inClusterKubeConfigDefaultContext
97+
98+
return newManager(config, restConfig, clientcmd.NewDefaultClientConfig(*clientCmdConfig, nil))
99+
}
100+
101+
func newManager(config *config.StaticConfig, restConfig *rest.Config, clientCmdConfig clientcmd.ClientConfig) (*Manager, error) {
102+
k8s := &Manager{
103+
staticConfig: config,
104+
cfg: restConfig,
105+
clientCmdConfig: clientCmdConfig,
64106
}
65107
if k8s.cfg.UserAgent == "" {
66108
k8s.cfg.UserAgent = rest.DefaultKubernetesUserAgent()
67109
}
110+
var err error
68111
// TODO: Won't work because not all client-go clients use the shared context (e.g. discovery client uses context.TODO())
69112
//k8s.cfg.Wrap(func(original http.RoundTripper) http.RoundTripper {
70113
// return &impersonateRoundTripper{original}
@@ -229,7 +272,6 @@ func (m *Manager) Derived(ctx context.Context) (*Kubernetes, error) {
229272
derived := &Kubernetes{
230273
manager: &Manager{
231274
clientCmdConfig: clientcmd.NewDefaultClientConfig(clientCmdApiConfig, nil),
232-
inCluster: m.inCluster,
233275
cfg: derivedCfg,
234276
staticConfig: m.staticConfig,
235277
},

0 commit comments

Comments
 (0)