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 / policy: allow all services for toServices when using highscale ipcache #26127

Merged
merged 1 commit into from Jun 15, 2023
Merged

k8s / policy: allow all services for toServices when using highscale ipcache #26127

merged 1 commit into from Jun 15, 2023

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Jun 12, 2023

Normally, toServices rules only allow Endpoints that are external to the cluster. This is to prevent conflict with entries already existing in the ipcache.

However, this is not relevant with highscale ipcache, where podips are normally excluded from the ipcache anyways. So, allow all services to be specified in toServices egress rules when highscale is enabled.

@squeed squeed added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-note/misc This PR makes changes that have no direct user impact. feature/high-scale-ipcache Relates to the high-scale ipcache feature. labels Jun 12, 2023
@squeed squeed requested review from a team as code owners June 12, 2023 14:30
christarazi
christarazi reviewed Jun 12, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

There are some lint errors regarding option.Config.HighScaleIPcacheEnabled()

pkg/k8s/rule_translate.go Outdated Show resolved Hide resolved
@squeed squeed added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jun 13, 2023
@squeed
Copy link
Contributor Author

squeed commented Jun 13, 2023 •
edited by maintainer-s-little-helper bot

/test

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

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with the host firewall and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service tftp://[fd04::12]:30741/hello failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/695/

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

Then please upload the Jenkins artifacts to that issue.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (92.62% similarity)

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Jun 13, 2023
Closed
christarazi
christarazi approved these changes Jun 13, 2023
View reviewed changes
pkg/k8s/rule_translate.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member

christarazi commented Jun 13, 2023 •
edited

Checkpatch is complaining with

  Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 76)

Maybe format the commit msg to

k8s, policy: enable toServices for high-scale ipcache

?

@christarazi
Copy link
Member

https://github.com/cilium/cilium/actions/runs/5259867178 is failing all for the same test 🤔

@squeed
k8s, policy: enable toServices rules for high-scale ipcache
cbb0d5c 
Normally, toServices rules only allow Endpoints that are external to the
cluster. This is to prevent conflict with entries already existing in
the ipcache.

However, this is not relevant with highscale ipcache, where podips are
normally excluded from the ipcache anyways. So, allow all services to be
specified in toServices egress rules when highscale is enabled.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
@squeed
Copy link
Contributor Author

squeed commented Jun 14, 2023

https://github.com/cilium/cilium/actions/runs/5259867178 is failing all for the same test thinking

cloudflare blip :-/.

tklauser
tklauser approved these changes Jun 14, 2023
View reviewed changes
pkg/k8s/rule_translate.go Show resolved Hide resolved
@squeed
Copy link
Contributor Author

squeed commented Jun 14, 2023

/test

@youngnick youngnick removed their request for review June 15, 2023 07:50
@squeed
Copy link
Contributor Author

squeed commented Jun 15, 2023

All approvals are in, CI is fully green. Merging

@squeed squeed merged commit 0b98248 into cilium:main Jun 15, 2023
60 checks passed
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 15, 2023
@joestringer
Copy link
Member

Is there a test for this at all? Doesn't Cilium just ignore these rule sections if they're not translated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
feature/high-scale-ipcache Relates to the high-scale ipcache feature. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.14 This issue will prevent the release of the next version of Cilium. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@squeed @christarazi @joestringer @tklauser