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

Basic Connect support for service registration on the agent #216

Merged
merged 4 commits into from
May 10, 2023

Conversation

firerain-fd
Copy link
Contributor

Added basic Consul Connect support when registering service on agent.
https://developer.hashicorp.com/consul/docs/connect/registration/sidecar-service

Added properties.
AgentServiceRegistration class:
Added properties.
Added classes:
AgentServiceConnect, AgentServiceProxy.
Added tests.
Copy link
Contributor

@mfkl mfkl left a comment

Choose a reason for hiding this comment

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

Looking good overall. Some optional fields are left to be implemented but this can be done in a follow up PR, whenever needed by users.

Thanks for the contribution.

Consul/Agent.cs Outdated
}

/// <summary>
/// AgentServiceProxy specifies the configuration for a Connect service proxy instance.This is only valid if Kind defines a proxy or gateway
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space needed after .

@@ -139,6 +139,9 @@ public class AgentService
public IDictionary<string, ServiceTaggedAddress> TaggedAddresses { get; set; }
public bool EnableTagOverride { get; set; }
public IDictionary<string, string> Meta { get; set; }

[JsonProperty(NullValueHandling = NullValueHandling.Ignore)] // If the Proxy property is serialized to have null value, a protocol error occurs when registering the service through the catalog (catalog/register) during an http request.
public AgentServiceProxy Proxy { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

While this seems to work fine without, maybe for clarity, it would be good to add the Kind property https://developer.hashicorp.com/consul/api-docs/agent/service

Kind (string: "") - The kind of service. Defaults to "" which is a typical Consul service. This value may also be "connect-proxy" for Connect proxies representing another service, "mesh-gateway" for instances of a mesh gateway, "terminating-gateway" for instances of a terminating gateway, or "ingress-gateway" for instances of a ingress gateway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. I added a Kind property of type ServiceKind.

@mfkl mfkl requested a review from marcin-krystianc May 10, 2023 05:23
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for your contribution. I've just commented on some extra empty lines (2 or more), would be good to remove them.

Consul/Agent.cs Outdated
public static ServiceKind TerminatingGateway => Map["terminating-gateway"];
public static ServiceKind IngressGateway => Map["ingress-gateway"];


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Unnecessary extra new lines.

Consul/Agent.cs Outdated

string Value { get; }


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Unnecessary extra new lines.

Consul/Agent.cs Outdated

ServiceKind(string value) => Value = value;


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Unnecessary extra new lines.

new object[] { "Ingress-gateway", true, ServiceKind.IngressGateway },
};


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Unnecessary extra new lines.

@marcin-krystianc marcin-krystianc merged commit 0bc4948 into G-Research:master May 10, 2023
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