Skip to content

Conversation

@harche
Copy link
Contributor

@harche harche commented Oct 8, 2025

With this change, the clients outside pkg/kuberntes, such as the one getting developed for openshift-mcp-server, can assemble a statically typed client.

With this change we can assemble a typed client like this, https://github.com/openshift/openshift-mcp-server/compare/main...harche:kubernetes-mcp-server:exec_tool?expand=1#diff-b880f18e8c0[…]405fe48bac8bR110
and then create an object (a pod in this case) like this, https://github.com/openshift/openshift-mcp-server/compare/main...harche:kubernetes-mcp-server:exec_tool?expand=1#diff-b880f18e8c0[…]405fe48bac8bR195

Even if we are making the rest config public, this doesn't increase the attack surface because the user would have been able to create arbitrary CRs allowed by their authorization using existing ResourcesCreateOrUpdate anyway.

@harche
Copy link
Contributor Author

harche commented Oct 8, 2025

/cc @mrunalp

@mrunalp
Copy link

mrunalp commented Oct 8, 2025

This seems reasonable to me.

@harche harche changed the title make ToRESTConfig for creating a kubernetes client outside pkg/kubernetes make ToRESTConfig public for creating a kubernetes client outside pkg/kubernetes Oct 8, 2025
@manusa
Copy link
Member

manusa commented Oct 9, 2025

This is partially OK.

However, if the config has configured denied access to a given resource, access WILL NOT be restricted and would be up to the toolset implementor to check the config (which won't happen)

Considering the following config:

[[denied_resources]]
version = "v1"

The implementation in the linked PR will still be able to execute.

I'm unaware of the downstream possible scenarios but I'd still suggest that we don't open it that much (i.e. completely exposing the RESTConfig).

My suggestion would be to give access to the AccessConstolClientset
And then, exposing whatever clientset is needed donwstream.

Unless I missed anything, the current implementation already provides access to every API that's needed.

So:

func (k *Kubernetes) AccessControlClientset() {
  return k.manager.accessControlClientSet
}

Should allow for:

k.AccessControlClientset().Pods($namespace).xxxWhateverInCoreV1PodInterface

If an interface is missing (Nodes or whatever), we can simply add a new method to the AccessConstolClientset

I believe that this serves as the main reasoning as to why the access to the underlying APIs should remain centralized in a central place and the API should only expose the computed clients.
If at any point we change access-control-related logic implementors automatically benefit from it and we won't have to struggle with changing implementations downstream.

@harche harche changed the title make ToRESTConfig public for creating a kubernetes client outside pkg/kubernetes make AccessControlClientset public for creating a kubernetes client outside pkg/kubernetes Oct 9, 2025
@harche
Copy link
Contributor Author

harche commented Oct 9, 2025

Thanks @manusa, I have updated the PR to make AccessControlClientset public instead.

@manusa manusa added this to the 0.1.0 milestone Oct 10, 2025
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thx!

@manusa manusa merged commit 65cc304 into containers:main Oct 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants