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(logging): correctly detect GKE resource #4092

Merged
merged 11 commits into from May 19, 2021
Merged

Conversation

@0xSage
Copy link
Contributor

@0xSage 0xSage commented May 12, 2021

Fixes: #4079

Changes:

  • Refactored resource autodetection logic to new file resource.go
  • Fixes previous incorrect detection where GKE logs were labeled as GCE. Labels still missing container_name because there's no unhacky way to get this info.
  • End to end GKE environment tests are now available for this here

Decisions:

@0xSage 0xSage marked this pull request as ready for review May 13, 2021
@0xSage 0xSage requested review from as code owners May 13, 2021
Copy link
Contributor

@daniel-sanche daniel-sanche left a comment

Labels still missing container_name because there's no unhacky way to get this info.

One (imperfect) option would be to try to detect CONTAINER_NAME (or something similar) and instruct users to populate it using the downward API

This could be useful as a second option for pod_name and namespace_name too, in case there are edge cases where the current heuristics don't work

logging/resource.go Show resolved Hide resolved
if err != nil {
return nil
}
namespaceBytes, err := ioutil.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
Copy link
Contributor

@daniel-sanche daniel-sanche May 13, 2021

Choose a reason for hiding this comment

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

I don't think we can guarantee this file will always be accessible. It looks like you're already doing error checking so it should be fine, just wanted to double check

Copy link
Contributor Author

@0xSage 0xSage May 14, 2021

Choose a reason for hiding this comment

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

correct, it will just be "" if readfile fails. We can rely on this to exist majority of the time though.

its a part of kubernetes downward api contract:
https://kubernetes.io/docs/tasks/access-application-cluster/access-cluster/#accessing-the-api-from-a-pod
Finally, the default namespace to be used for namespaced API operations is placed in a file at /var/run/secrets/kubernetes.io/serviceaccount/namespace in each container.

Copy link
Contributor

@daniel-sanche daniel-sanche May 18, 2021

Choose a reason for hiding this comment

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

Ok cool, yeah I just wanted to make sure all the necessary error-checking was done since I'm not super familiar with go. But it looks like it should be!

Labels: map[string]string{
"project_id": projectID,
"location": regionFromZone(zone),
"service_name": os.Getenv("K_SERVICE"),
Copy link
Contributor

@daniel-sanche daniel-sanche May 13, 2021

Choose a reason for hiding this comment

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

There are a lot of repeated magic strings in this file. Maybe turn these into constants?

Copy link
Contributor Author

@0xSage 0xSage May 14, 2021

Choose a reason for hiding this comment

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

Agree magic numbers are not great. I actually wrote one iteration with constants (camel case). But, since resource.labels have to be in snake case, I liked the explicit nature of having the snake-case keys inline with the variable setting.

Also go is statically typed, I like the idea of the compiler not having to know all the constants that the switch case might not end up accessing.

Copy link
Contributor

@daniel-sanche daniel-sanche May 18, 2021

Choose a reason for hiding this comment

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

I was also thinking of things like K_SERVICE, which are used in multiple places. For Python I put these at the top, but I think it's fine as-is too

logging/resource.go Outdated Show resolved Hide resolved
logging/resource.go Show resolved Hide resolved
@0xSage
Copy link
Contributor Author

@0xSage 0xSage commented May 14, 2021

Labels still missing container_name because there's no unhacky way to get this info.

One (imperfect) option would be to try to detect CONTAINER_NAME (or something similar) and instruct users to populate it using the downward API

This could be useful as a second option for pod_name and namespace_name too, in case there are edge cases where the current heuristics don't work

Adopted! @daniel-sanche, mind taking another look and stamping? Thank you.

@0xSage 0xSage requested a review from daniel-sanche May 14, 2021
@gcf-merge-on-green
Copy link
Contributor

@gcf-merge-on-green gcf-merge-on-green bot commented May 14, 2021

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

@0xSage 0xSage merged commit a2538e1 into master May 19, 2021
3 checks passed
@0xSage 0xSage deleted the gkeResourceAutodetection branch May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants