fix: use unique container ports in deployment#2973
fix: use unique container ports in deployment#2973zetaab wants to merge 5 commits intoenvoyproxy:mainfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
|
/retest |
|
@zirain fixed |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
this could be a good change for any deployment mode
There was a problem hiding this comment.
-
can we instead fix the portName here
in the gatewayapi layer and reusegateway/internal/gatewayapi/listener.go
Line 146 in 7f5cbe8
p.Namehere ?
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
|
/retest |
zirain
left a comment
There was a problem hiding this comment.
Lgtm, defer to @arkodg and @cnvergence
|
test fails are valid, so we are still missing something |
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 |
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
Signed-off-by: Jesse Haka <haka.jesse@gmail.com>
adb91b6 to
6e6bbf3
Compare
|
@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 |
|
Thanks for the update @zetaab, I will take a look again |
|
looks good, we might need to add this also to service ports to fix the issue |
|
we do not need to modify service ports. Service ports are not using names when referring to deployment ports. |
|
I am referring to the original issue #2964 and how to fix it, which is more related to the port values, #2964 (comment). |
| Protocol: protocol, | ||
| } | ||
| ports = append(ports, port) | ||
| uname := fmt.Sprintf("%s-%d", protocol, p.ContainerPort) |
There was a problem hiding this comment.
uname is not needed, you could directly use uniquePorts[port.Name]
| } | ||
| ports = append(ports, port) | ||
| uname := fmt.Sprintf("%s-%d", protocol, p.ContainerPort) | ||
| if _, ok := uniquePorts[uname]; !ok { |
There was a problem hiding this comment.
can we also add a comment here e.g. "Only append unique ports"
|
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 |
|
shouldn't this existing code in the gateway api translator gateway/internal/gatewayapi/listener.go Line 125 in 7f5cbe8 |
|
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 |
|
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 <arko@tetrate.io>
* 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 <arko@tetrate.io> * testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * move to helper pkg Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io>
…#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 <arko@tetrate.io> * testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * move to helper pkg Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
…#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 <arko@tetrate.io> * testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * move to helper pkg Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
* 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 <arko@tetrate.io> * testdata Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix test Signed-off-by: Arko Dasgupta <arko@tetrate.io> * move to helper pkg Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> * lint Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix e2e Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit c41247b) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <lior.okman@sap.com> (cherry picked from commit c2c9b43) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: use Patch API for infra-client (#3034) * fix(infrastructure): use Patch API instead Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: add interceptor for ApplyPatch on fake client Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: trigger make generate Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: remove update verb Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: SetUID no longer needed Signed-off-by: Ardika Bagus <me@ardikabs.com> --------- Signed-off-by: Ardika Bagus <me@ardikabs.com> (cherry picked from commit cc01bf5) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * refactor: infra client CreateOrUpdate to ServerSideApply (#3134) * refactor(infra-client): CreateOrUpdate to ServerSideApply Signed-off-by: Ardika Bagus <me@ardikabs.com> * test(infra-client): add e2e test for ServerSideApply Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: remove comment Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: fix linter Signed-off-by: Ardika Bagus <me@ardikabs.com> --------- Signed-off-by: Ardika Bagus <me@ardikabs.com> (cherry picked from commit 81108f2) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: duplicated xroutes are added to gatewayapi.Resources (#3282) fix duplicated xroutes Signed-off-by: Dingkang Li <dingkang1743@gmail.com> (cherry picked from commit 32c6876) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: add proxy protocol always as first listenerFilter (#3332) * add proxy protocol always as first listenerFilter Signed-off-by: Jesse Haka <haka.jesse@gmail.com> * add test Signed-off-by: Jesse Haka <haka.jesse@gmail.com> --------- Signed-off-by: Jesse Haka <haka.jesse@gmail.com> (cherry picked from commit 6d8f2dc) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: security policy reference grant from field type (#3386) fix: security policy reference grant from field Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com> (cherry picked from commit bd72474) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <lior.okman@sap.com> * Moved NamespacedNameAndType to the Kubernetes helpers, and renamed it to be clearer about what it has. Signed-off-by: Lior Okman <lior.okman@sap.com> * Also renamed the helper function. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved to the 'utils' package to be beside NamespacedName. Signed-off-by: Lior Okman <lior.okman@sap.com> * Renamed structure according to review, and updated the comments Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com> (cherry picked from commit 95e2e35) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix(translator): set ignoreCase for header matchers in extAuth (#3420) fix: set ignoreCase for header matchers in extAuth Signed-off-by: haoqixu <hq.xu0o0@gmail.com> (cherry picked from commit 8206e11) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <alex.volchok@sap.com> * Update controller.go Signed-off-by: Alex Volchok <alex.volchok@sap.com> * Update controller.go Signed-off-by: Alex Volchok <alex.volchok@sap.com> --------- Signed-off-by: Alex Volchok <alex.volchok@sap.com> (cherry picked from commit ff2c598) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * feat: backend TLS SAN validation (#3507) * BTLS: enforce SAN validation Signed-off-by: Guy Daich <guy.daich@sap.com> * use dedicated cert for ext-proc e2e test Signed-off-by: Guy Daich <guy.daich@sap.com> * fix ext-proc server client tls settings Signed-off-by: Guy Daich <guy.daich@sap.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> (cherry picked from commit dc201ba) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix: ReplaceFullPath not working for root path (/) (#3530) * fix: ReplaceFullPath not working for root path (/) Takes #2817 forward Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 8f83c3d) Signed-off-by: Arko Dasgupta <arko@tetrate.io> * 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 <arko@tetrate.io> * fix test rm invalid diff related to #3134 Signed-off-by: Arko Dasgupta <arko@tetrate.io> * fix GatewayInfraResourceTest Signed-off-by: Arko Dasgupta <arko@tetrate.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Ardika Bagus <me@ardikabs.com> Signed-off-by: Dingkang Li <dingkang1743@gmail.com> Signed-off-by: Jesse Haka <haka.jesse@gmail.com> Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com> Signed-off-by: haoqixu <hq.xu0o0@gmail.com> Signed-off-by: Alex Volchok <alex.volchok@sap.com> Signed-off-by: Guy Daich <guy.daich@sap.com> Co-authored-by: Lior Okman <lior.okman@sap.com> Co-authored-by: Ardika <me@ardikabs.com> Co-authored-by: Dingkang Li <dingkang1743@gmail.com> Co-authored-by: Jesse Haka <haka.jesse@gmail.com> Co-authored-by: Eguzki Astiz Lezaun <eastizle@redhat.com> Co-authored-by: xu0o0 <hqcat6@gmail.com> Co-authored-by: Alex Volchok <alex.volchok@sap.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Co-authored-by: zou rui <xiaorui.zou@gmail.com>
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