:sparkles: Add support for fetching multiple kubeconfig

[valaparthvi]

What this PR does / why we need it: This PR adds support to fetch a different type of Kubeconfig. clusterctl get kubeconfig <name> now fetches the user specific kubeconfig by default, and if unavailable, it falls back to fetching the system specific kubeconfig. It also adds a new flag --user which is true by default, and can be set to false to receive system specific kubeconfig.

See #3661 for more details.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #3661

[k8s-ci-robot]

Welcome @valaparthvi!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @valaparthvi. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[valaparthvi]

@richardcase Do you mind taking a look at this (whenever you find the time, of course)? I am not very sure about the documentation and comments, and I feel as if I might have missed out on test cases, but I didn't find any integration test that would need to be modified.

[sbueringer]

/ok-to-test

[richardcase]

@valaparthvi - thanks for working on this :+1: I think we should be aware of this comment from @fabriziopandini where we should default to returning the "user" kubeconfig if there is one and if there isn't then fallback to the current one as per the current behaviour of clusterctl.

What this means is that we should look for a secret with name that uses this format first [cluster-name]-user-kubeconfig. If that's not found, then fallback to looking for the secret with [cluster-name]-kubeconfig (which is the current behaviour).

We could add a flag that allows us to get the "system" kubeconfig instead from clusterctl instead of the user one. So something like: clusterctl get kubeconfig --system.

If you want to jump on a call any time feel free to ping me on slack.

[valaparthvi]

/test pull-cluster-api-test-main

    clusterclass_test.go:460: 
        Expected
            <*errors.StatusError | 0xc0012c5720>: {
                ErrStatus: {
                    TypeMeta: {Kind: "", APIVersion: ""},
                    ListMeta: {
                        SelfLink: "",
                        ResourceVersion: "",
                        Continue: "",
                        RemainingItemCount: nil,
                    },
                    Status: "Failure",
                    Message: "admission webhook \"validation.clusterclass.cluster.x-k8s.io\" denied the request: ClusterClass.cluster.x-k8s.io \"class4\" is forbidden: ClusterClass cannot be deleted because it is used by 1 Cluster(s)",
                    Reason: "Forbidden",
                    Details: {
                        Name: "class4",
                        Group: "cluster.x-k8s.io",
                        Kind: "ClusterClass",
                        UID: "",
                        Causes: nil,
                        RetryAfterSeconds: 0,
                    },
                    Code: 403,
                },
            }
        to be nil
--- FAIL: TestClusterClassWebhook_Delete_MultipleExistingClusters (0.51s)
}

[richardcase]

We can approach the clusterctl flag along with the refactor in a separate PR as it probably needs more discussion. @fabriziopandini thoughts?

I think we need the flag to allow a user of clusterctl to explicitly get the "system" kubeconfig and not the user kubeconfig in the scenario where a provider publishes 2. But as @fabriziopandini mentioned in the comments the flag can default to getting the user kubeconfig (if there is one) and fallback to the "system" kubeconfig.

From a package point of view we need to default to getting the "system" kubeconfig as this is the expected behavior for CAPI & provider controllers.

@ykakarap - i agree on the function signatures being a breaking API change and that we can just introduce a new function so as not to break any existing clients.

[valaparthvi]

@fabriziopandini @richardcase @ykakarap I've modified the PR according to your changes.

1. Added a new ~method~ function GetUserKubeconfig to WorkloadCluster.
2. Added a new function FromUserSecret
3. Added unit tests for both 1, and 2.
4. Modified clusterClient.GetKubeconfig method to fetch user kubeconfig by default, and unless that fails, or if system kubeconfig is explicitly asked for, then fetch system kubeconfig.

Also, my apologies for ghosting this PR for so long :raised_hands:

[ykakarap]

Also, my apologies for ghosting this PR for so long 🙌

No problem 🙂 Great to see that you are following up on the PR.

[valaparthvi]

@valaparthvi Looking at the compatibility notice of clusterctl as a library (https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/README.md) we are not providing any guarantees at the moment about the API. While it will be nice to not make any big breaking changes looks like we can get a pass to make some as needed.

With this in mind, it might be cleaner to make the GetUserKubeconfig function part of the WorkloadCluster interface instead of leaving it as a package level function.

