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

Support for Ingress Firewall (extra configs) #284

Closed
vladimirfx opened this issue Jan 12, 2024 · 13 comments · Fixed by #288
Closed

Support for Ingress Firewall (extra configs) #284

vladimirfx opened this issue Jan 12, 2024 · 13 comments · Fixed by #288

Comments

@vladimirfx
Copy link

Please add support for extra document machine config.

https://www.talos.dev/v1.6/talos-guides/network/ingress-firewall/

@budimanjojo
Copy link
Owner

Thanks for the feature request! I'm not sure how to support this in a nice way. Do you have any idea on how this should be implemented? Should they be in the same yaml file as the machine config? How should the talconfig.yaml file looks like?

@budimanjojo
Copy link
Owner

My imagination is to have per node configuration like this:

nodes:
  - firewallSpec:
      ingress:
        defaultAction: block
        rules:
          - name: kubelet-ingress
            portSelector:
              ports:
                - 10250
              protocol: tcp
            ingress:
              - subnet: 172.20.0.0/24
                except: 172.20.0.1/32

And that will create a file inside ./clusterconfig/<clustername>-<nodename>-firewall.yaml with a content like this:

apiVersion: v1alpha1
kind: NetworkDefaultActionConfig
ingress: block
---
apiVersion: v1alpha1
kind: NetworkRuleConfig
name: kubelet-ingress
portSelector:
  ports:
    - 10250
  protocol: tcp
ingress:
  - subnet: 172.20.0.0/24
    except: 172.20.0.1/32

Is this the desired implementation?

@vladimirfx
Copy link
Author

Maybe add it as an attribute to existing node config:

nodes:
  - hostname: master
    controlPlane: true
    ...
    firewall: 

And render to multi-document YAML with a --- separator. This greatly simplifies config the application.

@budimanjojo
Copy link
Owner

@vladimirfx should the process of applying the file using talosctl apply-config -f <filename> on both during bootstrapping and updating works fine if the file is merged with --- separator?
I personally don't use firewall rules for my cluster so I really don't know and I just tried to apply a NetworkDefaultActionConfig to one of my node and it died instantly T.T

@budimanjojo
Copy link
Owner

Turns out applying the config using talosctl apply-config -f <filename> wiped my entire installation. I really don't know what happened.

@aivanov-citc
Copy link

aivanov-citc commented Jan 15, 2024

Hello.
When using talosctl, all node and firewall settings must be in one file and each node has its own firewall settings, for example

apiVersion: v1alpha1
kind: NetworkDefaultActionConfig
ingress: block
---
apiVersion: v1alpha1
kind: NetworkRuleConfig
name: kubelet-ingress
portSelector:
  ports:
    - 10250
  protocol: tcp
ingress:
  - subnet: 172.20.0.0/24
    except: 172.20.0.1/32
---
version: v1alpha1
machine:
...
cluster:
...

In addition to the firewall section, maybe also add a separate section for extra manifests (perhaps this method applies not only firewall settings) in the node settings and include files according to the node machine configuration, for example talconfig.yaml

nodes:
  - hostname: master
    controlPlane: true
    ...
    extraManifests:
      - firewall-cp.yaml
  - hostname: worker
    controlPlane: false
    ...
    extraManifests:
      - firewall-work.yaml

@budimanjojo
Copy link
Owner

@aivanov-citc Thanks! Ah they must be in one file, making it makes more sense for talhelper to support this. I'll see how to make this work tomorrow.

@budimanjojo
Copy link
Owner

I have just created the PR that will close this issue, do you think this is good enough?

@aivanov-citc For the extraManifests feature that you suggested above, Is there any usecase for it? I don't think there's other configurations that should be applied to the nodes besides firewall? Or am I completely wrong here?

@budimanjojo
Copy link
Owner

I feel like, instead of:

firewallSpec:
  ingress:
    ...

Is it better if it's just:

ingressFirewall:
  ...

Not sure which one is better, I want to prepare for when Talos decided to add egress in the future.
Or maybe just:

firewall:
  ingress:
    ...

@vladimirfx
Copy link
Author

I have just created the PR that will close this issue, do you think this is good enough?

Looks good to me. I bet for ingressFirewall field name.

@aivanov-citc
Copy link

aivanov-citc commented Jan 17, 2024

@budimanjojo

For the extraManifests feature that you suggested above, Is there any usecase for it? I don't think there's other configurations that should be applied to the nodes besides firewall? Or am I completely wrong here?

extraManifests is a universal section and is being done more for the future, since Talos is thinking of supporting multi-document configuration, it would be nice to already have support

siderolabs/talos#7223

@budimanjojo
Copy link
Owner

@aivanov-citc Thanks! Makes sense if that's the direction they're heading. I'll create another issue and maybe I'll work on it later.

@budimanjojo
Copy link
Owner

I just created another PR (#298) so that this feature can be put inside controlPlane and worker so you can add manifests that will be applied to all node groups.

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 a pull request may close this issue.

3 participants