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

Multiple frontends for consulcatalog #3796

Merged
merged 3 commits into from Aug 27, 2018

Conversation

hsmade
Copy link
Contributor

@hsmade hsmade commented Aug 20, 2018

Motivation

#3597
We are facing the issue that our consul services have routes for multiple domains and share the fqdn between services. This means we would need more than one rule to express the routes for each service. This PR enables us to do so.

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

This is probably better implemented as an 'OR' for route rules. However, with the upcoming rewrite of the route rule syntax, this will have to wait. This PR implements segments on frontends for the consul catalog provider. It allows to create multiple frontends per service.

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.

Thanks for your contribution @hsmade
Few remarks ;)

@@ -151,6 +151,12 @@ Additional settings can be defined using Consul Catalog tags.
| `<prefix>.frontend.whiteList.sourceRange=RANGE` | Sets a list of IP-Ranges which are allowed to access.<br>An unset or empty list allows all Source-IPs to access. If one of the Net-Specifications are invalid, the whole list is invalid and allows all Source-IPs to access. |
| `<prefix>.frontend.whiteList.useXForwardedFor=true` | Uses `X-Forwarded-For` header as valid source of IP for the white list. |

### Multiple frontends for a single service

This comment was marked as outdated.

Attributes: service.Attributes,
TraefikLabels: service.TraefikLabels,
})
// loop over children of <prefix>.frontends.*
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new line before please

TraefikLabels: frontend.Labels,
})
}
return frontends
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new line before please

}
func getSegments(path string, prefix string, tree map[string]string) []*frontendSegment {
segments := make([]*frontendSegment, 0)
// FIXME: do this more efficient..
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename this FIXME

segmentNames[strings.SplitN(strings.TrimPrefix(key, path+"."), ".", 2)[0]] = true
}
}
// get labels for each segment found
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new line before please

Labels: labels,
})
}
return segments
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a new line before please

@hsmade
Copy link
Contributor Author

hsmade commented Aug 22, 2018

Reformatted as requested, thanks for your review!

@mmatur mmatur added kind/enhancement a new or improved feature. and removed contributor/waiting-for-corrections labels Aug 23, 2018
@mmatur mmatur force-pushed the consulcatalog-segmentation branch 2 times, most recently from 8853766 to 1d810d0 Compare August 24, 2018 16:20
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 👏

Thanks for your contribution @hsmade. I added integration tests and I rebased your PR.

@ldez ldez changed the title Implement segments for consulcatalog multiple frontends for consulcatalog Aug 27, 2018
Copy link
Member

@ldez ldez 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

@hsmade
Copy link
Contributor Author

hsmade commented Aug 27, 2018

Thanks for the merge!

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.

None yet

5 participants