-
Notifications
You must be signed in to change notification settings - Fork 110
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
Correct EKS getting started docs #543
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview succeeded! Please check the logs for test failures which will cause the production (main) build to fail.Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
LGTM
|
||
```batch | ||
eksctl create nodegroup --cluster my-calico-cluster --node-type t3.medium --max-pods-per-node 100 | ||
helm install calico projectcalico/tigera-operator --namespace tigera-operator --version {{releaseTitle}} |
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.
If this is eks specific. And it is for using tigera-operator helm chart. I believe the values file here might want to be set to "EKS"
. In which case you would need to pass in the --set installation.kubernetesProvider="EKS"
argument to the helm install command.
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.
According to https:/tigera/operator/blob/master/pkg/controller/installation/core_controller.go#L499
installation.kubernetesProvider="EKS" causes tigera-operator to:
- pick AmazonVPC CNI
- specify default IP pool of "172.16.0.0/16" + VXLAN
- disable BGP
- set autodetection to CanReach: "8.8.8.8"
Since the point of this section of the doc is to set up Calico CNI, not AmazonVPC CNI, I think we do NOT want kubernetesProvider="EKS"
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.
It will only pick those settings if no other settings are provided - they are the deafults. Even if you set the provider to empty, the operator will know it is running on EKS and default that field to EKS anyway, and thus default to AmazonVPC CNI.
To use Calico on EKS, you will need to explicitly tell it to use Calico CNI as part of your Installation (via values.yaml, or explicit CLI flags to set particular values).
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 will want to create a values.yaml file and pass it to helm, with something like this:
cat > values.yaml <<EOF
installation:
kubernetesProvider: EKS
cni:
type: Calico
calicoNetwork:
bgp: Disabled
EOF
Potentially also include IP pools, if we need to:
ipPools:
- cidr: <default cidr>
encapsulation: VXLAN
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.
@ctauchen this particular bit of feedback needs to be addressed before merge. I think it's the only outstanding technical feedback.
I think we need the following as part of this step in order to install correctly.:
- Create a values.yaml file to configure Calico as your network provider.
cat > values.yaml <<EOF
installation:
kubernetesProvider: EKS
cni:
type: Calico
calicoNetwork:
bgp: Disabled
ipPools:
- encapsulation: VXLAN
cidr: 192.168.0.0/16
EOF
- Install calico using helm
helm install calico projectcalico/tigera-operator --namespace tigera-operator --version {{releaseTitle}} -f values.yaml
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.
A few comments, which should be carried through globally.
Is there anyone you'd like to nominate for a technical review?
calico_versioned_docs/version-3.24/getting-started/kubernetes/managed-public-cloud/eks.mdx
Outdated
Show resolved
Hide resolved
calico_versioned_docs/version-3.25/getting-started/kubernetes/managed-public-cloud/eks.mdx
Outdated
Show resolved
Hide resolved
Co-authored-by: Christopher Tauchen <[email protected]>
Perhaps @caseydavenport? I have a recollection of Casey adding the namespace and helm installing into that namespace before. |
per @lwr20 's request, there are a few other things we need to do for our helm docs across the board: Our generic helm docs here have some content that is just wrong.
This is misleading and both too specific and too general. It should be re-written to be more precise and explain that for basic setups, default settings will just work. However there are many many reasons you might want to customize your settings. I'm not sure why it specifically calls out TLS as a reason there - it's one reason, but the implication by omission is that it's the only reason, which is wrong! Our helm upgrade docs are also wrong - they omit the step where you need to manually replace the CRDs prior to upgrade. We can handle those in this PR, or in separate follow-ons depending on how you want to do it. But it would be great to attack these while we're thinking about helm. |
@https:/byronmansfield Looks like code change that the docs PR is for still hasn't been merged yet - it's failing tests. We had someone restart it. If you can rebase, we can get this merged. |
@lwr20 I'm trying to clear out stale PRs. This one seems both stale and necessary. I get the impression that the changes are sound, but need to be updated to later versions. Casey's comments may have slowed folks down, but the general Helm changes can be considered separately. Have I got the right idea here? I'm happy to put up a new PR with these changes in the later versions. And put something in the backlog for the other Helm business. |
@@ -70,12 +70,18 @@ Before you get started, make sure you have downloaded and configured the [necess | |||
eksctl create cluster --name my-calico-cluster --without-nodegroup | |||
``` | |||
|
|||
1. Since this cluster will use {{prodname}} for networking, you must delete the `aws-node` daemon set to disable AWS VPC networking for pods. |
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 don't understand why the new wording is more desirable - the former was more abstractly correct, whereas the latter is one particular reason among several for why we don't want the aws-node daemonset.
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.
Maybe "to ensure that nodes are not configured with AWS VPC networking for pods" would be better than "to disable"?
EKS getting started docs are wrong:
helm install
step.Product Version(s):
Calico v3.24 upwards.
Issue:
Raised by CU Byron Mansfield in slack thread: https://calicousers.slack.com/archives/CPTH1KS00/p1680705961183599
Link to docs preview:
SME review:
DOCS review:
Additional information: