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

Fix existing ns issue #1741

Merged
merged 11 commits into from
Mar 22, 2023
Merged

Fix existing ns issue #1741

merged 11 commits into from
Mar 22, 2023

Conversation

lexfrei
Copy link
Contributor

@lexfrei lexfrei commented Mar 11, 2023

Description

Todos

  • Tests
  • Documentation
  • Release note

Release Note

None required

Aleksey Sviridkin added 2 commits March 12, 2023 02:57
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 12, 2023

I can't find any tests for the chart. Should I start it from scratch?
I can suggest this project to write tests for charts: https://github.com/helm-unittest/helm-unittest

@rbrtbnfgl
Copy link
Contributor

There aren't any test to verify it. I added the helm chart recently.
I'll check the project that you suggested.
Thanks again for your contribution.

@rbrtbnfgl
Copy link
Contributor

rbrtbnfgl commented Mar 14, 2023

Are you sure that this will fix the issue? It seems relate to the namespace with this fix using helm to create the namespace will lead to an issue when helm is trying to create the namespace resource.
On your setup when you used helmfile is enough to not specify the namespace?

@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 15, 2023

Yep, it looks like some magic happened with helmfile in the previous iteration. Let's leave it and discuss just the bare helm :)

For now, the flannel's documentation is not working for me:

a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ kubectl delete ns kube-flannel
Error from server (NotFound): namespaces "kube-flannel" not found
✗: 1 @ Wed Mar 15 15:33:26 MSK 2023
a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ helm install flannel --set podCidr="10.244.0.0/16" https://github.com/flannel-io/flannel/releases/latest/download/flannel.tgz --set flannel.backend="wireguard"
Error: INSTALLATION FAILED: create: failed to create: namespaces "kube-flannel" not found
✗: 1 @ Wed Mar 15 15:33:33 MSK 2023

However, we can't use native helm's --create namespace, coz templated namespace name will conflict with the existing one:

a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ helm upgrade --install flannel --namespace kube-flannel --set podCidr="10.244.0.0/16" --set flannel.backend="wireguard" https://github.com/flannel-io/flannel/releases/latest/download/flannel.tgz
Release "flannel" does not exist. Installing it now.
Error: namespaces "kube-flannel" already exists
✗: 1 @ Wed Mar 15 15:37:53 MSK 2023

So, we need to create the namespace first:

a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ kubectl create ns kube-flannel
namespace/kube-flannel created
✓ @ Wed Mar 15 15:57:43 MSK 2023
a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ kubectl label ns kube-flannel pod-security.kubernetes.io/enforce=privileged
namespace/kube-flannel labeled
✓ @ Wed Mar 15 15:57:47 MSK 2023
a.sviridkin@mbp:~/git/github.com/lexfrei/k8s$ helm install flannel --namespace kube-flannel --set podCidr="10.244.0.0/16" --set flannel.backend="wireguard" ./flannel
NAME: flannel
LAST DEPLOYED: Wed Mar 15 15:57:59 2023
NAMESPACE: kube-flannel
STATUS: deployed
REVISION: 1
TEST SUITE: None
✓ @ Wed Mar 15 15:58:00 MSK 2023

Some research shows that helm project maintainers don't want to support namespace creation with labeling, so we need to do it manually. (closed as "won't fix" here: helm/helm#3503 (comment))
My suggestion is to use kubectl for namespace creation and helm for the rest. So we need to delete the namespace from the chart and add kubectl command to the docs.

Another option is to use magic pre-hook, but I don't like it, because it's not obvious, is against the official helm practices and requires some research to understand what's going on.

P.S. My previous attempt to add helm labels to ns is working for adoption, but not for creation. So, it's not a solution or a simplification.

Aleksey Sviridkin added 3 commits March 15, 2023 16:12
added manual ns creation to helm section
code marked as bash now
unused links deleted
formatted with best practices

Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 15, 2023

Here are example tests for daemonset. if you like it, I can add basic tests for all templates

Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 15, 2023

Also, we don't need to mention namespacing in the metadata, coz it will be passed from the helm.

README.md Outdated Show resolved Hide resolved
chart/kube-flannel/templates/config.yaml Show resolved Hide resolved
chart/kube-flannel/tests/daemonset_test.yaml Show resolved Hide resolved
Aleksey Sviridkin and others added 4 commits March 21, 2023 16:19
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
Signed-off-by: Aleksey Sviridkin <a.sviridkin@sdventures.com>
@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 22, 2023

Should I bring tests for other components in another PR?

@rbrtbnfgl
Copy link
Contributor

No the tests that you have included are fine. I think I will add the tests with the automatic tests.

@lexfrei
Copy link
Contributor Author

lexfrei commented Mar 22, 2023

Feel free to ping me if you'll need help with writing tests for the chart

@rbrtbnfgl rbrtbnfgl merged commit f057792 into flannel-io:master Mar 22, 2023
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.

None yet

2 participants