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

Use kubernetes-client and Kubernetes Service Discovery Enhancements #432

Merged
merged 4 commits into from
Mar 10, 2020

Conversation

ogmaresca
Copy link

A re-do of #231 that uses https://github.com/kubernetes-client/csharp

  • Add support for service annotations to override the defaults on a per-service basis:
  • HealthChecksPath: Override the health check path.
  • HealthChecksPort: Override the health check port if it isn't using the first defined port.
  • HealthChecksScheme: Override the URI scheme (for HTTPS support).
  • Add support for limiting the service discovery to specified namespaces.
  • Intelligent kubernetes client config loading. If the pre-existing Host and Token config fields are not defined, then it'll use the in-cluster config if running in a pod and use the ~/.kube/config file otherwise.
  • Add IPv6 support for ClusterIP addresses.
  • Add ExternalName service type support
  • Add an example for role based access in the README.

@CarlosLanderas
Copy link
Contributor

@ggmaresca this is great!. No further need to add the kubernetes client. Im gonna review this and we merge this and the k8s operator.

@CarlosLanderas
Copy link
Contributor

@ggmaresca I've been testing the PR and looks great :). Can you address the review points?

Thanks!

@ogmaresca
Copy link
Author

@CarlosLanderas I've split out services.AddKubernetesServiceDiscovery();.

@CarlosLanderas
Copy link
Contributor

CarlosLanderas commented Mar 10, 2020

Oh, I've spelled it wrong, it should be
AddKubernetesDiscoveryService to match the appsettings. My fault. Could you change it and add the services null check that produces crashes when no services are found?

With that this should be reeeeeady

@ogmaresca
Copy link
Author

Did the method rename. What do you mean by the service null check?

@sungam3r
Copy link
Collaborator

Perhaps check this IServiceCollection services parameter on null.

}
catch (Exception)
{
_logger.LogError($"Error discovering service {item.Metadata.Name}. It might not be visible");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is more correct? visible or reachable?

try
{
var services = await _discoveryClient.GetServices(_discoveryOptions.ServicesLabel, _discoveryOptions.Namespaces, cancellationToken);
foreach (var item in services.Items)
Copy link
Contributor

Choose a reason for hiding this comment

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

This crashed while testing the branch as no services were found. services NullReference Exception. This should be checked.

Copy link
Contributor

@CarlosLanderas CarlosLanderas left a comment

Choose a reason for hiding this comment

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

Did the method rename. What do you mean by the service null check?

services in services.Items might be null and crash. I've suffered it while testing the branch. It's not your fault, it was already like that but if we fix it we merge this without further thoughts ;)

@CarlosLanderas CarlosLanderas merged commit e076ea0 into Xabaril:master Mar 10, 2020
@CarlosLanderas
Copy link
Contributor

@ggmaresca thanks for the contribution!. It is finally merged ;)

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

Successfully merging this pull request may close these issues.

3 participants