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

Add support for Calico, use RBAC and secrets encryption. #283

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

21repierre
Copy link

Tested with a single node cluster on Centos 9.

install.sh Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can't review this amount of YAML 😓

Can we just curl this YAML from the upstream?
https://github.com/projectcalico/calico/blob/v3.26.1/manifests/tigera-operator.yaml

Copy link
Author

Choose a reason for hiding this comment

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

Absolutly, it actually comes from this repo

Copy link
Member

Choose a reason for hiding this comment

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

This should be gitignored and curled on make.

providers:
- secretbox:
keys:
- name: key1
Copy link
Member

Choose a reason for hiding this comment

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

How is this key consumed?

Copy link
Author

Choose a reason for hiding this comment

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

According to the documentation the key should be used by kube-apiserver to encrypt the secrets.

Copy link
Member

Choose a reason for hiding this comment

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

Can we have more descriptive name?

Copy link
Author

Choose a reason for hiding this comment

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

I have confirmed we can use something better, currently I set it to secrets_key.
Do you have something in mind that would be better ?

@@ -51,7 +51,7 @@ function usage() {
echo
echo " --start=UNIT Enable and start the specified target after the installation, e.g. \"u7s.target\". Set to an empty to disable autostart. (Default: \"$start\")"
echo " --cri=RUNTIME Specify CRI runtime, \"containerd\" or \"crio\". (Default: \"$cri\")"
echo ' --cni=RUNTIME Specify CNI, an empty string (none) or "flannel". (Default: none)'
echo ' --cni=RUNTIME Specify CNI, an empty string (none), \"calico\" or "flannel". (Default: none)'
Copy link
Member

Choose a reason for hiding this comment

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

Can we test calico in CI ?

- name: "Smoke test (multi-node cluster with Flannel)"

Copy link
Author

Choose a reason for hiding this comment

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

We should be able to, I have never used Github CI but I can look into it.

@@ -19,6 +19,8 @@ exec $(dirname $0)/nsenter.sh kube-apiserver \
--service-account-signing-key-file=$XDG_CONFIG_HOME/usernetes/master/service-account-key.pem \
--advertise-address=$(cat $XDG_RUNTIME_DIR/usernetes/parent_ip) \
--allow-privileged \
--encryption-provider-config=$XDG_CONFIG_HOME/usernetes/master/secrets-encryption.yml \
--authorization-mode=Node,RBAC \
Copy link
Member

Choose a reason for hiding this comment

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

Could you split and squash the commits so that one commit has one topic?

Probably, this PR should have the following three commits:

  1. RBAC
  2. Secrets encryption
  3. Calico

Copy link
Author

Choose a reason for hiding this comment

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

How should I handle install.sh as it has edits for RBAC, Calico and secrets encryption ?

Copy link
Member

Choose a reason for hiding this comment

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

Just make commit 3 depend on commit 1 and 2?

Copy link
Member

Choose a reason for hiding this comment

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

The suffix should be something like .yaml.template, as this is not a complete manifest

- secretbox:
keys:
- name: key1
secret:
Copy link
Member

Choose a reason for hiding this comment

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

Can we use envsubst or something for filling up the secret value?

README.md Outdated
@@ -129,7 +129,7 @@ iptable_filter
ip6table_filter
nf_tables
x_tables
xt_MASQUERADE
xt_MASQUERADE # on older kernels: ipt_MASQUERADE and ip6t_MASQUERADE
Copy link
Member

Choose a reason for hiding this comment

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

A comment should be a separate line for ease of parsing

Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -51,7 +51,7 @@ function usage() {
echo
echo " --start=UNIT Enable and start the specified target after the installation, e.g. \"u7s.target\". Set to an empty to disable autostart. (Default: \"$start\")"
echo " --cri=RUNTIME Specify CRI runtime, \"containerd\" or \"crio\". (Default: \"$cri\")"
echo ' --cni=RUNTIME Specify CNI, an empty string (none) or "flannel". (Default: none)'
echo ' --cni=RUNTIME Specify CNI, an empty string (none), \"calico\" or "flannel". (Default: none)'
Copy link
Member

Choose a reason for hiding this comment

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

Could you update README.md too?

Signed-off-by: Pierre Boudvillain <[email protected]>
Signed-off-by: Pierre Boudvillain <[email protected]>
Signed-off-by: Pierre Boudvillain <[email protected]>
Signed-off-by: Pierre Boudvillain <[email protected]>
@AkihiroSuda
Copy link
Member

Do you plan to add a test?
#283 (comment)

@21repierre
Copy link
Author

Do you plan to add a test? #283 (comment)

I have tried to work on that by making a similar test to the flannel one (replacing flannel by calico in the docker-compose file and publishing ports according to the calico requirements), however I wasn't able to make it work.
It seems that the nodes can't reach the master:

  • Logs from calico-kube-controller running on node-crio: 2023-08-18 08:48:29.468 [INFO][1] main.go 138: Failed to initialize datastore error=Get "https://10.0.0.1:443/apis/crd.projectcalico.org/v1/clusterinformations/default": dial tcp 10.0.0.1:443: i/o timeout
  • Logs from coredns running on node-containerd: [ERROR] plugin/errors: 2 6277665988150219199.6938599929280645902. HINFO: read udp 10.0.100.194:37370->10.0.102.3:53: i/o timeout
  • Trying to curl the kubernetes API from a node also results on a timeout: curl: (7) Failed to connect to 10.0.100.100 port 6443 after 94953 ms: Couldn't connect to server

I'm not sure where the issue might come from.

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.

2 participants