fix: use unique container ports in deployment by zetaab · Pull Request #2973 · envoyproxy/gateway

zetaab · 2024-03-18T10:42:09Z

What type of PR is this?
fix: use unique ports in deployment

What this PR does / why we need it:

Kubernetes requires that container port numbers are unique. So those can exist only once. Current implementation is adding duplicate ports if there are like two listeners listening port 443.

This PR also reduces the amount of restarts needed for deployments #2965 because it will not modify deployment configuration anymore that often when listeners are modifierd.

Which issue(s) this PR fixes:

Fixes #2964

codecov · 2024-03-18T10:46:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.65%. Comparing base (4afe12b) to head (22da36a).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2973      +/-   ##
==========================================
+ Coverage   64.63%   64.65%   +0.02%     
==========================================
  Files         121      121              
  Lines       21124    21128       +4     
==========================================
+ Hits        13653    13661       +8     
+ Misses       6623     6620       -3     
+ Partials      848      847       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zirain · 2024-03-19T04:13:04Z

/retest

internal/infrastructure/kubernetes/proxy/resource.go

zetaab · 2024-03-19T18:35:08Z

@zirain fixed

arkodg · 2024-03-19T21:31:18Z

internal/infrastructure/kubernetes/proxy/resource.go

@cnvergence can you recall why this hash was added ? I think it was to limit the name length, so we want want to change the args we pass, but we still may need to use GetHashedName

the containerport name should not matter unless there is a kubernetes service which will refer to that name. https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2127-L2130

However, in case of envoy the services (type: loadbalancer) are using targetPort as Number instead of name. So the name is not used at all anywhere by default. https://github.com/kubernetes/api/blob/master/core/v1/types.go#L5410-L5418

I do not see why we should still use GetHashedName. The only reason for that would be that name is going to be too long, but I cannot see that case really. If the name is [protocol]-[port number] the maximum length is going to be SCTP-66666 which equals to 10. That is not too long.

We added the hash because we used listener port names as the container port names which could be larger than 15 chars
and in merged gateways we used the format with namespace/gateway/listener

this could be a good change for any deployment mode

can we instead fix the portName here

gateway/internal/gatewayapi/listener.go

Line 146 in 7f5cbe8

Name: infraPortName,

in the gatewayapi layer and reuse p.Name here ?
we'll need to make sure its not being used anywhere else

and can we define the infraIRListenerPortName()

gateway/internal/gatewayapi/helpers.go

Line 375 in 7f5cbe8

sorry for nits, but trying to keep the code simpler to maintain

zirain · 2024-03-20T07:07:55Z

/retest

zirain

Lgtm, defer to @arkodg and @cnvergence

cnvergence · 2024-03-21T08:06:49Z

test fails are valid, so we are still missing something

zetaab · 2024-03-21T08:11:04Z

@cnvergence

=== RUN   TestE2E/FileAccessLog/Stdout
    helpers.go:402: error fetching Gateway: client rate limiter Wait returned an error: context deadline exceeded
    accesslog.go:44: 
        	Error Trace:	/home/runner/go/pkg/mod/sigs.k8s.io/[email protected]/conformance/utils/kubernetes/helpers.go:423
        	            				/home/runner/go/pkg/mod/sigs.k8s.io/[email protected]/conformance/utils/kubernetes/helpers.go:337
        	            				/home/runner/work/gateway/gateway/test/e2e/tests/accesslog.go:44
        	Error:      	Received unexpected error:
        	            	error fetching Gateway: client rate limiter Wait returned an error: context deadline exceeded
        	Test:       	TestE2E/FileAccessLog/Stdout
        	Messages:   	error waiting for Gateway to have at least one IP address in status

for me this looks like we need better logs from envoy gateway controller, these logs in github actions does not tell much. There is some reason why controller does not populate the Gateway status ip address. It would be much easier to see the issue with logs

Signed-off-by: Jesse Haka <[email protected]>

zetaab · 2024-03-24T21:18:39Z

@cnvergence @arkodg @zirain I found the issue, kubernetes deployment container port name can contain only [a-z0-9-] uppercase chars are not allowed. It should work now

edit: e2e tests might be broken because of #3003

cnvergence · 2024-03-25T12:59:16Z

Thanks for the update @zetaab, I will take a look again

cnvergence · 2024-03-25T16:37:58Z

