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: ClientConfig marshals incorrectly #3896

Closed
wants to merge 1 commit into from
Closed

Conversation

free6om
Copy link

@free6om free6om commented Dec 26, 2023

WHY

the output of yaml.Marshal(&ClientConfig{}) is incorrect, this PR fixed this error.

How to reproduce it

  1. build a ClientConfig object clientConfig with Proxies field set
  2. call yaml.Marshal(&clientConfig) and set the output to yamlConfig
  3. call config.LoadConfigure(yamlConfig, &clientConfig, false) to unmarshal the output
  4. check the new unmarshalled clientConfig, the Proxies part is lost

@fatedier
Copy link
Owner

Where is the yaml.Marshal?

@free6om
Copy link
Author

free6om commented Dec 26, 2023

Where is the yaml.Marshal?

I use yaml pkg from sigs.k8s.io/yaml

@fatedier
Copy link
Owner

I don't really want to solve a problem that is not being used in this project.

@free6om
Copy link
Author

free6om commented Dec 26, 2023

here is the story:
I'm a K8s application developer and familiar with yaml format. And I prepare to use frp as a sub-system of my application, in which the other sub-systems talks with frpc through the config file.

I tried to update the yaml format frpc config file programmatically by calling sigs.k8s.io/yaml Marshal function, but the output seemed not as expected.

After digging the source code, I think there is a missing MarshalJSON function and shot this PR to fix it.

@free6om
Copy link
Author

free6om commented Dec 26, 2023

I don't really want to solve a problem that is not being used in this project.

thanks for the quick reply.
any suggestions to add a proxy to the frpc config file programmatically? if this PR is not the correct way.

@fatedier
Copy link
Owner

Sounds good.

But the problem is that this expands the external commitments of this portion of code. It means that we need to always ensure the consistency of this behavior. It may be achievable now, but will it bring difficulties for future changes? I'm not entirely sure.

Additionally, it seems that you haven't modified the usage of all these types of structures, such as visitor and plugin configurations. This is also one of my concerns. For the cases that are not used in the project, we lack sufficient test cases to ensure consistent behavior.

@free6om
Copy link
Author

free6om commented Dec 26, 2023

Sounds good.

But the problem is that this expands the external commitments of this portion of code. It means that we need to always ensure the consistency of this behavior. It may be achievable now, but will it bring difficulties for future changes? I'm not entirely sure.

Additionally, it seems that you haven't modified the usage of all these types of structures, such as visitor and plugin configurations. This is also one of my concerns. For the cases that are not used in the project, we lack sufficient test cases to ensure consistent behavior.

make sense to me.
I found a rather ugly but quick fix, for whom has the same requirement:

// clientConfigExt extends the `frp.ClientConfig` to support the correct marshal function.
type clientConfigExt struct {
	frp.ClientConfig `json:",inline"`
	Proxies          []frp.TCPProxyConfig `json:"proxies,omitempty"`
}

@free6om free6om closed this Dec 26, 2023
@fatedier
Copy link
Owner

If the proxy config includes the configuration of a plugin, there may still be issues.

@swghosh
Copy link

swghosh commented Aug 15, 2024

@free6om any workaround to further support both []TCPProxyConfig and []UDPProxyConfig in the "proxies"?

I'm a K8s application developer and familiar with yaml format. And I prepare to use frp as a sub-system of my application, in which the other sub-systems talks with frpc through the config file.

Exactly, my use case too :) and am stuck on the same problem!

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.

3 participants