-
Notifications
You must be signed in to change notification settings - Fork 140
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
Introducing sidecarInjection to ApplicationLayer resource #3460
Conversation
dd29447
to
d00989a
Compare
5dbc3d7
to
b6f5a90
Compare
b6f5a90
to
0a3cfb0
Compare
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.
First quick pass - mostly nits, but a couple of API level questions that will help me evaluate some of the other bits of code.
Co-authored-by: Casey Davenport <caseydavenport@users.noreply.github.com>
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.
OK, second round done. Will keep looking through this.
Overall I don't see any major bugs in the code, but I want to take a few more looks as I continue to get a better feel for the feature set and desired behaviors.
pkg/render/apiserver.go
Outdated
@@ -65,6 +67,11 @@ const ( | |||
tigeraAPIServerTLSSecretName = "tigera-apiserver-certs" | |||
APIServerSecretsRBACName = "tigera-extension-apiserver-secrets-access" | |||
MultiTenantManagedClustersAccessClusterRoleName = "tigera-managed-cluster-access" | |||
L7AdmissionControllerContainerName = "calico-l7admssctrl" | |||
L7AdmissionControllerEnvoyImage = "envoyproxy/envoy:v1.31-latest" |
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.
I think we need to hook this image up through the "components" tooling like we do for our other images, rather than hard coding it here. i.e., it should be configurable via enterprise_versions.yml
Additionally, I suspect that using latest
tagged images isn't going to fly here, as it means the version that is installed can vary over time.
@Behnam-Shobiri could you chime in on whether we need to have build ownership / registry ownership of this image from an enterprise CVE perspective?
I think we probably need to host a specific version of this in our own registry, though.
collectorImage string | ||
dikastesImage string | ||
dikastesEnabled bool | ||
envoyEnabled bool |
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.
I assume envoyEnabled
means perHostEnvoyEnabled
?
I think we should refactor these names to be explicit about whether they are referring to per-host, sidecar, or "any" version of the component for clarity.
commandArgs = append( | ||
commandArgs, | ||
"--waf-enabled", | ||
"--waf-log-file", filepath.Join(CalicologsVolumePath, "waf", "waf.log"), |
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.
should --waf-log-file
go under the if c.config.WAFEnabled
section below? What aobut --waf-ruleset-file
?
At face value these both seem like arguments that are only relevant when WAF is enabled.
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.
The --waf-enabled
is telling dikastes that the per-host enforcement is enabled for WAF, needed for internal decisions on requests that can come even from sidecars or TPROXY iptables rules. So the --waf-log-file
and --waf-ruleset-file
are the configs that are required by both.
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.
Perfect.
Honestly, the renaming of WAFEnabled -> PerHostWAFEnabled makes this SO much easier to read for me now - this makes sense.
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.
The --waf-enabled is telling dikastes that the per-host enforcement is enabled for WA
It is probably a good idea to rename this argument on Dikastes (or add a new one and deprecate this one)....
--per-host-waf-enabled
^ Would be more accurate.
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.
Only minor comments this time - otherwise this LGTM.
Thanks much @radixo !
api/v1/apiserver_types.go
Outdated
@@ -78,7 +78,7 @@ func init() { | |||
type APIServerDeploymentContainer struct { | |||
// Name is an enum which identifies the API server Deployment container by name. | |||
// Supported values are: calico-apiserver, tigera-queryserver | |||
// +kubebuilder:validation:Enum=calico-apiserver;tigera-queryserver | |||
// +kubebuilder:validation:Enum=calico-apiserver;tigera-queryserver;calico-l7admssctrl |
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.
Given this name shows up on a user-facing API, can we change the container name to avoid abbreviations?
calico-l7-admission-controller
commandArgs = append( | ||
commandArgs, | ||
"--waf-enabled", | ||
"--waf-log-file", filepath.Join(CalicologsVolumePath, "waf", "waf.log"), |
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.
Perfect.
Honestly, the renaming of WAFEnabled -> PerHostWAFEnabled makes this SO much easier to read for me now - this makes sense.
commandArgs = append( | ||
commandArgs, | ||
"--waf-enabled", | ||
"--waf-log-file", filepath.Join(CalicologsVolumePath, "waf", "waf.log"), |
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.
The --waf-enabled is telling dikastes that the per-host enforcement is enabled for WA
It is probably a good idea to rename this argument on Dikastes (or add a new one and deprecate this one)....
--per-host-waf-enabled
^ Would be more accurate.
Thanks a million @caseydavenport |
Description
Those changes are introduced to make the operator capable to install and control the new Sidecar for L7 features.
Changes:
Design doc of the new feature as follows:
https://docs.google.com/document/d/1C5VKry_HLTLZq6VmpwaznpSjwYm4aYqeEpeN54S2fas/edit?usp=sharing
P.S. The introduced feature doesn't break any of the current L7 Application Layer features (WAF, Logging and ALP).
For PR author
make gen-files
make gen-versions
For PR reviewers
A note for code reviewers - all pull requests must have the following:
kind/bug
if this is a bugfix.kind/enhancement
if this is a a new feature.enterprise
if this PR applies to Calico Enterprise only.