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

[Bugfix] Add the missing Service TaggedAddresses and Check Type fields to Txn API #22164

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joelczou
Copy link

@joelczou joelczou commented Feb 14, 2025

Description

Update: looks like there is an issue with API package unable to unmarshall TxnResult when the service has TaggedAddresses set. This issue should be resolved first before this PR.

Consul Transaction API Service Operation does not register the TaggedAddresses field in the AgentService struct that is sent from the client. Same with the Type field in the HealthCheck struct.

Testing & Reproduction steps

  1. Create a Transaction API input file txn.json
[
{
    "Node": {
        "Verb": "set",
        "Node": {
            "Node": "n1",
            "Address": "192.168.0.10"
        }
    }
},
{
    "Service": {
        "Verb": "set",
        "Node": "n1",
        "Service": {
            "ID": "s1",
            "Service": "s1",
            "Address": "192.168.0.10",
            "TaggedAddresses": {
                "lan": {
                    "Address": "192.168.0.10",
                    "Port": 8080
                }
            },
            "Port": 8080
        }
        }
},
{
    "Check": {
        "Verb": "set",
        "Check": {
            "Node": "n1",
            "CheckID": "healthcheck",
            "Name": "healthcheck",
            "Status": "passing",
            "ServiceID": "s1",
            "ServiceName": "s1",
            "Definition": {
                "HTTP": "http://localhost:8080",
                "Interval": "10s"
            },
            "Type": "http"
        }
    }
}
]
  1. Start Consul in Agent mode.
docker run -p 8500:8500 hashicorp/consul:1.20
  1. Submit the transaction, returns success but Service TaggedAddresses and Check Type are not registered.
curl --request PUT --data @txn2.json http://127.0.0.1:8500/v1/txn

{
    "Results": [
        {
            "Node": {
                "ID": "",
                "Node": "n1",
                "Address": "192.168.0.10",
                "Datacenter": "dc1",
                "TaggedAddresses": null,
                "Meta": null,
                "CreateIndex": 18,
                "ModifyIndex": 18
            }
        },
        {
            "Service": {
                "ID": "s1",
                "Service": "s1",
                "Tags": null,
                "Address": "192.168.0.10",
                "Meta": null,
                "Port": 8080,
                "Weights": {
                    "Passing": 1,
                    "Warning": 1
                },
                "EnableTagOverride": false,
                "Proxy": {
                    "Mode": "",
                    "MeshGateway": {},
                    "Expose": {}
                },
                "Connect": {},
                "PeerName": "",
                "CreateIndex": 18,
                "ModifyIndex": 18
            }
        },
        {
            "Check": {
                "Node": "n1",
                "CheckID": "healthcheck",
                "Name": "healthcheck",
                "Status": "passing",
                "Notes": "",
                "Output": "",
                "ServiceID": "s1",
                "ServiceName": "s1",
                "ServiceTags": null,
                "Type": "",
                "Interval": "",
                "Timeout": "",
                "ExposedPort": 0,
                "Definition": {
                    "Interval": "10s",
                    "HTTP": "http://localhost:8080"
                },
                "CreateIndex": 18,
                "ModifyIndex": 18
            }
        }
    ],
    "Errors": null

Relevant Unit Test:
https://github.com/jzou-rbx/consul/blob/4d76a43dc6d0ad9dacd6079c7b34d128baad12c7/agent/txn_endpoint_test.go#L533
does not test for the Type field in a check.

https://github.com/jzou-rbx/consul/blob/4d76a43dc6d0ad9dacd6079c7b34d128baad12c7/agent/txn_endpoint_test.go#L703 does not test for the TaggedAddresses field in a regular service.

The test build after the PR now work properly to include these fields when submitting the same input above to the Transaction API.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

1 similar comment
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@joelczou joelczou marked this pull request as ready for review February 14, 2025 17:15
@joelczou joelczou requested a review from a team as a code owner February 14, 2025 17:15
@joelczou
Copy link
Author

Would like to have backport/all label for this.

@joelczou joelczou changed the title Add the missing Service TaggedAddresses and Check Type fields to Txn API [Bugfix] Add the missing Service TaggedAddresses and Check Type fields to Txn API Feb 14, 2025
@joelczou joelczou marked this pull request as draft February 21, 2025 01:56
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.

2 participants