🌱 Update Config generation to use inClusterConfig #10729
Conversation
Signed-off-by: AbdullahAlShaad <[email protected]>
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/S
AbdullahAlShaad
changed the title
| } | ||
| return nil, err | ||
| } | ||
| restConfig := ctrl.GetConfigOrDie() |
There was a problem hiding this comment.
I think the issue with this is, the underlying function tries to use a global kubeconfig flag, which we don't set or use.
Instead I think what we should do is use rest.InCLusterConfig() in the error case as fallback.
There was a problem hiding this comment.
Updated. Anyway, though it would take a lot of refactoring, wouldn't it be better to use kubeconfig flag?
Signed-off-by: AbdullahAlShaad <[email protected]>
k8s-ci-robot
added
size/XS
| } | ||
| restConfig, err = rest.InClusterConfig() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Let's return an error that expresses that both the kubeconfig and the in cluster config didn't work (previosu error from l.146 + something for in cluster
There was a problem hiding this comment.
Updated the error message to "invalid kubeconfig file and failed to create inClusterConfig as well"
|
I think this should be okay, but would be good to get confirmation from someone more familiar with clusterctl Also not sure if we want to make this behavior opt-in with some additional flag or something (if we think we have to safeguard this a bit) |
Signed-off-by: AbdullahAlShaad <[email protected]>
AbdullahAlShaad
force-pushed
the
incluster
branch
from
32bab3f
to
54762ed
Compare
| if strings.HasPrefix(err.Error(), "invalid configuration:") { | ||
| return nil, errors.New(strings.Replace(err.Error(), "invalid configuration:", "invalid kubeconfig file; clusterctl requires a valid kubeconfig file to connect to the management cluster:", 1)) | ||
| if !strings.HasPrefix(err.Error(), "invalid configuration:") { | ||
| return nil, err |
There was a problem hiding this comment.
any reason to not use controller runtime builtin GetConfig() which handles the fallback for you? i.e.
https://github.com/kubernetes-sigs/controller-runtime/blob/1f5b39fa59d15fae78e521c9c9f2acabbbb3ea17/pkg/client/config/config.go#L60-L75
ctrconfig "sigs.k8s.io/controller-runtime/pkg/client/config"
restConfig, err := ctrconfig.GetConfig()
There was a problem hiding this comment.
Because GetConfig() uses a global "kubeconfig" flag which clusterctl does not set.
|
/lgtm I defer to others when to merge this and if to cherry-pick (or backport). My two cents is to avoid both unless there are specific needs. |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: c6ae222a8723175b32e841445f3cd565718fd954
|
|
Alrighty. Let's merge it I"ll cherry-pick into 1.8, because there is no 1.8 release yet, but not further back (otherwise folks would have to wait until December to get this change) /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
k8s-ci-robot
added
the
approved
|
/cherry-pick release-1.8 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.8 in a new PR and assign it to you. In response to this:
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. |
|
/aera clusterctl |
|
@sbueringer: new pull request created: #11006 In response to this:
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. |
What this PR does / why we need it:
Currently, we cannot use clusterctl binary from inside the pod because it can not find the cluster. This PR updates config generation to use GetConfigOrDie() method from
controller-runtimeso that clusterctl can discover the cluster even kubeconfig is not provided.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 #6286
/area clusterctl
--> area/clusterctl