Skip to content

feat: add controller namespace field to infrastructure render #5937

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.

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

Merged
merged 5 commits into from
May 9, 2025

Conversation

cnvergence
Copy link
Member

@cnvergence cnvergence commented May 6, 2025

What type of PR is this?

What this PR does / why we need it:

Added new infra resource render field - controller namespace, this helps to distinguish namespace names in the infra processing.

Which issue(s) this PR fixes:

Fixes #2629
Followup to the #5137

Release Notes: No

Copy link

codecov bot commented May 6, 2025

Codecov Report

Attention: Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.78%. Comparing base (57f0058) to head (dfc11ce).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ernal/infrastructure/kubernetes/ratelimit_infra.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5937      +/-   ##
==========================================
- Coverage   65.78%   65.78%   -0.01%     
==========================================
  Files         217      217              
  Lines       36077    36056      -21     
==========================================
- Hits        23734    23718      -16     
+ Misses      10864    10860       -4     
+ Partials     1479     1478       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -47,6 +47,9 @@ type Infra struct {
// Namespace is the Namespace used for managed infra.
Namespace string

// ControllerNamespace is the Namespace used for Envoy Gateway controller.
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this be available in Runner Config ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Runner config calls the NewInfra, I'm adding this field to be able to differentiate the namespace used for Infra and Controller

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

controller namespace will be fetched from the Config

Copy link
Contributor

Choose a reason for hiding this comment

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

so, we dont have access to the runner context anymore ? is that the issue ?

Copy link
Member

@zhaohuabing zhaohuabing May 8, 2025

Choose a reason for hiding this comment

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

@arkodg runner context is not in the Infra, only the needed fields are passed down to the Infra structure.

func NewInfra(cli client.Client, cfg *config.Server) *Infra {
	var infraNamespace string
	if !cfg.EnvoyGateway.GatewayNamespaceMode() {
		infraNamespace = cfg.ControllerNamespace
	}
	return &Infra{
		Namespace:           infraNamespace,
		ControllerNamespace: cfg.ControllerNamespace,    # cfg is runner config
		DNSDomain:           cfg.DNSDomain,
		EnvoyGateway:        cfg.EnvoyGateway,
		Client:              New(cli),
		logger:              cfg.Logger.WithName(string(egv1a1.LogComponentInfrastructureRunner)),
	}
}

@cnvergence cnvergence force-pushed the gateway-mode-infra-ns branch from 3c73692 to d4f541f Compare May 8, 2025 13:44
@cnvergence cnvergence changed the title feat: add controller namespace field to infrastructure runner feat: add controller namespace field to infrastructure render May 8, 2025
@cnvergence cnvergence force-pushed the gateway-mode-infra-ns branch from 83dc8ef to be1d701 Compare May 8, 2025 18:58
@zhaohuabing
Copy link
Member

Can we also rename the Namespace field in the Infra structure to ControllerNamesapce to avoid confusion?

type Infra struct {
	// Namespace is the Namespace used for managed infra.
	Namespace string

	// DNSDomain is the dns domain used by k8s services. Defaults to "cluster.local".
	DNSDomain string

	// EnvoyGateway is the configuration used to startup Envoy Gateway.
	EnvoyGateway *egv1a1.EnvoyGateway

	// Client wrap k8s client.
	Client *InfraClient

	logger logging.Logger
}

@cnvergence cnvergence added this to the v1.4.0 milestone May 9, 2025
@cnvergence cnvergence force-pushed the gateway-mode-infra-ns branch from 794b9ae to dfc11ce Compare May 9, 2025 09:09
Copy link
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.

LGTM thanks!

@zirain zirain merged commit b2ca697 into envoyproxy:main May 9, 2025
25 checks passed
@cnvergence cnvergence deleted the gateway-mode-infra-ns branch May 9, 2025 10:11
melsal13 referenced this pull request in melsal13/gatewayPersonal May 9, 2025
* Add controller namespace to infra

Signed-off-by: Karol Szwaj <[email protected]>

* make gen

Signed-off-by: Karol Szwaj <[email protected]>

* rebase code and add controller namespace helper

Signed-off-by: Karol Szwaj <[email protected]>

* rename to envoy namespace

Signed-off-by: Karol Szwaj <[email protected]>

* rename to ControllerNamespace

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: melsal13 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure Envoy Proxy fleet in different namespaces
5 participants