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

Add Service Fabric Provider #2117

Merged
merged 59 commits into from
Nov 27, 2017
Merged

Add Service Fabric Provider #2117

merged 59 commits into from
Nov 27, 2017

Conversation

lawrencegripper
Copy link
Contributor

Description

Here is our PR to add a Service Fabric (SF) Provider into Traefik. Reference issue: #2090

We’ve documented how the provider can be used. We wanted to check whether it was best to include these docs in the PR or to have them hosted externally. We have a basic usage intro and an advanced usage intro doc.

Notes:

For simple routing, we use Service Fabric's Application Parameters to define which services should be exposed. The provider picks up these parameters and then builds default frontends for each.

For more complex Traefik routing, we use a .tmpl file that can be pushed to SF’s configuration system. This is then read from disk by the provider and used to customise the Traefik setup. As such, the provider checks if a .tmpl file path has been provided, if not, is uses a glob search in the SF configuration directory to find the newest configuration available. Please note we have built this primarily for Windows based Service Fabric clusters. Testing on various cluster configurations is on going. We also plan to investigate supporting Service Fabric on Linux soon too.

Me and @jjcollinge both looking forward to hearing your feedback

Here is a quick screenshot of it up and running:

screen shot 2017-09-13 at 18 08 11

@juliens
Copy link
Member

juliens commented Sep 21, 2017

Thanks for this PR.

Here are my firsts remarks.

The others providers handle template in a different way than you do.

There is a default template which is embed in the traefik binary, and you can override this template with another by using the filename parameter in the provider configuration.
In order to have this mechanism, you only have to call GetConfiguration (it handles automatically the template overload)

Next, for example, in the Docker provider, there is no predefined Rule, the Rule is set by reading the container labels

WDYT about using the Parameter for settings the Rule Traefik will use ?

For example:

<Parameters>
    ...
    <Parameter Name="traefik.rule" Value="PathPrefix:/my-services" />
    ...
</Parameters>

Moreover you can see that the docker provider handles a lots of other configuration with labels

WDYT about creating an external repo containing your Service Fabric client ? It would be easier to manage and could be reused by other developers.

Feel free to join us on slack

@jjcollinge
Copy link
Contributor

Hey @juliens, thanks for the feedback. Myself and @lawrencegripper are currently travelling with work but it'd be good to have a chat on Slack next week to discuss the PR in a bit more depth - would that work for you?

@juliens
Copy link
Member

juliens commented Sep 21, 2017

Sure, don't hesitate to ping me 😉

@naeemkhedarun
Copy link

This is fantastic, thanks for your hard work! I'm giving the integration a go now with our clusters 👍

@lawrencegripper
Copy link
Contributor Author

@juliens Thanks for the feedback, we'd like to accept both of the suggestions.

We've chatted further and agree that removing the 'findtemplate' function is best way forward. We will go back to using the filename passed into the provider, like the other providers. We can change the deployment model in SF to have roughly the same effect when updating the template.

I like the idea of allowing parameters for defining rules. This also reduces the need for custom template files. As the parameters sit in the application manifest, with the application containing multiple services, things would get a bit messy if we used it. I'll investigate to see if we can find a way to do this at the service level rather than the application manifest, we did look at this option before but may have missed something which would allow us to do it.

Lastly we'll look at pulling out the ServiceFabric Client code into a separate repository to keep things clean.

@ldez ldez removed the bot/no-merge label Nov 22, 2017
@ldez
Copy link
Contributor

ldez commented Nov 25, 2017

The code has been moved here

server/server.go Outdated
@@ -559,6 +559,9 @@ func (s *Server) configureProviders() {
if s.globalConfiguration.DynamoDB != nil {
s.providers = append(s.providers, s.globalConfiguration.DynamoDB)
}
if server.globalConfiguration.ServiceFabric != nil {
Copy link
Contributor Author

@lawrencegripper lawrencegripper Nov 27, 2017

Choose a reason for hiding this comment

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

@ldez - I think this should be 's' rather than 'server'. Happy for me to push a fix?

Copy link
Contributor

@ldez ldez Nov 27, 2017

Choose a reason for hiding this comment

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

oh yes, I missed that 😄

Copy link
Contributor

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

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

@uditjh
Copy link

uditjh commented Dec 21, 2017

what is the networking mode to be used by the applications when load balancing them using traefik?
https://docs.microsoft.com/en-us/azure/service-fabric/service-fabric-networking-modes

@ldez
Copy link
Contributor

ldez commented Dec 21, 2017

You can go to the dedicate channel (#service-fabric) in the Traefik community Slack channel to ask question if you want.

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.

9 participants