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

fix(parser) bound v6 targets properly #4391

Merged
merged 4 commits into from
Jul 26, 2023
Merged

fix(parser) bound v6 targets properly #4391

merged 4 commits into from
Jul 26, 2023

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 21, 2023

What this PR does / why we need it:

Surround IPv6 Endpoint addresses with brackets when generating a target. Without these, the target port suffix parses as part of the address and assigns an incorrect address and (default) port.

Which issue this PR fixes:

Fixes #4369. Thanks @pnathan for locating the problem section of the parser code for us!

Special notes for your reviewer:

CI doesn't do v6-only at present. We should maybe consider that as a nightly test. There are quite possibly other v6-only problems. We don't have the scaffolding to actually do that yet though, so spot testing it is!

Local testing using https://kind.sigs.k8s.io/docs/user/configuration/#ip-family was to confirm via the stock image:

$ curl -ksv "https://[fc00:f853:ccd:e793::50]:8444/upstreams/httpbin.default.80.svc/targets"  | jq

{
  "data": [
    {
      "weight": 100,
      "tags": null,
      "id": "97177439-6e38-5366-9aae-415d0fed84fe",
      "target": "[fd00:0010:0244:0000:0000:0000:000e:0080]:8000",
      "created_at": 1689977170.046,
      "updated_at": 1689977170.046,
      "upstream": {
        "id": "5e5b8bb7-173e-5982-bbe4-56faf6a09264"
      }
    }
  ],
  "next": null
}

$ kubectl get endpointslices.discovery.k8s.io 
NAME            ADDRESSTYPE   PORTS   ENDPOINTS               AGE
httpbin-6jsgl   IPv6          80      fd00:10:244::e          12m
kubernetes      IPv6          6443    fc00:f853:ccd:e793::2   41m

After putting an image with this change in place, no more errant 80 octet, or whatever you call v6 segments:

$ curl -ksv "https://[fc00:f853:ccd:e793::50]:8444/upstreams/httpbin.default.80.svc/targets"  | jq
{
  "data": [
    {
      "updated_at": 1689978524.721,
      "id": "d347916d-a502-5b01-94bf-1be634433a7e",
      "weight": 100,
      "upstream": {
        "id": "5e5b8bb7-173e-5982-bbe4-56faf6a09264"
      },
      "target": "[fd00:0010:0244:0000:0000:0000:0000:000e]:80",
      "created_at": 1689978524.721,
      "tags": null
    }
  ],
  "next": null
}

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Surround IPv6 Endpoint addresses with brackets when generating a target.
Without these, the target port suffix parses as part of the address and
assigns an incorrect address and (default) port.
@rainest rainest marked this pull request as ready for review July 21, 2023 23:07
@rainest rainest requested a review from a team as a code owner July 21, 2023 23:07
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: -0.3% ⚠️

Comparison is base (3339b60) 66.5% compared to head (5cb4065) 66.3%.

❗ Current head 5cb4065 differs from pull request most recent head fb2afa9. Consider uploading reports for the commit fb2afa9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4391     +/-   ##
=======================================
- Coverage   66.5%   66.3%   -0.3%     
=======================================
  Files        158     158             
  Lines      18556   18555      -1     
=======================================
- Hits       12347   12302     -45     
- Misses      5456    5510     +54     
+ Partials     753     743     -10     
Files Changed Coverage Δ
internal/dataplane/parser/parser.go 79.1% <100.0%> (+0.2%) ⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md Outdated Show resolved Hide resolved
internal/dataplane/parser/parser.go Show resolved Hide resolved
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

Generally good, but can we add some unit tests for the fix?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 25, 2023
@rainest rainest enabled auto-merge (squash) July 25, 2023 18:32
pmalek
pmalek previously approved these changes Jul 25, 2023
randmonkey
randmonkey previously approved these changes Jul 26, 2023
programmer04
programmer04 previously approved these changes Jul 26, 2023
@rainest rainest dismissed stale reviews from programmer04, randmonkey, and pmalek via fb2afa9 July 26, 2023 16:56
@rainest rainest merged commit 7b111df into main Jul 26, 2023
28 checks passed
@rainest rainest deleted the fix/v6-targets branch July 26, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPv6 port appended to endpoint, causing connection failures.
5 participants