Audit consistent use of controllerNames and their enabled flags w/ test to verify #13017
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -104,7 +104,33 @@ func GetInMemoryGatewayParameters(cfg InMemoryGatewayParametersConfig) *kgateway | |
| // set for the agentgateway deployment. | ||
| func defaultAgentgatewayParameters(imageInfo *ImageInfo, omitDefaultSecurityContext bool) *kgateway.GatewayParameters { | ||
| gwp := defaultGatewayParameters(imageInfo, omitDefaultSecurityContext) | ||
|
|
||
| // Add agentgateway-specific configuration | ||
| agwConfig := &kgateway.Agentgateway{ | ||
| LogLevel: ptr.To("info"), | ||
| Image: &kgateway.Image{ | ||
| Registry: ptr.To(AgentgatewayRegistry), | ||
| Tag: ptr.To(AgentgatewayDefaultTag), | ||
| Repository: ptr.To(AgentgatewayImage), | ||
| PullPolicy: (*corev1.PullPolicy)(ptr.To(imageInfo.PullPolicy)), | ||
|
Contributor
There was a problem hiding this comment. We want to default to omitting the pull policy so that k8s can use it's unsurprising logic about 'latest means Always, and otherwise...' (see https://kubernetes.io/docs/concepts/containers/images/#imagepullpolicy-defaulting). This is confusing and maybe even casts in the wrong place, but also turns the empty string into "something" when I'd prefer it were nil. There's also validation to consider since we're using a string in ImageInfo, not corev1.PullPolicy. Perhaps use this (which will never error because imageInfo is something we control completely):
Contributor
There was a problem hiding this comment. nvm; you just moved this and it's working well enough with a pointer to the empty string because both data plane helm charts say '{{- if $gateway.image.pullPolicy }}'. I don't love the cast on the outside but corev1.PullPolicy is a string like imageInfo.PullPolicy...
Contributor
Author
There was a problem hiding this comment. FWIW I agree, maybe a followup? |
||
| }, | ||
| } | ||
|
|
||
| // Only add SecurityContext if not omitting defaults | ||
| if !omitDefaultSecurityContext { | ||
| agwConfig.SecurityContext = &corev1.SecurityContext{ | ||
| AllowPrivilegeEscalation: ptr.To(false), | ||
| ReadOnlyRootFilesystem: ptr.To(true), | ||
| RunAsNonRoot: ptr.To(true), | ||
| RunAsUser: ptr.To[int64](10101), | ||
| Capabilities: &corev1.Capabilities{ | ||
| Drop: []corev1.Capability{"ALL"}, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| gwp.Spec.Kube.Agentgateway = agwConfig | ||
|
|
||
| gwp.Spec.Kube.PodTemplate.ReadinessProbe.HTTPGet.Path = "/healthz/ready" | ||
| gwp.Spec.Kube.PodTemplate.ReadinessProbe.HTTPGet.Port = intstr.FromInt(15021) | ||
| gwp.Spec.Kube.PodTemplate.StartupProbe.HTTPGet.Path = "/healthz/ready" | ||
|
|
@@ -258,31 +284,13 @@ func defaultGatewayParameters(imageInfo *ImageInfo, omitDefaultSecurityContext b | |
| IstioMetaClusterId: ptr.To("Kubernetes"), | ||
| }, | ||
| }, | ||
| // Note: Agentgateway config is only added for agentgateway controller gateways | ||
| // via defaultAgentgatewayParameters(). For envoy gateways, we leave this nil. | ||
| }, | ||
| }, | ||
| } | ||
| if omitDefaultSecurityContext { | ||
| gwp.Spec.Kube.EnvoyContainer.SecurityContext = nil | ||
| } | ||
| return gwp.DeepCopy() | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the author of #13018, I want to understand this block better. Seems like we don't need this block or the test/deployer test cases would change with its addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah; you moved it in this file. You moved it because it was only in use if this function was using it, and because without 'Enabled' it smelled a little funny remaining. There's no point in having the same thing in both places. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it would have been dead code here