cilium icon indicating copy to clipboard operation
cilium copied to clipboard

enforce the 1 IP per type and reject all other IPs of the same type

Open NikhilSharmaWe opened this issue 3 years ago • 1 comments

Please ensure your pull request adheres to the following guidelines:

  • [x] For first time contributors, read Submitting a pull request
  • [x] All code is covered by unit and/or runtime tests where feasible.
  • [x] All commits contain a well written commit description including a title, description and a Fixes: #XXX line if the commit addresses a particular GitHub issue.
  • [x] All commits are signed off. See the section Developer’s Certificate of Origin
  • [x] Provide a title or release-note blurb suitable for the release notes.
  • [x] Thanks for contributing!

This PR is made to handle when K8s Nodes have more than one IP address per address type in their status field. Fixes: #20787

k8s: Enforce the 1 IP per type

NikhilSharmaWe avatar Aug 07 '22 09:08 NikhilSharmaWe

Commit ade978a489747f96c2cfda1d31173c9eaaf4000c does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@tommyp1ckles Made some changes, could you please take a look. Also is there any make rule for the repo to format the code or to point out the mistakes in the code. I tried running make lint but it is taking very it is stuck for a very long time.

NikhilSharmaWe avatar Aug 11 '22 05:08 NikhilSharmaWe

@NikhilSharmaWe General approach looks ok, looks like the build is failing though. Couple of comments as well.

tommyp1ckles avatar Aug 11 '22 22:08 tommyp1ckles

@christarazi Can you take a look to see if this adequately solves the issue at hand.

tommyp1ckles avatar Aug 11 '22 22:08 tommyp1ckles

@NikhilSharmaWe not sure what the problem with lint is, maybe check https://docs.cilium.io/en/stable/contributing/development/dev_setup/ to make sure you have the right dependencies installed etc.

The branch CI will also do lint checks and post comments if it finds any issues.

tommyp1ckles avatar Aug 11 '22 22:08 tommyp1ckles

@NikhilSharmaWe If you rebase the CI, that should fix the current build failure.

tommyp1ckles avatar Aug 15 '22 15:08 tommyp1ckles

@tommyp1ckles Rebased the branch

NikhilSharmaWe avatar Aug 15 '22 16:08 NikhilSharmaWe

c/c @aditighag

NikhilSharmaWe avatar Aug 24 '22 03:08 NikhilSharmaWe

@NikhilSharmaWe You can ping @cilium/tophat, or ping in the #development Slack channel if you need someone to trigger the tests.

aditighag avatar Aug 24 '22 14:08 aditighag

/test

Job 'Cilium-PR-K8s-1.23-kernel-4.19' failed:

Click to show.

Test Name

K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running

Failure Output


If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-4.19 so I can create one.

tommyp1ckles avatar Aug 25 '22 16:08 tommyp1ckles

What if this is a dual-stack cluster? Will we do the right thing there?

(It may be that cilium was wrong before, and this code isn't at fault. But we should find out)

squeed avatar Sep 02 '22 06:09 squeed

What if this is a dual-stack cluster? Will we do the right thing there?

What would dual-stack have to do with this code? This code is checking for address types such as InternalIP or ExternalIP, not family JFYI in case that's what might have caused confusion.

christarazi avatar Sep 02 '22 19:09 christarazi

What would dual-stack have to do with this code?

Right, but dual-stack nodes have multiple entries for the same address type. Like I said, it is possible that the existing code is making a bad assumption.

For example, this is from a dual-stack kind cluster on my laptop:

spec:
  podCIDR: 10.244.0.0/24
  podCIDRs:
  - 10.244.0.0/24
  - fd00:10:244::/64
  providerID: kind://docker/ovn/ovn-control-plane
status:
  addresses:
  - address: 172.18.0.2
    type: InternalIP
  - address: fc00:f853:ccd:e793::2
    type: InternalIP
  - address: ovn-control-plane
    type: Hostname
  ...

squeed avatar Sep 05 '22 14:09 squeed

Ah, gotcha. Understood. Thanks Casey!

@NikhilSharmaWe could you account for IP family as well in this logic?

christarazi avatar Sep 06 '22 21:09 christarazi

Thanks for the contribution! The PR has been inactive for some time, so I'm turning it into a draft until feedback is addressed.

qmonnet avatar Oct 13 '22 09:10 qmonnet

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Nov 13 '22 02:11 github-actions[bot]

This pull request has not seen any activity since it was marked stale. Closing.

github-actions[bot] avatar Nov 28 '22 02:11 github-actions[bot]