GetUserKubeconfig was explicitly defined as a function in the first place to avoid breaking the API.(https://github.com/kubernetes-sigs/cluster-api/pull/6346#discussion_r857891414) But, if it makes sense, I can implement this as a method.

[ykakarap]

@valaparthvi Looking at the compatibility notice of clusterctl as a library (https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/README.md) we are not providing any guarantees at the moment about the API. While it will be nice to not make any big breaking changes looks like we can get a pass to make some as needed. With this in mind, it might be cleaner to make the GetUserKubeconfig function part of the WorkloadCluster interface instead of leaving it as a package level function.

GetUserKubeconfig was explicitly defined as a function in the first place to avoid breaking the API.(#6346 (comment)) But, if it makes sense, I can implement this as a method.

I would suggest that you wait on implementing this till we get confirmation on a direction to take :)

[valaparthvi]

@valaparthvi Looking at the compatibility notice of clusterctl as a library (https://github.com/kubernetes-sigs/cluster-api/blob/main/cmd/clusterctl/README.md) we are not providing any guarantees at the moment about the API. While it will be nice to not make any big breaking changes looks like we can get a pass to make some as needed. With this in mind, it might be cleaner to make the GetUserKubeconfig function part of the WorkloadCluster interface instead of leaving it as a package level function.

GetUserKubeconfig was explicitly defined as a function in the first place to avoid breaking the API.(#6346 (comment)) But, if it makes sense, I can implement this as a method.

I would suggest that you wait on implementing this till we get confirmation on a direction to take :)

Can I take that up in another PR then? Or would you rather I do it in the same one?

[sbueringer]

Let's change the func to a method please (and in this PR directly). As Yuvaraj wrote we have no strong API guarantees on this package and making it a func makes the API in my opinion very inconsistent.

[ykakarap]

@valaparthvi I think you can go ahead with making the GetUserKubeconfig a method on the WorkloadCluster interface.

Thank you for being patient through the back and forth :)

[valaparthvi]

How do I fix the apidiff failures?

[sbueringer]

How do I fix the apidiff failures?

You don't have to. It just indicates to us that there's an API change and we can decide to accept it (it's optional for merge)

[valaparthvi]

Just discussed it with Yuvaraj, we would want to discuss this with Fabrizio first (he will be back next week)

I'm going to wait on the requested changes until we finalize the behavior for this command.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval by writing /assign @justinsb in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[valaparthvi]

@ykakarap I've worked on the changes you requested.

[k8s-ci-robot]

@valaparthvi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	e500ce3b7f65d11f1d38d8a560b2aab272b62f1a	link	false	/test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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/test-infra repository. I understand the commands that are listed here.

[valaparthvi]

Discussed in Aug 3rd Office hours We are open to idea on how to make UX nice but we cannot trade on security concern Also, it will be great if we can discuss this in terms of contract all the control plane providers should abide to. https://github.com/kubernetes-sigs/cluster-api/pull/6346#discussion_r936949742

@richardcase In response to the above comment by @fabriziopandini, I think I should close this PR until the terms of contract for all the control plane providers are finalized. Let me know if you think there should be another course of action, or if I can help with that.

[richardcase]

Discussed in Aug 3rd Office hours We are open to idea on how to make UX nice but we cannot trade on security concern Also, it will be great if we can discuss this in terms of contract all the control plane providers should abide to. #6346 (comment)

@richardcase In response to the above comment by @fabriziopandini, I think I should close this PR until the terms of contract for all the control plane providers are finalized. Let me know if you think there should be another course of action, or if I can help with that.

I get the concern around falling back and inadvertently getting a higher privileged kubeconfig. For me, this is around enabling a cluster operator to easily get a kubeconfig that can be given to users...and trying to have consistency in how its retrieved across providers. As an operator i'd hate to use 1 method for CAPA and another method for a different provider.

I like the idea of adding --type=user or something similar...but with no fallback.

But overall, what I am hearing from the comments is that more thought & design is needed for this change to ensure we cover all concerns raised....so i'm thinking we create a proposal for this.

@valaparthvi - did you want to work together on a proposal for this change?

@fabriziopandini - do you think doing a proposal would help?

[valaparthvi]

@valaparthvi - did you want to work together on a proposal for this change?

I would love to, but I'll need a lot of guidance on how to go forward with the proposal.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale