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

initial workaound for IPv6 workload support #2361

Closed
wants to merge 3 commits into from

Conversation

bonnyr
Copy link

@bonnyr bonnyr commented Jan 18, 2022

Proposed changes

IPv6 support in k8s is hitting mainstream and at some point IPv6 will become first class citizen in k8s deployments.
Ingress Controller code currently assumes that all workload addresses are IPv4, leading to failures using IC in IPv6 enabled
clusters (or pure IPv6 clusters - see robin.io).

Theres a bug addressing this : #991

Turns out the fix is rather simple (at least when validated against a pure IPv6 cluster).

The use cases that this fix can address include:

  1. IPv6 clusters where workloads (Service Endpoints mapped to upstreams) are assigned IPv6 addresses.
  2. Dual Stack clusters where workloads are assigned IPv4/IPv6 addresses
    Note that the change below (and the underlying assumption of IC) wrt an endpoint having a single address will need to be tested

####Note
At this stage testing has been performed manually on an IPv6 cluster with a simple nginx deployment serving as the
workload. The test environment has shown that:

  1. the initial nginx configuration generated includes the right format IP address for the upstreams
  2. scaling the deployment up and using the Plus API to add a new entry resulted in no error
  3. scaling the deployment down and using the Plus API to remove the deleted entry resulted in no error

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works - TBD
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation - N/A ?
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2022

Codecov Report

Merging #2361 (8a4b65c) into master (88a255b) will decrease coverage by 0.01%.
The diff coverage is 60.00%.

❗ Current head 8a4b65c differs from pull request most recent head a0ffc38. Consider uploading reports for the commit a0ffc38 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2361      +/-   ##
==========================================
- Coverage   53.70%   53.68%   -0.02%     
==========================================
  Files          48       48              
  Lines       14201    14206       +5     
==========================================
  Hits         7627     7627              
- Misses       6336     6340       +4     
- Partials      238      239       +1     
Impacted Files Coverage Δ
internal/k8s/controller.go 10.89% <42.85%> (+0.05%) ⬆️
internal/configs/ingress.go 76.64% <100.00%> (ø)
...ternal/k8s/appprotect/app_protect_configuration.go 86.16% <0.00%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88a255b...a0ffc38. Read the comment docs.

@brianehlert brianehlert linked an issue Jan 19, 2022 that may be closed by this pull request
@centromere
Copy link
Contributor

Thank you!

@centromere
Copy link
Contributor

@bonnyr The following changes were also needed in my cluster.

centromere@ee584a6

@nginx-bot nginx-bot force-pushed the feat-ipv6-upstreams branch 8 times, most recently from c8e422b to 8a4b65c Compare January 24, 2022 20:47
@nginx-bot nginx-bot force-pushed the feat-ipv6-upstreams branch from 8a4b65c to 2ddfa8f Compare January 25, 2022 22:18
tests are gated on building an ipv6 cluster and work as before when IPv6 is not specified
this needs to be reviewed and tests corrected (or conditional) on ipv6
@bonnyr
Copy link
Author

bonnyr commented Feb 1, 2022

@bonnyr The following changes were also needed in my cluster.

centromere@ee584a6

Thanks for the heads up. This was originally ignored but I have added this to facilitate unit tests

@centromere
Copy link
Contributor

@bonnyr I needed these changes also.

@centromere
Copy link
Contributor

Hi all. Is there any more development required for this to be accepted/merged?

@jjngx
Copy link
Contributor

jjngx commented Feb 28, 2022

Hi @centromere, I started working on this PR. We need to add unit and automated acceptance tests for this feature. This is WIP.

@gattytto
Copy link

hello and thank you so much for adding this feature!

could there pls be a tag for the docker image we can use meanwhile?

@brianehlert
Copy link
Collaborator

@gattytto When this has met our quality bar and merged it will be consumable using our 'edge' release prior to waiting for the next milestone release.

@lucacome lucacome marked this pull request as draft March 21, 2022 18:21
@lucacome
Copy link
Member

lucacome commented Apr 6, 2022

Closing this as #2576 was merged

@lucacome lucacome closed this Apr 6, 2022
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.

IPv6 SVCs not supported (missing [ ])
9 participants