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

Reduce logs with new Kubernetes security annotations #2506

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

ldez
Copy link
Contributor

@ldez ldez commented Dec 1, 2017

What does this PR do?

Reduce logs with new Kubernetes security annotations.

Motivation

Limit the number of unneeded logs.

More

  • Added/updated tests

Additional Notes

Fixes #2500

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

@timoreimann timoreimann self-requested a review December 1, 2017 06:42
@timoreimann
Copy link
Contributor

I spot one or two things that we may want to improve. Will have a closer look later, please don't merge just yet.

if annotation := meta.Annotations[name]; annotation != "" {
for _, v := range strings.Split(annotation, ",") {
pair := strings.Split(v, ":")
func getMapAnnotation(meta *v1beta1.Ingress, labelName string) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

label has a different connotation in Kubernetes. We should continue to call it name, or go with either annotationName or annotName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently I'm rewriting all labels/annotations system for all providers.
This function is extracted from this work.
I can change for now, but in the future we don't gonna separate labels and annotations.

}
}

if len(mapValue) == 0 {
log.Errorf("Could not load %q, skipping...", labelName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the log messages could be clearer with regards to what the user did wrong, both in terms of wording and how we log.

My suggestion would be to do the following:

  • Do a check against the length of values in line 47 and bail out immediately when it's zero along with a log.Errorf("Missing value for annotation %q", values).
  • Instead of logging an error right away in line 51, collect all illegal pairs first and print them in a comma-joint list inside a single log.Warnf().

WDYT?

Copy link
Contributor Author

@ldez ldez Dec 1, 2017

Choose a reason for hiding this comment

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

I agree with the first point.
For the second point, it does not seem necessary because this create a complexity only for logging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: the second point: My reasoning is that we collect all illegal pairs and show them to the user in a more concise manner.

It'd be nice to have IMHO, though not strictly necessary.

@ldez ldez force-pushed the fix/k8s-logs branch 3 times, most recently from 7dc4c4a to aac1fcd Compare December 1, 2017 10:59
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants