Skip to content

Conversation

@Cali0707
Copy link

@Cali0707 Cali0707 commented Oct 15, 2025

This PR adds a cluster provider that uses ACM and the cluster proxy addon to communicate with all the clusters managed by the hub cluster. This allows users to configure the connection in two ways:

  1. acm-kubeconfig - this uses a specified context from the users kubeconfig to connect to a remote hub cluster, and uses the route to the cluster proxy addon to connect to any managed clusters
  2. acm - this assumes that the mcp server is deployed within the hub cluster, and uses the cluster proxy addon service (instead of route) to talk to managed clusters

An example configuration for the acm-kubeconfig strategy which verifies TLS is as follows:

# ACM config rules
cluster_provider_strategy = "acm-kubeconfig"
[cluster_provider_configs.acm-kubeconfig]
context_name = "acm"
cluster_proxy_addon_host = "<cluster proxy route>"
cluster_proxy_addon_ca_file = "openshift-ca.crt"

An example configuration for the acm strategy which does not verify TLS is as follows:

cluster_provider_strategy = "acm"
[cluster_provider_configs.acm-kubeconfig]
cluster_proxy_addon_host = "<cluster proxy route>"
cluster_proxy_addon_skip_tls_verify = true

@openshift-ci openshift-ci bot requested review from ardaguclu and matzew October 15, 2025 14:12
@Cali0707
Copy link
Author

/hold

Let's wait to get containers#377 merged upstream first, and then I will rebase this PR on top of those changes

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2025
@Cali0707 Cali0707 mentioned this pull request Oct 24, 2025
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2025
@Cali0707
Copy link
Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2025
@Cali0707 Cali0707 force-pushed the acm-cluster-provider branch from 1b82cd1 to 1bccb73 Compare October 27, 2025 16:08
@matzew
Copy link
Member

matzew commented Oct 27, 2025

Thanks for the rebase and updating the PR.

I will take a look here tomorrow!

@matzew
Copy link
Member

matzew commented Oct 27, 2025

/assign @matzew

err := c.ACMProviderConfig.Validate()

if c.ContextName == "" {
err = errors.Join(err, fmt.Errorf("context_name is required is acm-kubeconfig strategy is used"))
Copy link
Member

Choose a reason for hiding this comment

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

if acm-kubeconfig strategy is used

@matzew
Copy link
Member

matzew commented Oct 28, 2025

LGTM

✅ Tested the acm-kubeconfig with a local running binary.

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2025
Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2025
@matzew
Copy link
Member

matzew commented Oct 28, 2025

@matzew
Copy link
Member

matzew commented Oct 29, 2025

@ardaguclu I guess the invalid JIRA reference, is that there is no colon (:) after the OCPMCP-9 ?
Are you able to edit the PR title?

@ardaguclu
Copy link
Member

ardaguclu commented Oct 30, 2025

/retitle OCPMCP-9: add acm cluster provide

@openshift-ci openshift-ci bot changed the title OCPMCP-9 add acm cluster provider OCPMCP-9: add acm cluster provide Oct 30, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 30, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 30, 2025

@Cali0707: This pull request references OCPMCP-9 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set.

In response to this:

This PR adds a cluster provider that uses ACM and the cluster proxy addon to communicate with all the clusters managed by the hub cluster. This allows users to configure the connection in two ways:

  1. acm-kubeconfig - this uses a specified context from the users kubeconfig to connect to a remote hub cluster, and uses the route to the cluster proxy addon to connect to any managed clusters
  2. acm - this assumes that the mcp server is deployed within the hub cluster, and uses the cluster proxy addon service (instead of route) to talk to managed clusters

An example configuration for the acm-kubeconfig strategy which verifies TLS is as follows:

# ACM config rules
cluster_provider_strategy = "acm-kubeconfig"
[cluster_provider_configs.acm-kubeconfig]
context_name = "acm"
cluster_proxy_addon_host = "<cluster proxy route>"
cluster_proxy_addon_ca_file = "openshift-ca.crt"

An example configuration for the acm strategy which does not verify TLS is as follows:

cluster_provider_strategy = "acm"
[cluster_provider_configs.acm-kubeconfig]
cluster_proxy_addon_host = "<cluster proxy route>"
cluster_proxy_addon_skip_tls_verify = true

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@ardaguclu
Copy link
Member

@ardaguclu I guess the invalid JIRA reference, is that there is no colon (:) after the OCPMCP-9 ?

@matzew /retitle command should work for such cases.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3720f23 and 2 for PR HEAD 1bccb73 in total

@matzew
Copy link
Member

matzew commented Oct 30, 2025

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2025
@Cali0707
Copy link
Author

/cc @matzew can you re-review?

Signed-off-by: Calum Murray <[email protected]>
Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2025
@Cali0707
Copy link
Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2025
@Cali0707 Cali0707 changed the title OCPMCP-9: add acm cluster provide OCPMCP-9: add acm cluster provider Oct 30, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2025
@Cali0707
Copy link
Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 30, 2025
@matzew
Copy link
Member

matzew commented Oct 30, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 30, 2025

@Cali0707: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Comment on lines +229 to +233
manager, err := p.managerForCluster(target)
if err != nil {
return nil, nil, fmt.Errorf("failed to get manager for cluster '%s', unable to verify token", target)
}
return manager.VerifyToken(ctx, token, audience)
Copy link
Member

@matzew matzew Oct 30, 2025

Choose a reason for hiding this comment

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

Now verifies tokens against the correct target cluster (should)

@openshift-merge-bot openshift-merge-bot bot merged commit 5d43b9e into openshift:main Oct 30, 2025
8 checks passed
macayaven added a commit to macayaven/openshift-mcp-server that referenced this pull request Oct 30, 2025
Add detailed documentation for the ACM (Advanced Cluster Management)
cluster provider feature, including:

- Overview and architecture of ACM multi-cluster support
- Configuration guide for both acm-kubeconfig and acm strategies
- Step-by-step setup instructions with examples
- Prerequisites and RBAC requirements
- Troubleshooting guide for common issues
- Security considerations and best practices
- Production deployment examples

This documentation complements the ACM cluster provider implementation
merged in PR openshift#58.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants