fix `ipallowlist` parser not handling `validation` type errors #11968

chengjoey · 2024-09-13T08:30:34Z

What this PR does / why we need it:

fix ipallowlist parser not handling validation type errors

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #11967

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

Signed-off-by: joey <[email protected]>

k8s-ci-robot · 2024-09-13T08:30:42Z

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-09-13T08:30:43Z

Hi @chengjoey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-09-13T08:30:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chengjoey
Once this PR has been reviewed and has the lgtm label, please assign gacko for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-09-13T08:30:50Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`e8c72c7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66e3f82cd8213d00083c9cca

longwuyuan · 2024-09-13T12:09:24Z

@chengjoey thanks for the contribution.

Can you kindly help and copy/paste visual data like ;

- run `make dev-env` in your laptop clone of your fork, in your branch
- kubectl create deploy httpd --image httpd:alpine --port 80
- kubectl expose deploy httpd --port 80
- kubectl create ing httpd --class nginx --rule httpd.local/"*"=httpd:80
- kubectl describe ing httpd
- curl  localhost -H "Host: httpd.local"
- podName=`kubectl -n ingress-nginx get pod | grep -i ingress-nginx-controller | awk '{print $1}'`
- kubectl -n ingress-nginx exec $podName -- cat /etc/nginx/nginx.conf | egrep -i "allow|deny"

chengjoey · 2024-09-13T12:51:57Z

@chengjoey thanks for the contribution.

Can you kindly help and copy/paste visual data like ;

- run `make dev-env` in your laptop clone of your fork, in your branch
- kubectl create deploy httpd --image httpd:alpine --port 80
- kubectl expose deploy httpd --port 80
- kubectl create ing httpd --class nginx --rule httpd.local/"*"=httpd:80
- kubectl describe ing httpd
- curl  localhost -H "Host: httpd.local"
- podName=`kubectl -n ingress-nginx get pod | grep -i ingress-nginx-controller | awk '{print $1}'`
- kubectl -n ingress-nginx exec $podName -- cat /etc/nginx/nginx.conf | egrep -i "allow|deny"

kubectl describe ing httpd

Name:             httpd
Labels:           <none>
Namespace:        default
Address:          
Ingress Class:    nginx
Default backend:  <default>
Rules:
  Host         Path  Backends
  ----         ----  --------
  httpd.local  
               /   httpd:80 (<none>)
Annotations:   <none>
Events:
  Type    Reason  Age   From                      Message
  ----    ------  ----  ----                      -------
  Normal  Sync    11s   nginx-ingress-controller  Scheduled for sync

curl localhost -H "Host: httpd.local"

<html><body><h1>It works!</h1></body></html>

kubectl -n ingress-nginx exec $podName -- cat /etc/nginx/nginx.conf | egrep -i "allow|deny"

        # allow configuring ssl session tickets
        # allow configuring custom ssl ciphers
                        # Allow websocket connections
                        allow 127.0.0.1;
                        allow ::1;
                        deny all;
                        # Allow websocket connections

@longwuyuan , could you please tell me why need do this, If this is necessary work, I think it should be added to GitHub CI

longwuyuan · 2024-09-13T13:14:43Z

@chengjoey sorry for the confusion.

I copy/paste wrong or made a typo
Please change that "kubectl create ing command like below to create ingress with the whitelist annotation

k create ing httpd --class nginx --rule httpd.local/"*"=httpd:80 --annotation nginx.ingress.kubernetes.io/whitelist-source-range='1.1.1.1 2.2.2.2'

Then the grep from nginx.conf will show if that annotation made any changes to nginx.conf inside pod

If you see the issue I have 3 pieces of data on this

The admission controller allows the rule
The nginx.conf has a comment explaining its a bad directive config

$ kubectl -n ingress-nginx exec ingress-nginx-controller-6494fbd8fb-25r7f -- cat /etc/nginx/nginx.conf | grep -i 1.1.1.1
# Location denied. Reason: "the annotation does not contain a valid IP address or network: invalid CIDR address: 1.1.1.1 2.2.2.2"

There is a relevant message in controller logs

E0912 17:31:03.025233 7 annotations.go:216] "error reading Ingress annotation" err="the annotation does not contain a valid IP address or network: invalid CIDR address: 1.1.1.1 2.2.2.2" name="Allowlist" ingress="default/httpd"

So requested you for similar data to make it visually clear what happens after your changes on these 3 aspects
Thank you for your contribution

longwuyuan · 2024-09-13T13:19:20Z

I can/could clone your branch and check but I am hoping to get it visually explained here for all so that it adds value on the lines of why did this slip through and yet stop any real-damage by not getting inserted into nginx.conf. Is it possible that Nginx reconcile stopped the damage but admissionController needs a n rule or improvement.

chengjoey · 2024-09-13T13:29:10Z

Please change that "kubectl create ing command like below to create ingress with the whitelist annotation
k create ing httpd --class nginx --rule httpd.local/"*"=httpd:80 --annotation nginx.ingress.kubernetes.io/whitelist-source-range='1.1.1.1 2.2.2.2'

k create ing httpd --class nginx --rule httpd.local/"*"=httpd:80 --annotation nginx.ingress.kubernetes.io/whitelist-source-range='1.1.1.1 2.2.2.2'

error: failed to create ingress: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation nginx.ingress.kubernetes.io/whitelist-source-range contains invalid value

fix ipallowlist parser not handling validation type errors

e8c72c7

Signed-off-by: joey <[email protected]>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 13, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 13, 2024

k8s-ci-robot added the needs-priority label Sep 13, 2024

k8s-ci-robot requested review from puerco and tao12345666333 September 13, 2024 08:30

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix `ipallowlist` parser not handling `validation` type errors #11968

fix `ipallowlist` parser not handling `validation` type errors #11968

chengjoey commented Sep 13, 2024 •

edited

Loading

k8s-ci-robot commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

netlify bot commented Sep 13, 2024 •

edited

Loading

longwuyuan commented Sep 13, 2024 •

edited

Loading

chengjoey commented Sep 13, 2024

longwuyuan commented Sep 13, 2024 •

edited

Loading

longwuyuan commented Sep 13, 2024

chengjoey commented Sep 13, 2024 •

edited

Loading

fix ipallowlist parser not handling validation type errors #11968

Are you sure you want to change the base?

fix ipallowlist parser not handling validation type errors #11968

Conversation

chengjoey commented Sep 13, 2024 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

k8s-ci-robot commented Sep 13, 2024

netlify bot commented Sep 13, 2024 • edited Loading

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

longwuyuan commented Sep 13, 2024 • edited Loading

chengjoey commented Sep 13, 2024

longwuyuan commented Sep 13, 2024 • edited Loading

longwuyuan commented Sep 13, 2024

chengjoey commented Sep 13, 2024 • edited Loading

fix `ipallowlist` parser not handling `validation` type errors #11968

fix `ipallowlist` parser not handling `validation` type errors #11968

chengjoey commented Sep 13, 2024 •

edited

Loading

netlify bot commented Sep 13, 2024 •

edited

Loading

longwuyuan commented Sep 13, 2024 •

edited

Loading

longwuyuan commented Sep 13, 2024 •

edited

Loading

chengjoey commented Sep 13, 2024 •

edited

Loading