-
Notifications
You must be signed in to change notification settings - Fork 2k
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 new Consul fields on ConsulUpstream #16745
Add new Consul fields on ConsulUpstream #16745
Conversation
Any update on this? |
@ vvarga007 sorry about the delay! - We're picking this up this week or next |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkazmierczak this is my review, let me know your thoughts on the changes and I can push them if we agree. I think I have also found a problem when making modifications to the upstream parameters which are not passed on to Consul.
@@ -0,0 +1,3 @@ | |||
```release-note:improvement | |||
connect: Added support for DestinationPeer DestinationType LocalBindSocketPath LocalBindSocketMode on upstream block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connect: Added support for DestinationPeer DestinationType LocalBindSocketPath LocalBindSocketMode on upstream block | |
consul/connect: Added support for `DestinationPeer`, `DestinationType`, `LocalBindSocketPath`, and `LocalBindSocketMode` on upstream block |
@@ -203,13 +203,19 @@ func (c *ConsulMeshGateway) Copy() *ConsulMeshGateway { | |||
} | |||
} | |||
|
|||
type UpstreamDestType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this custom type and just use a string for the struct parameter like we do with the other new additions. I believe it is originally done this way as the Consul API package does this.
@@ -1461,6 +1461,8 @@ func (c *ConsulMeshGateway) Validate() error { | |||
} | |||
} | |||
|
|||
type UpstreamDestType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as with the API package.
- `destination_name` `(string: <required>)` - Name of the upstream service. | ||
- `destination_namespace` `(string: <required>)` - Name of the upstream Consul namespace. | ||
- `destination_peer` `(string: "")` - Name of the name of the peer cluster containing the upstream service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `destination_peer` `(string: "")` - Name of the name of the peer cluster containing the upstream service. | |
- `destination_peer` `(string: "")` - Name of the peer cluster containing the upstream service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, thanks for the contribution!
Apart from @jrasell's comments, there's a few things missing that make updating new Upstream fields problematic.
- You need to update
upstreamsDifferent
, and - You need to add new fields to the hashing function
hashConnect
.
Otherwise job updates won't work correctly.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Description
Changes
ConsulUpstream
structs and updated their receiver functions.DestinationPeer
DestinationType
LocalBindSocketPath
LocalBindSocketMode
jobspec/parse_service.go
command/agent/job_endpoint.go
Testing Changes
To test these changes you need to build a Nomad binary and deploy a cluster with both Nomad and Consul agents and clients.
When you have your cluster running you can run the following job-spec:
Once the job is running and healthy, use Consul's API to get the terminating gateway's configuration:
curl --request GET http://127.0.0.1:8500/v1/catalog/service/count-dashboard-sidecar-proxy | json_pp
You will get the following JSON output:
Demo Video
ConsulUpstream.mp4