-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
"linkerd inject --manual" changes types of label values (string becomes float64) #3983
Comments
This might also be problematic for labels that has |
@grampelberg , i would like to work on this. |
@adrijshikhar awesome! Go for it! |
What does inject do basically? |
@adrijshikhar I would recommend going through the getting started tutorials and spending some time in the docs to answer that question =) |
@adrijshikhar Are you still working on this issue? If not, I would like to work on it. |
no i am not |
@grampelberg is this assigned to anyone? If not, then please assign this to me, I would like to give this issue a try. |
@Vineet-Sharma29 are you still working on this? |
@grampelberg No |
@grampelberg @ihcsim According to me there is somethings wrong with the "transform" function in cli/cmd/inject.go, specifically here: https://github.com/linkerd/linkerd2/blob/master/cli/cmd/inject.go#L193-L208. i.e. when interconversion occurs from yaml to json and then back to yaml. |
As @saurav-malani mentioned, It's not straight forward from what we discussed. @ihcsim Do you have some ideas on how to fix this? |
@Pothulapati @saurav-malani Can you help me to extract out that block of code into an unexported function and add some unit test to help identify which library is acting weird? Maybe something like: func applyPatch(original, patchJSON []byte) ([]byte, error) {
patch, err := jsonpatch.DecodePatch(patchJSON)
if err != nil {
return nil, err
}
origJSON, err := yaml.YAMLToJSON(original)
if err != nil {
return nil, err
}
injectedJSON, err := patch.Apply(original)
if err != nil {
return nil, err
}
return conf.JSONToYAML(injectedJSON)
} There is a good example on how to set up your unit test with simple inputs in the |
@ihcsim I commented out this portion and returned injectedJSON instead of injectedYAML. And, the output corresponding to it is exactly same as expected i.e. integers are not under double quotes and rest under double quotes. So, I believe there is nothing wrong till L208 and probably something goes wrong afterwards, specifically here: |
Are you able to track down what's going on in |
@ihcsim: cant seem to repro this using example above with build from latest commit on master. i wonder if it was fixed with a dependency upgrade or something. haven't actually sifted through the PRs.
|
Can confirm the issue in |
@adnxn You are right. I confirm that issue has been fixed due to dependency upgrade in #4221 Upstream PR - go-yaml/yaml#171 @KIVagant Latest stable release doesn't include that patch. |
Three years on (🤦♂) I've confirmed that our current Linkerd doesn't do this any more. Many thanks to everyone who helped out here!! |
Bug Report
What is the issue?
After
linkerd inject --manual
is applied, all Deployment labels lose double quotes around label values which causes problems with unobvious types.How can it be reproduced?
kubectl apply -f tmp-2.yaml
and make sure it works. New pods will start, for example:linkerd inject --manual tmp-2.yaml
As you can see here, all labels (and not only labels) lost double quotes around values. Now try to apply it:
Logs, error output, etc
Environment
Possible solution
Add quotes in yaml templates everywhere by default when possible.
Workaround:
Update the commit id in any way, if it is a commit id causing the problem.
The text was updated successfully, but these errors were encountered: