Skip to content
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

Fix redefine flag #2992

Merged

Conversation

filintod
Copy link
Contributor

@filintod filintod commented Jul 16, 2023

Description

This PR add something similar to kubernetes-sigs/controller-runtime#1999 to avoid redefined kubeconfig flag issues when using this library plus others that might already have kubeconfig as a flag.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #2993

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

Signed-off-by: Filinto Duran <[email protected]>
@filintod filintod requested review from a team as code owners July 16, 2023 18:33
@filintod filintod changed the title fix redefine flag Fix redefine flag Jul 16, 2023
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

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

Although this PR LGTM, I was not aware of this code and this behavior. It does seem odd that we're defining CLI flags in components-contrib. This seems anti-pattern?

@filintod
Copy link
Contributor Author

It does seem strange maybe to ease tests but don't know the original use case

@daixiang0
Copy link
Member

It does seem strange maybe to ease tests but don't know the original use case

Do we really register it in the CLI?

@ItalyPaleAle
Copy link
Contributor

Looks like it is getting registered. Running daprd --help:

  -kubeconfig string
    	(optional) absolute path to the kubeconfig file (default "/Users/alessandro/.kube/config")

I think this needs to change. We should allow setting the kubeconfig file using the metadata for each component.

@filintod can you change that please?

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

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

Let's refactor this to avoid adding CLI flags to daprd

@filintod
Copy link
Contributor Author

I can follow as how it is done in dapr https://github.com/dapr/dapr/blob/master/utils/utils.go#L50

with a custom kubeconfig env var KUBE_CONFIG

wdty?

@ItalyPaleAle
Copy link
Contributor

@filintod that method looks like is being used by Dapr control plane services only (operator, injector, sentry), and not by daprd.

I would prefer the Kubeconfig path to be a metadata option in all Kubernetes components. This is the pattern we use to configure all components.

Make CLI flag deprecated
Add support for env var

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: ItalyPaleAle <[email protected]>
@filintod
Copy link
Contributor Author

filintod commented Aug 7, 2023

thanks, sorry I forgot about this.

@ItalyPaleAle
Copy link
Contributor

ItalyPaleAle commented Aug 7, 2023

thanks, sorry I forgot about this.

No worries! It became a bit more of a pressure because of dapr/dapr#6766

Do the changes look good to you? Mind doing a quick review?
Essentially I needed to remove the use of flag.Parse because it could cause a panic if the entire app doesn't use the global flag definition.

I plan on doing the 2nd part, which is allowing setting kubeconfig path in components' metadata, in a separate PR.

@ItalyPaleAle ItalyPaleAle merged commit 5bf478a into dapr:master Aug 7, 2023
83 of 84 checks passed
ItalyPaleAle added a commit to ItalyPaleAle/dapr-components-contrib that referenced this pull request Aug 7, 2023
Related to dapr#2992

Passing a kubeconfig path should happen via metadata and not CLI flags, which are deprecated.

Also adds a "defaultNamespace" optional metadata to the Kubernetes secret store.

Signed-off-by: ItalyPaleAle <[email protected]>
@ItalyPaleAle
Copy link
Contributor

See #3060 for part 2

@ItalyPaleAle ItalyPaleAle added this to the v1.12 milestone Aug 15, 2023
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.

Add possibility of using this library with others where kubeconfig flag is also defined
3 participants