Skip to content

feat: support custom names for generated k8s resources#3537

Merged
arkodg merged 7 commits intoenvoyproxy:mainfrom
arkodg:dep-svc-name
Jun 7, 2024
Merged

feat: support custom names for generated k8s resources#3537
arkodg merged 7 commits intoenvoyproxy:mainfrom
arkodg:dep-svc-name

Conversation

@arkodg
Copy link
Copy Markdown
Contributor

@arkodg arkodg commented Jun 4, 2024

Relates to #3527

feat: support custom names for generated k8s resources
8a5705a 
Relates to envoyproxy#3527

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested a review from a team as a code owner June 4, 2024 22:45
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 79.48718% with 8 lines in your changes missing coverage. Please review.

Project coverage is 68.18%. Comparing base (607d8bc) to head (767b10e).
Report is 7 commits behind head on main.

Files Patch % Lines
internal/gatewayapi/helpers.go 0.00% 4 Missing ⚠️
internal/provider/kubernetes/predicates.go 77.77% 2 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3537      +/-   ##
==========================================
+ Coverage   68.16%   68.18%   +0.02%     
==========================================
  Files         167      168       +1     
  Lines       20370    20408      +38     
==========================================
+ Hits        13885    13915      +30     
- Misses       5485     5493       +8     
  Partials     1000     1000

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

@arkodg arkodg requested a review from a team June 4, 2024 22:52
cnvergence
cnvergence requested changes Jun 5, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

probably we would need to also change it in kubernetes provider

gateway/internal/provider/kubernetes/helpers.go

Line 151 in 607d8bc

func infraName(gateway *gwapiv1.Gateway, merged bool) string {

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 5, 2024

hey @cnvergence can you elaborate why thats needed, the above diff is changing the value right before setting the resource name ?

@arkodg arkodg closed this Jun 5, 2024
@arkodg arkodg reopened this Jun 5, 2024
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 5, 2024

oh @cnvergence I see the issue now, we are fetching based on name and not labels 😢

gateway/internal/provider/kubernetes/predicates.go

Line 467 in cdd4e06

key := types.NamespacedName{

&

gateway/internal/provider/kubernetes/predicates.go

Line 483 in cdd4e06

key := types.NamespacedName{

cc @dprotaso this is the issue seen in knative-extensions/net-gateway-api#738

@arkodg arkodg mentioned this pull request Jun 5, 2024
Closed
Arko Dasgupta added 2 commits June 5, 2024 14:44
use labels instead of name and ns to get deployment and svc
c47dae4 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
fix test
1ec8471 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg requested review from a team and cnvergence June 6, 2024 00:16
Arko Dasgupta added 2 commits June 5, 2024 17:25
fix mergeGateway e2e
46aa837 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
build fix
cd1b726 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
cnvergence
cnvergence reviewed Jun 6, 2024
View reviewed changes
internal/gatewayapi/helpers.go
}

// OwnerLabels returns the owner labels based on the mergeGateways setting
func OwnerLabels(gateway *gwapiv1.Gateway, mergeGateways bool) map[string]string {
Copy link
Copy Markdown
Member

@cnvergence cnvergence Jun 6, 2024

Choose a reason for hiding this comment

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

🚀

cnvergence
cnvergence approved these changes Jun 6, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@cnvergence cnvergence left a comment

Choose a reason for hiding this comment

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

LGTM

@davem-git
Copy link
Copy Markdown
Contributor

davem-git commented Jun 6, 2024

If this is addressed here, will it also fix when I rename the envoyDeployment:, the HPA envoyHpa? It no longer works it keeps the original generated name. There doesn't seem to be a way to patch that either

fix hpa deployment target
a526784 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 6, 2024

If this is addressed here, will it also fix when I rename the envoyDeployment:, the HPA envoyHpa? It no longer works it keeps the original generated name. There doesn't seem to be a way to patch that either

thanks for catching this ! ive fixed the deployment targetRef naming in hpa.
I havent changed the name on the hpa resource itself, that needs an API change, which can be handled in a future PR

lint
767b10e 
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
zhaohuabing

This comment was marked as off-topic.

Sign in to view

zhaohuabing
zhaohuabing reviewed Jun 6, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

🚀

@arkodg arkodg merged commit 33fceb0 into envoyproxy:main Jun 7, 2024
@arkodg arkodg deleted the dep-svc-name branch June 7, 2024 00:00
@davem-git
Copy link
Copy Markdown
Contributor

davem-git commented Jun 14, 2024

On this same issue. I've renamed the deployment by patching the name, this causes the gateway to break. While the deployment with 3 replicas I get this error

  Last Transition Time:  2024-06-14T15:02:58Z
    Message:               The Gateway has been scheduled by Envoy Gateway
    Observed Generation:   1
    Reason:                Accepted
    Status:                True
    Type:                  Accepted
    Last Transition Time:  2024-06-14T15:03:07Z
    Message:               Deployment replicas unavailable
    Observed Generation:   1
    Reason:                NoResources
    Status:                False
    Type:                  Programmed

the gateway status is this

NAME            CLASS           ADDRESS         PROGRAMMED   AGE
acme-envoy   acme-envoy   IPADDRESS   False        4m19s

@arkodg
Copy link
Copy Markdown
Contributor Author

arkodg commented Jun 14, 2024

@davem-git can you raise a separate GH issue to highlight the issue you are facing

arkodg pushed a commit to arkodg/gateway that referenced this pull request Aug 21, 2024
support custom name for ratelimit deployment
f9d26f2 
* should have been part of envoyproxy#3537

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg mentioned this pull request Aug 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zhaohuabing zhaohuabing zhaohuabing approved these changes

@cnvergence cnvergence cnvergence approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@arkodg @davem-git @zhaohuabing @cnvergence