Skip to content

Adds ability to use kubeconfig context #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

pawelprazak
Copy link

@pawelprazak pawelprazak commented Jul 20, 2018

Add support for multi-cluster use cases like kubectl does.

  • adds --context parameter to operator-sdk command
  • adds KUBERNETES_CONTEXT env var
  • adds buildConfigWithContext to client.go
  • fixes some error handling

With this you can do e.g.:

operator-sdk up local --kubeconfig=$HOME/.kube/config --context=eu-west-1c-nonprod

I've tried to contribute the buildConfigWithContext upstream but it was rejected:
kubernetes/kubernetes#66329
kubernetes/client-go#192

- adds --context parameter to operator-sdk command
- adds KUBERNETES_CONTEXT env var
- adds buildConfigWithContext to client.go
- fixes some error handling
@hasbro17
Copy link
Contributor

hasbro17 commented Aug 1, 2018

@pawelprazak Sorry for the delay in reviewing this.

Can you explain what happens if we omit the context flag?

operator-sdk up local --kubeconfig=$HOME/.kube/config

Would that result in the current behavior of just using the current-context set in the kubeconfig?

@pawelprazak
Copy link
Author

pawelprazak commented Aug 2, 2018

It uses the current context, at least that's how I've tested it.

I have multiple clusters in my kubeconfig so I'm not sure how to test it properly.

If the param is omitted it will pass an empty current context to the k8s client:
https://github.com/operator-framework/operator-sdk/pull/351/files#diff-ea27639bff6c17384c53f66ced0af2f8R172

@fanminshi
Copy link
Contributor

fanminshi commented Aug 2, 2018

@pawelprazak the pr looks good in general. But I want to see some manual tests.
Can you test the following scenarios?
Case: Without any flags. test the default behavior.
Case: With only --kubeconfig=$HOME/.kube/config to test if current context are used.
Case: With both --kubeconfig=$HOME/.kube/config but a different context than the default one --context=<non current context>
Case: with only context --context=<non current context> but no --kubeconfig=$HOME/.kube/config. to test whether default config file are used.

@pawelprazak
Copy link
Author

operator-sdk up local:

...
FATA failed to get a valid kubeconfig for the provided parameters, invalid configuration: no configuration has been provided 
exit status 1
Error: failed to run operator locally: exit status 1

operator-sdk up local --kubeconfig=$HOME/.kube/config:

...
FATA failed to get a valid kubeconfig for the provided parameters, invalid configuration: no configuration has been provided 
exit status 1
Error: failed to run operator locally: exit status 1

operator-sdk up local --kubeconfig=$HOME/.kube/config --context=eu-west-1c-nonprod:

...
INFO Watching <redacted>/v1alpha1, kind=Render, ns=default, re-sync every 5 seconds

It never worked for me without the context.

@fanminshi
Copy link
Contributor

@pawelprazak It never worked for me without the context.
Hmm, something not right. I expect that operator-sdk up local works with default behavior which uses the $HOME/.kube/config and the current context. Also i expect that the operator-sdk up local --kubeconfig=$HOME/.kube/config defaults to use the current context. Could you investigate why is it failing?

@pawelprazak
Copy link
Author

pawelprazak commented Aug 6, 2018 via email

@sttts
Copy link

sttts commented Aug 6, 2018

Is KUBERNETES_CONTEXT supported by clientcmd today? Does kubectl support it?

@sttts
Copy link

sttts commented Aug 6, 2018

About wrong behaviour: it is well known that invalid mengo library versions lead to contexts being dropped. Please double check that the used mengo revision is exactly the one required by the used client-go's Godeps.json.

@fanminshi
Copy link
Contributor

@pawelprazak could you test out the test scenarios with a default current context?

@pawelprazak
Copy link
Author

@sttts AFAIK kubectl only have --context

@pawelprazak
Copy link
Author

@fanminshi how do I do that, what should be the content of kubeconfig, I need more details if you want me to do more more tests.
In my setup there is 6 clusters and empty current context.

@sttts
Copy link

sttts commented Aug 7, 2018

@pawelprazak can we discuss introducing that context var in clientcmd instead of here? If we add that for operator-sdk we create a incompatible, inconsistent kubeconfig semantics in the ecosystem.

/cc @soltysh @juanvallejo @liggitt @deads2k

@pawelprazak
Copy link
Author

@sttts not sure what do you mean by clientcmd but a patch was rejected upstream kubernetes/kubernetes#66329

@sttts
Copy link

sttts commented Aug 7, 2018

but a patch was rejected upstream kubernetes/kubernetes#66329

The new function was rejected because the override mechanism already supports context overrides. I don't know about the KUBERNETES_CONTEXT discussion.

@deads2k
Copy link

deads2k commented Aug 7, 2018

Is this a client command that is trying to be consistent with consuming a kubeconfig? If so, it seems like you'd want something like https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/genericclioptions/config_flags.go .

@fanminshi
Copy link
Contributor

fanminshi commented Aug 7, 2018

@pawelprazak I believe the command kubectl config use-context sets the current context. I assume that your kubeconfig file has multiple contexts and each of the context represents a cluster. So by invoking kubectl config use-context <context-1> will result the current context to be context-1

also it seems like @sttts have a different opinion on add this new change. Let's discuss more before we move forward.

@pawelprazak
Copy link
Author

pawelprazak commented Oct 16, 2018

Actually, I think, #169 might solve the root of the problem that I was trying to solve.
The actual thing what would be very useful for me would be to provide my own kubernetes client where I can implement the kubeconfig handling the way I need it to get features like --context into my operators.

I don't actually need the CLI to be extended, since I use a custom Makefile instead of it, I've implemented it in this PR for completeness.

return config, err
}

// see clientcmd.BuildConfigFromFlags()
func buildConfigWithContext(kubeconfigPath string, context string) (*rest.Config, error) {
Copy link
Author

@pawelprazak pawelprazak Oct 16, 2018

Choose a reason for hiding this comment

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

FYI: this is the most important change that allows me to implement what I need, the rest is just supporting it

Copy link
Author

@pawelprazak pawelprazak Oct 16, 2018

Choose a reason for hiding this comment

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

from the looks of it, decorating GetConfig function https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/config/config.go#L59 should give me the same result (after controller-runtime refactoring is merged)

Copy link

@juanvallejo juanvallejo Oct 16, 2018

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

sorry, the first link gives me 404

Choose a reason for hiding this comment

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

@hasbro17
Copy link
Contributor

@pawelprazak With the SDK v0.1.0 and controller-runtime, the client and config creation is now handled by the user.
So you should be able to define a custom --context flag for your operator and pass it to operator-sdk up local:

$ operator-sdk up local --operator-flags "--context eu-west-1c-nonprod"

And use that context to build your kubeconfig and pass it to your reconciler's client.
https://github.com/operator-framework/operator-sdk/blob/master/doc/user/client.md#non-default-client

This way the SDK probably doesn't need to introduce new CLI changes or env vars.

@pawelprazak
Copy link
Author

Thank you, that's what I had in mind. Closing now.

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.

6 participants