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.

Sign up for GitHub

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

Jump to bottom

k8s: add support for ipFamilies to services #14914

Merged
merged 3 commits into from
Mar 18, 2021
Merged

k8s: add support for ipFamilies to services #14914

merged 3 commits into from
Mar 18, 2021

Conversation

fristonio
Copy link
Member

@fristonio fristonio commented Feb 9, 2021 •
edited

Fixes #13074

Service spec:

  spec:
    clusterIP: 192.168.128.200
    clusterIPs:
    - 192.168.128.200
    - fe00::22f7
    ipFamilies:
    - IPv4
    - IPv6
    ipFamilyPolicy: RequireDualStack
    ports:
    - port: 80
      protocol: TCP
      targetPort: 80
    selector:
      app: nginx
    sessionAffinity: None
    type: ClusterIP

Earlier a service created with dual-stack IP family was not parsed correctly for datapath service in Cilium:

cilium service list
ID   Frontend              Service Type   Backend
1    192.168.128.1:443     ClusterIP      1 => 10.128.15.194:6443
2    192.168.128.10:53     ClusterIP      1 => 192.168.64.44:53
                                          2 => 192.168.64.213:53
3    192.168.128.10:9153   ClusterIP      1 => 192.168.64.44:9153
                                          2 => 192.168.64.213:9153
4    192.168.128.200:80    ClusterIP      1 => 192.168.64.205:80
                                          2 => [fc00::b5d]:80

After the patch:

root@ipfamilies-test-instance:/home/cilium# cilium service list
ID   Frontend              Service Type   Backend
1    192.168.128.1:443     ClusterIP      1 => 10.128.15.194:6443
2    192.168.128.10:53     ClusterIP      1 => 192.168.64.44:53
                                          2 => 192.168.64.213:53
3    192.168.128.10:9153   ClusterIP      1 => 192.168.64.44:9153
                                          2 => 192.168.64.213:9153
4    192.168.128.200:80    ClusterIP      1 => 192.168.64.205:80
5    [fe00::22f7]:80       ClusterIP      1 => [fc00::b5d]:80

@fristonio fristonio added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 9, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 9, 2021
@joestringer joestringer moved this from Done to In progress in 1.10.0 Feb 12, 2021
@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch from 1114368 to e20e1c9 Compare February 22, 2021 09:05
@fristonio
Copy link
Member Author

test-me-please

@fristonio
Copy link
Member Author

retest-4.9

@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch 2 times, most recently from 7f7c3d8 to 05599e3 Compare February 23, 2021 07:24
@fristonio
Copy link
Member Author

retest-4.9

@fristonio fristonio marked this pull request as ready for review February 23, 2021 10:12
@fristonio fristonio requested review from a team as code owners February 23, 2021 10:12
@fristonio fristonio requested review from a team, kkourt and brb February 23, 2021 10:12
@fristonio fristonio requested a review from aanm February 23, 2021 10:12
@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch from 05599e3 to 8f17644 Compare February 23, 2021 13:32
aanm
aanm requested changes Feb 23, 2021
View reviewed changes
pkg/k8s/service.go Show resolved Hide resolved
pkg/k8s/service_cache.go Outdated Show resolved Hide resolved
pkg/k8s/service_cache.go Outdated Show resolved Hide resolved
pkg/k8s/service_cache.go Outdated Show resolved Hide resolved
pkg/k8s/watchers/watcher.go Outdated Show resolved Hide resolved
@aanm aanm removed their assignment Feb 23, 2021
@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch from 8f17644 to b69bada Compare February 24, 2021 16:52
@fristonio fristonio requested a review from a team February 24, 2021 16:52
@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch 2 times, most recently from 29c8f7d to 7063e1e Compare February 26, 2021 08:44
aanm
aanm requested changes Feb 26, 2021
View reviewed changes
pkg/ip/ip.go Outdated Show resolved Hide resolved
pkg/ip/ip.go Show resolved Hide resolved
pkg/ip/ip.go Outdated Show resolved Hide resolved
pkg/k8s/service.go Show resolved Hide resolved
pkg/k8s/service.go Show resolved Hide resolved
pkg/k8s/service.go Show resolved Hide resolved
@fristonio fristonio force-pushed the pr/fristonio/dualstack-ipfamilies branch from 7063e1e to 9fbd076 Compare February 26, 2021 11:05
@fristonio
Copy link
Member Author

