Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Propagate ports from main container service to additional services for the same container #2006

Merged

Conversation

g-linville
Copy link
Contributor

@g-linville g-linville commented Aug 2, 2023

This one is a bit tough to explain. Let's start with this Acornfile:

containers: "my-container": {
  image: "nginx:latest"
  ports: "80/http"
}

services: "nginx": {
  ports: "8080:80/http"
  container: "my-container"
}

Prior to this PR, the controller would render two Kubernetes Services for this. The first is my-container, accessible on port 80 and sending traffic to port 80 on the my-container container. The second is nginx, accessible on port 8080 and sending traffic to port 80 on the my-container container. Fairly straightforward and works great.

...that is, until Istio comes into the picture. Istio does not like when multiple services have the same target port on the same pod, but with different public-facing ports. When we were running like this, we had weird traffic routing issues where Envoy would forward traffic to a stale pod IP that no longer belonged to any pods.

The fix? When we create additional services for a specific container (i.e., the container field is specified, as it is in our example), propagate all of the HTTP ports from the "main" service for the container (the one whose name is the same as the name of the container). Istio will still complain when you run istioctl analyze -A, but the routing issues disappear and things work again.

Also in this PR, we block these additional services from being allowed to specify any non-HTTP ports.

I also added a new unit test to cover this case.

Checklist

  • The title of this PR would make a good line in Acorn's Release Note's Changelog
  • The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
  • All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
  • Commits follow contributing guidance
  • Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
  • Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
  • PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

…r the same container

Signed-off-by: Grant Linville <grant@acorn.io>
pkg/services/services.go Outdated Show resolved Hide resolved
pkg/ports/ports.go Outdated Show resolved Hide resolved
pkg/ports/ports.go Outdated Show resolved Hide resolved
Signed-off-by: Grant Linville <grant@acorn.io>
@g-linville g-linville merged commit 2e8339a into acorn-io:main Aug 3, 2023
4 checks passed
@g-linville g-linville deleted the coalesce-container-service-ports branch August 3, 2023 13:33
g-linville added a commit to g-linville/runtime that referenced this pull request Aug 4, 2023
g-linville added a commit to g-linville/runtime that referenced this pull request Aug 4, 2023
…vices for the same container (acorn-io#2006)"

This reverts commit 2e8339a.

Signed-off-by: Grant Linville <grant@acorn.io>
cloudnautique pushed a commit to cloudnautique/runtime that referenced this pull request Sep 28, 2023
…r the same container (acorn-io#2006)

Signed-off-by: Grant Linville <grant@acorn.io>
cloudnautique pushed a commit to cloudnautique/runtime that referenced this pull request Sep 28, 2023
…vices for the same container (acorn-io#2006)"

This reverts commit 2e8339a.

Signed-off-by: Grant Linville <grant@acorn.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants