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

DOCS-2334: Docs for L7 rearchitecture with Envoy as sidecar containers #1714

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

ctauchen
Copy link
Collaborator

@ctauchen ctauchen commented Oct 10, 2024

This PR is a continuation of #1648, which is now closed.

@ctauchen ctauchen requested a review from a team as a code owner October 10, 2024 12:26
Copy link

netlify bot commented Oct 10, 2024

Deploy Preview succeeded!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1fcee71
🔍 Latest deploy log https://app.netlify.com/sites/tigera/deploys/670e4f4519890f000847b91d
😎 Deploy Preview https://deploy-preview-1714--tigera.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 24 (no change from production)
Accessibility: 90 (no change from production)
Best Practices: 75 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 10, 2024

Deploy Preview for calico-docs-preview-next ready!

Name Link
🔨 Latest commit 1fcee71
🔍 Latest deploy log https://app.netlify.com/sites/calico-docs-preview-next/deploys/670e4f45cb58510008649a00
😎 Deploy Preview https://deploy-preview-1714--calico-docs-preview-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 41 (🔴 down 13 from production)
Accessibility: 90 (no change from production)
Best Practices: 83 (no change from production)
SEO: 86 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@ctauchen ctauchen changed the title DOCS-2334: Docs for L7 rearchitecture with Envoy as sidecar containers" DOCS-2334: Docs for L7 rearchitecture with Envoy as sidecar containers Oct 10, 2024
@@ -24,7 +24,7 @@ Historically, web application firewalls (WAFs) were deployed at the edge of your

### About {{prodname}} WAF

WAF is deployed in your cluster along with Envoy DaemonSet. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
WAF is deployed in your cluster with Envoy as a sidecar container on your pods. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There seems to be a difference between the old and new concerning services vs deployments. With sidecars, it looks like you need to annotate deployments rather than services.

Is it fair to guess that we should review all instances of 'service' and consider replacing with 'deployment'?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can swap from:
{{prodname}} proxies selected service traffic through Envoy,
to:
{{prodname}} proxies all traffic through Envoy sidecar,

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to change service to deployment where it is talking about the enable of the feature.

Copy link
Member

Choose a reason for hiding this comment

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

@ctauchen yes that's right. In this case I think the sentence can be

"WAF is deployed in your cluster with Envoy as a sidecar container on your pods. {{prodname}} proxies traffic for the selected pods through Envoy, checking HTTP requests using the industry-standard..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, changing service to deployment throughout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterkellydev @radixo The deployment/service question is more complex than I'd realized, particularly with the WAF doc. We have examples using services, a note talking about how you shouldn't apply WAF to system services, and so on.

I'm thinking that we may need someone who knows what they're doing here (not me) to look through and mark the s/service/deployment locations in the doc.

By my naive understanding, we shouldn't be talking about services much at all, now that we've switched the WAF delivery mechanism to deployments exclusively. What relevance do services have on this page?


**Limitations**
* L7 logging is not compatible with a service mesh such as Istio.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterkellydev In my notes I've got this limitation for all L7 features. In our current docs, it's mentioned only for ALP. I've added to all three, but let me know if I've got that wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Yes adding to all 3 is good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for confirming. Leaving these as they are.

Copy link
Collaborator Author

@ctauchen ctauchen left a comment

Choose a reason for hiding this comment

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

@radixo @peterkellydev I've drafted changes as discussed. This changes the docs to be sidecar-only. Please pay special attention to limitations -- I suspect some remain that apply only to daemonset mode.

You can disregard formatting changes -- I'll see that that all gets sorted in my final review.

* Application layer policy is not compatible with a service mesh such as Istio.
* Application layer policy can restrict only _ingress_ traffic.
It can not restrict egress traffic.
* Support for L7 attributes is limited to HTTP method and URL exact/prefix path.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure these two bullets about attributes and protocols are limitations - they're just what the feature currently supports. I know they're pre-existing as limitations so it's fine to leave them I guess, I just think the section is less about what the feature supports / was implemented so far vs. things like platform limitations etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Limitations are loosely defined, but I'd characterize them as anything that describes a functionality that a user might reasonably expect the product to have, but that the product doesn't have. Whether and how we call those out is a judgement call. I'll leave this for now, as you suggest, but we can certainly reconsider these.

* Host-networked client pods
* TLS traffic
* [LoadBalancer services](https://Kubernetes.io/docs/concepts/services-networking/service/#loadbalancer)
* Egress gateways
Copy link
Member

@peterkellydev peterkellydev Oct 11, 2024

Choose a reason for hiding this comment

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

Egress Gateway is no longer a limitation in the new mode.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Line removed, thanks.


```bash
kubectl annotate svc <service-name> -n <service-namespace> projectcalico.org/l7-logging-
kubectl patch deployment <deployment-name> -n <deployment-namespace> --type='json' -p '[{"op":"remove","path":"/spec/template/metadata/labels/applicationlayer.projectcalico.org~1sidecar"},{"op":"remove","path":"/spec/template/metadata/annotations/applicationlayer.projectcalico.org~1waf"}]'
Copy link
Member

Choose a reason for hiding this comment

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

@radixo does this look right for the disable command for WAF?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it is, the bad thing I left behind were the unnecessary spaces on the left, you can trim them. But about the ~1 string, is how you can scape the / on json paths.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove leading spaces and leave the rest as is.

**Limitations**
* L7 logging is not compatible with a service mesh such as Istio.
* L7 log collection is not supported for host-networked client pods.
* When selecting and deselecting traffic for L7 log collection, active connections may be disrupted.
Copy link
Member

Choose a reason for hiding this comment

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

This is true for all 3 L7 features too. Although pods will be recycled and rolled out after being selected for L7, there can be existing HTTP connections that get dropped and reconnected. So we haven't changed that, it was true in old model and new model. But it is true for all L7 docs and not just this one. (I wish we had a single doc :) )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, adding this to all L7 pages. Fits better as a note.

Copy link
Contributor

@radixo radixo left a comment

Choose a reason for hiding this comment

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

Just some small changes but the docs looks very good now.

@@ -24,7 +24,7 @@ Historically, web application firewalls (WAFs) were deployed at the edge of your

### About {{prodname}} WAF

WAF is deployed in your cluster along with Envoy DaemonSet. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
WAF is deployed in your cluster with Envoy as a sidecar container on your pods. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
Copy link
Contributor

Choose a reason for hiding this comment

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

We can swap from:
{{prodname}} proxies selected service traffic through Envoy,
to:
{{prodname}} proxies all traffic through Envoy sidecar,

@@ -24,7 +24,7 @@ Historically, web application firewalls (WAFs) were deployed at the edge of your

### About {{prodname}} WAF

WAF is deployed in your cluster along with Envoy DaemonSet. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
WAF is deployed in your cluster with Envoy as a sidecar container on your pods. {{prodname}} proxies selected service traffic through Envoy, checking HTTP requests using the industry-standard
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to change service to deployment where it is talking about the enable of the feature.

Comment on lines 93 to 94
```bash
kubectl apply -f - <<EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```bash
kubectl apply -f - <<EOF
```yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. I'll un-kubectl all of these EOF to discourage overwriting existing CRDs.

@@ -115,32 +97,43 @@ kind: ApplicationLayer
metadata:
name: tigera-secure
spec:
webApplicationFirewall: Enabled
sidecarInjection: Enabled
EOF
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EOF

kubectl apply -f https://raw.githubusercontent.com/GoogleCloudPlatform/microservices-demo/main/release/kubernetes-manifests.yaml
```

## Apply WAF to services
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Apply WAF to services
## Apply WAF to deployments


```bash
kubectl annotate svc <service-name> -n <service-namespace> projectcalico.org/l7-logging-
kubectl patch deployment <deployment-name> -n <deployment-namespace> --type='json' -p '[{"op":"remove","path":"/spec/template/metadata/labels/applicationlayer.projectcalico.org~1sidecar"},{"op":"remove","path":"/spec/template/metadata/annotations/applicationlayer.projectcalico.org~1waf"}]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it is, the bad thing I left behind were the unnecessary spaces on the left, you can trim them. But about the ~1 string, is how you can scape the / on json paths.


```bash
kubectl annotate svc <service-name> -n <service-namespace> projectcalico.org/l7-logging-
kubectl patch deployment <deployment-name> -n <deployment-namespace> --type='json' -p '[{"op":"remove","path":"/spec/template/metadata/labels/applicationlayer.projectcalico.org~1sidecar"},{"op":"remove","path":"/spec/template/metadata/annotations/applicationlayer.projectcalico.org~1waf"}]'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
kubectl patch deployment <deployment-name> -n <deployment-namespace> --type='json' -p '[{"op":"remove","path":"/spec/template/metadata/labels/applicationlayer.projectcalico.org~1sidecar"},{"op":"remove","path":"/spec/template/metadata/annotations/applicationlayer.projectcalico.org~1waf"}]'
kubectl patch deployment <deployment-name> -n <deployment-namespace> --type='json' -p '[{"op":"remove","path":"/spec/template/metadata/labels/applicationlayer.projectcalico.org~1sidecar"},{"op":"remove","path":"/spec/template/metadata/annotations/applicationlayer.projectcalico.org~1waf"}]'

@ctauchen ctauchen merged commit e21801f into tigera:main Oct 15, 2024
9 of 10 checks passed
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.

3 participants