Skip to content
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

missing the envoy-gateaway-config for certgen command #4935

Closed
qicz opened this issue Dec 16, 2024 · 6 comments · Fixed by #5045
Closed

missing the envoy-gateaway-config for certgen command #4935

qicz opened this issue Dec 16, 2024 · 6 comments · Fixed by #5045
Assignees
Labels
kind/bug Something isn't working
Milestone

Comments

@qicz
Copy link
Member

qicz commented Dec 16, 2024

Description:

What issue is being seen? Describe what should be happening instead of
the bug, for example: Envoy should not crash, the expected value isn't
returned, etc.

Currently, the certgen job does not mount the envoy-gateway-config, so it can not read the extension config from envoy-gateway-config.

Repro steps:

Include sample requests, environment, etc. All data and inputs
required to reproduce the bug.

Note: If there are privacy concerns, sanitize the data prior to
sharing.

Environment:

Include the environment like gateway version, envoy version and so on.

Logs:

Include the access logs and the Envoy logs.

@qicz qicz added the kind/bug Something isn't working label Dec 16, 2024
@qicz qicz self-assigned this Dec 16, 2024
@qicz
Copy link
Member Author

qicz commented Dec 17, 2024

the certgen command just read the OverwriteControlPlaneCerts from envoy-gateway-config. so there are three options,

opt1: init with true for the OverwriteControlPlaneCerts

// DefaultEnvoyGatewayProvider returns a new EnvoyGatewayProvider with default configuration parameters.
func DefaultEnvoyGatewayProvider() *EnvoyGatewayProvider {
return &EnvoyGatewayProvider{
Type: ProviderTypeKubernetes,
Kubernetes: &EnvoyGatewayKubernetesProvider{
LeaderElection: DefaultLeaderElection(),
},
}
}

to

// DefaultEnvoyGatewayProvider returns a new EnvoyGatewayProvider with default configuration parameters.
func DefaultEnvoyGatewayProvider() *EnvoyGatewayProvider {
	return &EnvoyGatewayProvider{
		Type: ProviderTypeKubernetes,
		Kubernetes: &EnvoyGatewayKubernetesProvider{
			LeaderElection: DefaultLeaderElection(),
			OverwriteControlPlaneCerts: true,
		},
	}
}

opt2: support OverwriteControlPlaneCerts logic for certgen command directly

// outputCertsForKubernetes outputs the provided certs to a secret in namespace ns.
func outputCertsForKubernetes(ctx context.Context, cli client.Client, cfg *config.Server, certs *crypto.Certificates) error {
var updateSecrets bool
if cfg.EnvoyGateway != nil &&
cfg.EnvoyGateway.Provider != nil &&
cfg.EnvoyGateway.Provider.Kubernetes != nil &&
cfg.EnvoyGateway.Provider.Kubernetes.OverwriteControlPlaneCerts != nil &&
*cfg.EnvoyGateway.Provider.Kubernetes.OverwriteControlPlaneCerts {
updateSecrets = true
}
secrets, err := kubernetes.CreateOrUpdateSecrets(ctx, cli, kubernetes.CertsToSecret(cfg.Namespace, certs), updateSecrets)

the line 90, set the parameter with true directly.

 secrets, err := kubernetes.CreateOrUpdateSecrets(ctx, cli, kubernetes.CertsToSecret(cfg.Namespace, certs), true) 

opt3: mount the envoy-gateway-config like EG deployment to cergen job for reading the OverwriteControlPlaneCerts.

@arkodg @zirain any suggestion? I am +1 for opt2

@arkodg
Copy link
Contributor

arkodg commented Jan 7, 2025

thanks for flagging this @qicz
I prefer opt 3, since the code today is reusing the same functions
will need to add something similar to

for certgen job template
cc @guydc as this relates to #4891

@arkodg arkodg added this to the v1.3.0 milestone Jan 7, 2025
@guydc
Copy link
Contributor

guydc commented Jan 9, 2025

@arkodg with opt 3, we have an issue: the job is executed in a pre-install/update hook, so it would execute before the configmap is created/updated and would either fail or run with the previous versions's config. Since this functionality is anyway (?) specific to cert-gen, maybe we can just make it a flag for that command?

@arkodg
Copy link
Contributor

arkodg commented Jan 9, 2025

good point, yeah flag works

@qicz
Copy link
Member Author

qicz commented Jan 10, 2025

for command flag +1

@zirain
Copy link
Member

zirain commented Jan 10, 2025

+1 for flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants