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

NetworkPolicies are missing from static deployment YAMLs #11048

Closed
ViliusS opened this issue Mar 1, 2024 · 14 comments
Closed

NetworkPolicies are missing from static deployment YAMLs #11048

ViliusS opened this issue Mar 1, 2024 · 14 comments
Assignees
Labels
kind/support Categorizes issue or PR as a support question. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@ViliusS
Copy link

ViliusS commented Mar 1, 2024

What happened:

It looks like NetworkPolicies in static YAML files were briefly available in 1.9.4, but they are no longer generated since #10438 . This makes admission webhooks unable to handle ingress changes.

What you expected to happen:

Network policies should be generated in static YAML files.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):
1.9.5 and 1.10.0

Kubernetes version (use kubectl version):
1.27.10

Environment:
Bare metal Kubernetes installed via kubeadm on Rocky Linux 9.3

  • How was the ingress-nginx-controller installed:
    nginx controller installed via static YAML files provided by official repo.
@ViliusS ViliusS added the kind/bug Categorizes issue or PR as related to a bug. label Mar 1, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 1, 2024
@longwuyuan
Copy link
Contributor

/triage accepted
/priority important-soon

@ViliusS thanks for reporting this. I will check and then maybe others will comment too
/assign

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Mar 1, 2024
@longwuyuan
Copy link
Contributor

  • Can you post your k get networkpolicies.networking.k8s.io -A
  • Need to dig if there is reason for the change

@longwuyuan
Copy link
Contributor

longwuyuan commented Mar 1, 2024

@strongjz @Gacko as the reporting user mentions, that PR did clearly disable netpol in values, among other changes to netpol. I am looking for reasons but wondering if you already are well aware and can comment on why we made that change.

@longwuyuan
Copy link
Contributor

@ViliusS what do you mean admission webhook can not handle changes ?

I am using v1.10.0 and I added and changed ingresses and so I am assuming admission webhook checked the changes and allowed them as my changes were valid

@longwuyuan
Copy link
Contributor

Does your issues description mean this webhook is there but not doing any checks on my ingress resource changes ?

% k get validatingwebhookconfigurations.admissionregistration.k8s.io ingress-nginx-admission     
NAME                      WEBHOOKS   AGE
ingress-nginx-admission   1          9d
[~] 

@Gacko
Copy link
Member

Gacko commented Mar 1, 2024

Before that PR there was a network policy basically allowing any ingress to the controller:

https://github.com/kubernetes/ingress-nginx/pull/10438/files#diff-543601d03ba70d2aa24aaa79c836e65577302c37eaebaa24decd42125426c5cb

This network policy was controlled by the admission webhook configuration, so without admission webhooks, the controller would not have a network policy at all, not even for the other ports.

Additionally all the other network policies were already disabled by default. So the network policy we are talking about probably was rendered by mistake and if your setup was relying on network policies would not have been enough to make Ingress NGINX run.

See the original PR for further reasoning: #10238

To sum it up: The PR just aligned when network policies are being rendered (not at all by default) and put responsibilities where they belong to (controller has it's own network policy, other components like admission webhooks just add or remove the respective ports from it instead of mistakenly allowing all ingress).

@longwuyuan
Copy link
Contributor

ok, what I discovered from the PR is that netol needs to be explicitly enabled in the helm chart so that means while generating the static yaml manifests, we are not enabling netpol

@Gacko
Copy link
Member

Gacko commented Mar 1, 2024

Yes, the static files were never meant for setups relying on network policies. Even if the static files would allow access to the webhook port, they would still miss policies for...

... egress from the controller pod to any
... egress from the admission webhook patch job to the API server
... probably some other access

@longwuyuan
Copy link
Contributor

ok @Gacko ' s comment explains much of it now

@ViliusS please do elaborate what you mean by "This makes admission webhooks unable to handle ingress changes." . From @Gacko 's comment, the layman takeaway seems like the netpol in place earlier was cosmetic of sorts so that PR changed it to optional via helm chart values file and static manifests are now created without that cosmetic netpol.

@longwuyuan
Copy link
Contributor

/remove-kind bug
/remove-priority important-soon
/kind support

@k8s-ci-robot k8s-ci-robot added kind/support Categorizes issue or PR as a support question. needs-priority and removed kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 1, 2024
@Gacko
Copy link
Member

Gacko commented Mar 2, 2024

/assign

@ViliusS
Copy link
Author

ViliusS commented Mar 5, 2024

I just tried a bare install of NGINX Controller 1.10.0 on a clean Kubernetes cluster and it works. Then tried on my production cluster and it works there too now. Sorry for the noise, no idea why it didn't work repeatedly before. Maybe that's because I have completely cleaned the admission Kubernetes Jobs before reinstalling controller this time.

Just for completeness of this ticket, by "This makes admission webhooks unable to handle ingress changes" I meant that admission webhook was there and it detected the ingress changes (be it new URL rule, or a completely new ingress), but it failed the validation with an error (sorry I don't have exact error anymore). Since the validation failed the changes didn't propagate to the controller.
The only way to fix this under 1.9.x series was to either change webhook validation policy to failurePolicy: Ignore or allow ingress/egress in the NetworkPolicy for the admission hook.

@Gacko
Copy link
Member

Gacko commented Mar 5, 2024

But your changes are working now?

@ViliusS
Copy link
Author

ViliusS commented Mar 6, 2024

Yes, it works now. I will close this ticket.

@ViliusS ViliusS closed this as not planned Won't fix, can't repro, duplicate, stale Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Categorizes issue or PR as a support question. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

No branches or pull requests

4 participants