search

kubernetes-sigs / controller-runtime

Repo for the controller-runtime subproject of kubebuilder (sig-apimachinery)
Apache License 2.0
2.37k stars 1.11k forks source link

flag redefined: kubeconfig: allow double vendoring this library but still register flags on behalf of users #878

Open akoserwal opened 4 years ago

akoserwal commented 4 years ago

Error:

 /var/folders/3d/_kgky9bj61g5144z004y51qr0000gp/T/go-build652443801/b001/e2e.test flag redefined: kubeconfig
panic: /var/folders/3d/_kgky9bj61g5144z004y51qr0000gp/T/go-build652443801/b001/e2e.test flag redefined: kubeconfig

goroutine 1 [running]:
flag.(*FlagSet).Var(0xc0000d2120, 0x244c6a0, 0xc00004e250, 0x224908b, 0xa, 0x225fced, 0x12)
    /usr/local/Cellar/go/1.13.8/libexec/src/flag/flag.go:848 +0x4ae
flag.(*FlagSet).StringVar(...)
    /usr/local/Cellar/go/1.13.8/libexec/src/flag/flag.go:751
github.com/operator-framework/operator-sdk/pkg/test.(*frameworkOpts).addToFlagSet(0xc00004e240, 0xc0000d2120)
    /Users/akoserwa/go/pkg/mod/github.com/operator-framework/operator-sdk@v0.15.1/pkg/test/framework.go:102 +0x1bf
github.com/operator-framework/operator-sdk/pkg/test.MainEntry(0xc00040d280)
    /Users/akoserwa/go/pkg/mod/github.com/operator-framework/operator-sdk@v0.15.1/pkg/test/main_entry.go:27 +0x50
github.com/atlasmap/atlasmap-operator/test/e2e.TestMain(...)
       _testmain.go:42 +0x136

Using :- 

framework “github.com/operator-framework/operator-sdk/pkg/test” where it is setting
> flagset.StringVar(&opts.kubeconfigPath, KubeConfigFlag, “”, “path to kubeconfig”)

And in my test case, I am using a library which is also using controller-runtime/operator-sdk github.com/RHsyseng/operator-utils/pkg/utils/openshift

 Method I am calling: openshift.IsOpenShift(framework.KubeConfig)

controller-runtime/pkg/client/config/config.go

// TODO: Fix this to allow double vendoring this library but still register flags on behalf of users
  flag.StringVar(&kubeconfig, “kubeconfig”, “”,
    “Paths to a kubeconfig. Only required if out-of-cluster.“)

I suspect fixing TODO will resolve it? If yes, I would like to work on it.

mengqiy commented 4 years ago

/cc @DirectXMan12

DirectXMan12 commented 4 years ago

So, due to some legacy cruft, controller-runtime expects to be the thing that registers that flag. We probably shouldn't but that's a pretty serious breaking change that I don't think we want to make just yet.

cc @shawn-hurley @camilamacedo86 @joelanford @estroz any of you have insight into what the operatorsdk package is doing that it's re-registering that flag?

DirectXMan12 commented 4 years ago

/assign @camilamacedo86

to answer or triage elsewhere.

estroz commented 4 years ago

The SDK's test framework uses some client-go APIs directly and requires the kubeconfig's namespace, so it needs the kubeconfig path. While the framework could/should be refactored to use pure controller-runtime, which would allow the framework to delegate registering this flag to controller-runtime, we can ~use a local flagset in the mean time~ retrieve the value using a lookup.

PR: https://github.com/operator-framework/operator-sdk/pull/2731

@akoserwal can you test my branch out to make sure it fixes your issue?

akoserwal commented 4 years ago

@estroz: Getting error:

flag provided but not defined: -singleNamespace
Usage of /var/folders/3d/_kgky9bj61g5144z004y51qr0000gp/T/go-build683683410/b001/e2e.test:
  -globalMan string
        path to operator manifest
  -kubeconfig string
        Paths to a kubeconfig. Only required if out-of-cluster.
  -localOperator
        enable if operator is running locally (not in cluster)
  -localOperatorArgs string
        flags that the operator needs (while using --up-local). example: "--flag1 value1 --flag2=value2"
  -master --kubeconfig

private variable: singleNamespaceMode bool

camilamacedo86 commented 4 years ago

Hi @estroz,

Since you are working on it already. /assign @estroz

estroz commented 4 years ago

@akoserwal try out SDK's master.

fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

lrascao commented 2 years ago

For future reference, this is the lookup workaround mentioned by @estroz :

kubeconfig := flag.Lookup("kubeconfig").Value.String()

mvleandro commented 1 year ago

For future reference, this is the lookup workaround mentioned by @estroz :

kubeconfig := flag.Lookup("kubeconfig").Value.String()

Thank you!

stevekuznetsov commented 5 months ago

/reopen

It's been a bit. This is still super annoying for basically anyone who happens to import any part of controller-runtime. Mutating global state in an init() in a library is fairly unpleasant - made worse by the fact that Go will only complain the second time the flag is registered, and because of init() ordering, controller-runtime is always the first one, so finding what just side-effect registered --kubeconfig is impossible.

I'd really suggest another look at this. If this were defined in some runtime code or in code that's unlikely to be imported unless by someone actually using controller-runtime, perhaps it would be less frustrating. In my case, I imported an API module that happened to use the controller-runtime Scheme builder. Really divorced from any runtime considerations.

A workaround for now is to re-set flag.CommandLine in my own init().

cc @joelanford @vincepri @alvaroaleman

k8s-ci-robot commented 5 months ago

@stevekuznetsov: Reopened this issue.

In response to [this](https://github.com/kubernetes-sigs/controller-runtime/issues/878#issuecomment-1898671000): >/reopen > >It's been a bit. This is still super annoying for basically anyone who happens to import any part of `controller-runtime`. Mutating global state in an `init()` in a library is fairly unpleasant - made worse by the fact that Go will only complain the _second_ time the flag is registered, and because of `init()` ordering, `controller-runtime` is always the first one, so finding what just side-effect registered `--kubeconfig` is impossible. > >I'd really suggest another look at this. If this were defined in some runtime code or in code that's unlikely to be imported unless by someone actually using `controller-runtime`, perhaps it would be less frustrating. In my case, I imported an API module that happened to use the `controller-runtime` Scheme builder. Really divorced from any runtime considerations. > >A workaround for now is to re-set `flag.CommandLine` in my own `init()`. > >cc @joelanford @vincepri @alvaroaleman > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-triage-robot commented 4 months ago

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

This bot triages un-triaged issues according to the following rules:

You can:

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

/lifecycle rotten

k8s-triage-robot commented 3 months ago

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

This bot triages issues according to the following rules:

You can:

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

/close not-planned

k8s-ci-robot commented 3 months ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/controller-runtime/issues/878#issuecomment-2004425295): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages issues 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: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
joelanford commented 3 months ago

/label lifecycle/frozen

k8s-ci-robot commented 3 months ago

@joelanford: The label(s) /label lifecycle/frozen cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to [this](https://github.com/kubernetes-sigs/controller-runtime/issues/878#issuecomment-2011049541): >/label lifecycle/frozen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.