looks good, we might need to add this also to service ports to fix the issue
Just thinking, could we maybe check for uniqueness in the infra IR translator?

zetaab · 2024-03-25T17:25:13Z

we do not need to modify service ports. Service ports are not using names when referring to deployment ports.

cnvergence · 2024-03-26T10:30:08Z

I am referring to the original issue #2964 and how to fix it, which is more related to the port values, #2964 (comment).
However, I approve of your port name change and I think we should get it merged.

arkodg · 2024-03-26T11:02:41Z

internal/infrastructure/kubernetes/proxy/resource.go

 				Protocol:      protocol,
 			}
-			ports = append(ports, port)
+			uname := fmt.Sprintf("%s-%d", protocol, p.ContainerPort)


uname is not needed, you could directly use uniquePorts[port.Name]

arkodg · 2024-03-26T11:03:42Z

internal/infrastructure/kubernetes/proxy/resource.go

 			}
-			ports = append(ports, port)
+			uname := fmt.Sprintf("%s-%d", protocol, p.ContainerPort)
+			if _, ok := uniquePorts[uname]; !ok {


can we also add a comment here e.g. "Only append unique ports"

cnvergence · 2024-03-26T11:32:21Z

Quickly checked today and the problem now resurfaces in the service

ERROR	watchable	message/watchutil.go:56	observed an error	{"runner": "infrastructure", "error": "failed to create or update service envoy-gateway-system/envoy-eg-internal-7f4ff7e4: for Create: Service \"envoy-eg-internal-7f4ff7e4\" is invalid: [spec.ports[2].nodePort: Duplicate value: 32079, spec.ports[2]: Duplicate value: core.ServicePort{Name:\"\", Protocol:\"TCP\", AppProtocol:(*string)(nil), Port:443, TargetPort:intstr.IntOrString{Type:0, IntVal:0, StrVal:\"\"}, NodePort:0}]"}

So I think it should be fixed in the translator

arkodg · 2024-03-26T11:42:13Z

shouldn't this existing code in the gateway api translator

gateway/internal/gatewayapi/listener.go

Line 125 in 7f5cbe8

if !containsPort(foundPorts, servicePort) {

already handle the uniquePort case ?

zetaab · 2024-03-26T12:36:36Z

looks like so, but for some reason these duplicate ports are ending to kubernetes deployment. So the logic is not fully applied to everywhere

arkodg · 2024-03-26T12:53:14Z

looks like so, but for some reason these duplicate ports are ending to kubernetes deployment. So the logic is not fully applied to everywhere

Worth debugging that and fixing that since there's already existing code that's trying to do the same thing

cnvergence · 2024-04-08T08:57:53Z

hi @zetaab, I have fixed that logic in the translator, would you like to add this port naming change?

Takes inspiration from envoyproxy#2973 to name port, not off the listener but off the port-proto ensuring that patch (during updates) also works Fixes: envoyproxy#3111 Signed-off-by: Arko Dasgupta <[email protected]>

* Use <proto>-<port> for naming service and container ports Takes inspiration from #2973 to name port, not off the listener but off the port-proto ensuring that patch (during updates) also works Fixes: #3111 Signed-off-by: Arko Dasgupta <[email protected]> * testdata Signed-off-by: Arko Dasgupta <[email protected]> * fix test Signed-off-by: Arko Dasgupta <[email protected]> * move to helper pkg Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> * lint Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]>

…#3130) * Use <proto>-<port> for naming service and container ports Takes inspiration from envoyproxy#2973 to name port, not off the listener but off the port-proto ensuring that patch (during updates) also works Fixes: envoyproxy#3111 Signed-off-by: Arko Dasgupta <[email protected]> * testdata Signed-off-by: Arko Dasgupta <[email protected]> * fix test Signed-off-by: Arko Dasgupta <[email protected]> * move to helper pkg Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> * lint Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <[email protected]>

* Use <proto>-<port> for naming service and container ports (#3130) * Use <proto>-<port> for naming service and container ports Takes inspiration from #2973 to name port, not off the listener but off the port-proto ensuring that patch (during updates) also works Fixes: #3111 Signed-off-by: Arko Dasgupta <[email protected]> * testdata Signed-off-by: Arko Dasgupta <[email protected]> * fix test Signed-off-by: Arko Dasgupta <[email protected]> * move to helper pkg Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> * lint Signed-off-by: Arko Dasgupta <[email protected]> * fix e2e Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <[email protected]> * bug: Tests are failing due to an expired certificate in one of the translator tests (#3476) Replaced a certificate in the test that had expired. The old certificate expired May 24 2024: Certificate: Data: Version: 1 (0x0) Serial Number: ca:7c:5c:b7:25:5d:bb:f9 Signature Algorithm: ecdsa-with-SHA256 Issuer: CN=test.example.com Validity Not Before: May 25 14:10:42 2023 GMT Not After : May 24 14:10:42 2024 GMT Subject: CN=test.example.com Subject Public Key Info: Public Key Algorithm: id-ecPublicKey Public-Key: (384 bit) pub: 04:78:cb:47:0b:78:48:7a:ad:90:b1:d9:2d:4a:2f: d9:35:1f:cc:28:d6:af:4a:6d:c7:36:7e:ed:1a:88: 1f:a9:aa:a7:f0:04:a0:1c:86:bb:c9:45:3e:f8:fb: 28:0c:3e:a4:7f:ef:82:7b:bb:ac:77:49:90:3b:54: a7:75:82:16:8f:64:0b:88:8c:f4:35:91:fc:07:f4: 2b:e2:2e:c9:d0:82:b0:b1:09:54:9e:e9:d9:aa:fe: 4a:63:d4:cb:41:ad:27 ASN1 OID: secp384r1 NIST CURVE: P-384 Signature Algorithm: ecdsa-with-SHA256 Signature Value: 30:65:02:31:00:86:4e:33:e4:86:37:4c:26:a7:be:57:51:44: 8e:6c:88:ea:3c:03:58:00:a3:5e:7a:53:9e:2c:54:b3:ab:82: 25:fe:4c:e4:be:4d:8c:56:e2:da:d8:de:d2:20:ca:13:55:02: 30:0c:2a:27:a7:fd:2b:a9:87:4f:06:ea:4e:2d:cc:48:4d:9d: d7:cf:73:88:6d:98:54:18:83:6d:e5:a9:c3:84:75:c9:ee:c6: 0d:1a:15:a2:8c:68:86:88:83:17:b9:7a:9b The new certificate is good for 10 years. Certificate: Data: Version: 3 (0x2) Serial Number: 42:29:94:01:e1:cb:32:dc:f8:b4:64:6d:9e:1e:28:8d:7b:5a:53:3b Signature Algorithm: ecdsa-with-SHA256 Issuer: CN=test.example.com Validity Not Before: May 25 09:11:37 2024 GMT Not After : May 23 09:11:37 2034 GMT Subject: CN=test.example.com Subject Public Key Info: Public Key Algorithm: id-ecPublicKey Public-Key: (384 bit) pub: 04:78:cb:47:0b:78:48:7a:ad:90:b1:d9:2d:4a:2f: d9:35:1f:cc:28:d6:af:4a:6d:c7:36:7e:ed:1a:88: 1f:a9:aa:a7:f0:04:a0:1c:86:bb:c9:45:3e:f8:fb: 28:0c:3e:a4:7f:ef:82:7b:bb:ac:77:49:90:3b:54: a7:75:82:16:8f:64:0b:88:8c:f4:35:91:fc:07:f4: 2b:e2:2e:c9:d0:82:b0:b1:09:54:9e:e9:d9:aa:fe: 4a:63:d4:cb:41:ad:27 ASN1 OID: secp384r1 NIST CURVE: P-384 X509v3 extensions: X509v3 Subject Key Identifier: DA:49:EA:13:99:CA:DE:10:D2:70:2B:27:E2:60:AA:E0:F4:7B:EA:50 X509v3 Authority Key Identifier: DA:49:EA:13:99:CA:DE:10:D2:70:2B:27:E2:60:AA:E0:F4:7B:EA:50 X509v3 Basic Constraints: critical CA:TRUE Signature Algorithm: ecdsa-with-SHA256 Signature Value: 30:65:02:30:6d:4e:25:4f:84:f4:38:7e:c4:de:c8:d1:55:0c: af:4b:e4:c0:a1:f3:59:de:fb:48:0a:96:07:65:29:9f:fe:7c: 3c:ee:f0:c9:ca:17:bc:cd:bd:a4:31:38:24:4f:c6:e5:02:31: 00:e6:9a:ce:52:60:4b:b8:0e:e7:23:6d:8a:69:a0:21:e5:d1: bb:e8:e9:09:6a:32:d6:8c:58:49:f4:76:86:e6:c1:b8:24:d3: 44:08:fa:1c:ef:34:70:c1:24:76:a9:35:8f Signed-off-by: Lior Okman <[email protected]> (cherry picked from commit c2c9b43) Signed-off-by: Arko Dasgupta <[email protected]> * fix: use Patch API for infra-client (#3034) * fix(infrastructure): use Patch API instead Signed-off-by: Ardika Bagus <[email protected]> * chore: add interceptor for ApplyPatch on fake client Signed-off-by: Ardika Bagus <[email protected]> * chore: trigger make generate Signed-off-by: Ardika Bagus <[email protected]> * chore: remove update verb Signed-off-by: Ardika Bagus <[email protected]> * chore: SetUID no longer needed Signed-off-by: Ardika Bagus <[email protected]> --------- Signed-off-by: Ardika Bagus <[email protected]> (cherry picked from commit cc01bf5) Signed-off-by: Arko Dasgupta <[email protected]> * refactor: infra client CreateOrUpdate to ServerSideApply (#3134) * refactor(infra-client): CreateOrUpdate to ServerSideApply Signed-off-by: Ardika Bagus <[email protected]> * test(infra-client): add e2e test for ServerSideApply Signed-off-by: Ardika Bagus <[email protected]> * chore: remove comment Signed-off-by: Ardika Bagus <[email protected]> * chore: fix linter Signed-off-by: Ardika Bagus <[email protected]> --------- Signed-off-by: Ardika Bagus <[email protected]> (cherry picked from commit 81108f2) Signed-off-by: Arko Dasgupta <[email protected]> * fix: duplicated xroutes are added to gatewayapi.Resources (#3282) fix duplicated xroutes Signed-off-by: Dingkang Li <[email protected]> (cherry picked from commit 32c6876) Signed-off-by: Arko Dasgupta <[email protected]> * fix: add proxy protocol always as first listenerFilter (#3332) * add proxy protocol always as first listenerFilter Signed-off-by: Jesse Haka <[email protected]> * add test Signed-off-by: Jesse Haka <[email protected]> --------- Signed-off-by: Jesse Haka <[email protected]> (cherry picked from commit 6d8f2dc) Signed-off-by: Arko Dasgupta <[email protected]> * fix: security policy reference grant from field type (#3386) fix: security policy reference grant from field Signed-off-by: Eguzki Astiz Lezaun <[email protected]> (cherry picked from commit bd72474) Signed-off-by: Arko Dasgupta <[email protected]> * bug: Route extension filters with different types but the same name and namespace aren't correctly cached (#3388) * Route extension filters are unstructured.Unstructured instances, so caching them should be done with both the name and type as a key. Signed-off-by: Lior Okman <[email protected]> * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman <[email protected]> * Also renamed the helper function. Signed-off-by: Lior Okman <[email protected]> * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman <[email protected]> * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman <[email protected]> --------- Signed-off-by: Lior Okman <[email protected]> (cherry picked from commit 95e2e35) Signed-off-by: Arko Dasgupta <[email protected]> * fix(translator): set ignoreCase for header matchers in extAuth (#3420) fix: set ignoreCase for header matchers in extAuth Signed-off-by: haoqixu <[email protected]> (cherry picked from commit 8206e11) Signed-off-by: Arko Dasgupta <[email protected]> * fix secrets/configmap updates do not trigger a controller reconcile (#3499) * ensure both secrets and config map reconcile upon changes ensure secret/config map changes trigger a reconcile Signed-off-by: Alex Volchok <[email protected]> * Update controller.go Signed-off-by: Alex Volchok <[email protected]> * Update controller.go Signed-off-by: Alex Volchok <[email protected]> --------- Signed-off-by: Alex Volchok <[email protected]> (cherry picked from commit ff2c598) Signed-off-by: Arko Dasgupta <[email protected]> * feat: backend TLS SAN validation (#3507) * BTLS: enforce SAN validation Signed-off-by: Guy Daich <[email protected]> * use dedicated cert for ext-proc e2e test Signed-off-by: Guy Daich <[email protected]> * fix ext-proc server client tls settings Signed-off-by: Guy Daich <[email protected]> --------- Signed-off-by: Guy Daich <[email protected]> (cherry picked from commit dc201ba) Signed-off-by: Arko Dasgupta <[email protected]> * fix: ReplaceFullPath not working for root path (/) (#3530) * fix: ReplaceFullPath not working for root path (/) Takes #2817 forward Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 8f83c3d) Signed-off-by: Arko Dasgupta <[email protected]> * chore: Remove namespace restriction for EnvoyProxy parametersRef reso… (#3544) chore: Remove namespace restriction for EnvoyProxy parametersRef resource (cherry picked from commit b870e39) Signed-off-by: Arko Dasgupta <[email protected]> * fix test rm invalid diff related to #3134 Signed-off-by: Arko Dasgupta <[email protected]> * fix GatewayInfraResourceTest Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: Lior Okman <[email protected]> Signed-off-by: Ardika Bagus <[email protected]> Signed-off-by: Dingkang Li <[email protected]> Signed-off-by: Jesse Haka <[email protected]> Signed-off-by: Eguzki Astiz Lezaun <[email protected]> Signed-off-by: haoqixu <[email protected]> Signed-off-by: Alex Volchok <[email protected]> Signed-off-by: Guy Daich <[email protected]> Co-authored-by: Lior Okman <[email protected]> Co-authored-by: Ardika <[email protected]> Co-authored-by: Dingkang Li <[email protected]> Co-authored-by: Jesse Haka <[email protected]> Co-authored-by: Eguzki Astiz Lezaun <[email protected]> Co-authored-by: xu0o0 <[email protected]> Co-authored-by: Alex Volchok <[email protected]> Co-authored-by: Guy Daich <[email protected]> Co-authored-by: zou rui <[email protected]>

zetaab requested a review from a team as a code owner March 18, 2024 10:42

zetaab changed the title ~~fix: use unique ports in deployment~~ fix: use unique container ports in deployment Mar 18, 2024

zirain reviewed Mar 19, 2024

View reviewed changes

internal/infrastructure/kubernetes/proxy/resource.go Outdated Show resolved Hide resolved

zetaab mentioned this pull request Mar 19, 2024

bug: duplicate port definition when using mergeGateway #2964

Closed

arkodg requested a review from cnvergence March 19, 2024 21:30

arkodg reviewed Mar 19, 2024

View reviewed changes

zirain reviewed Mar 20, 2024

View reviewed changes

zetaab added 4 commits March 24, 2024 23:06

use unique ports in deployment

1cf37cd

Signed-off-by: Jesse Haka <[email protected]>

use protocol-port format

8bcad81

Signed-off-by: Jesse Haka <[email protected]>

add test for duplicate port listeners

e015999

Signed-off-by: Jesse Haka <[email protected]>

use lowercase port name

6e6bbf3

Signed-off-by: Jesse Haka <[email protected]>

zetaab force-pushed the fix/deploymentuniqueports branch from adb91b6 to 6e6bbf3 Compare March 24, 2024 21:06

Merge branch 'main' into fix/deploymentuniqueports

22da36a

arkodg reviewed Mar 26, 2024

View reviewed changes

zetaab closed this Apr 7, 2024

zetaab deleted the fix/deploymentuniqueports branch April 7, 2024 08:52

arkodg mentioned this pull request Apr 8, 2024

Use <proto>-<port> for naming service and container ports #3130

Merged

Conversation

zetaab commented Mar 18, 2024

Uh oh!

codecov bot commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zirain commented Mar 19, 2024

Uh oh!

Uh oh!

zetaab commented Mar 19, 2024

Uh oh!

arkodg Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

zetaab Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnvergence Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

cnvergence Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

zirain commented Mar 20, 2024

Uh oh!

zirain left a comment

Choose a reason for hiding this comment

Uh oh!

cnvergence commented Mar 21, 2024

Uh oh!

zetaab commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zetaab commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cnvergence commented Mar 25, 2024

Uh oh!

cnvergence commented Mar 25, 2024

Uh oh!

zetaab commented Mar 25, 2024

Uh oh!

cnvergence commented Mar 26, 2024

Uh oh!

arkodg Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

cnvergence commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

zetaab commented Mar 26, 2024

Uh oh!

arkodg commented Mar 26, 2024

Uh oh!

cnvergence commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 18, 2024 •

edited

Loading

zetaab Mar 20, 2024 •

edited

Loading

cnvergence Mar 21, 2024 •

edited

Loading

zetaab commented Mar 21, 2024 •

edited

Loading

zetaab commented Mar 24, 2024 •

edited

Loading

cnvergence commented Mar 26, 2024 •

edited

Loading