test-gke

brb
brb approved these changes Mar 17, 2021
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

🚀

@kkourt kkourt added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 17, 2021
joestringer
joestringer approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I'm not quite sure why GitHub pulled in janitor codeowners for review, all of the files seem to have other codeowners already. Reviewers seem otherwise happy so I see no need to interdict.

@joestringer joestringer merged commit 1524304 into master Mar 18, 2021
1.10.0 automation moved this from In progress to Done Mar 18, 2021
@joestringer joestringer deleted the pr/fristonio/dualstack-ipfamilies branch March 18, 2021 23:02
This was referenced Apr 28, 2021
Closed
Merged
jrajahalme added a commit to jrajahalme/cilium that referenced this pull request Jun 25, 2021
@jrajahalme
k8s: Fix External Workloads service access
52a8eb3 
NewClusterService() and EqualsClusterService() were inadvertently
broken when support for ipFamilies was added, leaving the 'Ports' in
service empty. This made all cluster services inaccessible to External
Workloads.  Fix this by collecting the ports accross all
front-ends. This assumes that each front-end will be serving the same
ports.

Fixes: cilium#14914
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme mentioned this pull request Jun 25, 2021
Merged
aanm pushed a commit that referenced this pull request Jul 1, 2021
@jrajahalme @aanm
k8s: Fix External Workloads service access
929c28f 
NewClusterService() and EqualsClusterService() were inadvertently
broken when support for ipFamilies was added, leaving the 'Ports' in
service empty. This made all cluster services inaccessible to External
Workloads.  Fix this by collecting the ports accross all
front-ends. This assumes that each front-end will be serving the same
ports.

Fixes: #14914
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
twpayne pushed a commit that referenced this pull request Jul 1, 2021
@jrajahalme @twpayne
k8s: Fix External Workloads service access
16a8e15 
NewClusterService() and EqualsClusterService() were inadvertently
broken when support for ipFamilies was added, leaving the 'Ports' in
service empty. This made all cluster services inaccessible to External
Workloads.  Fix this by collecting the ports accross all
front-ends. This assumes that each front-end will be serving the same
ports.

Fixes: #14914
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
tklauser pushed a commit to tklauser/cilium that referenced this pull request Jul 1, 2021
@jrajahalme @tklauser
k8s: Fix External Workloads service access
263d9b6 
[ upstream commit 929c28f ]

NewClusterService() and EqualsClusterService() were inadvertently
broken when support for ipFamilies was added, leaving the 'Ports' in
service empty. This made all cluster services inaccessible to External
Workloads.  Fix this by collecting the ports accross all
front-ends. This assumes that each front-end will be serving the same
ports.

Fixes: cilium#14914
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
aanm pushed a commit that referenced this pull request Jul 2, 2021
@jrajahalme @aanm
k8s: Fix External Workloads service access
d47c550 
[ upstream commit 929c28f ]

NewClusterService() and EqualsClusterService() were inadvertently
broken when support for ipFamilies was added, leaving the 'Ports' in
service empty. This made all cluster services inaccessible to External
Workloads.  Fix this by collecting the ports accross all
front-ends. This assumes that each front-end will be serving the same
ports.

Fixes: #14914
Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@brb brb brb approved these changes

@kkourt kkourt kkourt approved these changes

@joestringer joestringer joestringer approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use ipFamilies in k8s 1.20
6 participants
@fristonio @brb @kkourt @joestringer @christarazi @aanm