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 new Consul fields on ConsulUpstream #16745

Conversation

horaciomonsalvo
Copy link
Contributor

Description

Changes

  • Added support for Upstream configuration:
    • Added new fields to ConsulUpstream structs and updated their receiver functions.
      • DestinationPeer
      • DestinationType
      • LocalBindSocketPath
      • LocalBindSocketMode
  • Updated jobspec/parse_service.go
  • Updated command/agent/job_endpoint.go
  • Update related tests
  • Update Nomad Documentation for ConsulUpstream struct.

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:

job "countdash" {
  datacenters = ["dc1"]

  group "dashboard" {
    network {
      mode = "bridge"

      port "http" {
        static = 9002
        to     = 9002
      }
    }

    service {
      name = "count-dashboard"
      port = "9002"

      connect {
        sidecar_service {
          proxy {
            upstreams {              
              destination_name = "count-api"
              local_bind_port  = 8080
              datacenter = "dc2"
              local_bind_address = "127.0.0.1"
              # New Fields:
              local_bind_socket_mode = "foo"
              destination_type = "prepared_query"                
              mesh_gateway {
                mode = "local"
              }
            }
          }
        }
      }
    }

    task "dashboard" {
      driver = "docker"

      env {
        COUNTING_SERVICE_URL = "http://${NOMAD_UPSTREAM_ADDR_count_api}"
      }

      config {
        image = "hashicorpdev/counter-dashboard:v3"
      }
    }
  }
}

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:

[
   {
      "ServiceAddress" : "172.31.21.166",
      "ServiceID" : "_nomad-task-c660c439-42fb-934f-cb02-a6153b64f6cb-group-dashboard-count-dashboard-9002-sidecar-proxy",
      "ServiceTaggedAddresses" : {
         "lan_ipv4" : {
            "Address" : "172.31.21.166",
            "Port" : 20233
         },
         "wan_ipv4" : {
            "Port" : 20233,
            "Address" : "172.31.21.166"
         },
         "consul-virtual" : {
            "Port" : 20233,
            "Address" : "240.0.0.2"
         }
      },
      "ServiceKind" : "connect-proxy",
      "TaggedAddresses" : {
         "wan_ipv4" : "172.31.21.166",
         "lan_ipv4" : "172.31.21.166",
         "wan" : "172.31.21.166",
         "lan" : "172.31.21.166"
      },
      "ServiceSocketPath" : "",
      "ID" : "9cab1f92-ea84-fe9a-9fd4-699c5dcef3a9",
      "NodeMeta" : {
         "consul-network-segment" : ""
      },
      "ServiceMeta" : {
         "external-source" : "nomad"
      },
      "Node" : "ip-172-31-21-166",
      "ServiceWeights" : {
         "Passing" : 1,
         "Warning" : 1
      },
      "ServicePort" : 20233,
      "CreateIndex" : 8812,
      "ServiceConnect" : {},
      "Address" : "172.31.21.166",
      "ModifyIndex" : 8812,
      "ServiceTags" : [],
      "Datacenter" : "dc1",
      "ServiceProxy" : {
         "Config" : {
            "bind_address" : "0.0.0.0",
            "envoy_stats_tags" : [
               "nomad.alloc_id=c660c439-42fb-934f-cb02-a6153b64f6cb",
               "nomad.group=dashboard",
               "nomad.job=countdash",
               "nomad.namespace=default"
            ],
            "bind_port" : 20233
         },
         "MeshGateway" : {},
         "DestinationServiceID" : "_nomad-task-c660c439-42fb-934f-cb02-a6153b64f6cb-group-dashboard-count-dashboard-9002",
         "Upstreams" : [
            {
               "LocalBindAddress" : "127.0.0.1",
               "Datacenter" : "dc2",
               "DestinationName" : "count-api",
               "LocalBindPort" : 8080,
               "MeshGateway" : {
                  "Mode" : "local"
               },
               "DestinationType" : "prepared_query",
               "LocalBindSocketMode" : "foo"
            }
         ],
         "LocalServicePort" : 9002,
         "DestinationServiceName" : "count-dashboard",
         "Expose" : {},
         "Mode" : "",
         "LocalServiceAddress" : "127.0.0.1"
      },
      "ServiceEnableTagOverride" : false,
      "ServiceName" : "count-dashboard-sidecar-proxy"
   }
]

Demo Video

ConsulUpstream.mp4

@vvarga007
Copy link

Any update on this?

@jrasell jrasell self-assigned this Sep 5, 2023
@mikenomitch
Copy link
Contributor

@ vvarga007 sorry about the delay! - We're picking this up this week or next

Copy link
Member

@jrasell jrasell left a 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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Member

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
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `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.

Copy link
Contributor

@pkazmierczak pkazmierczak left a 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.

  1. You need to update upstreamsDifferent, and
  2. You need to add new fields to the hashing function hashConnect.

Otherwise job updates won't work correctly.

Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants